-
Notifications
You must be signed in to change notification settings - Fork 98
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
Allow resize in both direcitons on Meta+Right click #860
base: master
Are you sure you want to change the base?
Conversation
Quick demo: 2024-09-13.02-33-27.mp4 |
Definitely has a some bugs with more windows on screen that I'll have to figure out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an initial implementation, it seems to work alright but I'm unfamiliar with this codebase so may have done something in a silly way.
Seems sensible, I also thought about how to fix this already, so here are some thoughts. Your implementation is quite close to how I envision this being solved, I would appreciate it, should you continue working on this.
@@ -94,6 +94,7 @@ impl PointerTarget<State> for ResizeForkTarget { | |||
location.as_global(), | |||
node, | |||
left_up_idx, | |||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably these would also gain the option to do more complex grabs at corners, but I guess we can leave this for now.
node: NodeId, | ||
output: WeakOutput, | ||
left_up_idx: usize, | ||
parent_left_up_idx: Option<usize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of splitting this in parent
and node
/, I would prefer, if we separately determine the left_idx
and left_node
and the up_idx
and the up_node
. (And have both be an Option
, though the grab should obviously never be created with both being None
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *_node
values would be instances of something like enum ResizeDest { Node, NodeParent }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looking at your other comment about having to walk up the tree until finding a split of the correct orientation, this needs to be a ResizeDest(usize)
meaning how many times we need to walk towards the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looking at your other comment about having to walk up the tree until finding a split of the correct orientation, this needs to be a ResizeDest(usize)
meaning how many times we need to walk towards the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *_node values would be instances of something like enum ResizeDest { Node, NodeParent }?
No just NodeId
s. You don't need to know how far you have walked, if you just mark the nodes we do the resize operations on. (Both of them, one for each axis.)
orientation, sizes, .. | ||
}) = parent.map(|p| tree.get_mut(&p).unwrap().data_mut()) | ||
{ | ||
if *orientation == child_orientation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can happen, we simply have to walk up the tree in a while
-loop and select the next parent
to check for it's orientation, until we potentially get None
(which will happen if there isn't any edge in the other direction as we hit the workspace/output border).
Fixes pop-os#858 This is just an initial implementation, it seems to work alright but I'm unfamilar with this codebase so may have done something in a silly way.
33cb8f0
to
e6c766a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty spot on what I had in mind! Just a couple of nitpicks. :)
Also please remember to remove the draft-status, once you want me to test this and eventually merge.
src/shell/layout/tiling/mod.rs
Outdated
@@ -2463,41 +2463,74 @@ impl TilingLayout { | |||
edges | |||
} | |||
|
|||
// Returns (left_node_idx, up_node_idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just create a struct definition for the return type at this point, so we can name these fields.
@@ -83,6 +83,10 @@ impl PointerTarget<State> for ResizeForkTarget { | |||
data.common.event_loop_handle.insert_idle(move |state| { | |||
let pointer = seat.get_pointer().unwrap(); | |||
let location = pointer.current_location(); | |||
let (left_node_idx, right_node_idx) = match orientation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left and right? Shouldn't this be left and up?
Just realized I swapped the resize grab directions, will fix tomorrow |
Ok, upon further review I don't think I regressed the grab--this snippet is from master (and it's the same on this branch) I think this branch is ready for testing/review. In my opinion the tiling "grab by hovering in the middle" is kinda strange as cosmic also allows you to drag the edges of the window...definitely a reason why I much prefer the Meta+Right click resize. 2024-10-08.21-29-46.mp4 |
Gentle reminder on this, is there anything left to do to move this forward? |
Closes #858
This is just an initial implementation, it seems to work alright but I'm unfamiliar with this codebase so may have done something in a silly way.