Sidebar item reordering part 1 - support item moving in sidebar model

browser/ui/BUILD.gn

@@ -312,6 +312,7 @@ source_set("ui") {
       "views/sidebar/sidebar_control_view.h",
       "views/sidebar/sidebar_item_added_feedback_bubble.cc",
       "views/sidebar/sidebar_item_added_feedback_bubble.h",
+      "views/sidebar/sidebar_item_controller.h",
       "views/sidebar/sidebar_item_view.cc",
       "views/sidebar/sidebar_item_view.h",
       "views/sidebar/sidebar_items_contents_view.cc",

browser/ui/brave_browser.cc

@@ -9,7 +9,6 @@
 #include "brave/browser/ui/brave_browser_window.h"
 #include "brave/browser/ui/sidebar/sidebar.h"
 #include "brave/browser/ui/sidebar/sidebar_controller.h"
-#include "brave/browser/ui/sidebar/sidebar_model.h"
 #include "brave/browser/ui/sidebar/sidebar_utils.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
@@ -27,7 +26,6 @@ BraveBrowser::BraveBrowser(const CreateParams& params) : Browser(params) {
   // sidebar UI. After that, UI will be updated for model's change.
   sidebar_controller_.reset(new sidebar::SidebarController(this, profile()));
   sidebar_controller_->SetSidebar(brave_window()->InitSidebar());
-  sidebar_controller_->model()->Init();
 #endif
 }
 

browser/ui/sidebar/BUILD.gn

@@ -57,3 +57,23 @@ source_set("browser_tests") {
     ]
   }
 }
+
+source_set("unit_tests") {
+  if (enable_sidebar) {
+    testonly = true
+
+    sources = [ "sidebar_unittest.cc" ]
+
+    deps = [
+      "//base",
+      "//brave/browser/ui",
+      "//brave/browser/ui/sidebar",
+      "//brave/components/sidebar",
+      "//chrome/test:test_support",
+      "//components/prefs",
+      "//components/sync_preferences:test_support",
+      "//content/test:test_support",
+      "//testing/gtest",
+    ]
+  }
+}

browser/ui/sidebar/sidebar_controller.cc

@@ -12,6 +12,7 @@
 #include "brave/browser/ui/sidebar/sidebar_service_factory.h"
 #include "brave/browser/ui/sidebar/sidebar_utils.h"
 #include "brave/components/sidebar/sidebar_service.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -97,6 +98,8 @@ void SidebarController::SetSidebar(Sidebar* sidebar) {
   sidebar_ = sidebar;
 
   UpdateSidebarVisibility();
