fix: switching workspaces on secondary monitor

[notTamion]

If i understood the reason for this "re-focus" correctly this should be the proper fix for the problem. When having mouse follows focus disabled and trying to switch the workspace on another monitor by first switching focus to that monitor and then changing workspace it doesn't matter which monitor you have focused as it will automatically focus the monitor your cursor is on

[LGUG2Z]

I was just thinking about doing this last night! 😅

Will build this tonight when I'm back home, but I think it looks good 🤞

[alex-ds13]

But this will break the fix that it is supposed to be doing! No matter if you have mff enabled or not when you have an empty workspace on one monitor, you are not able to focus it, since there is no container to focus, so you will never be able to change workspaces on that monitor. This code is made so that you can move your mouse there and be able to change workspaces.

I thought about this before and I think we could add a new setting like mouse_follows_monitor or some other name where the mouse would follow focus but only when changing focus across monitors, any other focus change wouldn't move the mouse. This way when you change focus to another monitor, the mouse would move and any command you call will work on the monitor you just focused.

This "mfm" should not be as annoying or intruding as the "mff" and could be set to true by default...

[alex-ds13]

Also this will create a new issue. Imagine two monitors with a bar on each monitor (komorebi-bar or YASB). Let's say there are multiple workspaces on both monitors, but currently both monitors have workspace with index 0 focused. You have a container focused on monitor 1, but now you want to change the workspace on monitor 2. You use the mouse to go to the bar on monitor 2 and press the button for a workspace (let's say it is workspace index 2), nothing happens on monitor 2 like you wanted, instead the change will happen on monitor 1 because it was the focused one. This is not the expected behavior at all, you don't want users to have to use a key bind to focus to another monitor to be able to use the mouse on the bar...

[notTamion]

you are not able to focus it

unless i have been doing something wrong in my testing, this is false. i was able to focus an empty monitor by using alt+l/k and then use alt+0-9 to switch to a different workspace.

regarding your second comment. this is indeed an issue. however i personally don't think just just having your mouse on a monitor warrants that monitor being the selected one. one idea i had was catching if the user clicks on the empty desktop and then setting the focus to that monitor. also is there a reason the bars don't specify a monitor when using those widgets? (considering focus-monitor-workspace exists)

i will come back to this tomorrow

[LGUG2Z]

also is there a reason the bars don't specify a monitor when using those widgets? (considering focus-monitor-workspace exists)

This is a good call out, we have the monitor index in the bar config file and we should use this when interacting with the workspaces widget.

[LGUG2Z]

i was able to focus an empty monitor by using alt+l/k and then use alt+0-9 to switch to a different workspace

I think it's worth focusing on reproducing this, tracking down any system settings that may be resulting in different behaviour for different users

[notTamion]

I think it's worth focusing on reproducing this, tracking down any system settings that may be resulting in different behaviour for different users

is this not an intended feature? in my opinion it should be

[notTamion]

Here is how empty monitors should be treated like in my opinion: you should be able to select them by using hotkeys (the cli commands) or by clicking on the desktop so you can switch between workspaces, spawn new applications or do other operations on them. bar applications should be responsible for telling komorebi what monitor they want their operation to be done on instead of relying on komorebi focusing whatever the mouse is on right now (considering the cli commands state that they will apply the operation to the "focused window/container/workspace/window" and say nothing about them changing the focus)

[LGUG2Z]

is this not an intended feature? in my opinion it should be

At this point I just want to understand what the factors are which lead to different behaviours for different people

Komorebi has a long standing history of not changing system settings on behalf of the user, and this isn't going to change. However if there are some settings which can be set to provide a consistent experience with regards to this issue, we can document them and add the to the quickstart guide for users to opt in to.

[LGUG2Z]

RE: Bar changes, they were implemented last night: 041ef57

[notTamion]

At this point I just want to understand what the factors are which lead to different behaviours for different people

