Fix Tracking Multiple Sessions #1187
Conversation
|
Thank you for your PR. |
|
@sienori I feel like there's a fundamental misunderstanding of what this PR does. I really wish you had given me some time to respond to this feedback so that we could have a discussion on the best way to move forward. Please see the original issue (#1186) and review the PR more thoroughly. There are several issues at play here that amount to a bug. First:
This PR fixes both of these issues while providing multiple safeguards that doesn't ruin user's tracked sessions:
Additionally, there's an settings option that can be turned if a user wants to be able to remove a window from a multi-window tracked session by closing one of the windows:
If you'd like me to move the option to remove a window from a tracked session to a new PR, I can certainly do that and we can discuss whether or not it should be an option that users should be able to choose to have if they'd like. But the original issue is still a bug that needs to be addressed. Let me know how you'd like to proceed. |
| }; | ||
|
|
||
| const deleteWindowFromSession = async (sessionId, originalWindowId, openedWindowId) => { | ||
| if (!getSettings("shouldTrackDeletedWindows")) return; |
There was a problem hiding this comment.
Unless the user has explicitly turned on shouldTrackDeletedWindows, no windows will removed from a tracked session.
| if (!getSettings("shouldTrackDeletedWindows")) return; | ||
|
|
||
| // If this was the last window for this session, we don't want to remove it | ||
| if (trackingWindows.filter(x => x.sessionId === sessionId).length === 0) return; |
There was a problem hiding this comment.
Prevents the last window in a tracked session from being removed from a tracked session.
| const focusedWindow = trackingWindows.find(x => x.openedWindowId === trackedWindowId || x.originalWindowId === trackedWindowId); | ||
| // If the last window that we were in isn't being tracked, don't track the new window | ||
| if (!focusedWindow) return; |
There was a problem hiding this comment.
BUG FIX for #1186: Prevents a tracked session from hijacking another session's window.
|
It’s not obvious to me that shoving new windows into sessions based on where the last focused window came from is any better than lumping them together into one session. Most importantly, how does it handle things like As things stand, opening additional ‘tracking’ sessions behaves in a way consistent with other state modifications and doesn’t require any special casing. Frankly, mapping browser state onto several active sessions seems like an inherently bad model to me. The browsers and window systems that we have today weren't designed with that in mind. It’s almost like instead of these per-session tracking flags there should be a single global flag that causes a continuous session reflecting whatever is currently opened to be maintained, with all other sessions treated as simple static snapshots. On the other hand, it does seem bad that closed windows pile up in tracking sessions until they are manually removed. This is particularly useless when the option for saving the former as bespoke sessions is active. The same logic that guides the ‘Save the session when exiting browser’ option can likely be reused to account for the exit scenario. |
|
@gldrk As I've been using this as my daily driver version of Tab Session Manager since I wrote the PR, I can tell you its very stable and intuitive, although it may not be immediately obvious simply by looking at the code given how simple the implementation of the solution was. The bug exists, the question is how do we handle it?First, the original issue in #1186 is still present. Opening up multiple tracked sessions appends the original session with the newly opened session's windows. Expand to see the original reproduction steps
As the extension sits right now, there can realistically only be a single tracked session open at once. Any additional sessions that are opened affect the first tracked session. This is incredibly dubious and requires manual intervention to "clean" sessions that have been polluted by another session's windows. The proposed solution is not to change the behavior, but to maintain it.Remember, opening multiple tracked sessions already exists. And, tracking new windows on a tracked session already exists. The difference is that Tab Session Manager does not properly track against multiple sessions. It falsely shows that both are being tracked, but tracks changes to the second session against the first. Opening a second tracked session adds all those windows to the first tracked session. It's bugged. This fix only affects users who are trying to use multiple tracked sessions at once. Tracking new windows against tracked sessions already exists.
This behavior is because I have already made the conscious decision as a user to enable "Track New Windows" for tracked sessions. In the event that I open a particular window that I don't want tracked to a session, the workflow is the same as it exists today: Removing the new window from the tracked session ensures that it is no longer tracked. This manual intervention is already required today, and it's an edge case that exists when you're breaking out of your normal workflow and there's a specific new window that you don't want tracked against your tracked session. Give it a try! 🙂If you're worried that it might not behave intuitively, I would encourage you to pull it down and test it yourself. Again, I use this on a daily basis, and it has definitely maximized my use-case and workflows while using this extension. Alternative: Allow it, or don't.If the alternative is to not allow opening multiple tracked sessions altogether, then that should be implemented, but I don't see the benefit in limiting a user to only one tracked session at a time. Either way, #1186 is an entirely valid bug, and I would highly recommend reading through it to get a better understanding of the root of the problem. Bonus: Keeping sessions cleanThe only bonus of this PR was the ability to turn on an option to automatically remove closed windows. That's not a bug, but it is a feature, and it too is gated, meaning that a user has to opt-in to this functionality. Bonus for the Bonus: "Close Session" optionAdditionally, if the worry is that a user should have the ability to close all windows in a session in one go without closing the entire browser, there's absolutely room to add an option to "Close Session", which would close all windows in an open session without removing them from the tracked session. |
RefocusingRemember, this bug is for:
This PR ensures that:
The only thing new that this PR introduces is an optional feature that will:
Current FunctionalityThis is existing behavior, not the behavior after this PR.
After this PRThis is how it functions with this PR
|
|
To demonstrate this, using the Current Version of Tab Session Manager, I did the following:
This PR also fixes that bug by making sure that New Windows are tracked against the Session you're actively working in. ScreenshotsSession 1 after opening both sessions and creating the first new window for Apple.com Session 2 after opening both sessions and creating the first new window for Apple.com Session 2 after opening both sessions and creating two new windows for Apple.com and Yahoo.com (the latter while focused on the Session 2 window) |
|
for me, a more ideal solution might be:
well, if default tracking the
|
|
@Binly42 That simply adds complexity and ambiguity. The methods I outlined simply implement the way that the browser currently behaves already in all scenarios. Introducing an additional layer will simply confuse a user. If I focus on a specific browser window, Alt+Tab to a different application, and open a link, the browser creates a tab on the last window I focused. This is true when I'm using Firefox and Chrome. The same is true if I'm running multiple profiles in Chrome. If my Work Profile is focused and I click on a link in another application, or I open a new window, that window or tab is tied to my Work Profile. The same is true if I then tab over a window that's attached to my Personal Profile and open a new window, or a link from an outside application. Last focused is the default behavior for all browsers when tracking this type of functionality, and it's most intuitive for the user. The last thing you want to do is add more layers of complexity. That doesn't solve the ambiguity, it adds to it. |
|
@Swivelgames |
|
@sienori I appreciate that! I tried to keep emotion out of it, because it would've been unprofessional and wouldn't have helped anything, but I'll admit I was quite frustrated that it was closed so abruptly 😂 No hard feelings, though, honestly! I understand it takes quite a bit of effort to continue to maintain popular repos. But I am quite glad this issue was fixed! I actually stopped using Tab Session Manager because of the issue after a while. I kept finding myself coming back to this and reimplementing my patch in order to continue to be able to use it on a daily basis, though. Tab Session Manager is incredibly useful, on top of the built-in Profiles. As for window deletion, I was daily driving this PR for quite a while with the option to delete windows intuitively, and it was quite intuitive and wasn't destructive 🙂 Let me know if you'd be interested in an additional contribution to add an optional setting to allow for window deletion. 👍 |
This fixes #1186 with the following changes:
This greatly improves the consistency of Session Tracking.