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

Fix behaviour of container_move_to_container, add move to con_id #6835

Closed
wants to merge 2 commits into from

Conversation

Myrdden
Copy link
Contributor

@Myrdden Myrdden commented Feb 15, 2022

Closes #6818
Okay so this PR does two things:

One, it removes this line

|| container_has_ancestor(container, destination)

In order to match the behaviour of i3's move functionality, which can be found here, posted below:

    /* We compare the focus order of the children of the lowest common ancestor. If con or
     * its ancestor is before target's ancestor then con should be placed before the target
     * in the focus stack. */
    Con *lca = lowest_common_ancestor(con, parent);
    if (lca == con) {
        ELOG("Container is being inserted into one of its descendants.\n");
        return;
    }

Basically, sway is checking on move that a destination is neither a descendant of the container to be moved, and that the container to be moved is not a descendant of the destination, whereas i3 only (rightly, I think) checks that the destination is not a descendant, as such a lot of valid moves in i3 simply don't work in sway and this fixes that.

The second, sort of incidental thing this PR adds is the command move [container|window] [to] con_id <con_id>. I use a script with a lot of "macros" to make navigating formerly-i3-and-now-sway easier, and I noticed that in decently upwards of half of them they all have the same repeated behaviour:

[con_id=$someDestination] mark _destination; move container to mark _destination; unmark _destination;

or something similar, so it seemed like there was a very obvious use case to just have that be

move container to con_id $someDestination

All of the rules surrounding whether a move is valid and how the move functions are identical to move-to-mark, this just saves some steps if you happen to have a con_id handy.

So yeah! Let me know what you think.

@Myrdden
Copy link
Contributor Author

Myrdden commented Feb 15, 2022

Upon further inspection of i3's codebase it looks like it's implementing additional logic that isn't present here that also possibly solves the focus issues I was seeing in #6818, so simply removing the conditional is likely not sufficient to actually get this to work as expected. I suppose I should read into this more...

@Myrdden
Copy link
Contributor Author

Myrdden commented Mar 15, 2022

Currently working on implementing:

  1. i3's equivalent of this command has an algorithm in place that's completely absent from sway's version, that says that when you move a window into a parent container, it's placed immediately after whatever in that parent container has focus, such that if you have:
H[a b V[c *d*] e f]

where d has focus, and you move to H[], via a mark or some other means, you'd end up with:

H[a b V[c] *d* e f]

with d immediately after the V[], because the V[] was in "focus" when it contained d. Obviously as sway does not currently allow you to move children into their parent containers, this algorithm I guess had no need to be implemented. But, in order to actually be par with i3, it probably should.

from #6818, when I have the time.

@korreman
Copy link

Thanks for taking the time to work out how this works in both sway and i3. With the general reluctance towards adding new features to sway, it's probably best to give the new command its own PR. Matching the behavior of an existing command to that of i3 should be easier to merge. The most important thing is to have some consistent way of moving containers to arbitrary positions in the tree.

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

Successfully merging this pull request may close these issues.

container_move_to_container doesn't behave like it's i3 equivalent
2 participants