just so i am understanding this correctly: you are not able to alt + l/k to focus an empty monitor? (it is a little "bugged" for me aswell as there isn't a border or anything to signal that its selected however then for example opening a new application puts it on that screen)

[LGUG2Z]

Correct - I am not able to reliably spawn new application windows on a monitor with an empty workspace or change workspaces with focus-workspace on that monitor without the "mouse trick"

[notTamion]

fyi this is my config https://gist.github.com/notTamion/943f8568e03d1dc13bdd08dd6e2116c6

EDIT:
hmm ok interesting i just tried spawning a new application again and now it seems to not spawn it on the focused screen anymore. changing the workspace will still work though

more context: i believe this happens at window_manager:1630. here it focuses the monitor even if no container is on it

[alex-ds13]

bar applications should be responsible for telling komorebi what monitor they want their operation to be done on instead of relying on komorebi focusing whatever the mouse is on right now (considering the cli commands state that they will apply the operation to the "focused window/container/workspace/window" and say nothing about them changing the focus)

Not all commands have variations that allow specifying monitor and workspace. Maybe one could argue that they should all have but that is not the point. The thing is there are some commands that work on the focused container/window and don't have an alternative to tell what monitor/workspace to act on. As an example you have the focus-stack-window which lets you focus a specific window on a stacked container by index. This command only works on the current focused container. If you have a bar that shows you multiple icons for each window on the stack and lets you press one of them to focus that window (komorebi-bar has this functionality!) that won't work correctly with your approach, because if the user has focus on monitor 1, but wants to change the window from the stack on monitor 2 by pressing its button, that won't work! Worse if the focused container on monitor 1 is also a stack, then it will focus said index on the container from monitor 1 instead of the one from monitor 2.

Again maybe the solution is to add alternative commands for the ones that need it which lets you specify the monitor and workspace.

EDIT: Ok, now that I've taken a closer look I see that you didn't change the implementation for the command I mentioned above. Also that command seems to be the only one along with the ones you've changed that use that "hack". However

• FocusStackWindow: does not have a monitor/workspace variation, would have to be implemented
• CycleFocusWorkspace: does not have a monitor/workspace variation, would have to be implemented
• CloseWorkspace: does not have a monitor/workspace variation, would have to be implemented
• FocusLastWorkspace: does not have a monitor/workspace variation, would have to be implemented
• FocusWorkspaceNumber: does have FocusMonitorWorkspaceNumber that could be used instead by bar implementations or others
• FocusWorkspaceNumbers: this one doesn't make much sense to have this mouse "hack" in the first place since it will change all monitors to that specific workspace index, so it doesn't matter on what monitor is the mouse...

So basically only one of those commands actually has an alternative for bars or others to use...
Maybe you might think that some of these commands don't need to have those variations, but as soon as we make these changes someone will show up complaining because they were making use of this behavior and now there is no alternative. For instace for the CycleFocusWorkspace imagine someone wants to have a button (left arrow) on the left of the workspace widget on a bar and another button (right arrow) on the right of said widget to cycle the focused workspace to the left/right by pressing those buttons (I'm pretty sure if you wanted you could do this on YASB right know, since it lets you create a custom widget that calls some command on press, so someone might have already made it...). Currently that would work just fine with multiple monitors, with the changes from this PR that would stop working and there would be no alternative command for the user to update...

[notTamion]

hmm ok interesting i just tried spawning a new application again and now it seems to not spawn it on the focused screen anymore. changing the workspace will still work though

so for some applications it will spawn the application on the correct screen and for some it won't for example terminal will always spawn on the same monitor regardless of what you have focused, brave will always spawn on the monitor you have focused. this also happens when both your monitors aren't empty

[notTamion]

@alex-ds13
basically the two options we have is:

1. have changing workspaces/do anything else that has the mouse focus thingy stay broken for hotkey users that have mff off
2. implement commands to specify window/workspace etc. for those operations and give bars time to use those new methods (this is the reason i converted my pr on yasb to a draft) before we remove this mouse reassignment

Obviously 1. is the easier option however that way we don't only screw people that use that setup, we also have commands which state that they affect the focused monitor/workspace etc. when in reality they use the mouse position (and in my opinion the correct way to use a tiling wm is with hotkeys and not the mouse)

[alex-ds13]

@alex-ds13 basically the two options we have is:

1. have changing workspaces/do anything else that has the mouse focus thingy stay broken for hotkey users that have mff off

2. implement commands to specify window/workspace etc. for those operations and give bars time to use those new methods (this is the reason i converted my pr on yasb to a draft) before we remove this mouse reassignment

Obviously 1. is the easier option however that way we don't only screw people that use that setup, we also have commands which state that they affect the focused monitor/workspace etc. when in reality they use the mouse position (and in my opinion the correct way to use a tiling wm is with hotkeys and not the mouse)

I agree with you. I too find it annoying sometimes when I used my mouse to do something on another monitor, then change focus with the keyboard and my commands after that act on the other monitor. I think option 2 would be the best, but there is still the issue that some users can't focus an empty workspace. That is why I was proposing a 3rd option where we would add a new config named mouse_follows_monitor_focus for instance, that when you've changed focus between monitors, if the mouse wasn't on the target monitor it would move it there. Either that or we need to understand exactly why is it that sometimes you can focus an empty workspace and sometimes you can't, so we could fix that and then implement option 2.

[alex-ds13]

@LGUG2Z From what I can see the issue where some users can't focus an empty workspace is cause by the lines that this PR is trying to fix. Because if you have monitor 1 focused and monitor 2 workspace is empty, when you try to move in the direction of monitor 2 to focus the monitor, komorebi is actually focusing that monitor, but since the mouse doesn't move there, then if you try to use a command to focus another workspace thinking you'll change workspace on monitor 2, since it detects that the mouse is on monitor 1 it refocus monitor 1 first and then applies the command.

With this PR, if you have mouse_follows_focus set to false, then the above will work. However if the user has mouse_follows_focus set to true, then it won't work, since @notTamion wrapped those lines on an if statement that checks for mouse_follows_focus == true. That is why I proposed above on the review to remove those lines entirely.

If we remove those lines entirely then you will be able to change focus to another monitor even when its workspace is empty! And any command you call next is handled on that monitor you just focused.
Now I do believe that there is an "issue" with the above when you are using mff, when you change focus to that empty monitor I believe the mouse should move there as well to the middle of the screen, but that is not happening.

