-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use chromium's side panel inside our side bar container, and insert Reading List #13769
Conversation
6b46214
to
1efc150
Compare
1efc150
to
c1b6219
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to reuse upstream's side panel UI 👍🏼
components/sidebar/sidebar_item.cc
Outdated
@@ -4,24 +4,34 @@ | |||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
#include "brave/components/sidebar/sidebar_item.h" | |||
#include "build/build_config.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this seems not needed here?
constexpr size_t kPanelWidthForBuiltIn = 260; | ||
constexpr size_t kPanelWidthForNonBuiltIn = 360; | ||
return IsBuiltInType(item) ? kPanelWidthForBuiltIn : kPanelWidthForNonBuiltIn; | ||
SidePanelEntry::Id SidePanelIdFromSideBarItem(const sidebar::SidebarItem item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const sidebar::SidebarItem& item
bool IsSidebarHasAllBuiltiInItems() const; | ||
int GetIndexOf(const SidebarItem& item) const; | ||
|
||
// Don't cache web_contents. It can be deleted during the runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From now, this SidebarModel
's main role is managing active index and fetching favicon :)
sidebar_panel_webview_->SetVisible(true); | ||
// When panel is opened, it will get focused. | ||
sidebar_panel_webview_->RequestFocus(); | ||
if (!item.open_in_panel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be more readable if positive condition comes first - if (item.open_in_panel)
.
} | ||
// if (item.open_in_panel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover
sidebar_panel_webview_ = | ||
AddChildView(std::make_unique<SidebarPanelWebView>(browser_->profile())); | ||
|
||
ReorderChildView(side_panel_, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add sidebar_control_view_
at index 0
, then maybe we don't need to reorder side_pane_
?
sidebar_control_view_ =
AddChildViewAt(std::make_unique<SidebarControlView>(browser_), 0);
cfc808a
to
e942a31
Compare
I have now pushed all functionality working well with the new toolbar icon. The Sidebar correctly shows active states when the panel is made active from external means (i.e. the side panel toolbar icon). Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.
|
2413b2f
to
aa7ef88
Compare
Test plans added to brave/brave-browser#17959 |
All feedback addressed @simonhong |
A Storybook has been deployed to preview UI for the latest push |
I realized that |
A Storybook has been deployed to preview UI for the latest push |
4a89ca6
to
691bc97
Compare
@simonhong I've addressed feedback again and added / amended some tests as appropriate. Please re-review when you can.
I've handled that in this PR now. I migrated the prefs such that the existing pref only stores user items (not built-in). And I made a new pref which simply contains a list of IDs of built-in items which the user has hidden.
This should now work as you describe. And if the user removes all panel items, then the toolbar icon will hide.
This is fixed. The issue was the delay between sidebar item getting activated and sidepanel view being shown (due to waiting for load event). |
A Storybook has been deployed to preview UI for the latest push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!🙌
Sorry for being picky. Please feel free to give me feedbacks for my comments.
@@ -0,0 +1,77 @@ | |||
CANVAS_DIMENSIONS, 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have copyright here?
ASSERT_TRUE( | ||
ui_test_utils::NavigateToURL(browser(), GURL("brave://settings/"))); | ||
|
||
EXPECT_TRUE(CanAddCurrentActiveTabToSidebar(browser())); | ||
controller()->AddItemWithCurrentTab(); | ||
// have 4 items. | ||
EXPECT_EQ(4UL, model()->GetAllSidebarItems().size()); | ||
// Has 1 item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Has 1 item. | |
// Add 1 item. |
or
// Has 1 item. | |
// Has 5 items. |
browser/ui/sidebar/sidebar_model.cc
Outdated
if (active_index_ == from) { | ||
new_active_index = to; | ||
} | ||
if (active_index_ == to) { | ||
new_active_index = (to < from) ? active_index_ + 1 : active_index_ - 1; | ||
} | ||
if (from > active_index_ && to < active_index_) { | ||
new_active_index = active_index_ + 1; | ||
} | ||
UpdateActiveIndexAndNotify(new_active_index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems from < active_index < to
case isn't handled, where we should decrease active index by one.
How about collapsing conditions like this?
if (active_index_ == from) {
new_active_index = to;
} else {
new_active_index = (to < from) ? active_index_ + 1 : active_index_ - 1;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - for some reason this was still working fine though 🤷
browser/ui/sidebar/sidebar_model.cc
Outdated
std::find_if(items.begin(), items.end(), [item](const auto& i) { | ||
return (item.built_in_item_type == i.built_in_item_type && | ||
item.url == i.url); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
- Let's use ranges
- capturing reference can be better I think.
std::find_if(items.begin(), items.end(), [item](const auto& i) { | |
return (item.built_in_item_type == i.built_in_item_type && | |
item.url == i.url); | |
}); | |
base::ranges::find_if(items, [&item](const auto& i) { | |
return (item.built_in_item_type == i.built_in_item_type && | |
item.url == i.url); | |
}); |
// Remember not to hide this item | ||
auto iter = std::find_if( | ||
built_in_items_to_hide.begin(), built_in_items_to_hide.end(), | ||
[item_id](const auto& default_item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with comment above, & capture and ranges.
@@ -225,75 +350,133 @@ SidebarService::ShowSidebarOption SidebarService::GetSidebarShowOption() const { | |||
return static_cast<ShowSidebarOption>(prefs_->GetInteger(kSidebarShowOption)); | |||
} | |||
|
|||
absl::optional<SidebarItem> SidebarService::GetDefaultPanelItem() const { | |||
const std::vector<SidebarItem::BuiltInItemType> preferred_item_types{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: NoDestructor
SidebarItem::BuiltInItemType::kBookmarks}; | ||
absl::optional<SidebarItem> default_item; | ||
for (const auto& type : preferred_item_types) { | ||
auto found_item_iter = std::find_if( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ranges?
if (type == SidebarItem::Type::kTypeBuiltIn) { | ||
if (const auto value = | ||
item.FindIntKey(kSidebarItemBuiltInItemTypeKey)) { | ||
auto id = static_cast<SidebarItem::BuiltInItemType>(*value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto id = static_cast<SidebarItem::BuiltInItemType>(*value); | |
auto type = static_cast<SidebarItem::BuiltInItemType>(*value); |
// 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/. | ||
|
||
// Need to include this header here otherwise SidePanel macro gets undef'ed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we include the corresponding header first to prevent the redefinitions. Should
#include "chrome/browser/ui/views/side_search/side_search_browser_controller.h" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we also need to keep the browser_view.h include as that also has a macro called SidePanel
@@ -3,16 +3,24 @@ | |||
* 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 "brave/browser/ui/views/frame/brave_browser_view_layout.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should include "chrome/browser/ui/views/frame/browser_view.h" first?
785b41f
to
45e5419
Compare
A Storybook has been deployed to preview UI for the latest push |
45e5419
to
b1d5662
Compare
@sangwoo108 @mkarolin @simonhong thanks for the reviews - feedback is addressed again. Changes can be seen here: https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477 |
A Storybook has been deployed to preview UI for the latest push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ it seems only network audit test should be fixed :)
@@ -63,34 +76,22 @@ SidebarItem GetBuiltInItemForType(SidebarItem::BuiltInItemType type) { | |||
return SidebarItem(); | |||
} | |||
|
|||
SidebarItem::BuiltInItemType GetBuiltInItemTypeForURL(const std::string& url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for deleting obsolete functions!
b1d5662
to
4ffd577
Compare
Updated to fix network audit test, review changes can now be seen at https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477 |
A Storybook has been deployed to preview UI for the latest push |
browser/ui/sidebar/sidebar_model.cc
Outdated
bool SidebarModel::IsSidebarHasAllBuiltiInItems() const { | ||
return GetSidebarService(profile_)->GetHiddenDefaultSidebarItems().empty(); | ||
} | ||
|
||
int SidebarModel::GetIndexOf(const SidebarItem& item) const { | ||
const auto items = GetAllSidebarItems(); | ||
const auto iter = | ||
std::find_if(items.begin(), items.end(), | ||
[item](const auto& i) { return item.url == i.url; }); | ||
base::ranges::find_if(items.begin(), items.end(), [&item](const auto& i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you use ranges
, I think you can omit begin(), end() of container.
base::ranges::find_if(items.begin(), items.end(), [&item](const auto& i) { | |
base::ranges::find_if(items, [&item](const auto& i) { |
} | ||
// Remember not to hide this item | ||
auto iter = base::ranges::find_if( | ||
built_in_items_to_hide.begin(), built_in_items_to_hide.end(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto:
built_in_items_to_hide.begin(), built_in_items_to_hide.end(), | |
built_in_items_to_hide, |
added_item.built_in_item_type; | ||
}); | ||
auto iter = | ||
base::ranges::find_if(default_items.begin(), default_items.end(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto:
base::ranges::find_if(default_items.begin(), default_items.end(), | |
base::ranges::find_if(default_items, |
absl::optional<SidebarItem> default_item; | ||
for (const auto& type : *preferred_item_types) { | ||
auto found_item_iter = base::ranges::find_if( | ||
items_.begin(), items_.end(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto:
items_.begin(), items_.end(), | |
items_, |
void OnActiveIndexChanged(int old_index, int new_index) override; | ||
void OnItemRemoved(int index) override; | ||
|
||
// SidePanelEntryObserver: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about having base::ScopedObservation<SidePanelEntry, SidePanelEntryObserver> as member variable for this?
I think there's no RemoveObserver() call for SidebarPanelEntry and that could be guaranteed by this RAII object.
|
||
// SidePanelRegistryObserver: | ||
void OnEntryRegistered(SidePanelEntry* entry) override; | ||
void OnEntryWillDeregister(SidePanelEntry* entry) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how about having base::ScopedMultiSourceObservation? For similar reason.
@@ -85,6 +93,7 @@ class SidebarControlView : public views::View, | |||
void UpdateSettingsButtonState(); | |||
void UpdateBackgroundAndBorder(); | |||
|
|||
raw_ptr<Delegate> delegate_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency
raw_ptr<Delegate> delegate_; | |
raw_ptr<Delegate> delegate_ = nullptr; |
|
||
SidebarItem GetBuiltInItemForURL(const std::string& url) { | ||
return GetBuiltInItemForType(GetBuiltInItemTypeForURL(url)); | ||
std::vector<SidebarItem::BuiltInItemType> GetDefaultBuiltInItemTypes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about returning const& in order to avoid copying the entire vector?
std::vector<SidebarItem::BuiltInItemType> GetDefaultBuiltInItemTypes() { | |
const std::vector<SidebarItem::BuiltInItemType>& GetDefaultBuiltInItemTypes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this nuance!
// Add the items the user has never seen (or never persisted). | ||
// Get the initial order of items so that we can attempt to | ||
// insert at the intended order. | ||
auto default_item_types = GetDefaultBuiltInItemTypes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If GetDefaultBuiltInItemTypes returns const&, const auto& will be better.
auto default_item_types = GetDefaultBuiltInItemTypes(); | |
const auto& default_item_types = GetDefaultBuiltInItemTypes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits 👍
4ffd577
to
650d1c3
Compare
@sangwoo108 I addressed your welcome nits via https://github.com/brave/brave-core/compare/4ffd577477..650d1c300b - thanks! |
A Storybook has been deployed to preview UI for the latest push |
…eading List Chromium's side panel is controlled by Brave's side bar buttons, via chromium's side panel coordinator
…e" to "exclusive" This allows new items to be easily added, and only store the item ref by ID instead of all item details
…ides if none are added
650d1c3
to
fd2a6b3
Compare
A Storybook has been deployed to preview UI for the latest push |
Chromium's side panel becomes controlled by Brave's side bar buttons, which uses chromium's side panel coordinator.
The goal of this PR is to get the best of both worlds - use Chromium's SidePanel related views, models, coordinators, WebContents management, and WebUI resources, whilst still using the awesome Brave SideBar from @simonhong.
There are many things we and chromium would like to use side panels for, and this could set us up to scale well for that.
Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.
Note: One consideration is to instead do the inverse of what I have done in this PR: Perhaps we move our SideBar to be a child of
browser_view_->right_aligned_side_panel()
(like the ComboBox is for Chromium) and have the toolbar button turn on SideBar + SidePanel, with no separation between the two. Some complication will need to be catered to where we want the SideBar to show even when there is no active SidePanel.TODO
Resolves brave/brave-browser#17959
Future considerations:
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
On brave/brave-browser#17959