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

Tiling drag, take 3 #3085

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Tiling drag, take 3 #3085

merged 1 commit into from
Jul 28, 2022

Conversation

orestisfl
Copy link
Member

@orestisfl orestisfl commented Dec 12, 2017

Fixes #2643.
This is a continuation of #2653.

I have changed the behaviour for the inner region from #2653. The inner region now behaves like move container to mark and supports no direction. This reduces the changes to core con.c functions and is simpler. It was also enough for the support of stacked & tabbed containers without any extra work.

The code needs some polishing here and there but I hope that the core functionality works as intended with zero crashes. I have opened this PR to get some first feedback and hopefully get some people to run the code on their machines.

Current status

TODOs

(removed completed items from #2653):

  • Support stacks and tabs
  • Drag onto the root window
  • Initiate tiling drag from titlebar This would break resizing
  • Emit move event properly on tiling drop.❓I need some help here, I don't know what the original intention was?
  • Refactor insert_con_into() into con.c Why?
  • Configure color of drop indicator❓ (even though using the focused color seems like a reasonable default, do we want to add the extra configuration?)
  • Bug: i3 crashes if a window closes while the dragging is indicating to drop it onto that window.
  • Accidentally mod + clicking a window can be annoying. Maybe only show the indicator after we have dragged the container outside of its original position.
  • Add a way to disable the whole thing
  • Only focus con if it had focus. To change this, route_click should also be modified since focus changes there.
  • I think the indicator sizes need some work. The outer indicators are big in small windows and small in big windows.
  • I am not sure if fb4fa33 is needed. Investigate?
  • Moving an unfocused container from another output shouldn't make it focused _con_move_to_con: don't change focus when moving to active workspace #3200
  • Resize from title bar w/ mouse broke I think
  • For consistency: should dragging unfocused floating containers also not make them focused?
  • Rearrange tabbed / stacked by dropping on the title? => out of scope
  • dragging & swapping ? -- can be done in future PR
  • Documentation: indicators + disable option.
  • Squash commits better to remove unneeded changes from con.c, potentially cluttering git blame.
  • Switch to meson
  • Update release notes

Tests

  • xcb_drag_prepare_cb: drain events #3193
  • dragging onto a bar
  • dragging onto an empty workspace
  • move events
  • dragging onto a container that closes before the drag is complete
  • config option
  • dragging onto stacked / tabbed containers => out of scope
  • more complicated layouts with a combination of orientations etc
  • moving an unfocused container doesn't make it focused
  • moving an unfocused container from another output doesn't make it focused
  • moving fullscreen containers
  • focus order
  • update filename

@Airblader
Copy link
Member

Emit move event properly on tiling drop.

I think @acrisci just meant that when you drop-move a window, we need to emit the regular window::move event. Perhaps this wasn't the case in the other PR(s) yet, perhaps it just hadn't been tested yet; I'm not sure.

Configure color of drop indicator

Let's hold off on that for now. I think the focused color is a reasonable choice. When the need arises we can still make it configurable.

I am not sure if fb4fa33 is needed. Investigate?

Perhaps @MForster can add some insight here. :-) I haven't looked into it yet.

Squash commits better to remove unneeded changes from con.c, potentially cluttering git blame (?).

Yes, let's please squash; but we can wait with that until we merge. To retain attributions, the commit message should just mention the other authors having worked on this previously.

Add a way to disable the whole thing?

That's a very good point. Yes, I think we should have this option.

I have opened this PR to get some first feedback and hopefully get some people to run the code on their machines.

Perhaps leave a comment at the existing two PRs so the people subscribed there get a notification that this has been picked up upon again? You can also drop an email to the mailing list and / or call for testers on reddit.

@orestisfl
Copy link
Member Author

I have opened this PR to get some first feedback and hopefully get some people to run the code on their machines.

You can also drop an email to the mailing list and / or call for testers on reddit.