Also with this approach there is no way of knowing that you just focused another monitor (when it's workspace is empty) so some users might be changing focus between windows, accidentally move to the other monitor without noticing it and then the next focus commands don't do anything unless they are in the direction of the previous monitor. Although this the same behavior that happens now anyway.

In summary to me these are the best options:

1. Remove these lines completely and create alternate commands that take a monitor index (like mentioned here). This would be somewhat of a breaking change for users that are used to this behavior and actually like it.
2. Wrap these lines on an if statement that checks for a new config with a name like legacy_focus_commands_behaviour which would be true by default, then create alternate commands that take a monitor index. This way we wouldn't be creating a breaking change. And the users that wanted to have this behavior like @notTamion could simply change that config to false. And we would have the alternate commands for bars to use.
3. Keep the lines as they are and create a new mouse_follows_monitor_focus config like I suggested above that moves the mouse to the focused monitor when you change focus across monitors. This one would have the benefit of you knowing when you change focus to an empty workspace since the mouse would move there.

[notTamion]

the problem with solution 3 is that it doesn't actually fix the problem but instead just hides it behind a config option:
with 3 this would essentially just work if people that have mff off for some reason would want their mouse to follow focus when switching monitors (which i personally don't want)

i like 1. and 2. the most

This is how i would do it:

1. in 1 release we add the new commands that allow specifying monitor/workspace and also make selecting an empty monitor/workspace more "supported" (adding a border when its selected, moving the mouse if mff is on) and also allow selecting it by clicking on the desktop. We also add this "legacy_focus_commands_behaviour" (default on) config option which if turned off disables the mouse position focus
2. then we give bar applications time to use these new commands
3. in the release after that we can then change the default of the config option to off (or just remove the mouse position focus completely) and have the issue fixed

@LGUG2Z opinion?

[alex-ds13]

1. in 1 release we add the new commands that allow specifying monitor/workspace and also make selecting an empty monitor/workspace more "supported" (adding a border when its selected, moving the mouse if mff is on) and also allow selecting it by clicking on the desktop. We also add this "legacy_focus_commands_behaviour" (default on) config option which if turned off disables the mouse position focus

What exactly do you mean with "adding a border when its selected"? A border to the monitor?

3. in the release after that we can then change the default of the config option to off (or just remove the mouse position focus completely) and have the issue fixed

I wouldn't change the default to off, but instead I would change the quickstart config example to have this config there with the value off. This way there would never be a breaking change for anyone ever. And new users would get this working as expected from the get go when they run komorebic quickstart.

But yeah this now is a matter of what does @LGUG2Z prefer...

BTW @LGUG2Z, why is this code here:

WindowsApi::center_cursor_in_rect(&WindowsApi::window_rect(
    window.hwnd,
)?)?;

Why are we centering the cursor on the target monitor when we move focus across monitors and the target monitor has a monocled window? Shouldn't this be done only if the user had mff?

[notTamion]

What exactly do you mean with "adding a border when its selected"? A border to the monitor?

Yeah like a Border around the work area, well more like inside the work area but the exact design can be tinkered on later

[LGUG2Z]

There is a lot for me to catch up on here and realistically I'm not going to be able to get back to this in any meaningful capacity until next week. Some high level thoughts:

• mouse_follows_focus is the "right" way to use komorebi, and I'm fine with having undefined behavior in the short-medium term if people elect to set this to false - we can add more docs around this to highlight that if people want to do this, they are in the wild west in their own
• I don't want to introduce a whole bunch of command variants to include monitor indices
• There is already a lot of new stuff going into v0.1.31 so I'm hesitant to make any significant changes in this area

[notTamion]

I don't want to introduce a whole bunch of command variants to include monitor indices

this is something i have been meaning to ask about. why aren't you just adding flags instead of adding a new subcommand for everything? i have only used clap once before but iirc this is something it supports

[alex-ds13]

• mouse_follows_focus is the "right" way to use komorebi, and I'm fine with having undefined behavior in the short-medium term if people elect to set this to false - we can add more docs around this to highlight that if people want to do this, they are in the wild west in their own

If this is the case, then why were these lines added in the first place? I'm talking about these:

// This is to ensure that even on an empty workspace on a secondary monitor, the
// secondary monitor where the cursor is focused will be used as the target for
// the workspace switch op
if let Some(monitor_idx) = self.monitor_idx_from_current_pos() {
    self.focus_monitor(monitor_idx)?;
}

Because it seems to me, from what I've tested, that if these lines were removed then you would be able to focus an empty workspace on a secondary monitor by using something like alt + h/j/k/l and then the commands would act on that monitor. The only bug is that the cursor doesn't move to the empty workspace when using mff, even though the focus did move, but this should be easy to fix.

When you have the time, try removing those lines completely and check the resulting behavior. I think you'll see it is the same behavior you have now as expected with the only difference that if you want to act on another monitor you first need to move focus there with the keyboard instead of with the mouse...

[notTamion]

i am not sure whether this is possible but: if we are able to catch moues clicks before the bar applications then we might be able to focus the clicked monitor and then the operations will be done on the correct monitor