gh-8460: Workspaces sync [2.0]#12531
Conversation
|
Can you also add sync support for Zen Mods if possible? |
Not in scope of this PR. |
|
Will unpinned tabs be synced in the future? |
|
They should be with this PR |
|
that there are now two competing PRs of the same feature shows pretty clear how important this feature is. i hope this will get merged soon. Have you looked at the code from the other guy? maybe he has some solutions for the issue you still have. syncing only on restart seems kinda weird as many people (at least the ones i know) never restart their browsers ever. |
|
We want it to be live syncing, so don't worry about that, this is just a POC. There are some main blockers though because we are not sure how the merging mechanism should work. Should it completely override? Merge both sessions? ... |
My two cents: just prompt the user when there's a conflict, like Steam does |
I would love for it to be treated as just one session - as if all Zen windows on all of my computers belong to the same browser profile, have the same tabs, etc. I imagine sync can get very complex, though. Perhaps failures would be more forgiving if there was a way to easily recover - if some other device closed 100 tabs, there should be a way to recover them with a shortcut like |
|
On the matter of unpinned tabs, please have that be a separatly configurable sync item from workspaces & pined tabs if it is reasonable to implement as such. I personally wouldn't want my unpinned tabs syncing automatically since my primary use for sync is between windows and Linux on the same device. I tend to have very different sets of tabs between the two and would just end up closing all the synced tabs anyways. For the few cases where I want an unpinned tabs to move between them I would just use the send to device feature like I currently do. If they aren't separate it isn't a deal breaker for me but I would appreciate not having the extra overhead if I can avoid it. |
|
Please mark it as ready for review once its done! There seems to be git conflicts as well? |
c853025 to
3354147
Compare
|
@kristijanribaric do you need some support on the PR? I'm down to help out. What's the current status? |
|
While the sync engine functions correctly, the conflict resolution logic could use some improvements, more specifically tab/workspace/folder creation and updating. With the current version, tabs can sometimes be created in the wrong workspace or outside of a folder while they should be in the folder. I didn't have time to go through all these edgecases, you are welcome to give it a go yourself |
|
just migrated from arc and this is easily the biggest thing i miss. workspace sync across mac + windows is what made arc's multi-device setup actually work for me, and getting this merged would be huge for the stream of arc refugees coming over. happy to test this on my setup when it's ready - got 4 workspaces with nested folders + essentials + ~250 pinned tabs already migrated from arc, so there's real data to throw at it across mac and windows. let me know if helpful. |
…ontextId not being properly aplied after sync
mr-cheffy
left a comment
There was a problem hiding this comment.
Note: We shouldn't sync tabs with zen-live-folder-item-id attribute, those are automatically generated anyways.
Also, when does a resync actually trigger? Would be nice to only do it after an requestIdleCallback
| * @returns {object} A deep clone of the sidebar data. | ||
| */ | ||
| getSidebarData() { | ||
| const sidebar = this.#sidebar; |
There was a problem hiding this comment.
this.#sidebar is already deep cloned
There was a problem hiding this comment.
Has this been fixed? @kristijanribaric I see you are still doing the JSON thingy
| return JSON.parse(JSON.stringify(sidebar)); | ||
| } | ||
|
|
||
| replaceSidebarData(sidebar, soon = true) { |
There was a problem hiding this comment.
I kept this separate intentionally.
saveState() rebuilds sidebar/session data from window state, runs the normal save pipeline, and is meant for local persistence.
Here I only want a raw replace-and-persist helper for sync apply, because incoming sync already produced the sidebar state I want to write. Calling saveState() here would immediately re-collect from the current windows and could overwrite or reshape the freshly merged sync result before the UI/apply path finishes, it would be inpredictable
So replaceSidebarData() exists to do the narrower thing, accept already prepared sidebar data, store it, and persist it, without another live collection step.
| return; | ||
| } | ||
| this.#notifySyncItemChanged(tabGroup); | ||
| Services.obs.notifyObservers( |
There was a problem hiding this comment.
Global here just means the singleton meta record, not a true browser-global object. Naming is hard.
… add createSyncableTabData method to ZenSyncManager.sys.mjs; refactor sidebar normalization and tab sorting; simplify tab data creation; remove redundant event listener removal in ZenWindowSync.sys.mjs; streamline tab data cleaning in ZenWorkspacesSync.sys.mjs
… and regular tabs
… handlers; fix potential id clashing when creating new containers; refactoring
| } | ||
|
|
||
| async delete() { | ||
| Services.obs.notifyObservers( |
| }); | ||
| // createLazyBrowser can't be pinned by default | ||
| this.window.gBrowser.pinTab(tab); | ||
| if (userContextId) { |
| if (originalIsEssential !== targetIsEssential) { | ||
| if (originalIsEssential) { | ||
| gZenPinnedTabManager.addToEssentials(aTargetItem); | ||
| gZenPinnedTabManager.addToEssentials(aTargetItem, { force: true }); |
|
|
||
| if (isTab) { | ||
| if (aOriginalItem.hasAttribute("zenDefaultUserContextId")) { | ||
| aTargetItem.setAttribute("zenDefaultUserContextId", aOriginalItem.getAttribute("zenDefaultUserContextId")); |
There was a problem hiding this comment.
You can use #maybeSyncAttributeChange
| isEssential: originalIsEssential, | ||
| isPinned: originalIsPinned, | ||
| }); | ||
|
|
| return; | ||
| } | ||
| this.#notifySyncItemChanged(tabGroup); | ||
| this.#notifyMetaChanged(); |
There was a problem hiding this comment.
Why would a group creation notify a meta change? What's the purpose of "meta changed"?
| this.#notifySyncItemChanged(tabGroup); | ||
| this.#notifyMetaChanged(); | ||
| this.#delegateGenericSyncEvent(aEvent, SYNC_FLAG_MOVE); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
Please call this.on_TabMove(aEvent); and return that
| await this._removeSyncedItems(removals); | ||
|
|
||
| // 3. Create/update pulled folders and tabs | ||
| await this._applyPulledItems(pulled); |
There was a problem hiding this comment.
nit: Private functions should be prefixed with #
| delete existingTab.zenStaticLabel; | ||
| const activeEntry = this.#getSyncedTabActiveEntry(tabData); | ||
| if (activeEntry?.title) { | ||
| gBrowser._setTabLabel(existingTab, activeEntry.title); |
There was a problem hiding this comment.
Changing the title and image without checking the tab is pending?
|
Really suffering from cognitive overload reviewing this PR 😅 We should've had divided it into smaller PRs |
|
I also thought about doing this in iterations. I think going into this issue with a one-shot PR is a bad idea because it's also tough to find out where sync issues happen between workspaces, folders and tabs. How about we try to do this in multiple steps with reimplementing actual Workspace sync first and attack tabs and folders later when workspaces sync correctly. |
|
@kristijanribaric should I make a spaces-sync branch and we can do smaller PRs to that branch instead? |
Don't you think that it will slow down that feature to months since you have already reviewed most of it? //it is just an opinion; it is on your behalf of course. Just still waiting for nightly release where I can sync my 5 computers to have a synced workspace already |
|
It won't slow it down to months, but it will definitely slow it down for some time, but if that means shipping a more polished feature, so be it, currently working on making spaces only sync |
I think the problem with a one-shot approach is that it's hard to stabilize all parts of it. I gave this two shots as well already and always run into syncing issues on one end or the other. This is not just to make reviewing easier but to actually proof one sync-feature is stable and complete. That's why I'd propose to move back again and look into getting Workspaces to sync in a reliable way first (including associated Firefox Tab Containers if possible/needed). When this is stable, merged and in production we could look forward into syncing tabs and folders next without having to bother that Workspaces behave flakey etc. |
A new sync implementation built on top of Firefox's existing Sync infrastructure, enabling cross-device synchronization of workspaces, containers, pinned tabs, and folders, reworked from the ground up.
What syncs
Changes
ZenWorkspacesStore,ZenWorkspacesTracker, andZenWorkspacesEngineto manage tracking and storing workspace changes across devicesZenSessionManagerto properly merge incoming remote data with the local sessionContextualIdentityService.sys.mjsto allow creating containers with an explicit ID, ensuring pinned tabs and workspaces reference the correct container across devicesservice.sys.mjsto register the new workspace sync engineWhile overall sync works, it needs further testing and refinement, thx
Update
Due to the large scope, this PR is getting split into fewer, more focused PR-s: