Fix folder location & ordinal for Crostini Apps on update

Currently, when updating an existing Crostini App, the
associated CrostiniAppItem's ordinal and folder id are not
preserved. This is because a new CrostiniAppItem is added
to replace the old one. This leads to icons which users
had previously removed from the Linux Apps folder to return
to the folder, for example. This CL saves the item ordinal
and folder id from the old app item, and re-sets them on
the new app item.

This CL also changes process for inserting a newly
installed app so that it removes the app before adding
it. This is needed for the case that the user reformats
their Chromebook without uninstalling Crostini, which
leaves residual app info in their preferences.

Bug: 822488
Change-Id: Id7442c6563851ad4997578da65340a5c2eeef3f5
Reviewed-on: https://chromium-review.googlesource.com/1150955
Commit-Queue: Renée Wright <rjwright@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578541}

chrome/browser/ui/app_list/crostini/crostini_app_item.cc

@@ -22,7 +22,9 @@ CrostiniAppItem::CrostiniAppItem(
     AppListModelUpdater* model_updater,
     const app_list::AppListSyncableService::SyncItem* sync_item,
     const std::string& id,
-    const std::string& name)
+    const std::string& name,
+    base::Optional<std::string> folder_id,
+    base::Optional<syncer::StringOrdinal> item_ordinal)
     : ChromeAppListItem(profile, id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   crostini_app_icon_ = std::make_unique<CrostiniAppIcon>(
@@ -36,9 +38,14 @@ CrostiniAppItem::CrostiniAppItem(
   } else {
     SetDefaultPositionIfApplicable();
 
-    // Crostini app is created from scratch. Move it to default folder.
-    DCHECK(folder_id().empty());
-    SetChromeFolderId(kCrostiniFolderId);
+    // Newly installed apps will go to the Linux Apps folder. Updated apps
+    // should stay in whatever folder they are in.
+    if (folder_id.has_value()) {
+      SetChromeFolderId(folder_id.value());
+    }
+    if (item_ordinal.has_value()) {
+      SetChromePosition(item_ordinal.value());
+    }
   }
 
   // Set model updater last to avoid being called during construction.

chrome/browser/ui/app_list/crostini/crostini_app_item.h

@@ -18,7 +18,9 @@ class CrostiniAppItem : public ChromeAppListItem,
                   AppListModelUpdater* model_updater,
                   const app_list::AppListSyncableService::SyncItem* sync_item,
                   const std::string& id,
-                  const std::string& name);
+                  const std::string& name,
+                  base::Optional<std::string> folder_id,
+                  base::Optional<syncer::StringOrdinal> item_ordinal);
   ~CrostiniAppItem() override;
 
   CrostiniAppIcon* crostini_app_icon() { return crostini_app_icon_.get(); }

chrome/browser/ui/app_list/crostini/crostini_app_model_builder.cc

@@ -28,7 +28,7 @@ void CrostiniAppModelBuilder::BuildModel() {
   crostini::CrostiniRegistryService* registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(profile());
   for (const std::string& app_id : registry_service->GetRegisteredAppIds()) {
-    InsertCrostiniAppItem(registry_service, app_id);
+    InsertCrostiniAppItem(registry_service, app_id, {}, {});
   }
 
   registry_service->AddObserver(this);
@@ -43,7 +43,9 @@ void CrostiniAppModelBuilder::BuildModel() {
 
 void CrostiniAppModelBuilder::InsertCrostiniAppItem(
     const crostini::CrostiniRegistryService* registry_service,
-    const std::string& app_id) {
+    const std::string& app_id,
+    base::Optional<std::string> folder_id,
+    base::Optional<syncer::StringOrdinal> item_ordinal) {
   if (app_id == kCrostiniTerminalId && !IsCrostiniEnabled(profile())) {
     // If Crostini isn't enabled, don't show the Terminal item until it
     // becomes enabled.
@@ -55,9 +57,9 @@ void CrostiniAppModelBuilder::InsertCrostiniAppItem(
     return;
 
   MaybeCreateRootFolder();
-  InsertApp(std::make_unique<CrostiniAppItem>(profile(), model_updater(),
-                                              GetSyncItem(app_id), app_id,
-                                              registration.Name()));
+  InsertApp(std::make_unique<CrostiniAppItem>(
+      profile(), model_updater(), GetSyncItem(app_id), app_id,
+      registration.Name(), folder_id, item_ordinal));
 }
 
 void CrostiniAppModelBuilder::OnRegistryUpdated(
@@ -69,11 +71,23 @@ void CrostiniAppModelBuilder::OnRegistryUpdated(
   for (const std::string& app_id : removed_apps)
     RemoveApp(app_id, unsynced_change);
   for (const std::string& app_id : updated_apps) {
+    const app_list::AppListSyncableService::SyncItem* sync_item =
+        GetSyncItem(app_id);
+    std::string folder_id = kCrostiniFolderId;
+    base::Optional<syncer::StringOrdinal> item_ordinal;
+    if (sync_item) {
+      folder_id = sync_item->parent_id;
+      item_ordinal = base::make_optional(sync_item->item_ordinal);
+    }
     RemoveApp(app_id, unsynced_change);
-    InsertCrostiniAppItem(registry_service, app_id);
+    InsertCrostiniAppItem(registry_service, app_id, folder_id, item_ordinal);
+  }
+  for (const std::string& app_id : inserted_apps) {
+    // If the app has been installed before and has not been cleaned up
+    // correctly, it needs to be removed.
+    RemoveApp(app_id, unsynced_change);
+    InsertCrostiniAppItem(registry_service, app_id, kCrostiniFolderId, {});
   }
-  for (const std::string& app_id : inserted_apps)
-    InsertCrostiniAppItem(registry_service, app_id);
 }
 
 void CrostiniAppModelBuilder::OnAppIconUpdated(const std::string& app_id,
@@ -90,12 +104,16 @@ void CrostiniAppModelBuilder::OnAppIconUpdated(const std::string& app_id,
 }
 
 void CrostiniAppModelBuilder::OnCrostiniEnabledChanged() {
+  const bool unsynced_change = false;
   if (IsCrostiniEnabled(profile())) {
+    // If Terminal has been installed before and has not been cleaned up
+    // correctly, it needs to be removed.
+    RemoveApp(kCrostiniTerminalId, unsynced_change);
     crostini::CrostiniRegistryService* registry_service =
         crostini::CrostiniRegistryServiceFactory::GetForProfile(profile());
-    InsertCrostiniAppItem(registry_service, kCrostiniTerminalId);
+    InsertCrostiniAppItem(registry_service, kCrostiniTerminalId,
+                          kCrostiniFolderId, {});
   } else {
-    const bool unsynced_change = false;
     RemoveApp(kCrostiniTerminalId, unsynced_change);
   }
 }

chrome/browser/ui/app_list/crostini/crostini_app_model_builder.h

@@ -34,7 +34,9 @@ class CrostiniAppModelBuilder
 
   void InsertCrostiniAppItem(
       const crostini::CrostiniRegistryService* registry_service,
-      const std::string& app_id);
+      const std::string& app_id,
+      base::Optional<std::string> folder_id,
+      base::Optional<syncer::StringOrdinal> item_ordinal);
 
   void OnCrostiniEnabledChanged();