We can probably do this when this is ready to merge (or after it is merged in a separate branch for limited time? dunno). I predict that writing excessive tests for this will take some time.

For now, it would be great if you and/or @stapelberg can run this.

@MForster
Copy link

I am not sure if fb4fa33 is needed. Investigate?

Perhaps @MForster can add some insight here. :-) I haven't looked into it yet.

I'm sure that it it was necessary when I implemented it :-). There may be simpler solutions.

@orestisfl
Copy link
Member Author

orestisfl commented Dec 14, 2017

I've been trying to write the tests. Maybe @stapelberg can help with my problem:

#!perl
use i3test i3_config => <<EOT;
# i3 config file (v4)
font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1

focus_follows_mouse no
floating_modifier Mod1

# 2 side by side outputs
fake-outputs 1000x500+0+0,1000x500+1000+0
EOT
use i3test::XTEST;

sub start_drag {
    my ($pos_x, $pos_y) = @_;

    $x->root->warp_pointer($pos_x, $pos_y);
    xtest_sync_with_i3;

    xtest_key_press(64);        # Alt_L
    xtest_button_press(1, $pos_x, $pos_y);
    xtest_sync_with_i3;
}

sub end_drag {
    my ($pos_x, $pos_y) = @_;

    $x->root->warp_pointer($pos_x, $pos_y);
    xtest_sync_with_i3;

    xtest_button_release(1, $pos_x, $pos_y);
    xtest_key_release(64);      # Alt_L
    xtest_sync_with_i3;
}

my $ws2 = fresh_workspace(output => 1);
my $ws1 = fresh_workspace(output => 0);
open_window;
my $A = get_focused($ws1);

start_drag(50, 50);
end_drag(1050, 50);

is(get_focused($ws2), $A, 'A moved to the right workspace');

done_testing;

I have tried every sync permutation for the end_drag subroutine. I think the sync after warp_pointer is needed but including it will sometimes make the test stuck, waiting for the sync forever.

@orestisfl
Copy link
Member Author

orestisfl commented Dec 14, 2017

Hmm, I think it gets stuck because i3 is inside drag_pointer's ev_run and doesn't reply to the sync message. I'll see if I can work around that.

@orestisfl
Copy link
Member Author

Possible feature: special keybind that when used with tiling windows it turns them to floating.

From https://www.reddit.com/r/i3wm/comments/7xrmxu/move_nonfloating_window_using_the_mouse/

@orestisfl
Copy link
Member Author

@stapelberg, since you are fairly active lately, do you have any input regarding the problem I described above?

Thanks.

@stapelberg
Copy link
Member

Sorry for the late reply.

I think your assessment is correct: we don’t correctly handle the sync event from within the drag_pointer event loop. It should be an easy addition to do so, though :). Let me know if you can’t get that to work, and thanks for being diligent regarding the tests — I think this will be our first test case performing any kind of dragging.

@orestisfl
Copy link
Member Author

They generally seem to be handled but they get stuck sometimes. The test sometimes hangs for 10-20 seconds and then continues. This is the gdb log.

(gdb) p (xcb_generic_event_t*) xcb_poll_for_event(conn)
$1 = (xcb_generic_event_t *) 0x604000016350
(gdb) p $1->response_type & 0x7F
$2 = 28
(gdb) p (xcb_generic_event_t*) xcb_poll_for_event(conn)
$3 = (xcb_generic_event_t *) 0x604000016390
(gdb) p $3->response_type & 0x7F
$4 = 33
(gdb) bt
#0  0x00007f801996df90 in epoll_pwait () at /usr/lib/libc.so.6
#1  0x00007f801a1a4e95 in  () at /usr/lib/libev.so.4
#2  0x00007f801a1a7455 in ev_run () at /usr/lib/libev.so.4
#3  0x0000555fa2b54f39 in drag_pointer (con=0x614000002440, event=0x604000013a10, confine_to=0, border=BORDER_TOP, cursor=8, callback=0x555fa2ba1f34 <drag_callback>, extra=0x7ffc393e7f60) at ../../i3/src/floating.c:848
#4  0x0000555fa2ba3043 in tiling_drag (con=0x614000002440, event=0x604000013a10) at ../../i3/src/tiling_drag.c:187
#5  0x0000555fa2b03d3f in route_click (con=0x614000002440, event=0x604000013a10, mod_pressed=true, dest=CLICK_INSIDE) at ../../i3/src/click.c:261
#6  0x0000555fa2b046cc in handle_button_press (event=0x604000013a10) at ../../i3/src/click.c:364
#7  0x0000555fa2b61707 in handle_event (type=4, event=0x604000013a10) at ../../i3/src/handlers.c:1551
#8  0x0000555fa2b71f9c in xcb_prepare_cb (loop=0x7f801a3ada40, w=0x60300000d6f0, revents=16384) at ../../i3/src/main.c:133
#9  0x00007f801a1a44e3 in ev_invoke_pending () at /usr/lib/libev.so.4
#10 0x00007f801a1a7fe1 in ev_run () at /usr/lib/libev.so.4
#11 0x0000555fa2b71d45 in ev_loop (loop=0x7f801a3ada40, flags=0) at /usr/include/ev.h:835
#12 0x0000555fa2b7a95c in main (argc=9, argv=0x7ffc393e9848) at ../../i3/src/main.c:964

It seems like it's possible to have leftover X events but ev_run doesn't get interrupted?

I'll continue tomorrow.

@stapelberg
Copy link
Member

Just checking — your git working directory does contain commit 0d8b671, yes? If not, that would be my suspicion.

@orestisfl
Copy link
Member Author

Yes, I had rebased with the latest next.

@orestisfl
Copy link
Member Author

I wonder if we are going against libev's documentation here:

You must not call ev_run (or similar functions that enter the current event loop) or ev_loop_fork from either ev_prepare or ev_check watchers. Other loops than the current one are fine, however. The rationale behind this is that you do not need to check for recursion in those watchers, i.e. the sequence will always be ev_prepare, blocking, ev_check so if you have one watcher of each kind they will always be called in pairs bracketing the blocking call.

@stapelberg
Copy link
Member

Fair point. If you can find a cleaner way to implement this, pull requests are very welcome :)

@orestisfl
Copy link
Member Author

Tbh, I don't really understand the rationale behind the quoted text, I don't think that the current implementation causes any problems as far as I can tell. I'll see if I can come up with a cleaner solution though.

For this PR though my problem is that I still don't completely understand why the i3sync event gets lost. I have tried some hacks that improved the situation a bit (eg an additional ev_idle or ev_check makes the 1-minute hang more rare) but I am still searching for a complete solution.

Since you are significantly more experienced with libev any ideas for the possible solution/cause might help.

@stapelberg
Copy link
Member

Can you give me the steps required to reproduce the issue please?

@orestisfl
Copy link
Member Author

I've pushed a test that should exhibit this behaviour.

@stapelberg
Copy link
Member

I looked into this a little bit, and I’m fairly convinced that the issue is that we’re draining the events in

