implement new setting: fallback output mode

[khyperia]

Partially fixes #448. I have no idea what a good name for this is, I just guessed "relative behavior mode".

This PR gets most of the way there, but has a glaring hole: as discussed in #446, Jay doesn't really have support for setting keyboard_node to an OutputNode or a WorkspaceNode. This means that for workspaces with no windows, the focus gets "lost": for example, seat.show_workspace does nothing when passed a workspace with no windows, which breaks my expected functionality - if I call seat.show_workspace, then open a window, I expect the window to be opened on that workspace.

So, I intend to implement that missing functionality in a follow-up PR, as I'm guessing you'd like such a tangly PR (implementing defocusing selected OutputNodes when the output is disconnected, etc.) to be separated from this option/feature PR. Let me know if you'd instead like that to be in this PR as well, though!

(Thanks for taking the time to review my PRs and look at my issues! I really appreciate it, I'm really enjoying using Jay, it's great)

[mahkoh]

I find this a bit hard to understand. I'd prefer to have specialized settings that do one thing only. For example

pub enum WindowOpenLocation {
    Cursor,
    Keyboard,
}

/// Where to focus when closing a window.
pub enum FocusFallback {
    CursorOutput,
    KeyboardOutput,
}

[khyperia]

Hmm, I don't agree:

The setting here is for every place the codebase calls WlSeatGlobal.get_output. That would mean a setting for:

• Where the new workspace is put when set_workspace is called on a new workspace
• Where the new workspace is put when show_workspace is called on a new workspace
• Which workspace is used for Seat.move_to_output
• Which window is focused when closing a window
• Which workspace a new window is opened on
• Which workspace new floating windows are opened on
• I honestly don't 100% understand ZwlrLayerShellV1.get_layer_surface's use of get_output, and would need a bit of help coming up with a name/description for a setting for that.

Granted, a couple of those could be merged, but that's still a lot of separate settings. In my opinion it's highly unlikely that a user will want to configure different behavior for each one individually, and will set them all to the same value. Additionally, any new uses of WlSeatGlobal.get_output will need to create a new setting, and then users will have to add that new setting to their configuration file for every update that includes a new use of get_output.

It seems better to have one global setting for whether Jay tends to follow mouse-centric behavior or keyboard-centric behavior.

[mahkoh]

Makes sense.

I'd still want this setting to be renamed to something like FallbackOutputMode and be a setting on a Seat it already is.

[khyperia]

Lovely, thanks! And yes, "relative behavior mode" is a terrible name I just guessed at, FallbackOutputMode is much better, thanks, haha.

[mahkoh]

This PR gets most of the way there, but has a glaring hole: as discussed in #446, Jay doesn't really have support for setting keyboard_node to an OutputNode or a WorkspaceNode.

I think the solution is to not use the keyboard node at all in get_keyboard_output. Instead, whenever WlSeatGlobal::keyboard_node is updated, you at that point look up the output of that node and store the OutputGlobalOpt of the output in a new field last_keyboard_output. If the node does not have an output (only the root node), then that field is not updated.

OutputGlobalOpt is designed to handle the case where an output is disconnected but still referenced in other objects.

[khyperia]

look up the output of that node and store the OutputGlobalOpt of the output in a new field last_keyboard_output

I ran into some complications while implementing this:

• Nodes can move outputs. So, whenever a node moves outputs, a check needs to be added to update last_keyboard_output if the moved window is the current keyboard_node. Maybe that's just tl_workspace_output_changed? Unsure if non-toplevels also need handling.
• When calling Seat.show_workspace on an empty workspace (conceptually "focusing" the workspace), then creating a new window, I want the window on that workspace. That would mean setting last_keyboard_output to that workspace's output, without keyboard_node ever actually pointing to that output, which feels a little dirty with the naming last_keyboard_output.
  • ... and also things like moving empty workspaces between outputs technically should be detected and move last_keyboard_output as well (if that workspace is "focused"), but that's enough of an edge case that I feel like that's fine to miss.

I'm totally fine with doing those, I just wanted to run them past you to see how you feel about those vs. adding support for having a WorkspaceNode and/or an OutputNode be the keyboard_node (either would work fine).

(Looking through the code, it looks like adding support for focusing WorkspaceNodes would be easiest, as they're only destroyed in one place in code right now AFAIK - OutputNode.show_workspace, which already had code handling references to the destroyed WorkspaceNode stored elsewhere. But again, up to you, totally reasonable if you'd rather do last_keyboard_output!)

[mahkoh]

I was busy with #451 but I'll look at this next week.

[khyperia]

