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

Add scrollbar offset theme constants to Tree #70901

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Jan 4, 2023

This PR adds scrollbar_offset_{left,top,right,bottom} theme constants to Tree. These constants move scrollbar edges toward the correspoinding border.


Tree allows setting a background stylebox while ScrollContainer don't. Scrollbars in Tree use stylebox margins when placing the scrollbars.

The screenshot below is taken from Editor Settings. The scrollbar on the left is from Tree, and the scrollbar on the right is from ScrollContainer.

before-after

From top to bottom:

  1. 4.0beta10: The Y positions are different. (Because editor theme sets content margin in Tree's panel stylebox.)
  2. After Fix Tree overflow without scrolling being enabled #70877: The scrollbar honours background stylebox content right margin, which makes the difference more significant.
  3. After this PR: Added offset in editor theme so that Tree scrollbars and ScrollContainer scrollbars look the same.

Production edit: Closes #73139

@YuriSizov
Copy link
Contributor

I don't like that we resort to hacks to fix our inconsistencies instead of making things more consistent. Personally, I constantly have to work around the limitation of the scroll container, adding a margin container and making it a clipping rect. So I wouldn't mind having more control in both elements here, which gives us an opportunity to harmonize them instead of ad-hoc fixing the Tree in this manner.

I also don't like that offsets do the opposite of their name and positive numbers are moving the scrollbar outwards instead of inwards. Though that's an easy change.

I would detach the position of scrollbars from the stylebox's margins completely in Tree. We can still use the 4 constants, or we can add a new stylebox that would only contribute margins (and thus has to be StyleBoxEmpty). Same in the scroll container, we need a set of margins for the content and another for the scrollbars. Can be 4 constants each, or 2 margin-only styleboxes.

@timothyqiu
Copy link
Member Author

Updated:

  1. Now the constants are named scrollbar_margin_*. They push the scrollbars inward, relative to the control's borders. When negative, which is the default, margins from the background panel stylebox are used.
  2. Make tree content aware of scrollbar offset.
  3. Adds scrollbar_{h,v}_separation constants to define the gap between tree content and scrollbars. There were originally no distance. The gap you saw was the result of the offset scrollbars.
  4. Don't reserve space at the bottom right corner if only one of the scrollbars is visible.

doc/classes/Tree.xml Outdated Show resolved Hide resolved
scene/gui/tree.cpp Outdated Show resolved Hide resolved
scene/gui/tree.cpp Outdated Show resolved Hide resolved
scene/gui/tree.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Okay, I'm done with the review. I think all the math checks out, so LGTM!

Approving tentatively but the comments above need to be resolved.

PS. I would like to repeat that it'd be nice to do the same to ScrollContainer, but it seems to be a harder task at the moment. Namely because we have a hack for the animation editor, introduced in b659fd6.

@timothyqiu
Copy link
Member Author

timothyqiu commented Apr 6, 2023

Updated.

I replaced _get_content_rect_offset() with a new _get_content_rect() so many other places calculating with bg->get_minimum_size() and scrollbar sizes can benefit.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

This is a great refactoring of the sizing logic, good job!

The fact that items no longer clip the scrollbar is so satisfying:
godot windows editor dev x86_64_2023-04-07_13-18-30

@YuriSizov YuriSizov merged commit a13635c into godotengine:master Apr 7, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@timothyqiu timothyqiu deleted the tree-scroll-offset branch April 7, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollbar overlaps text in Godot 4 RC1 "About" Dialog box
2 participants