From 0939e1630bd1c834b48bf4075567424b271def43 Mon Sep 17 00:00:00 2001 From: "stevenjb@chromium.org" Date: Wed, 12 Mar 2014 19:35:36 +0000 Subject: [PATCH] Fix incorrect use of AppListItemList::FindItem This CL should: * Fix a Folders bugs where existing items in folders were not found (causing them not to be updated and potentially creating duplicate items) * Protect the code against similar errors in the future * Correctly protect against an edge case where multiple items might have the same ordianl BUG=350165, 346117 For sync_app_list_helper.cc: R=jennyz@chromium.org, tapted@chromium.org TBR=zea@chromium.org Review URL: https://codereview.chromium.org/190953012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256603 0039d316-1c4b-4281-b951-d872f2087c98 --- .../test/integration/sync_app_list_helper.cc | 65 ++++++----- .../browser/ui/app_list/extension_app_item.cc | 10 +- .../browser/ui/app_list/extension_app_item.h | 5 +- .../app_list/extension_app_model_builder.cc | 29 ++--- .../extension_app_model_builder_unittest.cc | 8 +- .../browser/ui/app_list/fast_show_pickler.cc | 14 ++- .../test/fast_show_pickler_unittest.cc | 21 ++-- ui/app_list/app_list_folder_item.cc | 5 + ui/app_list/app_list_folder_item.h | 1 + ui/app_list/app_list_item.cc | 2 + ui/app_list/app_list_item.h | 4 + ui/app_list/app_list_item_list.cc | 77 +++++++++---- ui/app_list/app_list_item_list.h | 6 + ui/app_list/app_list_item_list_observer.h | 1 + ui/app_list/app_list_item_list_unittest.cc | 109 +++++++++++++++--- ui/app_list/app_list_model.cc | 52 +++++---- ui/app_list/app_list_model.h | 29 +++-- ui/app_list/app_list_model_unittest.cc | 72 ++++++------ ui/app_list/cocoa/apps_grid_controller.mm | 11 +- .../cocoa/apps_grid_controller_unittest.mm | 13 ++- ui/app_list/test/app_list_test_model.cc | 18 +-- ui/app_list/views/app_list_folder_view.cc | 6 +- ui/app_list/views/app_list_folder_view.h | 10 +- ui/app_list/views/app_list_main_view.cc | 8 +- ui/app_list/views/apps_container_view.cc | 2 +- ui/app_list/views/apps_grid_view_unittest.cc | 8 +- 26 files changed, 365 insertions(+), 221 deletions(-) diff --git a/chrome/browser/sync/test/integration/sync_app_list_helper.cc b/chrome/browser/sync/test/integration/sync_app_list_helper.cc index 79dc84e4ec07e6..ef862283ef8a6a 100644 --- a/chrome/browser/sync/test/integration/sync_app_list_helper.cc +++ b/chrome/browser/sync/test/integration/sync_app_list_helper.cc @@ -29,11 +29,9 @@ SyncAppListHelper* SyncAppListHelper::GetInstance() { return instance; } -SyncAppListHelper::SyncAppListHelper() : test_(NULL), setup_completed_(false) { -} +SyncAppListHelper::SyncAppListHelper() : test_(NULL), setup_completed_(false) {} -SyncAppListHelper::~SyncAppListHelper() { -} +SyncAppListHelper::~SyncAppListHelper() {} void SyncAppListHelper::SetupIfNecessary(SyncTest* test) { if (setup_completed_) { @@ -43,11 +41,11 @@ void SyncAppListHelper::SetupIfNecessary(SyncTest* test) { test_ = test; for (int i = 0; i < test->num_clients(); ++i) { - extensions::ExtensionSystem::Get( - test_->GetProfile(i))->InitForRegularProfile(true); + extensions::ExtensionSystem::Get(test_->GetProfile(i)) + ->InitForRegularProfile(true); } - extensions::ExtensionSystem::Get( - test_->verifier())->InitForRegularProfile(true); + extensions::ExtensionSystem::Get(test_->verifier()) + ->InitForRegularProfile(true); setup_completed_ = true; } @@ -59,32 +57,35 @@ bool SyncAppListHelper::AppListMatchesVerifier(Profile* profile) { AppListSyncableServiceFactory::GetForProfile(test_->verifier()); // Note: sync item entries may not exist in verifier, but item lists should // match. - if (service->model()->item_list()->item_count() != - verifier->model()->item_list()->item_count()) { + if (service->model()->top_level_item_list()->item_count() != + verifier->model()->top_level_item_list()->item_count()) { LOG(ERROR) << "Model item count: " - << service->model()->item_list()->item_count() - << " != " << verifier->model()->item_list()->item_count(); + << service->model()->top_level_item_list()->item_count() + << " != " + << verifier->model()->top_level_item_list()->item_count(); return false; } bool res = true; - for (size_t i = 0; i < service->model()->item_list()->item_count(); ++i) { - AppListItem* item1 = service->model()->item_list()->item_at(i); - AppListItem* item2 = verifier->model()->item_list()->item_at(i); + for (size_t i = 0; i < service->model()->top_level_item_list()->item_count(); + ++i) { + AppListItem* item1 = service->model()->top_level_item_list()->item_at(i); + AppListItem* item2 = verifier->model()->top_level_item_list()->item_at(i); if (item1->CompareForTest(item2)) continue; LOG(ERROR) << "Item(" << i << "): " << item1->ToDebugString() << " != " << item2->ToDebugString(); size_t index2; - if (!verifier->model()->item_list()->FindItemIndex(item1->id(), &index2)) { + if (!verifier->model()->top_level_item_list()->FindItemIndex(item1->id(), + &index2)) { LOG(ERROR) << " Item(" << i << "): " << item1->ToDebugString() << " Not in verifier."; } else { LOG(ERROR) << " Item(" << i << "): " << item1->ToDebugString() << " Has different verifier index: " << index2; - item2 = verifier->model()->item_list()->item_at(index2); - LOG(ERROR) << " Verifier Item(" << index2 << "): " - << item2->ToDebugString(); + item2 = verifier->model()->top_level_item_list()->item_at(index2); + LOG(ERROR) << " Verifier Item(" << index2 + << "): " << item2->ToDebugString(); } res = false; } @@ -118,7 +119,7 @@ bool SyncAppListHelper::AllProfilesHaveSameAppListAsVerifier() { void SyncAppListHelper::MoveApp(Profile* profile, size_t from, size_t to) { AppListSyncableService* service = AppListSyncableServiceFactory::GetForProfile(profile); - service->model()->item_list()->MoveItem(from, to); + service->model()->top_level_item_list()->MoveItem(from, to); } void SyncAppListHelper::MoveAppToFolder(Profile* profile, @@ -127,7 +128,7 @@ void SyncAppListHelper::MoveAppToFolder(Profile* profile, AppListSyncableService* service = AppListSyncableServiceFactory::GetForProfile(profile); service->model()->MoveItemToFolder( - service->model()->item_list()->item_at(index), folder_id); + service->model()->top_level_item_list()->item_at(index), folder_id); } void SyncAppListHelper::MoveAppFromFolder(Profile* profile, @@ -150,18 +151,18 @@ void SyncAppListHelper::CopyOrdinalsToVerifier(Profile* profile, AppListSyncableServiceFactory::GetForProfile(profile); AppListSyncableService* verifier = AppListSyncableServiceFactory::GetForProfile(test_->verifier()); - verifier->model()->item_list()->SetItemPosition( - verifier->model()->item_list()->FindItem(id), - service->model()->item_list()->FindItem(id)->position()); + verifier->model()->top_level_item_list()->SetItemPosition( + verifier->model()->FindItem(id), + service->model()->FindItem(id)->position()); } void SyncAppListHelper::PrintAppList(Profile* profile) { AppListSyncableService* service = AppListSyncableServiceFactory::GetForProfile(profile); - for (size_t i = 0; i < service->model()->item_list()->item_count(); ++i) { - AppListItem* item = service->model()->item_list()->item_at(i); - std::string label = - base::StringPrintf("Item(%d): ", static_cast(i)); + for (size_t i = 0; i < service->model()->top_level_item_list()->item_count(); + ++i) { + AppListItem* item = service->model()->top_level_item_list()->item_at(i); + std::string label = base::StringPrintf("Item(%d): ", static_cast(i)); PrintItem(profile, item, label); } } @@ -183,8 +184,8 @@ void SyncAppListHelper::PrintItem(Profile* profile, } return; } - VLOG(1) - << label << item->ToDebugString() - << " Page: " << s->GetPageOrdinal(id).ToDebugString().substr(0, 8) - << " Item: " << s->GetAppLaunchOrdinal(id).ToDebugString().substr(0, 8); + VLOG(1) << label << item->ToDebugString() + << " Page: " << s->GetPageOrdinal(id).ToDebugString().substr(0, 8) + << " Item: " + << s->GetAppLaunchOrdinal(id).ToDebugString().substr(0, 8); } diff --git a/chrome/browser/ui/app_list/extension_app_item.cc b/chrome/browser/ui/app_list/extension_app_item.cc index 697e69c6fa25d7..7daa0506663247 100644 --- a/chrome/browser/ui/app_list/extension_app_item.cc +++ b/chrome/browser/ui/app_list/extension_app_item.cc @@ -203,11 +203,6 @@ void ExtensionAppItem::UpdateIcon() { SetIcon(icon, true); } -void ExtensionAppItem::UpdateIconOverlay() { - if (has_overlay_ != NeedsOverlay()) - UpdateIcon(); -} - void ExtensionAppItem::Move(const ExtensionAppItem* prev, const ExtensionAppItem* next) { if (!prev && !next) @@ -335,6 +330,11 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() { return context_menu_->GetMenuModel(); } +void ExtensionAppItem::OnExtensionPreferenceChanged() { + if (has_overlay_ != NeedsOverlay()) + UpdateIcon(); +} + // static const char ExtensionAppItem::kItemType[] = "ExtensionAppItem"; diff --git a/chrome/browser/ui/app_list/extension_app_item.h b/chrome/browser/ui/app_list/extension_app_item.h index be256830f9bd77..f52e1273a7a1d5 100644 --- a/chrome/browser/ui/app_list/extension_app_item.h +++ b/chrome/browser/ui/app_list/extension_app_item.h @@ -51,9 +51,6 @@ class ExtensionAppItem : public app_list::AppListItem, // it gray. void UpdateIcon(); - // Only updates the icon if the overlay needs to be added/removed. - void UpdateIconOverlay(); - // Update page and app launcher ordinals to put the app in between |prev| and // |next|. Note that |prev| and |next| could be NULL when the app is put at // the beginning or at the end. @@ -92,6 +89,8 @@ class ExtensionAppItem : public app_list::AppListItem, // Overridden from AppListItem: virtual void Activate(int event_flags) OVERRIDE; virtual ui::MenuModel* GetContextMenuModel() OVERRIDE; + // Updates the icon if the overlay needs to be added/removed. + virtual void OnExtensionPreferenceChanged() OVERRIDE; virtual const char* GetItemType() const OVERRIDE; // Overridden from app_list::AppContextMenuDelegate: diff --git a/chrome/browser/ui/app_list/extension_app_model_builder.cc b/chrome/browser/ui/app_list/extension_app_model_builder.cc index da1fc5da32bc0a..13acfef0d3da7d 100644 --- a/chrome/browser/ui/app_list/extension_app_model_builder.cc +++ b/chrome/browser/ui/app_list/extension_app_model_builder.cc @@ -59,14 +59,14 @@ ExtensionAppModelBuilder::ExtensionAppModelBuilder( ExtensionAppModelBuilder::~ExtensionAppModelBuilder() { OnShutdown(); - model_->item_list()->RemoveObserver(this); + if (!service_) + model_->top_level_item_list()->RemoveObserver(this); } void ExtensionAppModelBuilder::InitializeWithService( app_list::AppListSyncableService* service) { DCHECK(!service_ && !profile_); model_ = service->model(); - model_->item_list()->AddObserver(this); service_ = service; profile_ = service->profile(); InitializePrefChangeRegistrar(); @@ -79,7 +79,7 @@ void ExtensionAppModelBuilder::InitializeWithProfile( app_list::AppListModel* model) { DCHECK(!service_ && !profile_); model_ = model; - model_->item_list()->AddObserver(this); + model_->top_level_item_list()->AddObserver(this); profile_ = profile; InitializePrefChangeRegistrar(); @@ -91,6 +91,8 @@ void ExtensionAppModelBuilder::InitializePrefChangeRegistrar() { switches::kEnableStreamlinedHostedApps)) return; + // TODO(calamity): analyze the performance impact of doing this every + // extension pref change. extensions::ExtensionsBrowserClient* client = extensions::ExtensionsBrowserClient::Get(); extension_pref_change_registrar_.Init( @@ -102,16 +104,7 @@ void ExtensionAppModelBuilder::InitializePrefChangeRegistrar() { } void ExtensionAppModelBuilder::OnExtensionPreferenceChanged() { - // TODO(calamity): analyze the performance impact of doing this every - // extension pref change. - app_list::AppListItemList* item_list = model_->item_list(); - for (size_t i = 0; i < item_list->item_count(); ++i) { - app_list::AppListItem* item = item_list->item_at(i); - if (item->GetItemType() != ExtensionAppItem::kItemType) - continue; - - static_cast(item)->UpdateIconOverlay(); - } + model_->NotifyExtensionPreferenceChanged(); } void ExtensionAppModelBuilder::OnBeginExtensionInstall( @@ -262,8 +255,7 @@ void ExtensionAppModelBuilder::SetHighlightedApp( ExtensionAppItem* ExtensionAppModelBuilder::GetExtensionAppItem( const std::string& extension_id) { - app_list::AppListItem* item = - model_->item_list()->FindItem(extension_id); + app_list::AppListItem* item = model_->FindItem(extension_id); LOG_IF(ERROR, item && item->GetItemType() != ExtensionAppItem::kItemType) << "App Item matching id: " << extension_id @@ -285,15 +277,14 @@ void ExtensionAppModelBuilder::UpdateHighlight() { void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index, size_t to_index, app_list::AppListItem* item) { + DCHECK(!service_); + // This will get called from AppListItemList::ListItemMoved after // set_position is called for the item. - app_list::AppListItemList* item_list = model_->item_list(); if (item->GetItemType() != ExtensionAppItem::kItemType) return; - if (service_) - return; - + app_list::AppListItemList* item_list = model_->top_level_item_list(); ExtensionAppItem* prev = NULL; for (size_t idx = to_index; idx > 0; --idx) { app_list::AppListItem* item = item_list->item_at(idx - 1); diff --git a/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc b/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc index 870e52ebe2293e..e55327b4719793 100644 --- a/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc +++ b/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc @@ -35,10 +35,10 @@ const char kPackagedApp2Id[] = "jlklkagmeajbjiobondfhiekepofmljl"; // Get a string of all apps in |model| joined with ','. std::string GetModelContent(app_list::AppListModel* model) { std::string content; - for (size_t i = 0; i < model->item_list()->item_count(); ++i) { + for (size_t i = 0; i < model->top_level_item_list()->item_count(); ++i) { if (i > 0) content += ','; - content += model->item_list()->item_at(i)->name(); + content += model->top_level_item_list()->item_at(i)->name(); } return content; } @@ -163,7 +163,7 @@ class ExtensionAppModelBuilderTest : public ExtensionServiceTestBase { TEST_F(ExtensionAppModelBuilderTest, Build) { // The apps list would have 3 extension apps in the profile. - EXPECT_EQ(kDefaultAppCount, model_->item_list()->item_count()); + EXPECT_EQ(kDefaultAppCount, model_->top_level_item_list()->item_count()); EXPECT_EQ(std::string(kDefaultApps), GetModelContent(model_.get())); } @@ -359,6 +359,6 @@ TEST_F(ExtensionAppModelBuilderTest, BookmarkApp) { EXPECT_TRUE(err.empty()); service_->AddExtension(bookmark_app.get()); - EXPECT_EQ(kDefaultAppCount + 1, model_->item_list()->item_count()); + EXPECT_EQ(kDefaultAppCount + 1, model_->top_level_item_list()->item_count()); EXPECT_NE(std::string::npos, GetModelContent(model_.get()).find(kAppName)); } diff --git a/chrome/browser/ui/app_list/fast_show_pickler.cc b/chrome/browser/ui/app_list/fast_show_pickler.cc index 500ce01ff31e62..132f82ed027ce2 100644 --- a/chrome/browser/ui/app_list/fast_show_pickler.cc +++ b/chrome/browser/ui/app_list/fast_show_pickler.cc @@ -201,19 +201,21 @@ scoped_ptr FastShowPickler::PickleAppListModelForFastShow( scoped_ptr result(new Pickle); if (!result->WriteInt(kVersion)) return scoped_ptr(); - if (!result->WriteInt((int) model->item_list()->item_count())) + if (!result->WriteInt((int)model->top_level_item_list()->item_count())) return scoped_ptr(); - for (size_t i = 0; i < model->item_list()->item_count(); ++i) { - if (!PickleAppListItem(result.get(), model->item_list()->item_at(i))) + for (size_t i = 0; i < model->top_level_item_list()->item_count(); ++i) { + if (!PickleAppListItem(result.get(), + model->top_level_item_list()->item_at(i))) { return scoped_ptr(); + } } return result.Pass(); } void FastShowPickler::CopyOver(AppListModel* src, AppListModel* dest) { - DCHECK_EQ(0u, dest->item_list()->item_count()); - for (size_t i = 0; i < src->item_list()->item_count(); i++) { - AppListItem* src_item = src->item_list()->item_at(i); + DCHECK_EQ(0u, dest->top_level_item_list()->item_count()); + for (size_t i = 0; i < src->top_level_item_list()->item_count(); i++) { + AppListItem* src_item = src->top_level_item_list()->item_at(i); scoped_ptr dest_item(new AppListItem(src_item->id())); CopyOverItem(src_item, dest_item.get()); dest->AddItemToFolder(dest_item.Pass(), src_item->folder_id()); diff --git a/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc b/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc index de84eee8a834b2..5451ba11fe2f3f 100644 --- a/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc +++ b/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc @@ -18,16 +18,17 @@ using app_list::AppListModel; class AppListModelPicklerUnitTest : public testing::Test { protected: void CheckIsSame(AppListModel* m1, AppListModel* m2) { - ASSERT_EQ(m1->item_list()->item_count(), m2->item_list()->item_count()); - for (size_t i = 0; i < m1->item_list()->item_count(); i++) { - ASSERT_EQ(m1->item_list()->item_at(i)->id(), - m2->item_list()->item_at(i)->id()); - ASSERT_EQ(m1->item_list()->item_at(i)->name(), - m2->item_list()->item_at(i)->name()); - ASSERT_EQ(m1->item_list()->item_at(i)->short_name(), - m2->item_list()->item_at(i)->short_name()); - CompareImages(m1->item_list()->item_at(i)->icon(), - m2->item_list()->item_at(i)->icon()); + ASSERT_EQ(m1->top_level_item_list()->item_count(), + m2->top_level_item_list()->item_count()); + for (size_t i = 0; i < m1->top_level_item_list()->item_count(); i++) { + ASSERT_EQ(m1->top_level_item_list()->item_at(i)->id(), + m2->top_level_item_list()->item_at(i)->id()); + ASSERT_EQ(m1->top_level_item_list()->item_at(i)->name(), + m2->top_level_item_list()->item_at(i)->name()); + ASSERT_EQ(m1->top_level_item_list()->item_at(i)->short_name(), + m2->top_level_item_list()->item_at(i)->short_name()); + CompareImages(m1->top_level_item_list()->item_at(i)->icon(), + m2->top_level_item_list()->item_at(i)->icon()); } } diff --git a/ui/app_list/app_list_folder_item.cc b/ui/app_list/app_list_folder_item.cc index ecdfdd0500518c..fd77e519ee9db5 100644 --- a/ui/app_list/app_list_folder_item.cc +++ b/ui/app_list/app_list_folder_item.cc @@ -178,6 +178,11 @@ size_t AppListFolderItem::ChildItemCount() const { return item_list_->item_count(); } +void AppListFolderItem::OnExtensionPreferenceChanged() { + for (size_t i = 0; i < item_list_->item_count(); ++i) + item_list_->item_at(i)->OnExtensionPreferenceChanged(); +} + bool AppListFolderItem::CompareForTest(const AppListItem* other) const { if (!AppListItem::CompareForTest(other)) return false; diff --git a/ui/app_list/app_list_folder_item.h b/ui/app_list/app_list_folder_item.h index 4fa20cfa17155c..b294e94a3cefb6 100644 --- a/ui/app_list/app_list_folder_item.h +++ b/ui/app_list/app_list_folder_item.h @@ -54,6 +54,7 @@ class APP_LIST_EXPORT AppListFolderItem : public AppListItem, virtual ui::MenuModel* GetContextMenuModel() OVERRIDE; virtual AppListItem* FindChildItem(const std::string& id) OVERRIDE; virtual size_t ChildItemCount() const OVERRIDE; + virtual void OnExtensionPreferenceChanged() OVERRIDE; virtual bool CompareForTest(const AppListItem* other) const OVERRIDE; // Calculates the top item icons' bounds inside |folder_icon_bounds|. diff --git a/ui/app_list/app_list_item.cc b/ui/app_list/app_list_item.cc index 2dc9a866b46ad4..cbb6f864e013c6 100644 --- a/ui/app_list/app_list_item.cc +++ b/ui/app_list/app_list_item.cc @@ -84,6 +84,8 @@ size_t AppListItem::ChildItemCount() const { return 0; } +void AppListItem::OnExtensionPreferenceChanged() {} + bool AppListItem::CompareForTest(const AppListItem* other) const { return id_ == other->id_ && folder_id_ == other->folder_id_ && diff --git a/ui/app_list/app_list_item.h b/ui/app_list/app_list_item.h index 5f4d4672fef227..dd92b95a3ebee3 100644 --- a/ui/app_list/app_list_item.h +++ b/ui/app_list/app_list_item.h @@ -82,6 +82,10 @@ class APP_LIST_EXPORT AppListItem { // Returns the number of child items if it has any (e.g. is a folder) or 0. virtual size_t ChildItemCount() const; + // Called when the extension preference changed. Used by ExtensionAppItem + // to update icon overlays. + virtual void OnExtensionPreferenceChanged(); + // Utility functions for sync integration tests. virtual bool CompareForTest(const AppListItem* other) const; virtual std::string ToDebugString() const; diff --git a/ui/app_list/app_list_item_list.cc b/ui/app_list/app_list_item_list.cc index 3ccff5e6b31ac1..55c6754f707c5c 100644 --- a/ui/app_list/app_list_item_list.cc +++ b/ui/app_list/app_list_item_list.cc @@ -49,33 +49,39 @@ void AppListItemList::MoveItem(size_t from_index, size_t to_index) { return; AppListItem* target_item = app_list_items_[from_index]; + DVLOG(2) << "MoveItem: " << from_index << " -> " << to_index << " [" + << target_item->position().ToDebugString() << "]"; + + // Remove the target item app_list_items_.weak_erase(app_list_items_.begin() + from_index); - app_list_items_.insert(app_list_items_.begin() + to_index, target_item); - // Update position + // Update the position AppListItem* prev = to_index > 0 ? app_list_items_[to_index - 1] : NULL; - AppListItem* next = to_index < app_list_items_.size() - 1 ? - app_list_items_[to_index + 1] : NULL; + AppListItem* next = + to_index < item_count() ? app_list_items_[to_index] : NULL; CHECK_NE(prev, next); - - // It is possible that items were added with the same ordinal. Rather than - // resolving a potentially complicated chain of conflicts, just set the - // ordinal before |next| (which will place it before both items). - if (prev && next && prev->position().Equals(next->position())) - prev = NULL; - syncer::StringOrdinal new_position; - if (!prev) + if (!prev) { new_position = next->position().CreateBefore(); - else if (!next) + } else if (!next) { new_position = prev->position().CreateAfter(); - else + } else { + // It is possible that items were added with the same ordinal. To + // successfully move the item we need to fix this. We do not try to fix this + // when an item is added in order to avoid possible edge cases with sync. + if (prev->position().Equals(next->position())) + FixItemPosition(to_index); new_position = prev->position().CreateBetween(next->position()); - VLOG(2) << "Move: " << target_item->position().ToDebugString() - << " Prev: " << (prev ? prev->position().ToDebugString() : "(none)") - << " Next: " << (next ? next->position().ToDebugString() : "(none)") - << " -> " << new_position.ToDebugString(); + } target_item->set_position(new_position); + + DVLOG(2) << "Move: " + << " Prev: " << (prev ? prev->position().ToDebugString() : "(none)") + << " Next: " << (next ? next->position().ToDebugString() : "(none)") + << " -> " << new_position.ToDebugString(); + + // Insert the item and notify observers. + app_list_items_.insert(app_list_items_.begin() + to_index, target_item); FOR_EACH_OBSERVER(AppListItemListObserver, observers_, OnListItemMoved(from_index, to_index, target_item)); @@ -95,16 +101,16 @@ void AppListItemList::SetItemPosition( // the position. size_t to_index = GetItemSortOrderIndex(new_position, item->id()); if (to_index == from_index) { - VLOG(2) << "SetItemPosition: No change: " << item->id().substr(0, 8); + DVLOG(2) << "SetItemPosition: No change: " << item->id().substr(0, 8); item->set_position(new_position); return; } // Remove the item and get the updated to index. app_list_items_.weak_erase(app_list_items_.begin() + from_index); to_index = GetItemSortOrderIndex(new_position, item->id()); - VLOG(2) << "SetItemPosition: " << item->id().substr(0, 8) - << " -> " << new_position.ToDebugString() - << " From: " << from_index << " To: " << to_index; + DVLOG(2) << "SetItemPosition: " << item->id().substr(0, 8) << " -> " + << new_position.ToDebugString() << " From: " << from_index + << " To: " << to_index; item->set_position(new_position); app_list_items_.insert(app_list_items_.begin() + to_index, item); FOR_EACH_OBSERVER(AppListItemListObserver, @@ -206,4 +212,31 @@ size_t AppListItemList::GetItemSortOrderIndex( return app_list_items_.size(); } +void AppListItemList::FixItemPosition(size_t index) { + DVLOG(1) << "FixItemPosition: " << index; + size_t nitems = item_count(); + DCHECK_LT(index, nitems); + DCHECK_GT(index, 0u); + // Update the position of |index| and any necessary subsequent items. + // First, find the next item that has a different position. + AppListItem* prev = app_list_items_[index - 1]; + size_t last_index = index + 1; + for (; last_index < nitems; ++last_index) { + if (!app_list_items_[last_index]->position().Equals(prev->position())) + break; + } + AppListItem* last = last_index < nitems ? app_list_items_[last_index] : NULL; + for (size_t i = index; i < last_index; ++i) { + AppListItem* cur = app_list_items_[i]; + if (last) + cur->set_position(prev->position().CreateBetween(last->position())); + else + cur->set_position(prev->position().CreateAfter()); + prev = cur; + } + FOR_EACH_OBSERVER(AppListItemListObserver, + observers_, + OnListItemMoved(index, index, app_list_items_[index])); +} + } // namespace app_list diff --git a/ui/app_list/app_list_item_list.h b/ui/app_list/app_list_item_list.h index 95aa342c0c934d..7490c44d5caabf 100644 --- a/ui/app_list/app_list_item_list.h +++ b/ui/app_list/app_list_item_list.h @@ -30,6 +30,8 @@ class APP_LIST_EXPORT AppListItemList { void RemoveObserver(AppListItemListObserver* observer); // Finds item matching |id|. NOTE: Requires a linear search. + // Generally this should not be used directly, AppListModel::FindItem + // should be used instead. AppListItem* FindItem(const std::string& id); // Finds the |index| of the the item matching |id| in |app_list_items_|. @@ -95,6 +97,10 @@ class APP_LIST_EXPORT AppListItemList { size_t GetItemSortOrderIndex(const syncer::StringOrdinal& position, const std::string& id); + // Fixes the position of the item at |index| when the position matches the + // previous item's position. |index| must be > 0. + void FixItemPosition(size_t index); + ScopedVector app_list_items_; ObserverList observers_; diff --git a/ui/app_list/app_list_item_list_observer.h b/ui/app_list/app_list_item_list_observer.h index fe8aa24a474b18..640be28a6fff51 100644 --- a/ui/app_list/app_list_item_list_observer.h +++ b/ui/app_list/app_list_item_list_observer.h @@ -22,6 +22,7 @@ class APP_LIST_EXPORT AppListItemListObserver { virtual void OnListItemRemoved(size_t index, AppListItem* item) {} // Triggered after |item| has been moved from |from_index| to |to_index|. + // Note: |from_index| may equal |to_index| if only the ordinal has changed. virtual void OnListItemMoved(size_t from_index, size_t to_index, AppListItem* item) {} diff --git a/ui/app_list/app_list_item_list_unittest.cc b/ui/app_list/app_list_item_list_unittest.cc index 8a4b4c93427c6a..93e51518d68f2d 100644 --- a/ui/app_list/app_list_item_list_unittest.cc +++ b/ui/app_list/app_list_item_list_unittest.cc @@ -17,10 +17,7 @@ namespace { class TestObserver : public AppListItemListObserver { public: - TestObserver() - : items_added_(0), - items_removed_(0) { - } + TestObserver() : items_added_(0), items_removed_(0), items_moved_(0) {} virtual ~TestObserver() { } @@ -34,12 +31,26 @@ class TestObserver : public AppListItemListObserver { ++items_removed_; } + virtual void OnListItemMoved(size_t from_index, + size_t to_index, + AppListItem* item) OVERRIDE { + ++items_moved_; + } + size_t items_added() const { return items_added_; } size_t items_removed() const { return items_removed_; } + size_t items_moved() const { return items_moved_; } + + void ResetCounts() { + items_added_ = 0; + items_removed_ = 0; + items_moved_ = 0; + } private: size_t items_added_; size_t items_removed_; + size_t items_moved_; DISALLOW_COPY_AND_ASSIGN(TestObserver); }; @@ -65,6 +76,14 @@ class AppListItemListTest : public testing::Test { } protected: + AppListItem* FindItem(const std::string& id) { + return item_list_.FindItem(id); + } + + bool FindItemIndex(const std::string& id, size_t* index) { + return item_list_.FindItemIndex(id, index); + } + scoped_ptr CreateItem(const std::string& name) { scoped_ptr item(new AppListItem(name)); size_t nitems = item_list_.item_count(); @@ -141,15 +160,15 @@ TEST_F(AppListItemListTest, FindItemIndex) { EXPECT_TRUE(VerifyItemListOrdinals()); size_t index; - EXPECT_TRUE(item_list_.FindItemIndex(item_0->id(), &index)); + EXPECT_TRUE(FindItemIndex(item_0->id(), &index)); EXPECT_EQ(index, 0u); - EXPECT_TRUE(item_list_.FindItemIndex(item_1->id(), &index)); + EXPECT_TRUE(FindItemIndex(item_1->id(), &index)); EXPECT_EQ(index, 1u); - EXPECT_TRUE(item_list_.FindItemIndex(item_2->id(), &index)); + EXPECT_TRUE(FindItemIndex(item_2->id(), &index)); EXPECT_EQ(index, 2u); scoped_ptr item_3(CreateItem(GetItemId(3))); - EXPECT_FALSE(item_list_.FindItemIndex(item_3->id(), &index)); + EXPECT_FALSE(FindItemIndex(item_3->id(), &index)); } TEST_F(AppListItemListTest, RemoveItemAt) { @@ -159,13 +178,13 @@ TEST_F(AppListItemListTest, RemoveItemAt) { EXPECT_EQ(item_list_.item_count(), 3u); EXPECT_EQ(observer_.items_added(), 3u); size_t index; - EXPECT_TRUE(item_list_.FindItemIndex(item_1->id(), &index)); + EXPECT_TRUE(FindItemIndex(item_1->id(), &index)); EXPECT_EQ(index, 1u); EXPECT_TRUE(VerifyItemListOrdinals()); scoped_ptr item_removed = RemoveItemAt(1); EXPECT_EQ(item_removed, item_1); - EXPECT_FALSE(item_list_.FindItem(item_1->id())); + EXPECT_FALSE(FindItem(item_1->id())); EXPECT_EQ(item_list_.item_count(), 2u); EXPECT_EQ(observer_.items_removed(), 1u); EXPECT_EQ(item_list_.item_at(0), item_0); @@ -185,12 +204,12 @@ TEST_F(AppListItemListTest, RemoveItem) { EXPECT_TRUE(VerifyItemListOrdinals()); size_t index; - EXPECT_TRUE(item_list_.FindItemIndex(item_1->id(), &index)); + EXPECT_TRUE(FindItemIndex(item_1->id(), &index)); EXPECT_EQ(index, 1u); scoped_ptr item_removed = RemoveItem(item_1->id()); EXPECT_EQ(item_removed, item_1); - EXPECT_FALSE(item_list_.FindItem(item_1->id())); + EXPECT_FALSE(FindItem(item_1->id())); EXPECT_EQ(item_list_.item_count(), 2u); EXPECT_EQ(observer_.items_removed(), 1u); EXPECT_TRUE(VerifyItemListOrdinals()); @@ -204,27 +223,85 @@ TEST_F(AppListItemListTest, MoveItem) { CreateAndAddItem(GetItemId(1)); CreateAndAddItem(GetItemId(2)); CreateAndAddItem(GetItemId(3)); + + EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3)); + + item_list_.MoveItem(0, 0); + EXPECT_EQ(0u, observer_.items_moved()); EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3)); item_list_.MoveItem(0, 1); + EXPECT_EQ(1u, observer_.items_moved()); EXPECT_TRUE(VerifyItemListOrdinals()); EXPECT_TRUE(VerifyItemOrder4(1, 0, 2, 3)); item_list_.MoveItem(1, 2); + EXPECT_EQ(2u, observer_.items_moved()); EXPECT_TRUE(VerifyItemListOrdinals()); EXPECT_TRUE(VerifyItemOrder4(1, 2, 0, 3)); - item_list_.MoveItem(2, 3); + item_list_.MoveItem(2, 1); + EXPECT_EQ(3u, observer_.items_moved()); EXPECT_TRUE(VerifyItemListOrdinals()); - EXPECT_TRUE(VerifyItemOrder4(1, 2, 3, 0)); + EXPECT_TRUE(VerifyItemOrder4(1, 0, 2, 3)); item_list_.MoveItem(3, 0); + EXPECT_EQ(4u, observer_.items_moved()); EXPECT_TRUE(VerifyItemListOrdinals()); - EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3)); + EXPECT_TRUE(VerifyItemOrder4(3, 1, 0, 2)); item_list_.MoveItem(0, 3); + EXPECT_EQ(5u, observer_.items_moved()); EXPECT_TRUE(VerifyItemListOrdinals()); - EXPECT_TRUE(VerifyItemOrder4(1, 2, 3, 0)); + EXPECT_TRUE(VerifyItemOrder4(1, 0, 2, 3)); +} + +TEST_F(AppListItemListTest, SamePosition) { + CreateAndAddItem(GetItemId(0)); + CreateAndAddItem(GetItemId(1)); + CreateAndAddItem(GetItemId(2)); + CreateAndAddItem(GetItemId(3)); + item_list_.SetItemPosition(item_list_.item_at(2), + item_list_.item_at(1)->position()); + EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3)); + EXPECT_TRUE(item_list_.item_at(1)->position().Equals( + item_list_.item_at(2)->position())); + // Moving an item to position 1 should fix the position. + observer_.ResetCounts(); + item_list_.MoveItem(0, 1); + EXPECT_TRUE(VerifyItemOrder4(1, 0, 2, 3)); + EXPECT_TRUE(item_list_.item_at(0)->position().LessThan( + item_list_.item_at(1)->position())); + EXPECT_TRUE(item_list_.item_at(1)->position().LessThan( + item_list_.item_at(2)->position())); + EXPECT_TRUE(item_list_.item_at(2)->position().LessThan( + item_list_.item_at(3)->position())); + // One extra move notification for fixed position. + EXPECT_EQ(2u, observer_.items_moved()); + + // Restore the original order. + item_list_.MoveItem(1, 0); + EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3)); + + // Set all four items to the same position. + item_list_.SetItemPosition(item_list_.item_at(1), + item_list_.item_at(0)->position()); + item_list_.SetItemPosition(item_list_.item_at(2), + item_list_.item_at(0)->position()); + item_list_.SetItemPosition(item_list_.item_at(3), + item_list_.item_at(0)->position()); + // Move should fix the position of the items. + observer_.ResetCounts(); + item_list_.MoveItem(0, 1); + EXPECT_TRUE(VerifyItemOrder4(1, 0, 2, 3)); + EXPECT_TRUE(item_list_.item_at(0)->position().LessThan( + item_list_.item_at(1)->position())); + EXPECT_TRUE(item_list_.item_at(1)->position().LessThan( + item_list_.item_at(2)->position())); + EXPECT_TRUE(item_list_.item_at(2)->position().LessThan( + item_list_.item_at(3)->position())); + // One extra move notification for fixed position. + EXPECT_EQ(2u, observer_.items_moved()); } TEST_F(AppListItemListTest, CreatePositionBefore) { diff --git a/ui/app_list/app_list_model.cc b/ui/app_list/app_list_model.cc index 5cdd6803316a6a..e79737e18bbb2a 100644 --- a/ui/app_list/app_list_model.cc +++ b/ui/app_list/app_list_model.cc @@ -15,16 +15,14 @@ namespace app_list { AppListModel::AppListModel() - : item_list_(new AppListItemList), + : top_level_item_list_(new AppListItemList), search_box_(new SearchBoxModel), results_(new SearchResults), status_(STATUS_NORMAL) { - item_list_->AddObserver(this); + top_level_item_list_->AddObserver(this); } -AppListModel::~AppListModel() { - item_list_->RemoveObserver(this); -} +AppListModel::~AppListModel() { top_level_item_list_->RemoveObserver(this); } void AppListModel::AddObserver(AppListModelObserver* observer) { observers_.AddObserver(observer); @@ -45,11 +43,12 @@ void AppListModel::SetStatus(Status status) { } AppListItem* AppListModel::FindItem(const std::string& id) { - AppListItem* item = item_list_->FindItem(id); + AppListItem* item = top_level_item_list_->FindItem(id); if (item) return item; - for (size_t i = 0; i < item_list_->item_count(); ++i) { - AppListItem* child_item = item_list_->item_at(i)->FindChildItem(id); + for (size_t i = 0; i < top_level_item_list_->item_count(); ++i) { + AppListItem* child_item = + top_level_item_list_->item_at(i)->FindChildItem(id); if (child_item) return child_item; } @@ -57,7 +56,7 @@ AppListItem* AppListModel::FindItem(const std::string& id) { } AppListFolderItem* AppListModel::FindFolderItem(const std::string& id) { - AppListItem* item = item_list_->FindItem(id); + AppListItem* item = top_level_item_list_->FindItem(id); if (item && item->GetItemType() == AppListFolderItem::kItemType) return static_cast(item); DCHECK(!item); @@ -66,7 +65,7 @@ AppListFolderItem* AppListModel::FindFolderItem(const std::string& id) { AppListItem* AppListModel::AddItem(scoped_ptr item) { DCHECK(!item->IsInFolder()); - DCHECK(!item_list()->FindItem(item->id())); + DCHECK(!top_level_item_list()->FindItem(item->id())); return AddItemToItemListAndNotify(item.Pass()); } @@ -104,14 +103,15 @@ const std::string& AppListModel::MergeItems(const std::string& target_item_id, return target_folder->id(); } - // Otherwise, remove the target item from |item_list_|, it will become owned - // by the new folder. + // Otherwise, remove the target item from |top_level_item_list_|, it will + // become owned by the new folder. scoped_ptr target_item_ptr = - item_list_->RemoveItem(target_item_id); + top_level_item_list_->RemoveItem(target_item_id); // Create a new folder in the same location as the target item. - scoped_ptr new_folder_ptr( - new AppListFolderItem(AppListFolderItem::GenerateId())); + std::string new_folder_id = AppListFolderItem::GenerateId(); + DVLOG(2) << "Creating folder for merge: " << new_folder_id; + scoped_ptr new_folder_ptr(new AppListFolderItem(new_folder_id)); new_folder_ptr->set_position(target_item->position()); AppListFolderItem* new_folder = static_cast( AddItemToItemListAndNotify(new_folder_ptr.Pass())); @@ -158,7 +158,8 @@ void AppListModel::MoveItemToFolderAt(AppListItem* item, dest_folder->item_list()->CreatePositionBefore(position)); AddItemToFolderItemAndNotify(dest_folder, item_ptr.Pass()); } else { - item_ptr->set_position(item_list_->CreatePositionBefore(position)); + item_ptr->set_position( + top_level_item_list_->CreatePositionBefore(position)); AddItemToItemListAndNotifyUpdate(item_ptr.Pass()); } } @@ -166,7 +167,7 @@ void AppListModel::MoveItemToFolderAt(AppListItem* item, void AppListModel::SetItemPosition(AppListItem* item, const syncer::StringOrdinal& new_position) { if (!item->IsInFolder()) { - item_list_->SetItemPosition(item, new_position); + top_level_item_list_->SetItemPosition(item, new_position); // Note: this will trigger OnListItemMoved which will signal observers. // (This is done this way because some View code still moves items within // the item list directly). @@ -209,7 +210,7 @@ void AppListModel::DeleteItem(const std::string& id) { FOR_EACH_OBSERVER(AppListModelObserver, observers_, OnAppListItemWillBeDeleted(item)); - item_list_->DeleteItem(id); + top_level_item_list_->DeleteItem(id); return; } AppListFolderItem* folder = FindFolderItem(item->folder_id()); @@ -222,6 +223,11 @@ void AppListModel::DeleteItem(const std::string& id) { child_item.reset(); // Deletes item. } +void AppListModel::NotifyExtensionPreferenceChanged() { + for (size_t i = 0; i < top_level_item_list_->item_count(); ++i) + top_level_item_list_->item_at(i)->OnExtensionPreferenceChanged(); +} + // Private methods void AppListModel::OnListItemMoved(size_t from_index, @@ -241,9 +247,10 @@ AppListFolderItem* AppListModel::FindOrCreateFolderItem( if (dest_folder) return dest_folder; + DVLOG(2) << "Creating new folder: " << folder_id; scoped_ptr new_folder(new AppListFolderItem(folder_id)); new_folder->set_position( - item_list_->CreatePositionBefore(syncer::StringOrdinal())); + top_level_item_list_->CreatePositionBefore(syncer::StringOrdinal())); AppListItem* new_folder_item = AddItemToItemListAndNotify(new_folder.PassAs()); return static_cast(new_folder_item); @@ -252,7 +259,7 @@ AppListFolderItem* AppListModel::FindOrCreateFolderItem( AppListItem* AppListModel::AddItemToItemListAndNotify( scoped_ptr item_ptr) { DCHECK(!item_ptr->IsInFolder()); - AppListItem* item = item_list_->AddItem(item_ptr.Pass()); + AppListItem* item = top_level_item_list_->AddItem(item_ptr.Pass()); FOR_EACH_OBSERVER(AppListModelObserver, observers_, OnAppListItemAdded(item)); @@ -262,7 +269,7 @@ AppListItem* AppListModel::AddItemToItemListAndNotify( AppListItem* AppListModel::AddItemToItemListAndNotifyUpdate( scoped_ptr item_ptr) { DCHECK(!item_ptr->IsInFolder()); - AppListItem* item = item_list_->AddItem(item_ptr.Pass()); + AppListItem* item = top_level_item_list_->AddItem(item_ptr.Pass()); FOR_EACH_OBSERVER(AppListModelObserver, observers_, OnAppListItemUpdated(item)); @@ -282,10 +289,9 @@ AppListItem* AppListModel::AddItemToFolderItemAndNotify( scoped_ptr AppListModel::RemoveItem(AppListItem* item) { if (!item->IsInFolder()) - return item_list_->RemoveItem(item->id()); + return top_level_item_list_->RemoveItem(item->id()); AppListFolderItem* folder = FindFolderItem(item->folder_id()); - DCHECK(folder); return RemoveItemFromFolder(folder, item); } diff --git a/ui/app_list/app_list_model.h b/ui/app_list/app_list_model.h index 76d4e357eed788..6d71e0b6a600a9 100644 --- a/ui/app_list/app_list_model.h +++ b/ui/app_list/app_list_model.h @@ -26,7 +26,7 @@ class SearchResult; // SearchBoxModel and SearchResults. The AppListItemList sub model owns a list // of AppListItems and is displayed in the grid view. SearchBoxModel is // the model for SearchBoxView. SearchResults owns a list of SearchResult. -// NOTE: Currently this class observes |item_list_|. The View code may +// NOTE: Currently this class observes |top_level_item_list_|. The View code may // move entries in the item list directly (but can not add or remove them) and // the model needs to notify its observers when this occurs. class APP_LIST_EXPORT AppListModel : public AppListItemListObserver { @@ -85,8 +85,8 @@ class APP_LIST_EXPORT AppListModel : public AppListItemListObserver { const std::string& folder_id, syncer::StringOrdinal position); - // Sets the position of |item| either in |item_list_| or the folder specified - // by |item|->folder_id(). + // Sets the position of |item| either in |top_level_item_list_| or the folder + // specified by |item|->folder_id(). void SetItemPosition(AppListItem* item, const syncer::StringOrdinal& new_position); @@ -98,10 +98,15 @@ class APP_LIST_EXPORT AppListModel : public AppListItemListObserver { const std::string& name, const std::string& short_name); - // Deletes the item matching |id| from |item_list_| or from its folder. + // Deletes the item matching |id| from |top_level_item_list_| or from the + // appropriate folder. void DeleteItem(const std::string& id); - AppListItemList* item_list() { return item_list_.get(); } + // Call OnExtensionPreferenceChanged() for all items in the model. + void NotifyExtensionPreferenceChanged(); + + AppListItemList* top_level_item_list() { return top_level_item_list_.get(); } + SearchBoxModel* search_box() { return search_box_.get(); } SearchResults* results() { return results_.get(); } Status status() const { return status_; } @@ -115,12 +120,12 @@ class APP_LIST_EXPORT AppListModel : public AppListItemListObserver { // Returns an existing folder matching |folder_id| or creates a new folder. AppListFolderItem* FindOrCreateFolderItem(const std::string& folder_id); - // Adds |item_ptr| to |item_list_| and notifies observers. + // Adds |item_ptr| to |top_level_item_list_| and notifies observers. AppListItem* AddItemToItemListAndNotify( scoped_ptr item_ptr); - // Adds |item_ptr| to |item_list_| and notifies observers that an Update - // occured (e.g. item moved from a folder). + // Adds |item_ptr| to |top_level_item_list_| and notifies observers that an + // Update occured (e.g. item moved from a folder). AppListItem* AddItemToItemListAndNotifyUpdate( scoped_ptr item_ptr); @@ -128,16 +133,18 @@ class APP_LIST_EXPORT AppListModel : public AppListItemListObserver { AppListItem* AddItemToFolderItemAndNotify(AppListFolderItem* folder, scoped_ptr item_ptr); - // Removes |item| from |item_list_| or calls RemoveItemFromFolder if + // Removes |item| from |top_level_item_list_| or calls RemoveItemFromFolder if // |item|->folder_id is set. scoped_ptr RemoveItem(AppListItem* item); // Removes |item| from |folder|. If |folder| becomes empty, deletes |folder| - // from |item_list_|. Does NOT trigger observers, calling function must do so. + // from |top_level_item_list_|. Does NOT trigger observers, calling function + // must do so. scoped_ptr RemoveItemFromFolder(AppListFolderItem* folder, AppListItem* item); - scoped_ptr item_list_; + scoped_ptr top_level_item_list_; + scoped_ptr search_box_; scoped_ptr results_; diff --git a/ui/app_list/app_list_model_unittest.cc b/ui/app_list/app_list_model_unittest.cc index 629f5eb14603ce..4d16f1666d6b91 100644 --- a/ui/app_list/app_list_model_unittest.cc +++ b/ui/app_list/app_list_model_unittest.cc @@ -102,7 +102,7 @@ class AppListModelTest : public testing::Test { } std::string GetModelContents() { - return GetItemListContents(model_.item_list()); + return GetItemListContents(model_.top_level_item_list()); } test::AppListTestModel model_; @@ -133,10 +133,10 @@ TEST_F(AppListModelTest, AppsObserver) { TEST_F(AppListModelTest, ModelGetItem) { const size_t num_apps = 2; model_.PopulateApps(num_apps); - AppListItem* item0 = model_.item_list()->item_at(0); + AppListItem* item0 = model_.top_level_item_list()->item_at(0); ASSERT_TRUE(item0); EXPECT_EQ(model_.GetItemName(0), item0->id()); - AppListItem* item1 = model_.item_list()->item_at(1); + AppListItem* item1 = model_.top_level_item_list()->item_at(1); ASSERT_TRUE(item1); EXPECT_EQ(model_.GetItemName(1), item1->id()); } @@ -159,19 +159,20 @@ TEST_F(AppListModelTest, SetItemPosition) { model_.PopulateApps(num_apps); // Adding another item will add it to the end. model_.CreateAndAddItem("Added Item 1"); - ASSERT_EQ(num_apps + 1, model_.item_list()->item_count()); - EXPECT_EQ("Added Item 1", model_.item_list()->item_at(num_apps)->id()); + ASSERT_EQ(num_apps + 1, model_.top_level_item_list()->item_count()); + EXPECT_EQ("Added Item 1", + model_.top_level_item_list()->item_at(num_apps)->id()); // Add an item between items 0 and 1. - AppListItem* item0 = model_.item_list()->item_at(0); + AppListItem* item0 = model_.top_level_item_list()->item_at(0); ASSERT_TRUE(item0); - AppListItem* item1 = model_.item_list()->item_at(1); + AppListItem* item1 = model_.top_level_item_list()->item_at(1); ASSERT_TRUE(item1); AppListItem* item2 = model_.CreateItem("Added Item 2"); model_.AddItem(item2); EXPECT_EQ("Item 0,Item 1,Added Item 1,Added Item 2", GetModelContents()); model_.SetItemPosition( item2, item0->position().CreateBetween(item1->position())); - EXPECT_EQ(num_apps + 2, model_.item_list()->item_count()); + EXPECT_EQ(num_apps + 2, model_.top_level_item_list()->item_count()); EXPECT_EQ(num_apps + 2, observer_.items_added()); EXPECT_EQ("Item 0,Added Item 2,Item 1,Added Item 1", GetModelContents()); } @@ -181,10 +182,10 @@ TEST_F(AppListModelTest, ModelMoveItem) { model_.PopulateApps(num_apps); // Adding another item will add it to the end. model_.CreateAndAddItem("Inserted Item"); - ASSERT_EQ(num_apps + 1, model_.item_list()->item_count()); + ASSERT_EQ(num_apps + 1, model_.top_level_item_list()->item_count()); // Move it to the position 1. observer_.ResetCounts(); - model_.item_list()->MoveItem(num_apps, 1); + model_.top_level_item_list()->MoveItem(num_apps, 1); EXPECT_EQ(1u, observer_.items_updated()); EXPECT_EQ("Item 0,Inserted Item,Item 1,Item 2", GetModelContents()); } @@ -194,17 +195,17 @@ TEST_F(AppListModelTest, ModelRemoveItem) { model_.PopulateApps(num_apps); // Remove an item in the middle. model_.DeleteItem(model_.GetItemName(1)); - EXPECT_EQ(num_apps - 1, model_.item_list()->item_count()); + EXPECT_EQ(num_apps - 1, model_.top_level_item_list()->item_count()); EXPECT_EQ(1u, observer_.items_removed()); EXPECT_EQ("Item 0,Item 2,Item 3", GetModelContents()); // Remove the first item in the list. model_.DeleteItem(model_.GetItemName(0)); - EXPECT_EQ(num_apps - 2, model_.item_list()->item_count()); + EXPECT_EQ(num_apps - 2, model_.top_level_item_list()->item_count()); EXPECT_EQ(2u, observer_.items_removed()); EXPECT_EQ("Item 2,Item 3", GetModelContents()); // Remove the last item in the list. model_.DeleteItem(model_.GetItemName(num_apps - 1)); - EXPECT_EQ(num_apps - 3, model_.item_list()->item_count()); + EXPECT_EQ(num_apps - 3, model_.top_level_item_list()->item_count()); EXPECT_EQ(3u, observer_.items_removed()); EXPECT_EQ("Item 2", GetModelContents()); } @@ -214,15 +215,17 @@ TEST_F(AppListModelTest, AppOrder) { model_.PopulateApps(num_apps); // Ensure order is preserved. for (size_t i = 1; i < num_apps; ++i) { - EXPECT_TRUE(model_.item_list()->item_at(i)->position().GreaterThan( - model_.item_list()->item_at(i - 1)->position())); + EXPECT_TRUE( + model_.top_level_item_list()->item_at(i)->position().GreaterThan( + model_.top_level_item_list()->item_at(i - 1)->position())); } // Move an app - model_.item_list()->MoveItem(num_apps - 1, 1); + model_.top_level_item_list()->MoveItem(num_apps - 1, 1); // Ensure order is preserved. for (size_t i = 1; i < num_apps; ++i) { - EXPECT_TRUE(model_.item_list()->item_at(i)->position().GreaterThan( - model_.item_list()->item_at(i - 1)->position())); + EXPECT_TRUE( + model_.top_level_item_list()->item_at(i)->position().GreaterThan( + model_.top_level_item_list()->item_at(i - 1)->position())); } } @@ -281,21 +284,21 @@ TEST_F(AppListModelFolderTest, FolderItem) { TEST_F(AppListModelFolderTest, MergeItems) { model_.PopulateApps(3); - ASSERT_EQ(3u, model_.item_list()->item_count()); - AppListItem* item0 = model_.item_list()->item_at(0); - AppListItem* item1 = model_.item_list()->item_at(1); - AppListItem* item2 = model_.item_list()->item_at(2); + ASSERT_EQ(3u, model_.top_level_item_list()->item_count()); + AppListItem* item0 = model_.top_level_item_list()->item_at(0); + AppListItem* item1 = model_.top_level_item_list()->item_at(1); + AppListItem* item2 = model_.top_level_item_list()->item_at(2); // Merge two items. std::string folder1_id = model_.MergeItems(item0->id(), item1->id()); - ASSERT_EQ(2u, model_.item_list()->item_count()); // Folder + 1 item + ASSERT_EQ(2u, model_.top_level_item_list()->item_count()); // Folder + 1 item AppListFolderItem* folder1_item = model_.FindFolderItem(folder1_id); ASSERT_TRUE(folder1_item); EXPECT_EQ("Item 0,Item 1", GetItemListContents(folder1_item->item_list())); // Merge an item from the new folder into the third item. std::string folder2_id = model_.MergeItems(item2->id(), item1->id()); - ASSERT_EQ(2u, model_.item_list()->item_count()); // 2 folders + ASSERT_EQ(2u, model_.top_level_item_list()->item_count()); // 2 folders AppListFolderItem* folder2_item = model_.FindFolderItem(folder2_id); EXPECT_EQ("Item 0", GetItemListContents(folder1_item->item_list())); EXPECT_EQ("Item 2,Item 1", GetItemListContents(folder2_item->item_list())); @@ -316,7 +319,7 @@ TEST_F(AppListModelFolderTest, AddItemToFolder) { model_.AddItem(folder); AppListItem* item0 = new AppListItem("Item 0"); model_.AddItemToFolder(item0, folder->id()); - ASSERT_EQ(1u, model_.item_list()->item_count()); + ASSERT_EQ(1u, model_.top_level_item_list()->item_count()); AppListFolderItem* folder_item = model_.FindFolderItem(folder->id()); ASSERT_TRUE(folder_item); ASSERT_EQ(1u, folder_item->item_list()->item_count()); @@ -331,7 +334,7 @@ TEST_F(AppListModelFolderTest, MoveItemToFolder) { AppListItem* item1 = new AppListItem("Item 1"); model_.AddItem(item0); model_.AddItem(item1); - ASSERT_EQ(3u, model_.item_list()->item_count()); + ASSERT_EQ(3u, model_.top_level_item_list()->item_count()); // Move item0 and item1 to folder. std::string folder_id = folder->id(); model_.MoveItemToFolder(item0, folder_id); @@ -360,15 +363,16 @@ TEST_F(AppListModelFolderTest, MoveItemToFolderAt) { model_.AddItem(new AppListFolderItem("folder1"))); model_.AddItem(new AppListItem("Item 2")); model_.AddItem(new AppListItem("Item 3")); - ASSERT_EQ(5u, model_.item_list()->item_count()); + ASSERT_EQ(5u, model_.top_level_item_list()->item_count()); EXPECT_EQ("Item 0,Item 1,folder1,Item 2,Item 3", GetModelContents()); // Move Item 1 to folder1, then Item 2 before Item 1. - model_.MoveItemToFolderAt( - model_.item_list()->item_at(1), folder1->id(), syncer::StringOrdinal()); + model_.MoveItemToFolderAt(model_.top_level_item_list()->item_at(1), + folder1->id(), + syncer::StringOrdinal()); EXPECT_EQ("Item 0,folder1,Item 2,Item 3", GetModelContents()); - model_.MoveItemToFolderAt( - model_.item_list()->item_at(2), folder1->id(), - folder1->item_list()->item_at(0)->position()); + model_.MoveItemToFolderAt(model_.top_level_item_list()->item_at(2), + folder1->id(), + folder1->item_list()->item_at(0)->position()); EXPECT_EQ("Item 2,Item 1", GetItemListContents(folder1->item_list())); EXPECT_EQ("Item 0,folder1,Item 3", GetModelContents()); // Move Item 2 out of folder to before folder. @@ -406,14 +410,14 @@ TEST_F(AppListModelFolderTest, MoveItemFromFolderToFolder) { // Move item1 from folder0 to folder1. folder0 should get deleted. model_.MoveItemToFolder(item1, folder1->id()); - ASSERT_EQ(1u, model_.item_list()->item_count()); + ASSERT_EQ(1u, model_.top_level_item_list()->item_count()); ASSERT_EQ(2u, folder1->item_list()->item_count()); EXPECT_EQ(folder1->id(), item1->folder_id()); EXPECT_EQ("Item 0,Item 1", GetItemListContents(folder1->item_list())); // Move item1 to a non-existant folder2 which should get created. model_.MoveItemToFolder(item1, "folder2"); - ASSERT_EQ(2u, model_.item_list()->item_count()); + ASSERT_EQ(2u, model_.top_level_item_list()->item_count()); ASSERT_EQ(1u, folder1->item_list()->item_count()); EXPECT_EQ("folder2", item1->folder_id()); AppListFolderItem* folder2 = model_.FindFolderItem("folder2"); diff --git a/ui/app_list/cocoa/apps_grid_controller.mm b/ui/app_list/cocoa/apps_grid_controller.mm index 998b11e93e66d5..38fec6fdc44616 100644 --- a/ui/app_list/cocoa/apps_grid_controller.mm +++ b/ui/app_list/cocoa/apps_grid_controller.mm @@ -191,7 +191,7 @@ - (void)setDelegate:(app_list::AppListViewDelegate*)newDelegate { if (delegate_) { app_list::AppListModel* oldModel = delegate_->GetModel(); if (oldModel) - oldModel->item_list()->RemoveObserver(bridge_.get()); + oldModel->top_level_item_list()->RemoveObserver(bridge_.get()); } // Since the old model may be getting deleted, and the AppKit objects might @@ -212,9 +212,10 @@ - (void)setDelegate:(app_list::AppListViewDelegate*)newDelegate { if (!newModel) return; - newModel->item_list()->AddObserver(bridge_.get()); - for (size_t i = 0; i < newModel->item_list()->item_count(); ++i) { - app_list::AppListItem* itemModel = newModel->item_list()->item_at(i); + newModel->top_level_item_list()->AddObserver(bridge_.get()); + for (size_t i = 0; i < newModel->top_level_item_list()->item_count(); ++i) { + app_list::AppListItem* itemModel = + newModel->top_level_item_list()->item_at(i); [items_ insertObject:[NSValue valueWithPointer:itemModel] atIndex:i]; } @@ -526,7 +527,7 @@ - (void)moveItemWithIndex:(size_t)itemIndex if (itemIndex == modelIndex) return; - app_list::AppListItemList* itemList = [self model]->item_list(); + app_list::AppListItemList* itemList = [self model]->top_level_item_list(); itemList->RemoveObserver(bridge_.get()); itemList->MoveItem(itemIndex, modelIndex); itemList->AddObserver(bridge_.get()); diff --git a/ui/app_list/cocoa/apps_grid_controller_unittest.mm b/ui/app_list/cocoa/apps_grid_controller_unittest.mm index 4f22f18e4856b2..13634d818b9938 100644 --- a/ui/app_list/cocoa/apps_grid_controller_unittest.mm +++ b/ui/app_list/cocoa/apps_grid_controller_unittest.mm @@ -448,7 +448,8 @@ void SetMenuReadyForTesting(bool ready) { EXPECT_EQ(std::string("|Item 0,Item 1,Item 2|"), GetViewContent()); // Update the title via the ItemModelObserver. - app_list::AppListItem* item_model = model()->item_list()->item_at(2); + app_list::AppListItem* item_model = + model()->top_level_item_list()->item_at(2); model()->SetItemName(item_model, "UpdatedItem"); EXPECT_NSEQ(@"UpdatedItem", [button title]); @@ -473,7 +474,7 @@ void SetMenuReadyForTesting(bool ready) { EXPECT_EQ(2u, [[GetPageAt(0) content] count]); EXPECT_EQ(std::string("|Item 0,Item 1|"), GetViewContent()); - app_list::AppListItemList* item_list = model()->item_list(); + app_list::AppListItemList* item_list = model()->top_level_item_list(); model()->CreateAndAddItem("Item 2"); ASSERT_EQ(3u, item_list->item_count()); @@ -497,7 +498,7 @@ void SetMenuReadyForTesting(bool ready) { EXPECT_EQ(std::string("|Item 0,Item 1,Item 2|"), GetViewContent()); // Test swapping items (e.g. rearranging via sync). - model()->item_list()->MoveItem(1, 2); + model()->top_level_item_list()->MoveItem(1, 2); EXPECT_EQ(std::string("|Item 0,Item 2,Item 1|"), GetViewContent()); } @@ -534,7 +535,7 @@ void SetMenuReadyForTesting(bool ready) { } TEST_F(AppsGridControllerTest, ModelRemovePage) { - app_list::AppListItemList* item_list = model()->item_list(); + app_list::AppListItemList* item_list = model()->top_level_item_list(); model()->PopulateApps(kItemsPerPage + 1); ASSERT_EQ(kItemsPerPage + 1, item_list->item_count()); @@ -555,7 +556,7 @@ void SetMenuReadyForTesting(bool ready) { EXPECT_EQ(2u, [apps_grid_controller_ pageCount]); EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]); app_list::AppListItem* item_model = - model()->item_list()->item_at(kItemsPerPage); + model()->top_level_item_list()->item_at(kItemsPerPage); // Highlighting an item should activate the page it is on. item_model->SetHighlighted(true); @@ -591,7 +592,7 @@ void SetMenuReadyForTesting(bool ready) { // Two things can be installing simultaneously. When one starts or completes // the model builder will ask for the item to be highlighted. app_list::AppListItem* alternate_item_model = - model()->item_list()->item_at(0); + model()->top_level_item_list()->item_at(0); item_model->SetHighlighted(false); alternate_item_model->SetHighlighted(true); EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]); diff --git a/ui/app_list/test/app_list_test_model.cc b/ui/app_list/test/app_list_test_model.cc index 357342acfd55c5..7a908127c40cab 100644 --- a/ui/app_list/test/app_list_test_model.cc +++ b/ui/app_list/test/app_list_test_model.cc @@ -64,7 +64,7 @@ std::string AppListTestModel::GetItemName(int id) { } void AppListTestModel::PopulateApps(int n) { - int start_index = static_cast(item_list()->item_count()); + int start_index = static_cast(top_level_item_list()->item_count()); for (int i = 0; i < n; ++i) CreateAndAddItem(GetItemName(start_index + i)); } @@ -75,10 +75,10 @@ void AppListTestModel::PopulateAppWithId(int id) { std::string AppListTestModel::GetModelContent() { std::string content; - for (size_t i = 0; i < item_list()->item_count(); ++i) { + for (size_t i = 0; i < top_level_item_list()->item_count(); ++i) { if (i > 0) content += ','; - content += item_list()->item_at(i)->id(); + content += top_level_item_list()->item_at(i)->id(); } return content; } @@ -86,12 +86,14 @@ std::string AppListTestModel::GetModelContent() { AppListTestModel::AppListTestItem* AppListTestModel::CreateItem( const std::string& id) { AppListTestItem* item = new AppListTestItem(id, this); - size_t nitems = item_list()->item_count(); + size_t nitems = top_level_item_list()->item_count(); syncer::StringOrdinal position; - if (nitems == 0) + if (nitems == 0) { position = syncer::StringOrdinal::CreateInitialOrdinal(); - else - position = item_list()->item_at(nitems - 1)->position().CreateAfter(); + } else { + position = + top_level_item_list()->item_at(nitems - 1)->position().CreateAfter(); + } item->SetPosition(position); SetItemName(item, id); return item; @@ -104,7 +106,7 @@ AppListTestModel::AppListTestItem* AppListTestModel::CreateAndAddItem( return static_cast(item); } void AppListTestModel::HighlightItemAt(int index) { - AppListItem* item = item_list()->item_at(index); + AppListItem* item = top_level_item_list()->item_at(index); item->SetHighlighted(true); } diff --git a/ui/app_list/views/app_list_folder_view.cc b/ui/app_list/views/app_list_folder_view.cc index faeb59567e2bf6..8668622b14b682 100644 --- a/ui/app_list/views/app_list_folder_view.cc +++ b/ui/app_list/views/app_list_folder_view.cc @@ -71,11 +71,11 @@ AppListFolderView::AppListFolderView(AppsContainerView* container_view, SetFillsBoundsOpaquely(false); #endif - model_->item_list()->AddObserver(this); + model_->AddObserver(this); } AppListFolderView::~AppListFolderView() { - model_->item_list()->RemoveObserver(this); + model_->RemoveObserver(this); // Make sure |items_grid_view_| is deleted before |pagination_model_|. RemoveAllChildViews(true); } @@ -129,7 +129,7 @@ bool AppListFolderView::OnKeyPressed(const ui::KeyEvent& event) { return items_grid_view_->OnKeyPressed(event); } -void AppListFolderView::OnListItemRemoved(size_t index, AppListItem* item) { +void AppListFolderView::OnAppListItemWillBeDeleted(AppListItem* item) { if (item == folder_item_) { items_grid_view_->OnFolderItemRemoved(); folder_header_view_->OnFolderItemRemoved(); diff --git a/ui/app_list/views/app_list_folder_view.h b/ui/app_list/views/app_list_folder_view.h index b58d5e4ee6a67f..95d4b2219be609 100644 --- a/ui/app_list/views/app_list_folder_view.h +++ b/ui/app_list/views/app_list_folder_view.h @@ -34,7 +34,7 @@ class PaginationModel; class AppListFolderView : public views::View, public FolderHeaderViewDelegate, - public AppListItemListObserver, + public AppListModelObserver, public ui::ImplicitAnimationObserver { public: AppListFolderView(AppsContainerView* container_view, @@ -84,15 +84,15 @@ class AppListFolderView : public views::View, // Hides the view immediately without animation. void HideViewImmediately(); - // views::View overrides: + // views::View virtual gfx::Size GetPreferredSize() OVERRIDE; virtual void Layout() OVERRIDE; virtual bool OnKeyPressed(const ui::KeyEvent& event) OVERRIDE; - // Overridden from AppListItemListObserver: - virtual void OnListItemRemoved(size_t index, AppListItem* item) OVERRIDE; + // AppListModelObserver + virtual void OnAppListItemWillBeDeleted(AppListItem* item) OVERRIDE; - // ui::ImplicitAnimationObserver overrides: + // ui::ImplicitAnimationObserver virtual void OnImplicitAnimationsCompleted() OVERRIDE; AppsGridView* items_grid_view() { return items_grid_view_; } diff --git a/ui/app_list/views/app_list_main_view.cc b/ui/app_list/views/app_list_main_view.cc index 850ca78ae398f0..f82236482d33fb 100644 --- a/ui/app_list/views/app_list_main_view.cc +++ b/ui/app_list/views/app_list_main_view.cc @@ -174,13 +174,13 @@ void AppListMainView::PreloadIcons(gfx::NativeView parent) { const int tiles_per_page = kPreferredCols * kPreferredRows; const int start_model_index = selected_page * tiles_per_page; - const int end_model_index = std::min( - static_cast(model_->item_list()->item_count()), - start_model_index + tiles_per_page); + const int end_model_index = + std::min(static_cast(model_->top_level_item_list()->item_count()), + start_model_index + tiles_per_page); pending_icon_loaders_.clear(); for (int i = start_model_index; i < end_model_index; ++i) { - AppListItem* item = model_->item_list()->item_at(i); + AppListItem* item = model_->top_level_item_list()->item_at(i); if (item->icon().HasRepresentation(scale)) continue; diff --git a/ui/app_list/views/apps_container_view.cc b/ui/app_list/views/apps_container_view.cc index 95ec3b74bc0449..66b774d9880d8e 100644 --- a/ui/app_list/views/apps_container_view.cc +++ b/ui/app_list/views/apps_container_view.cc @@ -51,7 +51,7 @@ AppsContainerView::AppsContainerView(AppListMainView* app_list_main_view, AddChildView(app_list_folder_view_); apps_grid_view_->SetModel(model_); - apps_grid_view_->SetItemList(model_->item_list()); + apps_grid_view_->SetItemList(model_->top_level_item_list()); SetShowState(SHOW_APPS, false); /* show apps without animation */ } diff --git a/ui/app_list/views/apps_grid_view_unittest.cc b/ui/app_list/views/apps_grid_view_unittest.cc index 5adacb1956f382..ed8898364862ca 100644 --- a/ui/app_list/views/apps_grid_view_unittest.cc +++ b/ui/app_list/views/apps_grid_view_unittest.cc @@ -110,7 +110,7 @@ class AppsGridViewTest : public views::ViewsTestBase { apps_grid_view_->SetLayout(kIconDimension, kCols, kRows); apps_grid_view_->SetBoundsRect(gfx::Rect(gfx::Size(kWidth, kHeight))); apps_grid_view_->SetModel(model_.get()); - apps_grid_view_->SetItemList(model_->item_list()); + apps_grid_view_->SetItemList(model_->top_level_item_list()); test_api_.reset(new AppsGridViewTestApi(apps_grid_view_.get())); } @@ -126,7 +126,7 @@ class AppsGridViewTest : public views::ViewsTestBase { } AppListItemView* GetItemViewForPoint(const gfx::Point& point) { - for (size_t i = 0; i < model_->item_list()->item_count(); ++i) { + for (size_t i = 0; i < model_->top_level_item_list()->item_count(); ++i) { AppListItemView* view = GetItemViewAt(i); if (view->bounds().Contains(point)) return view; @@ -135,7 +135,7 @@ class AppsGridViewTest : public views::ViewsTestBase { } gfx::Rect GetItemTileRectAt(int row, int col) { - DCHECK_GT(model_->item_list()->item_count(), 0u); + DCHECK_GT(model_->top_level_item_list()->item_count(), 0u); gfx::Insets insets(apps_grid_view_->GetInsets()); gfx::Rect rect(gfx::Point(insets.left(), insets.top()), @@ -208,7 +208,7 @@ TEST_F(AppsGridViewTest, EnsureHighlightedVisible) { EXPECT_EQ(1, pagination_model_->selected_page()); // Highlight last one in the model and last page should be selected. - model_->HighlightItemAt(model_->item_list()->item_count() - 1); + model_->HighlightItemAt(model_->top_level_item_list()->item_count() - 1); EXPECT_EQ(kPages - 1, pagination_model_->selected_page()); }