Respect bookmark-bar-ntp flag and also "Always Show Bookmarks Bar" menu option#1639
Respect bookmark-bar-ntp flag and also "Always Show Bookmarks Bar" menu option#1639vuon9 wants to merge 1 commit into
Conversation
|
Reviews (1): Last reviewed commit: "fix: respect bookmark bar visibility pre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
vuon9
force-pushed
the
fix/respect-bookmark-bar-pref-ntp
branch
2 times, most recently
from
90e0ebb to
31c08eb
Compare
vuon9
changed the title
|
Updated and added outcome metrix + demo videos to align with the ungoogled-chromium flag. |
vuon9
force-pushed
the
fix/respect-bookmark-bar-pref-ntp
branch
from
31c08eb to
38399d8
Compare
When kShowBookmarkBar pref is false, ShouldShowBookmarkBar() now returns false before reaching the NTP special-case, hiding the bar on new tab pages regardless of bookmark count.
vuon9
force-pushed
the
fix/respect-bookmark-bar-pref-ntp
branch
from
38399d8 to
2375d6c
Compare
| - if (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII("bookmark-bar-ntp") == "never") { | ||
| - return false; | ||
| - } | ||
| - | ||
| Profile* profile = browser_->GetProfile(); | ||
| if (profile->IsGuestSession()) { | ||
| return false; | ||
| @@ -200,6 +196,10 @@ | ||
| return false; | ||
| } | ||
|
|
||
| + if (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII("bookmark-bar-ntp") == "never") { | ||
| + return false; | ||
| + } |
There was a problem hiding this comment.
could you explain how the behavior differs after moving the if() down here? i don't really understand what this does
There was a problem hiding this comment.
Setting 'bookmark-bar-ntp' to 'never' will consistently hide the bookmarks bar, overriding the 'menu: Always Show Bookmarks Bar' setting. By this move, the bookmarks bar will be hidden if 'menu: Always Show Bookmarks Bar' is unchecked. Please see the matrix table i have in the description to see each case.
There was a problem hiding this comment.
flags are meant to be the highest priority, which is why to me it makes sense when it is on the top of the function. i think the original solution made sense, where we always apply based on prefs::kShowBookmarkBar regardless of if the page is NTP or not
There was a problem hiding this comment.
Yeah, you're right, the original one is actually for highest priority. But do you think that makes more sense if we adjust like this, actually it can handle more cases, including the issue i have mentioned.
vuon9
changed the title
|
This issue #1349 does not happen in a newly created profile. But old profiles does have this issue. |
|
If no bookmarks (in any folder on the bookmarks bar, including other bookmarks folder) and no tab groups are present, it can be toggled on NTP in old profiles. This is why it works as expected in new profiles since we never create any bookmarks/tab groups in the demo profile. If you do create any of these, the issue appears there as well. |
I was confused about the first comment, but I understand now. My PR is actually about addressing the issue that I want to toggle show/hide the bookmarks bar by the browser menu option (also respecting the flag) no matter it has items or not, but it's not forcing to show/hide by the flag itself. To be honest, I hope the default Chromium feature will be like this. |
3 participants
For your pull request to not get closed without review, please confirm that:
(an approved feature request, or confirmed bug).
otherwise I have marked my PR as draft.
organization if I lied by checking any of these checkboxes.
Tested on (check one or more):
Allow completely toggling the bookmarks bar appearance in new tabs, respecting the ungoogled-chromium flag:
#bookmark-bar-ntpand also "Always Show Bookmarks Bar". So it's basically a pull request to change behavior of upstream ungoogled-chromium's flag behavior, it doesn't kill the flag itself but expands to control the bookmarks bar better.I used AI to debug and create this patch. To address this issue: #1349
Demo: Toggling Cmd+Shift+B on new tabs
Outcome matrix based on combination of flag
#bookmark-bar-ntpand View > Always Show Bookmarks Bar option:Demos:
bookmark-bar-ntp-default.mov
bookmark-bar-ntp-never.mov
Fixes #1349