Most recently used tab with Ctrl-Tab cycling #4671

guifel · 2020-02-18T10:57:23Z

I implemented MRU cycling feature as requested in the issue. I added a new option under the section Tab "Cycle through the most recently used tabs with Ctrl-Tab" to activate the feature. Deactivated by default.

This is inspired by how MRU cycling is implemented at the ChromeOS level.

Submitter Checklist:

Test Plan:

TBD

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

guifel · 2020-02-18T11:04:31Z

To do before ready for review:

Fix and adapt this for MacOS. I don't have a Mac at the moment, I might get one next week.
Add build flags to not apply these changes for iOS and Android
Support regular previous/next cycling while using the page up/down keys (Ctrl-Page up/down) when MRU is activated.
Figure out the bug with the gtk event loop crashing the browser when spamming keys on the new page tab (Might be unrelated to this)

guifel · 2020-02-25T21:40:48Z

Ufffff, that was harder than I first expected but I've now implemented everything I wanted :)

First, I am a daily Brave user and what you're doing is awesome. This was my biggest frustration when moving from Firefox so I had a try. It's my first contribution to Brave and experience with the Chromium code base. It is certainly not yet perfect, I need some feedback.

I have build and tested the feature on Linux (Ubuntu), Windows and Mac.

The only missing piece is that Ctrl-Previous and Ctrl-Next are doing MRU switching as well. I originally wanted to do a simple switch to the previous and next tab even when the feature is activated. Unfortunately, Chromium treats Ctrl-Next and Ctrl-Previous the same way as Ctrl-Tab and Ctrl-Maj-Tab down to some quite lower level controllers. If we really need this feature we will have to do some more changes at the Chromium level.

petemill · 2020-02-26T19:13:38Z

Thank you so much for getting deep in to this issue @guifel and persisting to get it working! I'll definitely review the UX impact and have a go at understanding if we can split the keyboard shortcuts as you mention.

Special thanks for also putting the effort in to do the patching and overrides in a clean way.

patches/chrome-browser-ui-tabs-tab_strip_model.h.patch

patches/chrome-browser-ui-browser.cc.patch

patches/chrome-browser-ui-tabs-tab_strip_model.h.patch

iefremov · 2020-02-27T06:32:13Z

browser/ui/tabs/brave_tab_strip_model.h

+  int current_mru_cycling_index = -1;
+
+  // List of tab indexes sorted by most recently used
+  std::vector<int> mru_cycle_list;


have you checked base::MRUCache?

I was not aware of this class but I am not sure it is a good fit here. I am calculating from scratch the ordered set of tab indices at the beginning of each cycling session with std:sort and the timestamp of last use.
base:MRUCache keeps track of the ordering by calling the Put function whenever the item is used.

To use MRUCache I would need to call Put each time a tab is accessed, not sure this is possible form TabStripModel.

Hmm. So it looks like that timestamp is being set in TabStripModel::ActivateTabAt. What about overriding that method and calling base::MRUCache.Put when the new tab is activated instead of doing your own MRU?

My problem with base::MRUCache is that it's a map data-structure with key-value pairs. What I need is a simple MRU queue.
Also, I thought that relying on this already existing timestamp would be better than maintaining an additional index.

browser/ui/tabs/brave_tab_strip_model.cc

guifel · 2020-03-07T15:02:19Z

@yrliou @iefremov do you have anymore feedback ? Is anything blocking you ?

Helldembez · 2020-03-07T15:16:33Z

@guifel You are my hero.

guifel · 2020-03-07T15:55:46Z

Thanks @Helldembez 🤣 but the PR is not yet merged. Brave team seems busy.

bsclifton · 2020-03-07T17:32:38Z

@guifel do you think you can give this a rebase and squash the commits up a bit? I wouldn't squash into one commit, but try to group similar ones together (ex: where you fix review feedback). Let me know if you need help with that 😄

And huge thanks for submitting this- we'll definitely work with you to get it merged 😄

guifel · 2020-03-09T11:14:16Z

@bsclifton done. In the first commit I add the toggle button in the settings (which touches many files..) and in the second commit we have the core logic of the MRU tab switch. The 2 last commits are the requested changes by @yrliou and @iefremov.

During the mru cycling, tab can be closed with shortcut. In that case, current cycling is ended and new cycling will be started.

Can't resolve now for chrome/browser circular dependency.

* Delete redundant IsKeyEvent() checking * Don't use MRU outside of TabStripModel.

simonhong · 2020-10-07T07:24:36Z

@guifel, Finally merged! 🎉

guifel · 2020-10-07T08:51:54Z

Awesome to see this feature in the master! Thank you guys for the amazing job.

bsclifton · 2020-10-07T16:08:28Z

@guifel thanks for your contribution and also your patience 😄 This is a great feature!
@simonhong thanks for championing this (rebasing, fixing items, etc) and taking it across the finish line

This should be in Nightly in the next day or so 😄

rjshrjndrn · 2020-10-07T17:31:19Z

woohoo at last... Here comes Brave With C-Tab

kindrowboat · 2020-11-18T14:01:17Z

FYI: I have submitted an issue to Chromium to consider merging/cherry-picking these commits into Chromium. https://bugs.chromium.org/p/chromium/issues/detail?id=1150395

bsclifton · 2020-11-19T18:24:42Z

@motevets awesome, thanks! That would be a great one to upstream

believerd · 2023-05-23T07:42:02Z

