Configurable mouse focus

skewballfox commented 3 months ago

resolves #564

Still learning the codebase, and right now this is just some boilerplate. As mentioned in 564 there would be 3 mouse focus policies:

focus requires click (current and default)
focus changes when mouse moves to a new output
focus follows mouse (sloppily or lazily)

The first requires 0 changes and the code for click to focus still needs to execute when following one of the other policies (if there is a delay before window focusing, clicking a window should still focus it.). So the only place code needs to be added is in the two todo!()s currently in src/input/mod.rs. I think I can probably extract the contents of this conditional into a function, with some minor changes to reuse the existing code for moving keyboard focus to a window or region.

Drakulix commented 3 months ago

Thanks for working on this!

Though after some internal discussions we have now a better idea of how this feature should exactly look like: https://github.com/pop-os/cosmic-comp/issues/564#issuecomment-2197140658

skewballfox commented 3 months ago

Thanks for working on this!

Though after some internal discussions we have now a better idea of how this feature should exactly look like: #564 (comment)

no worries. Were you guys planning on doing the refactors separately or should I attempt to do those in this PR?

specifically these changes:

To facilitate this, we will need a bit of refactoring, as to what previously was the active_output of a seat. Parts of the code assume that output will contain a focused element. But with this change conceptionally the "focused output" and the "active output" (the one with the cursor on) will now be different things.

Drakulix commented 3 months ago

no worries. Were you guys planning on doing the refactors separately or should I attempt to do those in this PR?

specifically these changes:

To facilitate this, we will need a bit of refactoring, as to what previously was the active_output of a seat. Parts of the code assume that output will contain a focused element. But with this change conceptionally the "focused output" and the "active output" (the one with the cursor on) will now be different things.

If you want to, you may very well give this a go. Basically you can just grep the whole code base for seat.active_output() and see where it afterwards makes the assumption, that the returned output has a focused element.

I don't think most cases qualify, but those that do should instead do something like seat.get_keyboard().current_focus() to get the focused element and figure out the output by querying the Shell.

Otherwise I'll get around to it eventually.

skewballfox commented 2 months ago

One thing I'm fuzzy on is the places where I've encountered active_output() being used to get the current focused window, they generally involve filtering to a CosmicMapped instance(example: handle_shortcut_action)

Are variants like popups coercable to CosmicMapped or CosmicSurface?

Asking because right now I'm trying implement a function to call during Action::Close on KeyboardFocusTarget but PopupKind::InputMethod(PopupSurface) doesn't have a useful method for this.

Drakulix commented 2 months ago

One thing I'm fuzzy on is the places where I've encountered active_output() being used to get the current focused window, they generally involve filtering to a CosmicMapped instance(example: handle_shortcut_action)

Are variants like popups coercable to CosmicMapped or CosmicSurface?

Popups might be related to their parent: https://github.com/pop-os/cosmic-comp/blob/f02520c194624d2bcd31c7d37e442754dbbd59a1/src/shell/focus/target.rs#L163

Asking because right now I'm trying implement a function to call during Action::Close on KeyboardFocusTarget but PopupKind::InputMethod(PopupSurface) doesn't have a useful method for this.

However I don't think that method works for input-method. What you probably would need to do there (and what cosmic-comp likely fails to do in other places, IME support isn't well-tested yet..) is using get_parent and then shell.element_for_surface.

skewballfox commented 2 months ago

hey for the actions affecting a workspace, such as MigrateWorkspaceToNextOutput, those should also be switched to get the focused output rather than the active output correct?

skewballfox commented 2 months ago

I'm going to assume this is the case, though if I need to roll back changes for those specific actions let me know

skewballfox commented 2 months ago

Give me a few, tried to squash the commits and now it's including changes from other commits in the list of changes

Drakulix commented 2 months ago

hey for the actions affecting a workspace, such as MigrateWorkspaceToNextOutput, those should also be switched to get the focused output rather than the active output correct?

I think for that specific one it might make more sense to use the active output, though there might not be much point into moving an empty workspace? :sweat_smile: But my gut feeling would be to use the active_output, whenever an action concerns a whole workspace and use the output with the focused element, whenever the action concerns mostly that element or focus in some way.

I'm going to assume this is the case, though if I need to roll back changes for those specific actions let me know

Sure. Do you want me to review what's already there?