+  sidebar_model_->Init(HistoryServiceFactory::GetForProfile(
+      browser_->profile(), ServiceAccessType::EXPLICIT_ACCESS));
 }
 
 void SidebarController::UpdateSidebarVisibility() {

browser/ui/sidebar/sidebar_model.cc

@@ -6,14 +6,14 @@
 #include "brave/browser/ui/sidebar/sidebar_model.h"
 
 #include <string>
+#include <utility>
 
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "brave/browser/ui/sidebar/sidebar_model_data.h"
 #include "brave/browser/ui/sidebar/sidebar_service_factory.h"
 #include "brave/components/sidebar/sidebar_item.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
-#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/image_fetcher/image_fetcher_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_key.h"
@@ -61,14 +61,15 @@ SidebarModel::SidebarModel(Profile* profile)
 
 SidebarModel::~SidebarModel() = default;
 
-void SidebarModel::Init() {
+void SidebarModel::Init(history::HistoryService* history_service) {
   // Start with saved item list.
   for (const auto& item : GetAllSidebarItems())
     AddItem(item, -1, false);
 
   sidebar_observed_.Add(GetSidebarService(profile_));
-  history_observed_.Add(HistoryServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS));
+  // Can be null in test.
+  if (history_service)
+    history_observed_.Add(history_service);
 }
 
 void SidebarModel::AddObserver(Observer* observer) {
@@ -106,6 +107,35 @@ void SidebarModel::OnItemAdded(const SidebarItem& item, int index) {
   AddItem(item, index, true);
 }
 
+void SidebarModel::OnItemMoved(const SidebarItem& item, int from, int to) {
+  // Cache active model data to find its new index after moving.
+  SidebarModelData* active_data = nullptr;
+  if (active_index_ != -1) {
+    active_data = data_[active_index_].get();
+  }
+
+  std::unique_ptr<SidebarModelData> data = std::move(data_[from]);
+  data_.erase(data_.begin() + from);
+  data_.insert(data_.begin() + to, std::move(data));
+
+  for (Observer& obs : observers_)
+    obs.OnItemMoved(item, from, to);
+
+  if (!active_data)
+    return;
+
+  // Find new active items index.
+  const int data_size = data_.size();
+  for (int i = 0; i < data_size; ++i) {
+    if (data_[i].get() == active_data) {
+      UpdateActiveIndexAndNotify(i);
+      return;
+    }
+  }
+
+  NOTREACHED();
+}
+
 void SidebarModel::OnWillRemoveItem(const SidebarItem& item, int index) {
   if (index == active_index_)
     UpdateActiveIndexAndNotify(-1);
@@ -149,14 +179,14 @@ void SidebarModel::RemoveItemAt(int index) {
   }
 }
 
-void SidebarModel::SetActiveIndex(int index) {
+void SidebarModel::SetActiveIndex(int index, bool load) {
   if (index == active_index_)
     return;
 
   // Don't load url if it's already loaded. If not, new loading is started
   // whenever item is activated.
   // TODO(simonhong): Maybe we should have reload option?
-  if (index != -1 && !IsLoadedAt(index))
+  if (load && index != -1 && !IsLoadedAt(index))
     LoadURLAt(GetAllSidebarItems()[index].url, index);
 
   UpdateActiveIndexAndNotify(index);
@@ -202,6 +232,9 @@ void SidebarModel::LoadURLAt(const GURL& url, int index) {
 }
 
 void SidebarModel::UpdateActiveIndexAndNotify(int new_active_index) {
+  if (new_active_index == active_index_)
+    return;
+
   const int old_active_index = active_index_;
   active_index_ = new_active_index;
 

browser/ui/sidebar/sidebar_model.h

@@ -56,6 +56,7 @@ class SidebarModel : public SidebarService::Observer,
     virtual void OnItemAdded(const SidebarItem& item,
                              int index,
                              bool user_gesture) {}
+    virtual void OnItemMoved(const SidebarItem& item, int from, int to) {}
     virtual void OnItemRemoved(int index) {}
     virtual void OnActiveIndexChanged(int old_index, int new_index) {}
     virtual void OnFaviconUpdatedForItem(const SidebarItem& item,
@@ -71,12 +72,13 @@ class SidebarModel : public SidebarService::Observer,
   SidebarModel(const SidebarModel&) = delete;
   SidebarModel& operator=(const SidebarModel&) = delete;
 
-  void Init();
+  void Init(history::HistoryService* history_service);
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  void SetActiveIndex(int index);
+  // |false| is used in unit test.
+  void SetActiveIndex(int index, bool load = true);
   // Returns true if webcontents of item at |index| already loaded url.
   bool IsLoadedAt(int index) const;
   bool IsSidebarHasAllBuiltiInItems() const;
@@ -93,6 +95,7 @@ class SidebarModel : public SidebarService::Observer,
 
   // SidebarService::Observer overrides:
   void OnItemAdded(const SidebarItem& item, int index) override;
+  void OnItemMoved(const SidebarItem& item, int from, int to) override;
   void OnWillRemoveItem(const SidebarItem& item, int index) override;
   void OnItemRemoved(const SidebarItem& item, int index) override;
 
@@ -104,6 +107,8 @@ class SidebarModel : public SidebarService::Observer,
                     base::Time visit_time) override;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SidebarModelTest, ItemsChangedTest);
+
   // Add item at last.
   void AddItem(const SidebarItem& item, int index, bool user_gesture);
   void RemoveItemAt(int index);

browser/ui/sidebar/sidebar_unittest.cc

@@ -0,0 +1,117 @@
+/* Copyright (c) 2021 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <memory>
+
+#include "base/feature_list.h"
+#include "base/scoped_observation.h"
+#include "base/test/scoped_feature_list.h"
+#include "brave/browser/ui/sidebar/sidebar_model.h"
+#include "brave/browser/ui/sidebar/sidebar_service_factory.h"
+#include "brave/components/sidebar/features.h"
+#include "brave/components/sidebar/sidebar_service.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/prefs/pref_service.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sidebar {
+
+class SidebarModelTest : public testing::Test, public SidebarModel::Observer {
+ public:
+  SidebarModelTest() = default;
+
+  ~SidebarModelTest() override = default;
+
+  void SetUp() override {
+    scoped_feature_list_.InitAndEnableFeature(kSidebarFeature);
+    profile_ = std::make_unique<TestingProfile>();
+    service_ = SidebarServiceFactory::GetForProfile(profile_.get());
+    model_.reset(new SidebarModel(profile_.get()));
+    observation_.Observe(model_.get());
+  }
+
+  // SidebarModel::Observer overrides:
+  void OnItemMoved(const SidebarItem& item, int from, int to) override {
+    on_item_moved_called_ = true;
+  }
+  void OnActiveIndexChanged(int old_index, int new_index) override {
+    on_active_index_changed_called_ = true;
+  }
+
+  void ClearState() {
+    on_item_moved_called_ = false;
+    on_active_index_changed_called_ = false;
+  }
+
+  Profile* profile() { return profile_.get(); }
+  SidebarModel* model() { return model_.get(); }
+  SidebarService* service() { return service_; }
+
+  content::BrowserTaskEnvironment browser_task_environment_;
+  bool on_item_moved_called_ = false;
+  bool on_active_index_changed_called_ = false;
+  SidebarService* service_ = nullptr;
+  std::unique_ptr<TestingProfile> profile_;
+  std::unique_ptr<SidebarModel> model_;
+  base::ScopedObservation<SidebarModel, SidebarModel::Observer> observation_{
+      this};
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(SidebarModelTest, ItemsChangedTest) {
+  EXPECT_EQ(0UL, model()->data_.size());
+  model()->Init(nullptr);
+  EXPECT_EQ(service()->items().size(), model()->data_.size());
+
+  EXPECT_EQ(-1, model()->active_index());
+
+  // Move item at 1 to at index 2.
+  // Total size and active index is not changed when currently active index is
+  // -1.
+  const size_t model_size = model()->data_.size();
+  // Cache data at index 1.
+  auto* model_data = model()->data_[1].get();
+  EXPECT_FALSE(on_item_moved_called_);
+  EXPECT_FALSE(on_active_index_changed_called_);
+
+  service()->MoveItem(1, 2);
+
+  EXPECT_TRUE(on_item_moved_called_);
+  EXPECT_FALSE(on_active_index_changed_called_);
+  EXPECT_EQ(model_data, model()->data_[2].get());
+  EXPECT_EQ(-1, model()->active_index());
+  EXPECT_EQ(model_size, model()->data_.size());
+
+  model()->SetActiveIndex(1, false);
+  EXPECT_EQ(1, model()->active_index());
+
+  // Move item at 1 to 2. This causes active index change because item at 1 was
+  // active item. After moving, active item index should be 2.
+  ClearState();
+  service()->MoveItem(1, 2);
+  EXPECT_TRUE(on_item_moved_called_);
+  EXPECT_TRUE(on_active_index_changed_called_);
+  EXPECT_EQ(2, model()->active_index());
+
+  // Moving item from 1 to 0 doesn't affect active index.
+  ClearState();
+  service()->MoveItem(1, 0);
+  EXPECT_TRUE(on_item_moved_called_);
+  EXPECT_FALSE(on_active_index_changed_called_);
+  EXPECT_EQ(2, model()->active_index());
+
+  // Moving item from 3 to 0 affect active index. Items behind the active
+  // item(at 2) to the front of active index. So, active item is also moved from
+  // 2 to 3 index.
+  ClearState();
+  service()->MoveItem(3, 0);
+  EXPECT_TRUE(on_item_moved_called_);
+  EXPECT_TRUE(on_active_index_changed_called_);
+  EXPECT_EQ(3, model()->active_index());
+}
+
+}  // namespace sidebar

browser/ui/views/sidebar/sidebar_item_controller.h

@@ -0,0 +1,19 @@
+/* Copyright (c) 2021 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_ITEM_CONTROLLER_H_
+#define BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_ITEM_CONTROLLER_H_
+
+class SidebarItemController {
+ public:
+  virtual void MaybeStartDrag() = 0;
+  virtual void ContinueDrag() = 0;
+  virtual void EndDrag() = 0;
+
+ protected:
+  ~SidebarItemController() {}
+};
+
+#endif  // BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_ITEM_CONTROLLER_H_

browser/ui/views/sidebar/sidebar_item_view.cc

@@ -6,11 +6,18 @@
 #include "brave/browser/ui/views/sidebar/sidebar_item_view.h"
 
 #include "brave/browser/themes/theme_properties.h"
+#include "brave/browser/ui/views/sidebar/sidebar_item_controller.h"
 #include "brave/grit/brave_theme_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/canvas.h"
 
+SidebarItemView::SidebarItemView(Delegate* delegate,
+                                 SidebarItemController* controller)
+    : SidebarButtonView(delegate), controller_(controller) {
+  DCHECK(controller_);
+}
+
 SidebarItemView::~SidebarItemView() = default;
 
 void SidebarItemView::OnPaintBorder(gfx::Canvas* canvas) {
@@ -36,3 +43,23 @@ void SidebarItemView::OnPaintBackground(gfx::Canvas* canvas) {
     }
   }
 }
+
+bool SidebarItemView::OnMousePressed(const ui::MouseEvent& event) {
+  controller_->MaybeStartDrag();
+  return true;
+}
+
+bool SidebarItemView::OnMouseDragged(const ui::MouseEvent& event) {
+  controller_->ContinueDrag();
+  return true;
+}
+
+void SidebarItemView::OnMouseReleased(const ui::MouseEvent& event) {
+  SidebarButtonView::OnMouseReleased(event);
+
+  controller_->EndDrag();
+}
+
+void SidebarItemView::OnMouseCaptureLost() {
+  controller_->EndDrag();
+}