Commit f7edd5af authored by Yury Khmel's avatar Yury Khmel Committed by Commit Bot

Revert "Fix folder location & ordinal for Crostini Apps on update"

This reverts commit aa28143d.

Reason for revert: 
Sorry, I did not review this, let revert this one and discuss.
I have few questions. I asked to add description in order to understand what you going to achieve.
Let talk about possible solutions and reland new one.

Original change's description:
> 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}

TBR=stevenjb@chromium.org,rjwright@chromium.org,khmel@chromium.org

Change-Id: Idf60204ad9bb6cc6dc215537dcab5520649fbfa4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 822488
Reviewed-on: https://chromium-review.googlesource.com/1152273Reviewed-by: default avatarYury Khmel <khmel@chromium.org>
Commit-Queue: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578550}
parent f28fcf91
......@@ -22,9 +22,7 @@ CrostiniAppItem::CrostiniAppItem(
AppListModelUpdater* model_updater,
const app_list::AppListSyncableService::SyncItem* sync_item,
const std::string& id,
const std::string& name,
base::Optional<std::string> folder_id,
base::Optional<syncer::StringOrdinal> item_ordinal)
const std::string& name)
: ChromeAppListItem(profile, id) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
crostini_app_icon_ = std::make_unique<CrostiniAppIcon>(
......@@ -38,14 +36,9 @@ CrostiniAppItem::CrostiniAppItem(
} else {
SetDefaultPositionIfApplicable();
// 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());
}
// Crostini app is created from scratch. Move it to default folder.
DCHECK(folder_id().empty());
SetChromeFolderId(kCrostiniFolderId);
}
// Set model updater last to avoid being called during construction.
......
......@@ -18,9 +18,7 @@ class CrostiniAppItem : public ChromeAppListItem,
AppListModelUpdater* model_updater,
const app_list::AppListSyncableService::SyncItem* sync_item,
const std::string& id,
const std::string& name,
base::Optional<std::string> folder_id,
base::Optional<syncer::StringOrdinal> item_ordinal);
const std::string& name);
~CrostiniAppItem() override;
CrostiniAppIcon* crostini_app_icon() { return crostini_app_icon_.get(); }
......
......@@ -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,9 +43,7 @@ void CrostiniAppModelBuilder::BuildModel() {
void CrostiniAppModelBuilder::InsertCrostiniAppItem(
const crostini::CrostiniRegistryService* registry_service,
const std::string& app_id,
base::Optional<std::string> folder_id,
base::Optional<syncer::StringOrdinal> item_ordinal) {
const std::string& app_id) {
if (app_id == kCrostiniTerminalId && !IsCrostiniEnabled(profile())) {
// If Crostini isn't enabled, don't show the Terminal item until it
// becomes enabled.
......@@ -57,9 +55,9 @@ void CrostiniAppModelBuilder::InsertCrostiniAppItem(
return;
MaybeCreateRootFolder();
InsertApp(std::make_unique<CrostiniAppItem>(
profile(), model_updater(), GetSyncItem(app_id), app_id,
registration.Name(), folder_id, item_ordinal));
InsertApp(std::make_unique<CrostiniAppItem>(profile(), model_updater(),
GetSyncItem(app_id), app_id,
registration.Name()));
}
void CrostiniAppModelBuilder::OnRegistryUpdated(
......@@ -71,23 +69,11 @@ 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, 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, {});
InsertCrostiniAppItem(registry_service, app_id);
}
for (const std::string& app_id : inserted_apps)
InsertCrostiniAppItem(registry_service, app_id);
}
void CrostiniAppModelBuilder::OnAppIconUpdated(const std::string& app_id,
......@@ -104,16 +90,12 @@ 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,
kCrostiniFolderId, {});
InsertCrostiniAppItem(registry_service, kCrostiniTerminalId);
} else {
const bool unsynced_change = false;
RemoveApp(kCrostiniTerminalId, unsynced_change);
}
}
......
......@@ -34,9 +34,7 @@ class CrostiniAppModelBuilder
void InsertCrostiniAppItem(
const crostini::CrostiniRegistryService* registry_service,
const std::string& app_id,
base::Optional<std::string> folder_id,
base::Optional<syncer::StringOrdinal> item_ordinal);
const std::string& app_id);
void OnCrostiniEnabledChanged();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment