-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
Add MRU window navigation actions #977
base: main
Are you sure you want to change the base?
Conversation
The MRU actions `focus-window-mru-previous` and `focus-window-mru-next` are used to navigate windows in most-recently-used or least-recently-used order. Whenever a window is focused, it records a timestamp that be used to sort windows in MRU order. This timestamp is not updated immediately, but only after a small delay (lock-in period) to ensure that the focus wasn't transfered to another window in the meantime. This strategy avoids upsetting the MRU order with focus events generated by intermediate windows when moving between two non contiguous windows. The lock-in delay can be configured using the `focus-lockin-ms` configuration argument. Calling either of the `focus-window-mru` actions starts an MRU window traversal sequence if one isn't already in progress. When a sequence is in progress, focus timestamps are no longer updated. A traversal sequence ends when: - either the `Mod` key is released, the focus then stays on the chosen window and its timestamp is immediately refreshed, - or if the `Escape` key is pressed, the focus returns to the window that initially had the focus when the sequence was started.
When the focused window is closed during an MRU traversal, it moves to the previous window in MRU order instead of the default behavior.
Some comments before looking in-depth. First of all, MRU Alt-Tab is certainly one of the things I missed when testing out a blanket But the reasons I was so far reluctant to open an issue for this are:
Also, GNOME Shell differentiates between Alt-Tab and Mod-Tab where one of them goes across windows of the same app, and the other one doesn't. Do we need this?
How does this work elsewhere like Mutter/GNOME Shell? Does it also have a lock-in delay? Does it need to be configurable in niri? |
Something similar is available for Sway in With respect to everything moving around on its own, I was kind of happy that this was happening on workspaces that had (too) many windows. Moving through them was a hassle, arguably I should just organize my windows better ;) I know that I personally missed this feature from swayr and have been really savoring having it back! I agree with your point regarding the presumption that the next/prev bindings use the Hardcoding would mean appropriating the shortcut, which is also a problem if you have users that like things the way they are now! With the PR as it is there shouldn't be any user visible impact if they don't have bindings defined for the Wrt the Alt/Mod-Tab distinction, iirc it allows navigation in a given application's windows or in all the windows (I haven't used Gnome in a while). I haven't given any thought to how the former is accomplished. Presumably some kind of app-id filter would be needed. |
I played around with several values and settled on 500 millis but since this felt very arbitrary I made it configurable. Granted this would be an obscure setting that the majority of users would ignore except the few who think the default is terrible. |
Been mulling this over, and all I can come up with is a system that involves release key bindings, e.g.:
This would allow users to pick arbitrary modifier and key combos to trigger MRU traversal. My other ideas (e.g. auto determine leader keys) look as if they lead to clunky designs. In swayr traversal can be configured to expire after given delay (in my experience this is not always very intuitive). Any thoughts on whether it is worthwhile to start tinkering with a release binding mechanism to support this? |
Could you check what other similar programs or systems in programs use for this? I guess Mutter doesn't have any because it's only relevant for focus-follows-mouse and directional keynav, which it doesn't have. But for tiling WMs it should be relevant.
Even if you "just" implement release keybindings (I expect this to be quite involved and error-prone on its own), then you kinda just take up the whole Mod release keybinding for something that should honestly happen automatically. That doesn't sound very good. (Besides, Mod release should get bound to overview when that happens.)
Well yeah but the alternatives so far seem very clunky, making me think that hardcoding is the better solution, at least for now. Not sure |
Wrt the delays chosen for other similar solutions:
As for jumping into the release key binding adventure just for this MRU feature, Hard to argue against the simplicity of the hard-coded approach, so would you be |
Alt-(Shift-)Tab should work, yeah. Actually, we can do it like this: if the user has anything bound to Alt-(Shift-)Tab, then it will call their bind, otherwise it will do the built-in Alt-Tab. We already do this for e.g. Mod+MouseLeft. |
- Add a `PRESET_BINDINGS` containing MRU navigation actions. `PRESET_BINDINGS` are overridden by user configuration so these remain available if the user needs them for another purpose - Releasing the `Alt` key ends any in-progress MRU window traversal
These actions are configured in presets but no longer available for the bindings section of the configuration
Had been forgotten in prior commit and was using `Mod` instead of `Alt`
With the last few commits, this should be the resulting behavior. I still have to adjust to the Alt vs Mod layout but otherwise I find it quite comfortable to use. |
An alternative for the lock-in delay could be user interaction with the window, i.e. any keyboard key or pointer button (not movement or wheel) that is actually sent to the application and not suppressed by the compositor. Maybe a combination of the two, whichever comes first? Also I really dislike having |
I'm not convinced though, I often flip back between code and doc windows - the code windows get interacted with plenty, but the doc windows mostly just gets stared at. For now, my impression is that the implemented behavior feels quite natural - but that's obviously as subjective as it gets!
Same here! Although MRU/LRU is quite common in projects I've dabbled in, I do concede that it may not be as generally understood as one could hope for.
If I'm not mistaken, the "MRU" actions should not be available in the config. However they should be visible using IPC - not sure that makes any difference wrt your argument though. Anyway I don't have any strong beliefs here, so I'm perfectly happy to go along with whatever the consensus is. |
This would work I think and shouldn't be too hard to achieve (I'm guessing). I can give it a shot time allowing. |
As per suggestion by @bbb651, focus is locked-in immediately if a window is interacted with, ie. receives key events or pointer clicks. This change is also an opportunity to make the lockin timer less aggresive.
The latest commit implements the behavior suggested by @bb651 above. I haven't changed the name of the actions, will wait for @YaLTeR to weigh in on that one. |
Now that there is a more general Niri::lockin_focus method, leverage it in WindowMRU.
Tested and works great for me! I don't see a big problem with the hard coded alt-tab since that is the default shortcut on pretty much every common (and not so common) OS or desktop environment. Since niri already has a My opinion is worth less than $0.02 but I think this should suit most people's needs fairly well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking a while to get to this. Haven't given this a test yet, but here's my initial batch of comments.
- Swapped meaning of next and previous for MRU traversal - Fixed comment that still referred to `Mod` as leader key for MRU traversal instead of `Alt` - Fixed doc comments that were missing a period - Stop using BinaryHeap in `WindowMRU::new()` - Replaced `WindowMRU::mru_with()` method with a simpler `advance()` - Simplified `Alt` key release handling code in `State::on_keyboard()`
No longer perform the mru-commit/lockin_focus in the next event loop callback. Instead it is handled directly when it is determined that an event (pointer or kbd) is forwarded to the active window.
src/input/mod.rs
Outdated
.event_forwarded_to_focused_client | ||
.store(true, std::sync::atomic::Ordering::Relaxed); | ||
// focus should be locked-in. | ||
self.niri.lockin_focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess this should also happen on touch taps and tablet touches and possibly other stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I've sprinkled mru_commit()
in interactions that are forwarded to the client App (to the extent I understood what was going on). However tbh I mostly can't test out if it's really working as intended.
is_inhibiting_shortcuts, | ||
) | ||
}; | ||
if matches!(intercept_result, FilterResult::Forward) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably ok but fwiw you don't have to handle this inside. If keyboard.input()
returns None
, that means the closure returned FilterResult::Forward
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean something like match ...input(...) { None => { mru_commit(); return}, Some(None) => return, _ => ()}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now there's an if let Some(Some(bind)) = keyboard.input(...)
and it has an } else {
branch, I think you can put this line into that else branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not exactly since then it will catch Some(None)
too which is not what we want.. Idk maybe there's a cleaner way here
/// Focus the next window in most-recently-used order. | ||
FocusWindowMruNext {}, | ||
/// Focus the previous window in most-recently-used order. | ||
FocusWindowMruPrevious {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a data point, GNOME calls this "Switch Applications". Then if you manually dig in dconf, the other direction is "switch-applications-backward" with a comment of "Reverse switch applications".
- `focus_lockin` variables and configuration item renamed to `mru_commit`. - added the Esc key to `suppressed_keys` if it was used to cancel an MRU traversal. - removed `WindowMRU::mru_next` and `WindowMRU::mru_previous` methods as they didn't really provide more than the generic `WindowMRU::advance` method. - removed obsolete `Niri::event_forwarded_to_focused_client` boolean - added calls to `mru_commit()` (formerly `focus_lockin`) in: - `State::on_pointer_axis()` - `State::on_tablet_tool_axis()` - `State::on_tablet_tool_tip()` - `State::on_tablet_tool_proximity()` - `State::on_tablet_tool_button()` - `State::on_gesture_swipe_begin()` - `State::on_gesture_pinch_begin()` - `State::on_gesture_hold_begin()` - `State::on_touch_down()`
Is there a reason why this PR is still marked as Draft? Something still missing? |
It's working fine for me, however I can't really speak to the changes for touch/tablets. I'll "ready for review" it then. |
The MRU actions `focus-window-mru-previous` and `focus-window-mru-next` are used to navigate windows in most-recently-used or least-recently-used order. Whenever a window is focused, it records a timestamp that be used to sort windows in MRU order. This timestamp is not updated immediately, but only after a small delay (lock-in period) to ensure that the focus wasn't transfered to another window in the meantime. This strategy avoids upsetting the MRU order with focus events generated by intermediate windows when moving between two non contiguous windows. The lock-in delay can be configured using the `focus-lockin-ms` configuration argument. Calling either of the `focus-window-mru` actions starts an MRU window traversal sequence if one isn't already in progress. When a sequence is in progress, focus timestamps are no longer updated. A traversal sequence ends when: - either the `Mod` key is released, the focus then stays on the chosen window and its timestamp is immediately refreshed, - or if the `Escape` key is pressed, the focus returns to the window that initially had the focus when the sequence was started. Rename WindowMRU fields Improve window close handling during MRU traversal When the focused window is closed during an MRU traversal, it moves to the previous window in MRU order instead of the default behavior. Removed dbg! calls Merge remote-tracking branch 'upstream/main' into window-mru Hardcode Alt-Tab/Alt-shift-Tab for MRU window nav - Add a `PRESET_BINDINGS` containing MRU navigation actions. `PRESET_BINDINGS` are overridden by user configuration so these remain available if the user needs them for another purpose - Releasing the `Alt` key ends any in-progress MRU window traversal Remove `focus-window-mru` actions from config These actions are configured in presets but no longer available for the bindings section of the configuration Cancel MRU traversal with Alt-Esc Had been forgotten in prior commit and was using `Mod` instead of `Alt` Rephrase some comments Fix Alt-Esc not cancelling window-mru Merge remote-tracking branch 'upstream/main' into window-mru Lock-in focus immediately on user interaction As per suggestion by @bbb651, focus is locked-in immediately if a window is interacted with, ie. receives key events or pointer clicks. This change is also an opportunity to make the lockin timer less aggresive. Merge remote-tracking branch 'upstream/main' into window-mru Simplify WindowMRU::new Now that there is a more general Niri::lockin_focus method, leverage it in WindowMRU. Replace Duration with Instant in WindowMRU timestamp Merge remote-tracking branch 'upstream/main' into window-mru Address PR comments - partial - Swapped meaning of next and previous for MRU traversal - Fixed comment that still referred to `Mod` as leader key for MRU traversal instead of `Alt` - Fixed doc comments that were missing a period - Stop using BinaryHeap in `WindowMRU::new()` - Replaced `WindowMRU::mru_with()` method with a simpler `advance()` - Simplified `Alt` key release handling code in `State::on_keyboard()` Simplify early-mru-commit logic No longer perform the mru-commit/lockin_focus in the next event loop callback. Instead it is handled directly when it is determined that an event (pointer or kbd) is forwarded to the active window. Handle PR comments - `focus_lockin` variables and configuration item renamed to `mru_commit`. - added the Esc key to `suppressed_keys` if it was used to cancel an MRU traversal. - removed `WindowMRU::mru_next` and `WindowMRU::mru_previous` methods as they didn't really provide more than the generic `WindowMRU::advance` method. - removed obsolete `Niri::event_forwarded_to_focused_client` boolean - added calls to `mru_commit()` (formerly `focus_lockin`) in: - `State::on_pointer_axis()` - `State::on_tablet_tool_axis()` - `State::on_tablet_tool_tip()` - `State::on_tablet_tool_proximity()` - `State::on_tablet_tool_button()` - `State::on_gesture_swipe_begin()` - `State::on_gesture_pinch_begin()` - `State::on_gesture_hold_begin()` - `State::on_touch_down()` Merge remote-tracking branch 'upstream/main' into window-mru Merge remote-tracking branch 'upstream/main' into window-mru
Gonna give it some testing. First thing I noticed: at compositor startup, when I have auto-started windows on different workspaces, MRU doesn't go through all of the windows until I focus them the first time. And similarly, if a window is never focused on spawn (e.g. a dialog for an unfocused window, or with For mru-commit-ms config, probably put it into a new section (like the new |
It's also weird that as you're navigating the MRU, it will scroll around and mess up all your workspaces and monitors, and won't restore them back when you pick a window or cancel. This wouldn't be a problem if MRU used a dialog to select windows instead of jumping to them. But that's a chunk of work by itself so idk. |
Fair comment, I'll add in something to include windows without a focus timestamp in the MRU list. This shouldn't be any trouble.
I guess so, I don't really see anyone tinkering with this setting much. A dedicated section for only this would feel a bit bloated. So I'll just set the default 750ms as a static value and comment out the config related parts.
Yup, especially with tabbing! Improving this was actually up next on my todo-wishlist but I haven't gotten actually started anything. |
For now the value is hard-coded to 750ms
I think it would be cool to use the MRU order when closing a window, currently (in 25.02, haven't tested this PR but I don't see changes related to it) it seems to have logic to switch back to the previous window if you open a window then close it without switching focus in between, but for other cases it focuses the window to the right. This could be an option, not sure what the default should be. |
Yeah, the current logic more or less follows the behavior you'd find in tab bars (think Firefox tabs or some such). Full MRU would be annoying (think switching workspace, then immediately closing a window, you don't want it to switch back). Maybe limited MRU within a workspace. But idk, I feel that the current logic feels fairly good |
This PR adds
focus-window-mru-previous
andfocus-window-mru-next
actions tonavigate windows in least-recently-used (respectively most-recently-used) order.
This is another quality-of-life modification that many users are probably
already accustomed to from other OSs/DEs/WMs using the Alt/Cmd/Mod-[Shift-]Tab
binding.
Whenever a window is focused, it records a timestamp that be used to sort
windows in MRU order. This timestamp is not updated immediately, but only after
a small delay (lock-in period) to ensure that the focus wasn't transferred to
another window in the meantime. This strategy avoids upsetting the MRU order
with focus events generated by intermediate windows when moving between two
non contiguous windows.
The lock-in delay can be configured using the
focus-lockin-ms
configurationargument.
Calling either of the
focus-window-mru
actions starts an MRU window traversalsequence if one isn't already in progress. When a sequence is in progress,
focus timestamps are no longer updated.
A traversal sequence ends when:
Mod
key is released, the focus then stays on the chosen windowand its timestamp is immediately refreshed,
Escape
key is pressed, the focus returns to the window thatinitially had the focus when the sequence was started.
Ending the sequence when
Mod
is released works nicely under the assumptionthat the
find-focus-mru
actions have a binding that usesMod
as the modifierkey. A more general solution would involve defining a binding that triggers on
the release of an arbitrary modifier, combined with some sort of
mru-exit
(?) action.