while ((event = xcb_poll_for_event(conn)) != NULL) {
, but don’t handle the case where new events become available as part of the dragloop callback in
dragloop->callback(
: to process replies, xcb needs to read from the X11 connection, which might mean reading new events.

Indeed, adding a quick and dirty “drain:” label in front of the while loop and jumping to it before the final xcb_flush(conn) call in xcb_drag_prepare_cb fixes the hang for me.

Note that this doesn’t eliminate all causes of flakiness for 295-tiling-drag.t for me. One other issue is that you’re calling xtest_sync_with_i3 after calling $x->root->warp_pointer, which uses the wrong X11 connection: the XTEST code has a separate X11 connection. So, make that sync_with_i3. Even with that fix, the test is still flaky: sometimes i3 does not seem to receive any pointer update events. Not entirely sure why.

I hope this helps you make some progress on this. Let me know if you need further help.

@orestisfl
Copy link
Member Author

Thanks! I had done something similar with your drain but I didn't know about i3_sync. I combined:

  1. the drain label
  2. the sync fix
  3. ev_run once -> ev_loop

I ran it hundreds of times with a loop and it never hanged.

orestisfl added a commit to orestisfl/i3 that referenced this pull request Mar 21, 2018
As discussed in PR i3#3085, X11 events can appear while
dragloop->callback() is running.
@orestisfl orestisfl added the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Mar 24, 2018
orestisfl added a commit to orestisfl/i3 that referenced this pull request Mar 25, 2018
Seems to be the intention, indicated by this comment (con.c:1308-1310):
    /* For split containers, we use the currently focused container within it.
     * This allows setting marks on, e.g., tabbed containers which will move
     * con to a new tab behind the focused tab. */

Related to i3#3085.
orestisfl added a commit to orestisfl/i3 that referenced this pull request Jul 22, 2022
Fixes i3#2643

Inner drop region behaves like move to mark.
The outer region is close to the edge (currently 30px from the edge).
This will place the container as a sibling in the given direction within
the parent container. If the move direction goes against the orientation
of the parent container, tree_move() is called.

Contributors:
Co-authored-by: Orestis Floros <orestisflo@gmail.com>
See i3#3085
- Inner drop region behaves like move to mark
- Handle workspaces
- Fix crash when target closes
- Initiate tiling drag from titlebar
- Hide indicator until container is dragged outside of original position
- Calculate outer_threshold using percentages instead of fixed pixel
values
- Emit 'move' event properly
- Don't focus previously unfocused containers
- Use tree_split() on different orientation
- Fix redundant split containers
- DT_PARENT
- Readability & optimizations
- Limit parent threshold by render_deco_height()
- Tests
- Fullscreen container handling
- Initiate drag from title bar
- Fix issue of EnterNotify events still triggering after drag_callback
  is called
- Include decorations for drop target calculation

Co-authored-by: Michael Forster <email@michael-forster.de>
See i3#2178
- Original implementation of tiling drag + indicator window
> A container can be dragged by the title bar to one of the four sides
> of another container. That container will then be split either
> horizontally or vertically.

Co-authored-by: Tony Crisci <tony@dubstepdish.com>
See i3#2653
- Original implementation of outer/inner drop region indicator:
> There are two drop regions per direction.
>
> The inner region is closer to the center of the window. Dropping on
> this region will split the target container and put the container
> within the split at the given direction beside the target container.
>
> The outer region is close to the edge (currently 30px from the edge).
> This will place the container as a sibling in the given direction within
> the parent container.
>
> Dropping into the outer region moves the con beside the target. If the
> move direction goes against the orientation of the parent container, the
> con moves out of the row.
- Fix crash: Ignore containers without a managed window (eg i3bar)
Copy link
Member

@stapelberg stapelberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just a few small finishing touches.

direction_t direction;
drop_type_t drop_type;
xcb_window_t indicator = 0;
const struct callback_params params = {&indicator, &target, &direction, &drop_type};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I think we can remove the const qualifier, which will result in better code overall — if the params need to be mutable in this case, we might as well reflect that in the signature.


ipc_send_window_event("move", con);
break;
case DT_PARENT:;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL :)

Thus far, I have usually created a new scope to be able to declare variables after a label:

case DT_PARENT: {
  const bool …
}

Can you see if that works here? I would prefer a new scope over the subtle ;. If a new scope doesn’t work, at least add a comment explaining the ;

testcases/t/316-drag-container.t Outdated Show resolved Hide resolved
orestisfl added a commit to orestisfl/i3 that referenced this pull request Jul 28, 2022
Fixes i3#2643

Inner drop region behaves like move to mark.
The outer region is close to the edge (currently 30px from the edge).
This will place the container as a sibling in the given direction within
the parent container. If the move direction goes against the orientation
of the parent container, tree_move() is called.

Contributors:
Co-authored-by: Orestis Floros <orestisflo@gmail.com>
See i3#3085
- Inner drop region behaves like move to mark
- Handle workspaces
- Fix crash when target closes
- Initiate tiling drag from titlebar
- Hide indicator until container is dragged outside of original position
- Calculate outer_threshold using percentages instead of fixed pixel
values
- Emit 'move' event properly
- Don't focus previously unfocused containers
- Use tree_split() on different orientation
- Fix redundant split containers
- DT_PARENT
- Readability & optimizations
- Limit parent threshold by render_deco_height()
- Tests
- Fullscreen container handling
- Initiate drag from title bar
- Fix issue of EnterNotify events still triggering after drag_callback
  is called
- Include decorations for drop target calculation

Co-authored-by: Michael Forster <email@michael-forster.de>
See i3#2178
- Original implementation of tiling drag + indicator window
> A container can be dragged by the title bar to one of the four sides
> of another container. That container will then be split either
> horizontally or vertically.

Co-authored-by: Tony Crisci <tony@dubstepdish.com>
See i3#2653
- Original implementation of outer/inner drop region indicator:
> There are two drop regions per direction.
>
> The inner region is closer to the center of the window. Dropping on
> this region will split the target container and put the container
> within the split at the given direction beside the target container.
>
> The outer region is close to the edge (currently 30px from the edge).
> This will place the container as a sibling in the given direction within
> the parent container.
>
> Dropping into the outer region moves the con beside the target. If the
> move direction goes against the orientation of the parent container, the
> con moves out of the row.
- Fix crash: Ignore containers without a managed window (eg i3bar)
@orestisfl
Copy link
Member Author

Regarding the const qualifier (can't reply in-thread for some reason), the benefit of the old method is that it allows to specify both read-only and read-write arguments, otherwise it's not easy to tell just by looking at the struct.

I have made the change but I would still favor the old code 1. because of the point above 2. because it minimizes risk of a last-minute regression for this PR.

@stapelberg
Copy link
Member

Regarding the const qualifier (can't reply in-thread for some reason), the benefit of the old method is that it allows to specify both read-only and read-write arguments, otherwise it's not easy to tell just by looking at the struct.

I have made the change but I would still favor the old code 1. because of the point above 2. because it minimizes risk of a last-minute regression for this PR.

That’s fair. We can leave the const qualifier in.

Can you update and rebase to pick up the CI fixes please?

Fixes i3#2643

Inner drop region behaves like move to mark.
The outer region is close to the edge (currently 30px from the edge).
This will place the container as a sibling in the given direction within
the parent container. If the move direction goes against the orientation
of the parent container, tree_move() is called.

Contributors:
Co-authored-by: Orestis Floros <orestisflo@gmail.com>
See i3#3085
- Inner drop region behaves like move to mark
- Handle workspaces
- Fix crash when target closes
- Initiate tiling drag from titlebar
- Hide indicator until container is dragged outside of original position
- Calculate outer_threshold using percentages instead of fixed pixel
values
- Emit 'move' event properly
- Don't focus previously unfocused containers
- Use tree_split() on different orientation
- Fix redundant split containers
- DT_PARENT
- Readability & optimizations
- Limit parent threshold by render_deco_height()
- Tests
- Fullscreen container handling
- Initiate drag from title bar
- Fix issue of EnterNotify events still triggering after drag_callback
  is called
- Include decorations for drop target calculation

Co-authored-by: Michael Forster <email@michael-forster.de>
See i3#2178
- Original implementation of tiling drag + indicator window
> A container can be dragged by the title bar to one of the four sides
> of another container. That container will then be split either
> horizontally or vertically.

Co-authored-by: Tony Crisci <tony@dubstepdish.com>
See i3#2653
- Original implementation of outer/inner drop region indicator:
> There are two drop regions per direction.
>
> The inner region is closer to the center of the window. Dropping on
> this region will split the target container and put the container
> within the split at the given direction beside the target container.
>
> The outer region is close to the edge (currently 30px from the edge).
> This will place the container as a sibling in the given direction within
> the parent container.
>
> Dropping into the outer region moves the con beside the target. If the
> move direction goes against the orientation of the parent container, the
> con moves out of the row.
- Fix crash: Ignore containers without a managed window (eg i3bar)
@orestisfl
Copy link
Member Author

Done :)

@stapelberg stapelberg merged commit ebcd1d4 into i3:next Jul 28, 2022
@stapelberg
Copy link
Member

Merged! 🎉

Thanks for your work on this!

@orestisfl orestisfl deleted the issue-2643 branch July 28, 2022 10:06
@vincentbernat
Copy link
Contributor

vincentbernat commented Jul 28, 2022

Thanks for the patch! I have just tested it (on top of 4.20.1) and I have found a case where it does not work:

(splitv)
 (splith)
  (splith)
    emacs
    (splitv)
      vbeterm
      vbeterm

If I drag the top vbeterm on top of the bottom vbeterm, the positions of the two are swapped as expected. But if I drag the bottom vbeterm on top of the top one, the two windows are not swapped. The indicator is correct (I get an orange area on the center of the target term), but when releasing the mouse button, it does nothing.

Edit: I have tested on top of i3-gaps, so maybe this does not happen with vanilla i3. I'll test on top of vanilla i3 a bit later.

@orestisfl
Copy link
Member Author

@vincentbernat Thanks for trying this already

The semantics of drag & drop are not to swap containers, but similar to move to mark. So, the behavior is correct.

Copy & paste from the docs in this PR:

Drop on container::
    This happens when the mouse is relatively near the center of a container.
    If the mouse is released, the result is exactly as if you had run the
    +move container to mark+ command. See <<move_to_mark>>.

@vincentbernat
Copy link
Contributor

Understood, thanks for the clarification!

vincentbernat added a commit to vincentbernat/homemanager-configuration that referenced this pull request Jul 28, 2022
@MForster
Copy link

MForster commented Jul 29, 2022

Thanks a lot for seeing this through to completion! I always felt bad about teasing the feature with an unfinished patch and then letting it drop. Seeing how much work this was I wouldn't have been able to to see it through.

So, thanks again for picking this up and finishing it!

@autoteelar
Copy link

how do i disable it?

i looked in documentation and it tells you pretty much everything but that

@stapelberg
Copy link
Member

Can you explain why you want to disable the feature, as opposed to just not using it? Is it getting in your way somehow?

@orestisfl
Copy link
Member Author

how do i disable it?

i looked in documentation and it tells you pretty much everything but that

You can't. Please track updates on a possible setting for this here: #5155

@autoteelar
Copy link

autoteelar commented Sep 24, 2022

Can you explain why you want to disable the feature, as opposed to just not using it? Is it getting in your way somehow?

yeah, well i usually click the tabs to change them, and now that this is added if you move your mouse within like 3px while clicking it drags them off.

i also move my windows with keyboard

basically tldr, i don't plan on using it, and it gets in the way in a pretty big way in my specific use case. i can understand why it was added and why other people might want it and see it as a useful feature as well though,

and well, it was also at least intended to have a way to disable this feature if you scroll to the top, airblader agreed its a good idea. (quoted here)

「Add a way to disable the whole thing?

That's a very good point. Yes, I think we should have this option.」

i guess that about sums it up.

@ra-om
Copy link

ra-om commented Sep 26, 2022

Left click is maybe not the best choice. Would be nice to be able to change which mouse button triggers the drag.

@orestisfl
Copy link
Member Author

It's probably time to lock this thread to encourage proper structured discussion via new issues. This is a released feature now so future changes need to be treated as bugs/enhancements.

Thank you to everyone that contributed to this discussion over the years, feedback has been very valuable!

@i3 i3 locked as resolved and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dragging tiled windows with the mouse.