Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

rustn00b
Copy link
Contributor

This PR adds focus-window-mru-previous and focus-window-mru-next actions to
navigate 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 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.

Ending the sequence when Mod is released works nicely under the assumption
that the find-focus-mru actions have a binding that uses Mod as the modifier
key. 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.

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.
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 14, 2025

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 open-floating true workflow for a week. And you can't implement MRU well outside the compositor.

But the reasons I was so far reluctant to open an issue for this are:

  • I'm used to it having a GUI switcher like in GNOME Shell. Especially in the case of MRU across workspaces, it helps when you can visually pick a window without everything moving around underneath you.
  • It's unclear how to bind it with the current system because it is not a discrete action. You mention that it kind of assumes Mod currently; I'm not sure it's a good idea to have it bindable at all if it relies on that assumption to work properly. Besides, I'd expect it to be Alt-Tab rather than Mod. And pressing Shift-Tab instead of Tab should go backward, etc. Tbh I'd almost prefer to just hardcode it and not expose as an action for now, until we figure out a better way?

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?

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.

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?

@rustn00b
Copy link
Contributor Author

rustn00b commented Jan 14, 2025

Something similar is available for Sway in swayr (with a lockin delay). The delay is nice to ignore focus updates resulting from navigation actions with no "semantic" meaning. For instance, if you have focus-follow-mouse active, the windows you move over while on your way to your intended target probably shouldn't cause your "mental" map of recent windows to diverge with the WM's version. Likewise, with scripts that perform multiple focus updates over IPC when these updates are just a means to designate the target of the following actions. For instance, I have one that resizes a bunch of columns at once by focus each one and then setting their width and then finally restores the focus to the window that originally had the focus when the script was called. As a user I don't need to be aware these focus events ever happened.

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 Mod key. A more generic approach would be to detect the (first?) modifier key for these MRU actions and end the MRU sequence when that modifier key is released. Of course for it to feel right for the user, the modifier would need to be the same for both actions (not an unreasonable assumption).

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 focus-window-mru actions.

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.

@rustn00b
Copy link
Contributor Author

Does it need to be configurable in niri?

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.

@rustn00b rustn00b marked this pull request as draft January 15, 2025 08:56
@rustn00b
Copy link
Contributor Author

Been mulling this over, and all I can come up with is a system that involves release key bindings, e.g.:

  • 1 binding for focus-window-mru-previous, say Mod-Tab
  • 1 binding for focus-window-mru-next, say Mod-Shift-Tab
  • 1 release binding on Mod that cancels any active MRU traversal.

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?

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 15, 2025

I played around with several values and settled on 500 millis but since this felt very arbitrary I made it configurable.

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.

Been mulling this over, and all I can come up with is a system that involves release key bindings

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.)

Hardcoding would mean appropriating the shortcut, which is also a problem if you have users that like things the way they are now!

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

@rustn00b
Copy link
Contributor Author

Wrt the delays chosen for other similar solutions:

  • swayr: 750ms "focus lockin_delay" by default swayr-configuration
  • mutter: seems to have had a 150ms at some point (not sure about
    the exact value bcs there are references stretching from 50ms to
    200ms in various forums and mailing lists), but the delay was applied
    before focus was transferred to the target window (and this annoyed at
    least some users: https://gitlab.gnome.org/GNOME/mutter/-/issues/888). This
    delayed-focus system appears to still be present and can be toggled with the
    focus-change-on-pointer-rest gsetting in org.gnome.mutter.

As for jumping into the release key binding adventure just for this MRU feature,
it does indeed look like a tail-wagging-the-dog kind of situation ;)

Hard to argue against the simplicity of the hard-coded approach, so would you be
in favor of Alt-[Shift-]tab then? What would the mapping in
the winit case then be, Modifiers::ISO_LEVEL3_SHIFT-[Shift-]tab?

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 15, 2025

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`
@rustn00b rustn00b marked this pull request as ready for review January 16, 2025 17:17
@rustn00b
Copy link
Contributor Author

rustn00b commented Jan 16, 2025

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.

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.

@bbb651
Copy link
Contributor

bbb651 commented Jan 17, 2025

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 mru in the name, it's non-obvious to the point of forcing people to look it up, a config should generally prioritize clarity over conciseness and the rest of niri is really good at that. I can't think of a good name, but even focus-window-recent-previous and focus-window-recent-next would be better, it's slightly broken English but it at least gives you a general idea. (I think just focus-window-previous/next isn't clear enough, and should be reserved for logical direction actions).

@rustn00b
Copy link
Contributor Author

rustn00b commented Jan 17, 2025

any keyboard key or pointer button (not movement or wheel) that is actually sent to the application and not suppressed by the compositor

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!

I can't think of a good name

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.

a config should ...

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.

@rustn00b
Copy link
Contributor Author

Maybe a combination of the two, whichever comes first?

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.
@rustn00b
Copy link
Contributor Author

The latest commit implements the behavior suggested by @bb651 above.
It turns out that this change allows for a less aggressive lock-in timer while at the same time feeling responsive to user inputs. Thanks to @bb651 for the great suggestion!

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.
@20after4
Copy link

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 focus-window-previous action (which is what I was using before trying this PR) and since you can override the hard coded mapping, there seems to be enough flexibility to be generally useful.

My opinion is worth less than $0.02 but I think this should suit most people's needs fairly well.

Copy link
Owner

@YaLTeR YaLTeR left a 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.

@rustn00b rustn00b marked this pull request as draft January 26, 2025 19:08
- 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();
Copy link
Owner

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?

Copy link
Contributor Author

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) {
Copy link
Owner

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.

Copy link
Contributor Author

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, _ => ()}?

Copy link
Owner

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?

Copy link
Owner

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

Comment on lines +220 to +231
/// Focus the next window in most-recently-used order.
FocusWindowMruNext {},
/// Focus the previous window in most-recently-used order.
FocusWindowMruPrevious {},
Copy link
Owner

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()`
@YaLTeR
Copy link
Owner

YaLTeR commented Feb 11, 2025

Is there a reason why this PR is still marked as Draft? Something still missing?

@rustn00b
Copy link
Contributor Author

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.

@rustn00b rustn00b marked this pull request as ready for review February 11, 2025 19:40
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
@YaLTeR
Copy link
Owner

YaLTeR commented Feb 13, 2025

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 open-focused false), then it is not added to the MRU, although it should (at least to the end).

For mru-commit-ms config, probably put it into a new section (like the new clipboard), but need to think of a name. Or maybe just don't have it in the config at all for this PR?

@YaLTeR
Copy link
Owner

YaLTeR commented Feb 13, 2025

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.

@rustn00b
Copy link
Contributor Author

MRU doesn't go through all of the windows until I focus them the first time

Fair comment, I'll add in something to include windows without a focus timestamp in the MRU list. This shouldn't be any trouble.

Or maybe just don't have it in the config at all for this PR?

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.

it will scroll around and mess up all your workspaces and monitors

Yup, especially with tabbing! Improving this was actually up next on my todo-wishlist but I haven't gotten actually started anything.

@bbb651
Copy link
Contributor

bbb651 commented Feb 22, 2025

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.

@YaLTeR
Copy link
Owner

YaLTeR commented Feb 22, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants