-
-
Notifications
You must be signed in to change notification settings - Fork 844
feat(panes): FocusLastPane action
#4241
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
base: main
Are you sure you want to change the base?
feat(panes): FocusLastPane action
#4241
Conversation
a554baa to
2efba34
Compare
279a7da to
14d8b71
Compare
|
Hey @DawidPietrykowski - thanks for putting this together. I have not tested this, but unless I missed something going over these changes, I think this falls into the usual trap we have with our architecture in regards to the CLI. Zellij is a true multiplayer application, meaning each connected user has its own cursor and can be focused on different panes as well as different tabs. If two users are focused on the same pane in the same tab and one of them issues this command, Zellij does not have a good way of distinguishing which user should have its focus switched. With a keybinding we don't have this problem since those are user-aware (being Zellij entities). I realize there are existing CLI actions that do this, but these are legacy and do not behave well in the above scenario. I would opt for fixing them before adding new ones with similar behavior. The proper fix for this case (and many others), would be to add a mechanism that would detect the proper connected user to operate on in a robust way. This would be something like: find the last user to issue input in this pane while being focused on it and switch that user. If you'd like to work on such a mechanism in a separate PR (optionally adding all the missing CLI actions to it as well), I'd be happy to review and accept that contribution and then we can move on to this one, which has an additional problem: we already have a last-focused mechanism, but that one is suffering from issues this one likely will in the future: it needs to be manually updated by each method that changes focus. Invariably over the years in a code-base this is "forgotten" and not all methods update it. It would be best to add a mechanism that does this implicitly so the developer does not need to remember to do it (might be that this solution handles it better, I have not looked deeply into it). Let me know if you'd like to work on that mechanism and need pointers. |
|
Hi @imsnif, thanks for taking a look at this. I can see your point, though adding such mechanism and making sure it covers as many edge cases as possible + modifying exiting code that changes panes to use a common interface feels like a bigger effort. I'd consider working on it but I'd like to focus on the scrollbuffer performance first (from #4237 (comment)). I suppose this PR could just wait for those changes to happen first, unless you're open to one last action before the refactor :) Also, what specifically are you referencing here?
|
|
Hey @DawidPietrykowski - I just want to make sure our wires didn't get crossed somewhere: would you still like to work on one of these (either the grid refactor or the action mechanism refactor)? I hope to get a proper CLI interface that would be able to identify clients and properly reply on finished actions into the next version (which should allow for this PRs functionality), either as a contribution if you'd like to work on it or if I find time to do it myself in between the other requirements. Let me know. |
|
Hi, I'm still working on th grid refactor but it's a bigger effort than I initially thought so combined with personal stuff it's taking a bit more time. I didn't touch the CLI action mechanism yet so feel free to make changes in this area. Would be great to get this functionality merged so I'm happy to hear that it'll potentially get unblocked soon. |
Yeah, it's a humongous task to be sure. Respect even just for sinking your teeth into it. Feel free to ask on Discord if you have any questions. |
14d8b71 to
34ad9b9
Compare
34ad9b9 to
2940970
Compare
2940970 to
d39d8f9
Compare
This PR adds
FocusLastPaneaction as described in #4142The "last pane" is considered per-user per-tab per-mode. Which means that:
Tested quite extensively locally and I'm pretty sure that most of the cases are covered. At least I couldn't break it but there could of course be something I missed.
Some notes:
FocusPreviousPane, focus based on direction or mouse clicks save the last paneAlso added tests and keybinds (
Alt \for default and;fortmux)