Skip to content
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

Sort tabs with less steps #1377

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@piroor
Copy link

commented Apr 18, 2019

This should fix #1373,

piroor added some commits Apr 18, 2019

@groovecoder

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

This is a big addition and change ... will probably take a long while to get to review this. 😢

@piroor

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

Here are some additional information for reviewing:

  • The largest addition differ.js is a ported version from the diff module of npm. I just modified it to remove needless codes for this usecase.
  • The differ detects three type differences: "added", "removed", and "unchanged". Rearranging of tabs is based on a logic:
    1. Prepare three arrays: initial order (beforeIds), final order (afterIds), and the current order (currentIds: this is required because tabs.move() is asynchronous and we need to track the order of tabs after tab moves.)
    2. Calculate differences between the initial order and the final order.
    3. Extract only "added" differences. Any "removed" difference should be ignored, because those tabs are moved with corresponding "addded" differences. Of course there is no removed tab while while rearranging.
    4. Calculate destination position of "added" tabs, based on the current order (currentIds).
    5. Move "added" tabs actually via tabs.move().
    6. Update the current order (currentIds) for the next calculation.
    7. Repeat steps 4-7 for all "added" differences.
@piroor

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

@jonathanKingston

This comment has been minimized.

Copy link
Member

commented May 7, 2019

In the article you mention that the native API tries to move tabs if it's not required. I haven't tested but the code in Firefox at least seems that isn't the case: https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/browser/base/content/tabbrowser.js#3710

I suspect the existing algorithm could be improved greatly:

  • Reduce the move distance within the container by preferring to keep the tab's position
  • Flatten the sort map to create an ordering array for all tabs in the window
  • Call one move API operation per window for all tabs and await the move completing
  • Sort the focused window first and then sort other windows after to reduce the feedback lag for users

If this difference algorithm still has improvements over that, perhaps we should be including it natively. It seems to me that just calling one api request await browser.tabs.move([1,12,8,7,2....], {windowId}); would solve the problem greatly.

@piroor

This comment has been minimized.

Copy link
Author

commented May 8, 2019

Yes, Firefox doesn't move tab if the specified index is same to the current one. However, tabs.move() is not well-optimized. This is a worst case example:

// originally tabs are ordered as: [7,1,2,3,4,5,6]
browser.tabs.onMoved.addListener(tabId => console.log('moved ', tabId));
browser.tabs.move([1,2,3,4,5,6,7], { index: 0 });

This case actually triggers 6 tab moves as:

move  1
move  2
move  3
move  4
move  5
move  6

Such a case will happen for example when tabs 1-6 are opened with the "default" container and the tab 7 is opened with a specific container. I think that implementing a FMAC-specific algorithm to calculate the minimum tab moves on various cases looks hard, and this is the reason why I proposed the differential algorithm as the universal algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.