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

Improvements to SplitContainer including a drag bar background StyleBox #72680

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

Koyper
Copy link
Contributor

@Koyper Koyper commented Feb 3, 2023

Improves SplitContainer in a number of ways making it much easier to script, and tidies up the code as follows:

  • A new split_bar_background StyleBox allows solid color, gradients or textures to fill in the split bar behind or in place of the grabber icon.
  • An essential new property drag_area_scrollbar_offset is required if one of the child controls has a scrollbar or other selectable control up against the split bar, in order to prevent the drag area from blocking mouse selection of the scroll bar or control.
  • New signals drag_started and drag_released make it easier to script saving and restoring the split_offset
  • A new property dragging_enabled separates dragging from dragger_visibility so the appearance of the drag bar is independent from whether dragging is enabled or not.
  • A new method get_drag_area_control returns the Control so buttons or any other type of Control can ride along with the split bar (see demo video below).
  • New properties drag_area_margin_begin and drag_area_margin_end allow for scripting the long-axis size of the drag area and background, which makes some nice uses like drawers simple to script (see video below).
  • A new drag_area_highlight_in_editor property allows you to see the drag area during development.

- A new virtual method _splits_ready() with a corresponding signal so a script knows when the split container children have been resized after the initial sort. This was a big problem because formerly, SplitContainer._ready() was called before the children had been fit to the container, and there was no easy way to get that information.

This PR also fixes this issue and adds the theme fix: #71862 and #71861

Adds this enhancement #61534

Screen.Recording.2023-02-03.at.12.29.31.PM.mov

This is all the GDScript neccessary for the buttons:

func _ready():
	var button := $Button
	var button_right := $ButtonRight
	remove_child(button)
	remove_child(button_right)
	hsplit.get_drag_area_control().add_child(button)
	hsplit.get_drag_area_control().add_child(button_right)
	button.position.y = -150 
	button_right.position.y = -50
Screen.Recording.2023-02-03.at.12.36.25.PM.mov
Screen.Recording.2023-02-03.at.12.40.10.PM.mov

Here's the editor with the drag areas visible:

Screen.Recording.2023-02-03.at.12.58.21.PM.mov

Production edit: Closes #61534, closes godotengine/godot-proposals#6230 and closes godotengine/godot-proposals#6231.

@Koyper Koyper force-pushed the split_container_improvements branch 6 times, most recently from 27eaf38 to 6848051 Compare February 3, 2023 19:30
@Koyper Koyper marked this pull request as ready for review February 3, 2023 19:49
@Koyper Koyper requested review from a team as code owners February 3, 2023 19:49
@Koyper Koyper force-pushed the split_container_improvements branch from 6848051 to dd8e5d9 Compare February 3, 2023 19:59
@YuriSizov YuriSizov added this to the 4.x milestone Feb 3, 2023
@YuriSizov
Copy link
Contributor

That's a lot. It needs a proposal, or several, to explain various enhancements. Probably better done as several PRs too, but that would be more obvious after the proposals.

@Koyper
Copy link
Contributor Author

Koyper commented Feb 3, 2023 via email

@KoBeWi
Copy link
Member

KoBeWi commented May 23, 2023

Needs rebase.

A new virtual method _splits_ready() with a corresponding signal so a script knows when the split container children have been resized after the initial sort.

You can also connect sort_children with CONNECT_DEFERRED and CONNECT_ONE_SHOT for the same result I think. Or ready with CONNECT_DEFERRED.

@Koyper
Copy link
Contributor Author

Koyper commented May 24, 2023

You can also connect sort_children with CONNECT_DEFERRED and CONNECT_ONE_SHOT for the same result I think. Or ready with CONNECT_DEFERRED.

