experiment: anchored tabs (name TBD) - #1578
Conversation
|
Reviews (1): Last reviewed commit: "helium/anchored-tabs: refresh patches fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tab.extra_data, true /* from_session_restore */, is_active_browser); | ||
| DCHECK(web_contents); | ||
|
|
||
| - TabUIHelper::RestoreCustomTitleFromExtraData(web_contents, tab.extra_data); |
There was a problem hiding this comment.
restored_tab_offset is always 0
restored_tab_offset is declared as 0 and never mutated, so tab_index = i + initial_tab_count + restored_tab_offset is identical to tab_index = i + initial_tab_count. The variable appears to have been intended to offset restored-tab indices by 1 to account for the anchored NTP inserted at position 0 during helium_anchor_restore, but the offset was never set. Either the offset should be set to 1 when helium_anchor_restore is true (so restored tabs land after the NTP), or the variable should be removed and the original expression restored. Without the offset, the NTP at index 0 gets displaced to a non-deterministic position as subsequent restored tabs are inserted at indices 0, 1, 2, ….
| + break; | ||
| + } |
There was a problem hiding this comment.
ShowTabRenameDialog sets the dialog title to IDS_TAB_CXMENU_RENAMETAB ("Rename tab…" with a trailing ellipsis), but a dedicated IDS_TAB_RENAME_DIALOG_TITLE string ("Rename tab", no ellipsis) is already defined in the same GRD patch for exactly this purpose. Similarly, the textfield label is hardcoded as u"New tab name" rather than using IDS_TAB_RENAME_DIALOG_PLACEHOLDER. Both i18n strings are declared but unused, which means they are dead string resources and the dialog text cannot be localised. The SetTitle call should reference IDS_TAB_RENAME_DIALOG_TITLE, and the AddTextfield label should use l10n_util::GetStringUTF16(IDS_TAB_RENAME_DIALOG_PLACEHOLDER).
| + const bool should_defer_navigation = | ||
| + (features::IsHeliumAnchoredTabsEnabled() && !local_tab_->IsActivated() && | ||
| + contents()->WasDiscarded()) || | ||
| + (data_sharing::features::IsDataSharingFunctionalityEnabled() && | ||
| + !local_tab_->IsActivated()); | ||
| + if (!should_defer_navigation) { | ||
| PerformNavigation(url); | ||
| } else { | ||
| favicon::FaviconService* favicon_service = | ||
| @@ -278,6 +284,14 @@ void SavedTabGroupWebContentsListener::D | ||
| SavedTabGroupTab* tab = group->GetTab(local_tab_id()); | ||
| CHECK(tab); | ||
|
|
||
| + if (features::IsHeliumAnchoredTabsEnabled()) { | ||
| + TabUIHelper* const helper = TabUIHelper::From(local_tab_); | ||
| + if (helper && !helper->anchored_url().is_valid()) { | ||
| + helper->SetAnchoredURL(tab->url(), tab->title()); | ||
| + } | ||
| + return; | ||
| + } | ||
| + | ||
| service_->NavigateTab(group->local_group_id().value(), local_tab_id(), | ||
| contents()->GetURL(), contents()->GetTitle()); | ||
|
|
||
| Index: src/chrome/browser/ui/tabs/tab_menu_model.cc | ||
| =================================================================== | ||
| --- src.orig/chrome/browser/ui/tabs/tab_menu_model.cc |
There was a problem hiding this comment.
DeferredNavigationCommitted skips tab-group sync service notification when anchored tabs is enabled
The early return added here prevents service_->NavigateTab(...) from being called after a deferred navigation commits in a saved tab group. As a result, the TabGroupSyncService never learns about the navigation, so the saved-tab-group cloud state retains the stale URL indefinitely. For shared (synced) tab groups this means collaborators see an outdated URL for the tab for the lifetime of the session. The SetAnchoredURL call only updates in-memory state; calling service_->NavigateTab(...) afterward (even with the anchored URL rather than the current URL) would keep sync consistent without breaking the anchored-URL invariant.
There was a problem hiding this comment.
this is intentional.. anchored tabs should keep their saved url stable while the live tab can still navigate, syncing contents()->GetURL() here would replace the anchor
There was a problem hiding this comment.
That's a fair point — the whole semantic of an anchor is that the saved URL stays fixed regardless of where the live tab navigates, so propagating the current URL back to sync would break that invariant. Understood.
The one edge case worth keeping in mind: if a user explicitly changes the anchor (e.g. sets a new anchor URL), you'd want that to sync out. As long as SetAnchoredURL with persist=true routes through service_->NavigateTab (or an equivalent) for that case, this is fine.
Tip: You can give me more context with .greptile/rules.md so I get this right next time.
There was a problem hiding this comment.
yep, the command, SetAnchoredURL updates local/session state, then SetAnchoredURLToCurrent calls NavigateTab with the new anchor
| + for (int i = model_index + 1; i < model->count(); ++i) { | ||
| + if ((!target_group.has_value() || | ||
| + model->GetTabGroupForTab(i) != target_group) && | ||
| + model->GetWebContentsAt(i) && | ||
| + !model->GetWebContentsAt(i)->WasDiscarded()) { | ||
| + return i; | ||
| + } | ||
| + } | ||
| + for (int i = model_index - 1; i >= 0; --i) { | ||
| + if ((!target_group.has_value() || | ||
| + model->GetTabGroupForTab(i) != target_group) && | ||
| + model->GetWebContentsAt(i) && | ||
| + !model->GetWebContentsAt(i)->WasDiscarded()) { | ||
| + return i; | ||
| + } | ||
| + } | ||
| + if (model_index + 1 < model->count()) { | ||
| + content::WebContents* const contents = | ||
| + model->GetWebContentsAt(model_index + 1); | ||
| + if (contents && !contents->WasDiscarded()) { | ||
| + return model_index + 1; | ||
| + } | ||
| + } | ||
| + if (model_index - 1 >= 0) { | ||
| + content::WebContents* const contents = | ||
| + model->GetWebContentsAt(model_index - 1); | ||
| + if (contents && !contents->WasDiscarded()) { | ||
| + return model_index - 1; | ||
| + } | ||
| + } | ||
| + | ||
| + return std::nullopt; | ||
| +} | ||
| + | ||
| +bool ActivateFallbackTab(TabStripModel* model, | ||
| + int model_index, | ||
| + std::optional<tab_groups::TabGroupId> target_group) { | ||
| + const std::optional<int> fallback_index = | ||
| + FindFallbackTabIndex(model, model_index, target_group); |
There was a problem hiding this comment.
FindFallbackTabIndex: last two if blocks are redundant with the preceding loops
The first two for loops already iterate over model_index + 1 … count()-1 and model_index - 1 … 0, so the two trailing if checks for model_index ± 1 are always reached only after those exact indices have already been visited. The difference is that the trailing blocks omit the target_group filter, which means they can return a tab in the same group as the one being hibernated — but only if it is non-discarded. This subtle fallback is not documented and may be unintentional; if the intent really is "return any adjacent non-discarded tab regardless of group", that logic could be pulled into a clearly named third pass to avoid confusion.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
this is also intentional, the first passes prefer a tab outside the group, but the final fallback avoids opening a new tab when a usable live tab is right next to it
There was a problem hiding this comment.
That makes sense — the two-tier strategy is clear once you know it: prefer a tab outside the group, then fall back to any adjacent live tab to avoid spawning a new one.
A short comment above those final if blocks would make this obvious to future readers without requiring them to reason it out. Something like:
// Fallback: if no out-of-group tab exists, accept any adjacent live tab
// to avoid opening a new tab unnecessarily.Otherwise, happy to defer to your judgement here.
Tip: You can give me more context with .greptile/rules.md so I get this right next time.
|
I recently switched zen to helium, loved this pr, waiting for this patch! |
|
extreamilty improtant feature! looking forward might be what i missing to do the transisiton |
|
To me this fits as a pinned tab in terms of naming convention. I think Zen got this right with how they name items. I feel like a pinned tab means the original URL should be stored and resetable. |
|
as mentioned in the original pr i've been using this as my daily driver for about 3 weeks, it's been a pleasant experience and i haven't felt the need to reach for zen (my previous daily driver) since switching a few bugs (one resulting in an easily reproducible crash) but nothing that's affected my day to day too harshly some parts of the experiment is still rough around the edges and the aforementioned bugs still need to be addressed. I have a busy period ahead of me for the next 2 - 3 weeks so until i'm able to give this some love i'll be marking the pr as a draft as it's definitely not in the state where it's fit for merge @dumbmoron / @wukko any feedback would be greatly appreciated for me to come back to after my busy period and if you'd like to further cleanup your prs feel free to close this one and i'll request y'all to re-open this one / i can make a new one once i've made some much needed changes |
|
@wukko mate, hi — I'm really interested in this review and happy to help if needed. Let me know. |
|
Hi @Umbranoxio — built the macOS version from your PR today, this is huge! One small ask: could standalone tabs be anchorable too, not only ones inside tab groups? |
|
make update to mr made standalone tabs be able to be anchorable too |
|
This is AWESOME!! Would be huge for folks coming from Dia/Arc - super excited to see it implemented, thank you for your hard work! |
|
guys, this is totaly musthave, do ya need any help? |
|
May a pinned tab have no group? I found that forcing the creation of a group before pinning any tab is an issue. Besides, Ctrl+D or another shortcut for pinning a tab can help. |
|
As others have said, this PR is the thing keeping me from ditching Zen and FINALLY finding a Chrome replacement to Arc. It's the single must-have feature that I expect from any browser. I'm not familiar of how to build it to start using it, even if rough, but I'll definitely do some research and start testing this. Thank you SO much. |
8 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):
this pr adds an experiment i've just been calling
anchored-tabs... it's pretty similar arc/zen style tab handling so they work more like "bookmarks" or "folders" -- i focused primarily at the vertical tab experiencehere's a lil video showing some of it off
zenium.lol.mp4
this was a weekend project but i've been working on how i'd approach this in my head for quite some time now, i'll be using my own fork (which just has these changes) as my daily driver from here on out so if i spot any issues i'll try and maintain it as much as i can
sorry for the yap but also worth noting; lots of hacks were required to make this work the way it does, i understand & respect this is a very large set of changes coming outa nowhere and i had considered just keeping it for myself so if it's too much of a burden i'd understand if this was put on the backburner and/or picked apart to be repurposed for a nicer PR in the future
What's missing / unpolished(?)
i'm sure upon review more will be found but:
some missing i18nContinue where you left off, so what the newOn startupsetting i've added[Experimental] Continue where you left off with tab anchoringdoes is enable the anchored tabs flag while also acting likeContinue where you left off... the fact that this was such a mouthful to explain definitely makes me think this needs a different approach-definitely out of scope, but feels missing when put into the context of what this PR is trying to achieve
Changelog
available on request, y'all seem to want short descriptions and this is already super long so i've amended it
Resolves #172 resolves #258 resolves #999 resolves #1240 resolves #1388 resolves #1404 resolves #1444 resolves #1476 resolves #1501 resolves #1503 (some only partially)