Thanks! No rush. (And to be clear, I haven't implemented either of last_keyboard_output or focusing WorkspaceNodes yet, that push was just renaming the setting - half-waiting for you to give feedback on which is better, but also half-waiting for me to have time/energy to fully 100% implement either approach)

[mahkoh]

How about this?

[mahkoh]

When calling Seat.show_workspace on an empty workspace (conceptually "focusing" the workspace), then creating a new window, I want the window on that workspace. That would mean setting last_keyboard_output to that workspace's output, without keyboard_node ever actually pointing to that output, which feels a little dirty with the naming last_keyboard_output.

I don't have an answer to this yet. Maybe setting prev_keyboard_node_output to the output of the workspace would work.

[khyperia]

How about this?

Mmm! Setting it to the old keyboard_node's output (not the new keyboard_node being set) is very subtle and took me a while to see, that's pretty clever.

But yeah, I'm not sure how to handle focusing an empty workspace with this way of handling it... unfortunately I'm extremely fried from coding at my day job right now :P

[mahkoh]

I'm also fine with merging this as is without solving the empty workspace issue.

[khyperia]

Sorry for disappearing for almost a year. The difficulty in implementing this kind of demotivated me and I ended up using Sway for a while instead.

I'm also fine with merging this as is without solving the empty workspace issue.

I've rebased this PR and removed the prev_keyboard_node_output/etc. attempts to solve the empty workspace issue, and am thinking of trying to work on it in a future PR.

There's three options I see as reasonable:

• Allow WorkspaceNodes to have the keyboard focus. This is what Sway implements, and how it solves the same empty workspace issue. (fwiw, Sway focuses its equivalent of WorkspaceNode, not its OutputNode, if the workspace is empty)
• Have some kind of prev_keyboard_node_output that pseudo-focuses workspaces or outputs. Some tricky code here to keep this value in sync with the "real" keyboard focus, e.g. when the keyboard-focused window moves between outputs/workspaces, the pseudo-focus needs to move as well.
• Merge Add mouse-refocus option to center cursor on focused window #665 (or equivalent PR), which effectively turns keyboard focus actions into cursor actions, so I can give up on trying to make the keyboard focus authoritative, because the cursor will be there anyway. If this is the case, this PR doesn't need to be merged, because I probably won't use it anyway.

In my opinion, the first option is the cleanest, and it's also how other compositors solve the same issue, but may not be possible with how Jay is set up. If I can't get it to work, I'll try the second, and otherwise, I guess I'll wait for the third!

In any case, if you still feel this PR is mergeable as-is, feel free to do so.

[khyperia]

Update: The one place workspaces are destroyed (OutputNode::show_workspace) already handles swapping focus if a workspace node is selected directly, by nature of how the code is written (collect_kb_foci2 picks up on the fact that the old WorkspaceNode is focused, and then the seat's focus gets refocused via ws.node_do_focus(Direction::Unspecified) on the newly shown workspace).

So, implementing focusing empty workspaces is a matter of two lines of code. I've added that as a commit in this PR, feel free to force-push-remove it and we can continue in another PR.

[mahkoh]

This all looks good in principle. I have pushed another commit that contains a few more changes I would make to integrate this better with other functionality.

[khyperia]

Can float.child be None? I could refactor this to properly fall back to focusing the workspace if it's None, but from a few moments of testing I can't figure out a way for it to be None.

[mahkoh]

I don't think it can be None. But you can replace this by && let Some(child) = ... if you want.

[mahkoh]

I've pushed another version with the workspace-focus highlighting. I don't quite remember if I had any other concerns. I'll have to do another review. But this looks fine conceptually.

Please rebase your branch to integrate my some ideas commit into yours.

[khyperia]

Please rebase your branch to integrate my some ideas commit into yours.

Cleaned up the commits, and added handling for float.child being None.

I don't quite remember if I had any other concerns

When testing this, I ran into two things:

• FocusFollowsMouse does not focus empty workspaces
  • There's a bit of a style thing that I'm not sure of. Could add WorkspaceNode::node_on_pointer_enter calling WlSeatGlobal::enter_workspace which, if focus_follows_mouse is true etc., calls self.focus_node(ws). But, this would be a bit "wasteful", focusing WorkspaceNodes just before the toplevel is focused immediately after (if the workspace isn't empty).
  • Or, could add WorkspaceNode::node_on_pointer_focus, which would only be called on an empty workspace, and calling WlSeatGlobal::enter_workspace there. But node_on_pointer_focus currently takes &self, not Rc<Self>, which would need to be changed, to be able to call focus_node(Rc<dyn Node>). (Should node_on_pointer_unfocus also take Rc<Self> for symmetry too?)
  • Or say "eh, FocusFollowsMouseMode::True with FallbackOutputMode::Focus is a weird thing to do, it's ok if it's a bit jank"
• Sway's UI (via waybar/etc.) considers a workspace "focused" if it or a child of it is focused. The bar separator being highlighted only when the workspace node itself being focused directly is a slight change to that. Tbh, I like the current behavior for the bar separator color way of showing it (especially on single monitor setups, it'd be permanently blue/focused, instead of gray), but it's a difference in behavior to keep in mind if ext-workspace-v1 gets focused workspace support.

[khyperia]

Perhaps I'll look into proposing a change to ext-workspace-v1 to add that.

Just FYI, I have done so here. No idea if folks will be receptive to it or if it'll be accepted!