I just tried these, and sort_children emits too early, but ready with CONNECT_DEFERRED seems to work. The problem I was trying to solve invloves child Controls that are expensive to fit, which unpredictably cause SplitContainer `_ready()' to be called too early.

If there are still issues with the ready approach I will eventually know about it, so I could remove the proposed _splits_ready() and accompanying signal from the PR and put a new PR together later if an ugly head arises?

@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2023

If there are still issues with the ready approach I will eventually know about it, so I could remove the proposed _splits_ready() and accompanying signal from the PR and put a new PR together later if an ugly head arises?

Yeah right now the signal is not needed for anything. It's also inconsistent with other containers which don't expose anything like this.

@Koyper Koyper force-pushed the split_container_improvements branch from dd8e5d9 to 0607c60 Compare May 24, 2023 15:25
@Koyper
Copy link
Contributor Author

Koyper commented May 24, 2023

This push included a rebase and removed the _splits_ready() virtual method and related signal.

@Koyper Koyper force-pushed the split_container_improvements branch 2 times, most recently from dc48cd1 to d661619 Compare May 24, 2023 15:43
@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2023

An essential new property drag_area_scrollbar_offset is required if one of the child controls has a scrollbar or other selectable control up against the split bar, in order to prevent the drag area from blocking mouse selection of the scroll bar or control.

Can you show an example when this is useful? The child nodes of SplitContainer can't be shrunk past their minimum size, so it's impossible to overlap scrollbars and drag area.

New signals drag_started and drag_released make it easier to script saving and restoring the split_offset

Isn't dragged signal enough?

New properties drag_area_margin_begin and drag_area_margin_end allow for scripting the long-axis size of the drag area and background, which makes some nice uses like drawers simple to script (see video below).

Which video?

A new drag_area_show_drag_area property allows you to see the drag area during development.

Shouldn't this be editor-only? Also the double drag_area in name is awkward.

@KoBeWi

This comment was marked as resolved.

@Koyper Koyper force-pushed the split_container_improvements branch 2 times, most recently from 805ea5b to 2cce2aa Compare April 27, 2024 14:32
@Koyper
Copy link
Contributor Author

Koyper commented Apr 27, 2024

@kitbdev All the changes we discussed were made - thanks for the excellent suggestions!

@akien-mga akien-mga modified the milestones: 4.3, 4.4 May 28, 2024
@akien-mga
Copy link
Member

Sorry for the delay following up.
We're now in feature freeze for 4.3, so I'm moving this to the 4.4 milestone. My understanding is that it's ready to go otherwise, so we can merge early on in the 4.4 dev cycle. It will need a rebase to solve present merge conflicts.

@Koyper
Copy link
Contributor Author

Koyper commented May 28, 2024

We're now in feature freeze for 4.3, so I'm moving this to the 4.4 milestone. My understanding is that it's ready to go otherwise, so we can merge early on in the 4.4 dev cycle. It will need a rebase to solve present merge conflicts.

Sounds good. I will take care of the conflicts shortly.

@Koyper Koyper force-pushed the split_container_improvements branch from 2cce2aa to 8ae0b1f Compare June 1, 2024 16:17
@Koyper
Copy link
Contributor Author

Koyper commented Sep 4, 2024

In reference to #90411, there is quite a bit of overlapping effort (and similar thinking) in cleaning up SplitContainer, even before any additional features are implemented. Would it be easier to implement #90411 on top of this PR, since this PR addresses some more fundamental deficiencies? What is the sensible way to merge the efforts?

@akien-mga
Copy link
Member

In reference to #90411, there is quite a bit of overlapping effort (and similar thinking) in cleaning up SplitContainer, even before any additional features are implemented. Would it be easier to implement #90411 on top of this PR, since this PR addresses some more fundamental deficiencies? What is the sensible way to merge the efforts?

CC @kitbdev

@kitbdev
Copy link
Contributor

kitbdev commented Sep 5, 2024

I don't think there is much overlap in cleaning up SplitContainer, besides some minor doc issues. #90411 just needs to touch a lot of things due to logic changes. Where they do clean up, they clean up different things like _get_sortable_child SortableVisbilityMode, #includes, or _get_separation().

#90411 needs some fixes to properly handle changing child visibility, so it isn't ready yet, and I don't want to hold up other PRs.
I don't mind rebasing on top of this. Though I would suggest some changes like moving draw back to the dragger.

scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
@Koyper Koyper force-pushed the split_container_improvements branch 2 times, most recently from f9946ba to 40e25ad Compare September 6, 2024 00:46
@Koyper
Copy link
Contributor Author

Koyper commented Sep 6, 2024

I don't think there is much overlap in cleaning up SplitContainer, besides some minor doc issues. #90411 just needs to touch a lot of things due to logic changes. Where they do clean up, they clean up different things like _get_sortable_child SortableVisbilityMode, #includes, or _get_separation().

#90411 needs some fixes to properly handle changing child visibility, so it isn't ready yet, and I don't want to hold up other PRs. I don't mind rebasing on top of this. Though I would suggest some changes like moving draw back to the dragger.

Thanks for offering to rebase on top of this. I did put the draw back into the dragger, and I see now why it needs to be there so you can have multiple dragger instances with more than two children. Each dragger now also saves its stylebox Rect2, so you should be able to keep an array of draggers and have them all update correctly. I believe the SortableVisbilityMode is also now there per a prior PR that hadn't yet been rebased here. Thanks for your comments, and I look forward to the multi-child improvements!

scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
@Koyper Koyper force-pushed the split_container_improvements branch 3 times, most recently from 0c76e98 to ca935ff Compare September 14, 2024 01:21
@Koyper Koyper force-pushed the split_container_improvements branch 2 times, most recently from 319f82b to 20b6755 Compare September 15, 2024 00:31
Copy link
Contributor

@kitbdev kitbdev 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 to me.

I still wish there was a better way to do the drag_area_highlight_in_editor than exposing it.

@akien-mga akien-mga merged commit cb86afd into godotengine:master Sep 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Koyper Koyper deleted the split_container_improvements branch September 17, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants