hyprwm / Hyprland

Hyprland is an independent, highly customizable, dynamic tiling Wayland compositor that doesn't sacrifice on its looks.
https://hyprland.org
BSD 3-Clause "New" or "Revised" License
19.52k stars 818 forks source link

Dispatcher `movewindoworgroup` to move into/out-of an unlocked group #2851

Closed spikespaz closed 1 month ago

spikespaz commented 1 year ago

Description

I would like a dispatcher to move windows into/out-of groups when the window is not in a locked group.

This effectively combines moveintogroup and moveoutofgroup, but allows using the same bind for both dispatchers. This would work because groups can be locked, in which case this dispatcher would move the group rather than a window.

bindm = mouse:276, movewindoworgroup
bind = SUPER_SHIFT, left, movewindoworgroup, l
bind = SUPER_SHIFT, right, movewindoworgroup, r
bind = SUPER_SHIFT, up, movewindoworgroup, u
bind = SUPER_SHIFT, down, movewindoworgroup, d

Maybe name it one of these:

spikespaz commented 1 year ago

This does not work because all dispatchers are triggered, and they conflict.

bindm = , mouse:276, moveintogroup
bindm = , mouse:276, moveoutofgroup
bindm = , mouse:276, movewindow

Is there some magical order in which these work?

spikespaz commented 1 year ago

I just realized that when using bindm the movewindow dispatcher can behave like moveintogroup (drop a window into a group), but when used with regular bind a window will swap positions with the entire group instead of joining it.

memchr commented 1 year ago

This does not work because all dispatchers are triggered, and they conflict.

bindm = , mouse:276, moveintogroup
bindm = , mouse:276, moveoutofgroup
bindm = , mouse:276, movewindow

Is there some magical order in which these work?

Doesn't CKeybindManager::mouse only support movewindow' andresizewindow'?

https://github.com/hyprwm/Hyprland/blob/7e8a21202729ca806cd244fb37e5b45abb4cc69b/src/managers/KeybindManager.cpp#L1952 https://github.com/hyprwm/Hyprland/blob/7e8a21202729ca806cd244fb37e5b45abb4cc69b/src/managers/KeybindManager.cpp#L1969

memchr commented 1 year ago

because groups can be locked, in which case this dispatcher would move the group rather than a window

m_sGroupData.locked is currently used to prevent new windows from being added to the group, rather than to prevent dispatchers (esp. moveIntoGroup) from modifying the group, as the default behaviour is not to check this flag (misc:moveintogroup_lock_check).

move the group

Does this mean moving the container like what swapActive or moveActiveTo does?


If the default behaviour of dispatchers ignoring m_sGroupData.locked is to be retained, what do you think about moving the active window outside its group in another direction when the dispatcher is called on a group, and behaving like moveintogroup when called on a window?

active is window/group direction is behaviour
group group behave like moveIntoGroup
group window/null behave like moveOutOfGroup
window group behave like moveIntoGroup
window window/null do nothing
floating group behave like moveOutOfGroup
floating window do nothing
spikespaz commented 1 year ago

Doesn't CKeybindManager::mouse only support movewindow' and resizewindow'?

It's been a while so I'm not sure how clearly I remember, but I'm quite sure having those three dispatchers made it behave really weirdly. Like it would pick up the window and put it back, for an example of the behavior of one particular order.

I'm trying to remember if that was my testing with the keyboard but I don't think so, because I remember walking away and forgetting about it, then coming back and thinking "what the heck is happening to my mouse 5".

So I'm not sure what that means, but regardless of how the code is written, I'm almost positive those three lines caused funky things.

spikespaz commented 1 year ago

≥ Does this mean moving the container like what swapActive or moveActiveTo does?

Yes for the former, but what ismoveactiveto again? Not looking at the wiki ATM.

moving the active window outside its group in another direction when the dispatcher is called on a group, and behaving like moveintogroup when called on a window

That sounds exactly like the behavior I'm after.

Your first table row doesn't make sense to me. Oh, I get it. First cell I need to think "window in group".

Row number 4: active is window, direction is window --do nothing. I don't think that's what I want, that should behave as movewindow or swapactive, unless I'm misunderstanding again.

the dispatcher

I assume you're talking about a new one, movewindoworgroup?

memchr commented 1 year ago

I assume you're talking about a new one, movewindoworgroup?

Yes, by the dispatcher I mean the hypothetical new dispatcher who will move the window or group.

Row number 4: active is window, direction is window --do nothing. I don't think that's what I want, that should behave as movewindow or swapactive, unless I'm misunderstanding again.

Having the new dispatcher swap window position movewindow or swapactive would be inconsistent with how the dispatcher it is supposed to combine works.

The existing dispatchers, namely moveintogroup and moveoutofgroup, operate on at least one group by adding or removing window in the group(s). In addition, these dispatchers typically increase or decrease the total number of containers in a workspace, but never swap containers.

This inconsistent behaviour could confuse users who are familiar with the current functionality and defaults.

Therefore, in my opinion, the new dispatcher should not swap container positions by default when both the active and target containers are windows. Instead, it would be better to provide an option to enable container swapping via a configuration flag, for those who want that behavior.

https://github.com/hyprwm/Hyprland/blob/7e8a21202729ca806cd244fb37e5b45abb4cc69b/src/managers/KeybindManager.cpp#L2053-L2054

https://github.com/hyprwm/Hyprland/blob/7e8a21202729ca806cd244fb37e5b45abb4cc69b/src/managers/KeybindManager.cpp#L2074-L2075

memchr commented 1 year ago

That sounds exactly like the behavior I'm after

Please note that this hypothetical behaviour ignores m_sGroupData.locked and does not swap group containers like movewindow does, in the same light of maintaining consistency, with the additional reason being that moveintogroup also ignores m_sGroupData.locked by default.

memchr commented 1 year ago

The current onWindowCreatedTiling() methods do not currently allow you to move the window out of a group in a specific direction from any built-in layout. It only respects the mouse position or force_split for dwindle.

spikespaz commented 1 year ago

My idea for the movewindoworgroup was for it to always have a reasonable behavior (always move) if at all possible.

Having the new dispatcher swap window position movewindow or swapactive would be inconsistent with how the dispatcher it is supposed to combine works.

Two thoughts on this. This is inherently rather complicated, and therefore just calling the functions of other dispatchers or combining them would not be appropriate. For brevity, I use terms like AND and THEN below to describe behaviors; however this is not actually what I think should happen. We need unique code.

This inconsistent behaviour could confuse users who are familiar with the current functionality and defaults.

Because you wish to remain "consistent" with how these work (and this consistency does not adhere to my ideal functionality) I have introduced two behavioral "modes", A and B. Behavior A is essentially "this dispatcher is generic and dispatches other behaviors, and Behavior B is "move the window if it is possible".

Maybe instead of two behaviors (movewindoworgroup,a and movewindoworgroup,b) we should add another dispatcher for Behavior B, perhaps moveorswapwindoworgroup.

Active Tile Direction/Target Tile Behavior A Behavior B Reason
Window in Group Group moveoutofgroup THEN moveintogroup moveoutofgroup May want moveoutofgroup only because it is more versatile. The window can be moved a second time if moveintogroup is desired. Which dispatcher behavior is selected could be a choice.
Window in Group Window moveoutofgroup " Self-evident.
Window in Group null moveoutofgroup " Self-evident.
Window Group moveintogroup " Self-evident.
Window Window Do nothing. swapactive Self-evident.
Window null Do nothing. " Self-evident.
Window in Floating Group null moveoutofgroup moveoutofgroup AND float Which behavior depends on if the user wants a window which is already in a floating group to remain floating, or if it should find a position in the tiled layout. Choice.
Floating Window null Do nothing. " Self-evident.

Now, I would like to see something very similar to work with bindm, because that is how I intend to use it. I will make a new table with my ideas after a short break.

spikespaz commented 1 year ago

For another new dispatcher to work with bindm (and probably a control key):