Hi developer, there seems to be a bug in the MRU implementation. If you open just one new tab using open link in new tab(right click) then Ctrl-Tab won't work.

bsclifton assigned guifel Feb 24, 2020

guifel closed this Feb 25, 2020

guifel force-pushed the guifel-1/mru-clycling-ctrl-tab branch from a34b6a7 to 2b0f75c Compare February 25, 2020 12:51

guifel reopened this Feb 25, 2020

guifel force-pushed the guifel-1/mru-clycling-ctrl-tab branch 3 times, most recently from 03b4d1c to 4cb05fc Compare February 25, 2020 14:41

guifel marked this pull request as ready for review February 25, 2020 21:40

guifel requested a review from bridiver as a code owner February 25, 2020 21:40

guifel requested review from petemill and bsclifton February 25, 2020 21:44

yrliou reviewed Feb 26, 2020

View reviewed changes

iefremov self-requested a review February 27, 2020 06:30

iefremov reviewed Feb 27, 2020

View reviewed changes

browser/ui/tabs/brave_tab_strip_model.cc Outdated Show resolved Hide resolved

guifel requested review from yrliou and iefremov February 27, 2020 15:25

guifel force-pushed the guifel-1/mru-clycling-ctrl-tab branch 4 times, most recently from cd290d8 to 3c842c9 Compare March 9, 2020 11:03

guifel and others added 19 commits October 7, 2020 13:46

Move event handlers to BraveBrowserView, kill MRUTabCyclingController

9260d18

Move MRU setting to appearance section @rebron

9770c6c

Minor cleanup

0da75ec

Use EventMonitor to monitoring events from NativeWindow

de822ae

Add check to avoid recursive calls

90625a4

Minor cleanup

e59770f

Update patches

afbeddc

Move MRU setting after Zoom settings in appearance section

eac9b31

nit

b250686

Cleanup after rebasing

1f75346

Add options to settings page under zoom

e776b36

Fix test build error/crash

9d7133d

Fix crash when tab is closed while mru cycling

0678835

During the mru cycling, tab can be closed with shortcut. In that case, current cycling is ended and new cycling will be started.

Disable gn_check failure

79b8fd5

Can't resolve now for chrome/browser circular dependency.

Minor cleanup

0d162c1

Use separate target for browser test

4afcdc6

Address review comments

cf4e12e

* Delete redundant IsKeyEvent() checking * Don't use MRU outside of TabStripModel.

Deleted leftover after rebasing

af04848

Add test target directly to browser_tests

9a91afa

simonhong force-pushed the guifel-1/mru-clycling-ctrl-tab branch from 28da6b5 to 9a91afa Compare October 7, 2020 04:46

simonhong merged commit c15b8ce into brave:master Oct 7, 2020

andconta mentioned this pull request Oct 15, 2020

MRU tab feature not switching to the correct tab brave/brave-browser#12157

Closed

gerardbosch mentioned this pull request Oct 17, 2023

"Cycle through the most recently used tabs with Ctrl-Tab" affects any shortcut for the Next tab action brave/brave-browser#31774

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Most recently used tab with Ctrl-Tab cycling #4671

Most recently used tab with Ctrl-Tab cycling #4671

guifel commented Feb 18, 2020 •

edited by simonhong

Loading

guifel commented Feb 18, 2020 •

edited

Loading

guifel commented Feb 25, 2020

petemill commented Feb 26, 2020

iefremov Feb 27, 2020

guifel Feb 27, 2020

kkuehlz Apr 10, 2020 •

edited

Loading

guifel Apr 24, 2020

guifel commented Mar 7, 2020

Helldembez commented Mar 7, 2020

guifel commented Mar 7, 2020

bsclifton commented Mar 7, 2020 •

edited

Loading

guifel commented Mar 9, 2020

simonhong commented Oct 7, 2020

guifel commented Oct 7, 2020

bsclifton commented Oct 7, 2020

rjshrjndrn commented Oct 7, 2020

kindrowboat commented Nov 18, 2020

bsclifton commented Nov 19, 2020

believerd commented May 23, 2023

Most recently used tab with Ctrl-Tab cycling #4671

Most recently used tab with Ctrl-Tab cycling #4671

Conversation

guifel commented Feb 18, 2020 • edited by simonhong Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

guifel commented Feb 18, 2020 • edited Loading

guifel commented Feb 25, 2020

petemill commented Feb 26, 2020

iefremov Feb 27, 2020

Choose a reason for hiding this comment

guifel Feb 27, 2020

Choose a reason for hiding this comment

kkuehlz Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

guifel Apr 24, 2020

Choose a reason for hiding this comment

guifel commented Mar 7, 2020

Helldembez commented Mar 7, 2020

guifel commented Mar 7, 2020

bsclifton commented Mar 7, 2020 • edited Loading

guifel commented Mar 9, 2020

simonhong commented Oct 7, 2020

guifel commented Oct 7, 2020

bsclifton commented Oct 7, 2020

rjshrjndrn commented Oct 7, 2020

kindrowboat commented Nov 18, 2020

bsclifton commented Nov 19, 2020

believerd commented May 23, 2023

guifel commented Feb 18, 2020 •

edited by simonhong

Loading

guifel commented Feb 18, 2020 •

edited

Loading

kkuehlz Apr 10, 2020 •

edited

Loading

bsclifton commented Mar 7, 2020 •

edited

Loading