I am usually busy with other stuff and will only check comments every now and then, but if you feel like you want some input on the actual code, please explicitly ping me any time. Otherwise it isn't clear to me, what is just a rebase or warrants a full review.

skewballfox commented 2 months ago

@Drakulix If you could review primarily the changes to handle_shortcut_actions (starting on line 1645 of src/input/mod.rs) I'd appreciate it. mainly to see which actions should be rolled back or if I missed any that need to be switched.

There are a few functions I'm not sure whether to convert or not:

map_window in the shell module. I suspect it should be.
update for ContextMenu in src/shell/graphs/mod.rs, mainly because I'm lacking a bit of context of for what this is
the stuff related to layers, but mainly because I'm not sure what a layer is in this context. I'm assuming it has to do with the z order or windows, but I can't see how a layer would have keyboard focus

edit: Outside of those I think I've moved all the relevant seat.active_output() calls to instead refer to the focused_output, and I added some functions to for handling focused output to seat. I tried running locally (replacing the instance of cosmic-comp in /usr/bin), and something is causing it to crash. I need to look into it more.

skewballfox commented 2 months ago

would it be okay if I added a bool argument to set_focus to indicate whether the pointer should also be updated if focus changes?

If a focus change is caused by the pointer(movement or click), then even if pointer follows focus is set it shouldn't change position.

the alternative would be to check if the keyboard focus target is different from the active focus at the current call sites, prior to calling set_focus(along with filtering for positions that would cause set_focus to return early)

if there is another option you'd rather go with, let me know

Edit: Also, would it be okay if I moved the setting of focus data (active focus, keyboard focus) inside a check to make sure the keyboard focus has changed (if keyboard focus target != previous value of active focus)

Drakulix commented 2 months ago

would it be okay if I added a bool argument to set_focus to indicate whether the pointer should also be updated if focus changes?

If a focus change is caused by the pointer(movement or click), then even if pointer follows focus is set it shouldn't change position.

Makes sense. This is the kinda behaviour that isn't readable from the config, because it depends on the type of focus change. So imo a new argument is a good solution.

Edit: Also, would it be okay if I moved the setting of focus data (active focus, keyboard focus) inside a check to make sure the keyboard focus has changed (if keyboard focus target != previous value of active focus)

Setting the focus to the same value is harmless and shouldn't cause any issues.

skewballfox commented 2 months ago

also, I got focus follows mouse working, at least on the local system I've been using for testing

EDIT: update, found a bug with focus follows mouse where if a panel menu (like wifi or system panel) overlaps a window, moving the pointer over the region of the window occluded by the menu focuses the window. I'm guessing those menus don't show up as pointer focus targets. I think this might be relevant if they are subsurfaces. The most recent issue fixed it for overlays, but I haven't figured out how to determine if the keyboard focus target is the top level surface within it's own layer yet, so things like trying to hover over a docked app menu will still focus the underlying window.

also, cursor_follows_focus doensn't work at all, I'm guessing ptr.motion doesn't do what I was expecting.

skewballfox commented 2 months ago

so it turns out the issue might not be related to layer order, and instead a lack of delay for refocus. I can only hover over an overlay panel if I drag the mouse down the side of the output where the window isn't present, then onto the panel. I think the gap between the panel and the popup is stealing focus. I'll roll back that latest commit locally to retest and confirm in the morning.

skewballfox commented 2 months ago

So I looked a bit more into how to handle delaying focus enough to allow time to move the cursor to windows floating over the a focusable window. If we do go with the option to use a delay, we can likely do so with self.common.event_loop_handle.insert_source and a calloop::timer::Timer.