Hovered Tile Behavior A Behavior B Reason
Window in a Group moveoutofgroup THEN movewindow " Self-evident.
Window movewindow " Self-evident.
Window in Floating Group moveoutofgroup THEN movewindow moveoutofgroup AND float THEN movewindow The original movewindow can always move the group itself, so no special behavior (option or C) is necessary.
Floating Window movewindow " Self-evident.
memchr commented 1 year ago

moveoutofgroup THEN moveintogroup

No, moveintogroup doesn't check for PWINDOW->m_sGroupData.pNextWindow.

spikespaz commented 1 year ago

moveoutofgroup THEN moveintogroup

No, moveintogroup doesn't check for PWINDOW->m_sGroupData.pNextWindow.

I'm annoyed, because you didn't read the preface.

For brevity, I use terms like AND and THEN below to describe behaviors; however this is not actually what I think should happen. We need unique code.

memchr commented 1 year ago

I'm annoyed, because you didn't read the preface.

I am truly sorry for the frustration or offence, but this is to clear up any confusion about what the dispatcher actually does, so we can collaborate more effectively.

If you prefer, we can stop this conversation.

https://github.com/hyprwm/Hyprland/blob/7155b4c26606d7681bacb2517b1d19495336c6c3/src/managers/KeybindManager.cpp#L2036-L2069

NOTE:

     if (!PWINDOW->m_sGroupData.pNextWindow) 
         PWINDOW->m_dWindowDecorations.emplace_back(std::make_unique<CHyprGroupBarDecoration>(PWINDOW)); 
memchr commented 1 year ago

@spikespaz Hi, sorry for the intrusion, would you like to test this PR? #3006

memchr commented 1 year ago

The new dispatcher is currently named movewindoworgroup, a new configuration variable binds:check_group_lock has been added which causes the dispatcher to behave alternatively around locked groups.

The dispatcher accepts any args that moveintogroup accepts.

For example, you can test it like this

bind = , g, togglegroup,
bind = SHIFT, g, moveoutofgroup,
bind = , slash, lockactivegroup, toggle

bind = SHIFT,h, movewindoworgroup, l
bind = SHIFT,l, movewindoworgroup, r
bind = SHIFT,k, movewindoworgroup, u
bind = SHIFT,j, movewindoworgroup, d

bind = , h, movefocus, l
bind = , l, movefocus, r
bind = , k, movefocus, u
bind = , j, movefocus, d

bind = , comma, changegroupactive, b
bind = , period, changegroupactive, f

bind = , t, setgrouplockchecking, toggle

binds {
    check_group_lock = true
}

misc {
    group_focus_removed_window = false
}
memchr commented 1 year ago

Other changes to the behaviour of existing dispersers are

Thank you very much for your time and feedback.

fleurc commented 11 months ago

Hi, I have no way to meaningfully make this enhancement any better, but I'd like to enter the discussion by pointing out that I've found this Issue thread when trying to implement a dispatcher of the name "movewindoworgroup" described in Hyprland Wiki

If this is not an implemented feature or a feature in testing only, why have it be described as available to use inside of the Wiki?

memchr commented 11 months ago

If this is not an implemented feature or a feature in testing only, why have it be described as available to use inside of the Wiki

Already implemented and merged: #3006 (with the exception of the mouse bindings), released in v0.30

and the wiki, to my knowledge, always documents the last git main branch.


@vaxerski Should this be closed?

fleurc commented 11 months ago

Is it implemented as of version 0.29.1-1?

fleurc commented 11 months ago

I ask because in said version, hyprland told me no such dispatcher existed

memchr commented 11 months ago

I ask because in said version, hyprland told me no such dispatcher existed

Already implemented and merged: https://github.com/hyprwm/Hyprland/pull/3006 (with the exception of the mouse bindings), released in v0.30

released in v0.30

fleurc commented 11 months ago

Ah thank you, I hadn't read that, very sorry for the inconvenience.

spikespaz commented 10 months ago

@memchr

I'm annoyed

I apologize for this interaction.

izmyname commented 1 month ago

Wasn't this implemented already, btw? I mean, there's a dispatcher, but the issue is still open.