-
-
Notifications
You must be signed in to change notification settings - Fork 22.9k
Implement automatic snapping in split containers #106323
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
base: master
Are you sure you want to change the base?
Implement automatic snapping in split containers #106323
Conversation
df9270c
to
42271a0
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.
Tested locally, it mostly works as expected.
Some feedback:
-
There's an issue where clicking the bottom panel for the first time will hide it immediately (since it tries to resize to the minimum possible size, which now hides it entirely). This is also why the bottom panel isn't visible anymore while the editor is still starting (and restoring scenes from the previous session).
-
Hiding the bottom panel completely when you drag it all the way down might be confusing. I can see a lot of users wondering where the bottom panel went and having to resort to restarting the editor to get it back.
-
Maybe dragging the bottom panel all the way down should just collapse it (like when you click on the name of the current bottom panel), and hiding it entirely should be left to a dedicated button in the bottom panel.
-
This issue also applies to the side docks, but it's probably less extreme. In any case, we might need to show some kind of notice the first time you collapse any of the docks in an editor session. One of my UX pet peeves in software is when I manage to hide something and have no idea how to undo it 🙂1
-
- Additionally, if you hide both docks and toggle distraction-free mode, it will appear to do nothing (except cause a very slight UI shift on the left side, which may indicate a leftover spacer control or similar):
distraction_free_hide_docks.mp4
- This is a good opportunity to make distraction-free mode hide the bottom panel again, now that you get more control over what to hide. This has been a recurring request over the years: godotengine/godot-proposals#9141
Footnotes
-
If you've pressed Tab while using GIMP, you know. ↩
Sorry about the misclick — I'm on mobile. |
While testing this PR, I noticed that in the ProjectSettings or EditorSettings window, the left panel was already snaps to the edge. This behavior doesn't seem intentional — is that expected? |
Also it's possible to hide center panel, this is definitely not intended 😄 |
42271a0
to
289e7af
Compare
289e7af
to
61af908
Compare
…r drags the splitter beyond a threshold after the minimum size of a child, it "snaps" to the appropriate edge (left/right for horizontal arrangement, top/bottom for vertical arrangement), causing the child look as if it disappeared. The splitter can still be dragged back to reveal the child. Visually this looks similar to collapsing but functionally it differs as it only affects the visual state of the nodes.
61af908
to
780a344
Compare
Made some changes based on the comments:
For the bottom button panel i tried to have it enter in the collapsed mode automatically while dragging but because the buttons themselves are inside the splitter and collapsing it causes all sorts of other side effects the code was becoming way too gnarly for something that seems like a very special case, so instead i made the bottom panel collapse after the drag finishes (the effect is almost the same, except you can drag the splitter all the way to the bottom but once you release the mouse button the buttons "pop" back out). To make this happen while dragging it'd require moving the buttons outside the splitter, but this felt like a much more intrusive change to me. Also note that while the default setting is to allow autosnapping for both children, this can be changed in cases where autosnapping needs to be done only for one child or disabled. However i think having the default be to autosnap for both is more useful as it'd allow pretty much every splitter to snap and save screen real estate across the editor without any changes (it does cause the unit tests for dragging to fail though so i'll need to see what to do with that). |
Using FIRST/SECOND as snap target will conflict with #90411 |
Btw the title of this commit is far too long, the commit description and title should be brief and clear |
case NOTIFICATION_EXIT_TREE: { | ||
SplitContainer *center_split = Object::cast_to<SplitContainer>(get_parent()); | ||
ERR_FAIL_NULL(center_split); | ||
center_split->disconnect(SNAME("drag_ended"), callable_mp(this, &EditorBottomPanel::_center_split_drag_ended)); |
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.
Why are you disconnecting the signal?
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.
Restoring the state as it was before the node entered the tree since i connect the signal on NOTIFICATION_ENTER_TREE
i thought it'd be a good idea to disconnect it in NOTIFICATION_EXIT_TREE
.
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.
But bottom panel enters and exits tree when the editor starts and closes, it happens only once. It's irrelevant if the signal stays connected.
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.
You are correct, if it happens only once then it is not necessary to remove it, i'll update the code later to remove this part.
@@ -100,6 +112,31 @@ void EditorBottomPanel::_update_disabled_buttons() { | |||
right_button->set_disabled(h_scroll->get_value() + h_scroll->get_page() == h_scroll->get_max()); | |||
} | |||
|
|||
void EditorBottomPanel::_center_split_drag_started() { | |||
SplitContainer *center_split = Object::cast_to<SplitContainer>(get_parent()); | |||
ERR_FAIL_NULL(center_split); |
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.
Pretty sure this check is only needed once, when connecting the signal. The bottom dock does not move.
void SplitContainer::_fit_child_in_rect_with_visibility_update(Control *p_child, const Rect2 &p_rect) { | ||
// For very small rects (like when a size is set to 0 via autosnapping) hide the child instead |
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 misleading, because visibility is not being changed.
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.
Depends on how you think about it, from a technical perspective it changes the scale so it doesn't actually change its visibility flag, but from the user's perspective it makes the child disappear, so it does change its visibility. The goal of the function is to update the visibility from the end user's perspective even if from the technical side it changes its scale - hence the reason i added the comment to clarify what is going on and why.
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.
You could say like "disappear from view" because "hide", "show", "visibility", etc. have very specific meanings for CanvasItem & co.
@@ -245,7 +263,26 @@ void SplitContainer::_compute_split_offset(bool p_clamp) { | |||
// Clamp the split offset to acceptable values. | |||
int first_min_size = first->get_combined_minimum_size()[axis_index]; | |||
int second_min_size = second->get_combined_minimum_size()[axis_index]; | |||
computed_split_offset = CLAMP(wished_size, first_min_size, size - sep - second_min_size); | |||
|
|||
// Check autosnapping |
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.
// Check autosnapping | |
// Check autosnapping. |
Same in all comments.
Closes godotengine/godot-proposals#12417.
Implement automatic snapping in split containers so that when the user drags the splitter beyond a threshold after the minimum size of a child, it "snaps" to the appropriate edge (left/right for horizontal arrangement, top/bottom for vertical arrangement), causing the child look as if it disappeared. The splitter can still be dragged back to reveal the child.
Visually this looks similar to collapsing but functionally it differs as it only affects the visual state of the nodes.
The proposal contains a video of this in action.
Make the individual editor side docks minimizable using keyboard shortcuts godot-proposals#1441 (requires mouse to hide individual docks).