An alternative (and apparently preferred way is this comment about gnome's implementation is still accurate) is to delay changing window focus until the pointer has stopped moving. I haven't figured out how to detect that though.

skewballfox commented 2 months ago

@Drakulix This is ready for review. both settings are working. Thanks for all the help

just to help with you or anyone else who wants to test the new settings

echo true > ~/.config/cosmic/com.system76.CosmicComp/v1/focus_follows_cursor
echo true > ~/.config/cosmic/com.system76.CosmicComp/v1/cursor_follows_focus

juarezr commented 1 month ago

Adding a third possible mode:

Mouse follows focus
Useful especially with multiple monitors and keyboard-centric workflows (Alt+tab and Focus prev/next/last keys)

skewballfox commented 1 month ago

@juarezr that's actually what "cursor follows focus" is: the pointer warps to the top left corner of the newly focused window if that focus change was caused by a keyboard event.

it's been implemented in the PR.

juarezr commented 1 month ago

@juarezr that's actually what "cursor follows focus" is: the pointer warps to the top left corner of the newly focused window if that focus change was caused by a keyboard event.

it's been implemented in the PR.

@skewballfox ,

Amazing work!

IMHO, it is still possible to improve the "cursor follows focus" behavior a bit:

You can move the cursor to the last position it was placed in the window instead of always moving to the top left corner.
By doing this you take advantage of the locality.
This is a very basic heuristic but it helps to place the cursor in areas where the user clicks most of the time or in the place it interrupted a previous workflow/task.
This behavior is implemented in forge-ext/forge#310. If by chance you want to try this behavior, just install this gnome extension.
Of course, this is just a suggestion. ;)

skewballfox commented 1 month ago

@Drakulix This is ready to be tested again. Let me know if I should keep or revert the cursor_follows_focus changes. No rush though.

Drakulix commented 4 weeks ago

A couple of issues I am still noticing using this PR:

All three config options don't reload at runtime. This needs some changes here: https://github.com/pop-os/cosmic-comp/blob/master/src/config/mod.rs#L571
Changing the active output, doesn't take away focus for empty outputs (as intended!), but the focus indicator still vanishes, this probably needs some changes in the rendering code.
Toggling tiling/floating doesn't warp the cursor (should it?)
Switching between workspaces doesn't warp the cursor, if the new target isn't below it (should it?)

On the latter two, I am not sure how aggressive people would expect "cursor_follows_focus" to be, but both scenarios feel like a significant enough change to me, that I would expect it to warp.

However I am certainly not the target audience of any of these config options, so I am very much open to discuss those.

skewballfox commented 4 weeks ago

EDIT: first two fixed

On the latter two, I am not sure how aggressive people would expect "cursor_follows_focus" to be, but both scenarios feel like a significant enough change to me, that I would expect it to warp.

If I'm being honest, also not the target audience for cursor_follows_focus, or at least I've never used it/seen it in another DE. I don't think it should trigger for events caused by the mouse (such as clicking to launch something from the doc), but that's something I could live with if it's the desired behavior.

I can try to extract, add a function to fire when the focus doesn't change, but for events like the focused window warps into another position

Switching between workspaces doesn't warp the cursor, if the new target isn't below it (should it?)

you mean below the cursor right? as in two windows in the new workspace, the last focused on that workspace was window y, but window x is now under the cursor after the switch?

theshatterstone commented 3 weeks ago

Hello. I am a Hyprland user, where I have mouse_follows_focus enabled and when it comes to the last 2 bullets by Draculix, toggling between tiling and floating does NOT warp the cursor or focus, and switching between workspaces also doesn't affect or change the focus. The only case in which mouse_follows_focus affects the mouse is when the focus changes via a keybind. So in terms of the way it should work, I don't think such things like points 3 and 4 should be the case.

skewballfox commented 3 weeks ago

It's good to go now

Drakulix commented 3 weeks ago

Thanks again for all the work and feedback that went into this :tada:!

KaKi87 commented 3 weeks ago

Hi, How to know when this will be available from APT updates ? Thanks

leviport commented 3 weeks ago

I can fit it into today's update release.

KaKi87 commented 3 weeks ago

Wow, wonderful, thanks !

theshatterstone commented 3 weeks ago

Hello, I know this is far from the best place for it, but does anyone know where/how should I report that cosmic-comp at the COPR repo failed to build, so these don't work yet?

Drakulix commented 3 weeks ago

Hello, I know this is far from the best place for it, but does anyone know where/how should I report that cosmic-comp at the COPR repo failed to build, so these don't work yet?

https://pagure.io/fedora-cosmic/SIG

theshatterstone commented 3 weeks ago

I emailed the COPR maintainer and they fixed it, thanks! Though it does feel like there's a slight delay in focus switching.

skewballfox commented 3 weeks ago

@theshatterstone the delay is adjustable. the default is 250(milliseconds), you could set it to 100ms like this:

echo 100 > ~/.config/cosmic/com.system76.CosmicComp/v1/focus_follows_cursor_delay

pop-os / cosmic-comp

Configurable mouse focus #582