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

Can't move two focused windows to a different monitor without first toggling back to a different view mode. #5382

Open
Samueru-sama opened this issue Jan 20, 2023 · 16 comments · May be fixed by #5521
Labels
4.22 bug missing-log Read the CONTRIBUTING.md file for instructions

Comments

@Samueru-sama
Copy link

Samueru-sama commented Jan 20, 2023

Current Behavior

I use multiple monitors with i3 with the following shortcut to spawn two terminal windows at the same time:

bindsym Control+Mod1+t exec xfce4-terminal -e htop, exec xfce4-terminal

It makes two xfce4 terminal windows spawn at the same time, one is a just a plain terminal to be able to input commands and the other is htop.

If I focus both windows with this:

bindsym $mod+a focus parent

And then try to move the two focused windows to another monitor using the arrow keys:

move focused window
bindsym $mod+Shift+Left move left
bindsym $mod+Shift+Down move down
bindsym $mod+Shift+Up move up
bindsym $mod+Shift+Right move right

i3 doesn't do anything, no windows move to different monitor. There is also no error warning or anything similar.

I have to focus back to child and toggle between vertical and horizontal tiling using:

bindsym $mod+s layout toggle split

After doing so, now using the arrow keys to move the two focused windows to another monitor works, even though the two windows were tiled in the original position before

Expected Behavior

The two windows should just move between monitors like it does with just one window, or like it does after I toggle back between two different view modes.

Reproduction Instructions

Use the commands described above.

Here is my config: https://pastebin.com/dAbG3e6X

Environment

i3-wm version 4.22-3 from the official arch repositories.

  • Linux Distribution & Version: Arch Linux X86_64, Kernel 6.1.7-arch1-1.

  • Are you using a compositor (e.g., xcompmgr or compton): Yes, picom.

@i3bot
Copy link

i3bot commented Jan 20, 2023

I don’t see a link to logs.i3wm.org. Did you follow https://i3wm.org/docs/debugging.html? (In case you actually provided a link to a logfile, please ignore me.)

@i3bot i3bot added missing-log Read the CONTRIBUTING.md file for instructions missing-version labels Jan 20, 2023
@i3bot
Copy link

i3bot commented Jan 20, 2023

I don’t see a version number. Could you please copy & paste the output of i3 --version into this issue?

@Samueru-sama
Copy link
Author

version

i3 version 4.22 (2023-01-02) © 2009 Michael Stapelberg and contributors

I also found that it happens even if I individually spawn two windows, they will not move between monitors unless I toggle between vertical and split view first.

Here's a short video showing the issue: https://streamable.com/b166ed

This only happens if I use the move left/right commands in i3, it doesn't happen if I instead tell i3 to move to a workspace number.

bindsym $mod+Shift+Left move left
bindsym $mod+Shift+Down move down
bindsym $mod+Shift+Up move up
bindsym $mod+Shift+Right move right

I also tried 'bindsym $mod+Shift+Left move container left' but didn't work.

@EllaTheCat
Copy link

Hi. Could someone please take another look at this bug report please, I think the reporter has asked a fundamental question. I became aware of this only very recently, we're talkng about it over on reddit. https://www.reddit.com/r/i3wm/comments/12zh5ep/cant_move_two_selected_windows_with_the_move/

There are some of us for whom navigating the i3 tree does not come easily. I think that might be a legacy of learning i3 and encountering something that should work, should be simple, but is neither. Consequently we develop bad habits and fail to use i3 to its full potential.

The OP thinks this behaviour is a bug. I don't, because in years of using i3 it's apparent that you guys know what you are doing, it's expected behaviour, but I have no clue at all why it must be so, aside from a hunch re focus wrapping.

Either way, if i3 did work as the OP insists it should, it would feel right. I worked through the OP example, and I just became amazed why something so intuitive isn't a feature.

If it can't be done, i can live with that, but on the off chance OP is actually addressing something of importance, please give this another look.

/bump

@orestisfl
Copy link
Member

This does look like a bug but I don't have time right now to investigate.

@orestisfl orestisfl added the bug label Apr 30, 2023
@slyshot
Copy link
Contributor

slyshot commented May 26, 2023

When workspaces are not user-configured, a split container is not made for them by workspace_attach_to(from workspace.c), and that's not called from _con_attach(con.c). I'm not exactly sure why this is. A comment says

When the container is not a split container (but contains a window)
and is attached to a workspace, we check if the user configured a
workspace_layout. This is done in workspace_attach_to, which will
provide us with the container to which we should attach (either the
workspace or a new split container with the configured
workspace_layout).

and indeed, this is what happens. No split container is made to carry the windows in the workspace, they're just attached to the workspace itself. So, when you select a parent with $mod+a, you select the workspace, which tree_move reasonably refuses to move.

This seems to be intentional, so I'm weary about trying to modify this behavior, but I don't see why a container shouldn't be made as a child to the workspace and a parent to all the windows no matter what. This would solve this problem, and it seems like it can be done with no change to functionality.

The reason changing the layout seems to fix it is that when you change the layout, con_set_layout from con.c makes a split container and moves the workspace cons into it, so it has a container between the workspace and the windows to move.

Apologies if I'm missing anything, thanks.

slyshot added a commit to slyshot/i3 that referenced this issue Jun 1, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 1, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 1, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 1, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 1, 2023
@slyshot slyshot linked a pull request Jun 1, 2023 that will close this issue
slyshot added a commit to slyshot/i3 that referenced this issue Jun 5, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 5, 2023
@Samueru-sama
Copy link
Author

Samueru-sama commented Jun 12, 2023

When workspaces are not user-configured, a split container is not made for them by workspace_attach_to(from workspace.c), and that's not called from _con_attach(con.c). I'm not exactly sure why this is. A comment says

When the container is not a split container (but contains a window)
and is attached to a workspace, we check if the user configured a
workspace_layout. This is done in workspace_attach_to, which will
provide us with the container to which we should attach (either the
workspace or a new split container with the configured
workspace_layout).

and indeed, this is what happens. No split container is made to carry the windows in the workspace, they're just attached to the workspace itself. So, when you select a parent with $mod+a, you select the workspace, which tree_move reasonably refuses to move.

This seems to be intentional, so I'm weary about trying to modify this behavior, but I don't see why a container shouldn't be made as a child to the workspace and a parent to all the windows no matter what. This would solve this problem, and it seems like it can be done with no change to functionality.

The reason changing the layout seems to fix it is that when you change the layout, con_set_layout from con.c makes a split container and moves the workspace cons into it, so it has a container between the workspace and the windows to move.

Apologies if I'm missing anything, thanks.

Hi slyshot, thank you for making the PR fixing the issue, I noticed that it hasn't been merged yet so I just went ahead and compiled i3 manually to test it out.

I used the aur pkgbuild that is used for i3-git from the aur as reference for building i3, so maybe I did something wrong, no idea.

While it fixes the issue of not being able to move the two windows together (yay!) I also noticed another issue, if you are focused on an empty workspace and try to move that workspace either left or right i3 crashes instantly.

Notice: I don't want to move workspaces between monitors, this is something that just happened by accident, I wasn't focused on workspace with a window and I just hit the move command trying to move the window and because I was focused on an empty workspace it resulted in the crash.

Maybe this happened because I build it manually and isn't an issue with the PR, no idea.

slyshot added a commit to slyshot/i3 that referenced this issue Jun 14, 2023
@slyshot
Copy link
Contributor

slyshot commented Jun 14, 2023

Wow, a silly mistake on my part.
The issue was pretty simple, when the workspace has no children, workspace_encapsulate chooses to return NULL. Solution is just to check it's return value and to not continue if it's NULL.
Thanks.

@Samueru-sama
Copy link
Author

Samueru-sama commented Jun 15, 2023

Wow, a silly mistake on my part. The issue was pretty simple, when the workspace has no children, workspace_encapsulate chooses to return NULL. Solution is just to check it's return value and to not continue if it's NULL. Thanks.

Hi, I've just tested it again and now it no longer crashes, but I've found another issue, though much smaller this time.

Once you spawn two windows, focus parent and move them to the next monitor, focus is kept on the original monitor instead of following the two windows.

What's interesting is that if I then focus the two windows that were moved again, and I move them back to the other monitor, now the focus does follow the windows as I move them around.

So to reproduce the issue:

*Spawn two windows on the same monitor

*Focus parent and then move to next monitor using the move command.

You will notice that the windows moved to the target monitor, but the focus is still on the monitor where the windows were originally.

*Now focus parent again on the two windows that just were moved to the other monitor

*And move them again

You will notice that now focus does indeed follow the windows as you move them between monitors back and forth, it is only the first time where focus is lost.

@slyshot
Copy link
Contributor

slyshot commented Jun 16, 2023

This was trickier to find. I got to sleep, but looking at it, tree_move isn't really able to change focus at all, since it's caller will undo that. So unless I'm missing something, sneaking a container in during the move may not be an option unless this is changed?

i3/src/commands.c

Lines 1615 to 1619 in 3ae5f31

/* The move command should not disturb focus. con_exists is called because
* tree_move calls tree_flatten. */
if (focused != initially_focused && con_exists(initially_focused)) {
con_activate(initially_focused);
}

Moving the initially_focused variable to tree_move, setting it to con at the start and after the workspace_encapsulate, and focusing on it before every return statement and at the end, makes everything work as expected so far as I've seen.

That's obviously not the right thing to do, but it proves that this is where we're at. Seems like some functions in tree_move change focus as an unwanted side effect. Before I did the work of setting focus practically everywhere in tree_move, trying to move multiple windows led to only one of them being focused after the move, but that doesn't seem like something move_to_output_directed would or should do. I wonder if a bugfix for all the functions that ought to preserve focus but don't is permitted and/or called for, given that the whole need to save focus before tree_move to apply after has some smell to it anyways.
EDIT: This is getting to be outside of the scope of this issue, but as I'm bringing up the idea of a new issue already, I might as well give the example for it's necessity more thoroughly. workspace_show is the culprit for the move_to_output_directed problem, although I can't see why right now(And it may be by design. It seems like it'd be inconvenient if, when returning to a workspace, the multiple windows you had previously focused came back into focus, instead of just one). con_focus(con) after it here

i3/src/move.c

Lines 241 to 243 in 3ae5f31

con_focus(con);
focused = old_ws;
workspace_show(ws);
'

seems to solve that, and focus shouldn't change on a movement to another output, so that seems like a perfectly reasonable addition to the function. It doesn't seem like that'd effect mouse warping either. A similar change to all the functions which have an undesired side effect(If there are more) like this might completely eliminate the need for saving focus before and after calls to them, and stop bugs in the future due to presuming focus to be safe for things like this. I'll work on this, as it may be the best precursor for finishing this issue.

EDIT 2: Figured it out. workspace_show calls con_descend_focused with old_ws as focused because of this workaround, and it goes down the focus queue from the new workspace, starting from the old workspace, until it can't anymore(tailq_empty). If the focus was the actual con, it would stop there, but it descends into its children (Which are the next in the focus queue -- is this how it should be?) I'm not perfectly knowledgeable about how the focus queue is arranged, but this seems to be he cause in workspace_show. It's due to this unusual usage. Well, we're already doing something unusual here, so I think my pull rq will just be this unless I find any other side effects, which I haven't yet.

Thanks samueru, you're doing very good work finding these problems.

slyshot added a commit to slyshot/i3 that referenced this issue Jun 18, 2023
@slyshot
Copy link
Contributor

slyshot commented Jun 18, 2023

Because the workaround seems like a problem of it's own(Although a fairly minute one), I made it it's own pull request (Here #5529), even though it's required for this. I can make a branch with both commits so it's not too much of a hastle to look at it if you'd like, @Samueru-sama .

slyshot added a commit to slyshot/i3 that referenced this issue Jun 18, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 18, 2023
@slyshot
Copy link
Contributor

slyshot commented Jun 18, 2023

Apologies for all the fixups, I have some trouble getting out of my normal programming habits. I'll try to work on that, hope that's not too annoying for anybody.

@Samueru-sama
Copy link
Author

Because the workaround seems like a problem of it's own(Although a fairly minute one), I made it it's own pull request (Here #5529), even though it's required for this. I can make a branch with both commits so it's not too much of a hastle to look at it if you'd like, @Samueru-sama .

Yes that would be great. Thanks for taking a look at the issue.

@slyshot
Copy link
Contributor

slyshot commented Jun 19, 2023

Tell me if there's a glaring problem, I'm not at a computer for the day so I can't check before sending. https://github.com/slyshot/i3/tree/full_move_multiple

@Samueru-sama
Copy link
Author

Tell me if there's a glaring problem, I'm not at a computer for the day so I can't check before sending. https://github.com/slyshot/i3/tree/full_move_multiple

Alright I've tested it for several hours, so far great! focus is kept on the windows and everything works like it should, thank you so much for fixing it!

slyshot added a commit to slyshot/i3 that referenced this issue Jun 19, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 19, 2023
@slyshot
Copy link
Contributor

slyshot commented Jun 19, 2023

(No changes in the code -- just pulling i3 on all my branches, since I was behind on some commits)

Alright I've tested it for several hours, so far great! focus is kept on the windows and everything works like it should, thank you so much for fixing it!

That's great. I'll keep investigating in case I've missed something, but this seems to be the right way to go about this. Thanks.

slyshot added a commit to slyshot/i3 that referenced this issue Jun 28, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 28, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 28, 2023
slyshot added a commit to slyshot/i3 that referenced this issue Jun 28, 2023
@orestisfl orestisfl linked a pull request Jul 15, 2023 that will close this issue
slyshot added a commit to slyshot/i3 that referenced this issue Jul 23, 2023
Samueru-sama added a commit to Samueru-sama/i3 that referenced this issue May 18, 2024
Fixed the multiple window movement issue from i3#5382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.22 bug missing-log Read the CONTRIBUTING.md file for instructions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants