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

Fix Tree minimum size calculation #91095

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

timothyqiu
Copy link
Member

Fixes #91047

  • Takes scrollbar space and column title buttons into account
  • Fixes first column min size sometimes missing one level of indent
  • Fixes cell min size ignoring text overrun behavior and item inner margin
  • Updates min size when hide_root or column_title_visible changes

Also fixed wrong description of item_inner_margin_* constants. They are actually applied to cells instead of items (rows) in the PR that introduced them. The naming is unfortunate, but I don't think it's worth breaking compat for.


This PR accidentially fixed the problem that Scene dock uses both text ellipsis and horizontal scrollbar (and the scrollbar did not allow you to see the whole text) 🤣

Before After
Peek 2024-04-24 10-06 Peek 2024-04-24 10-03

If the horizontal scrollbar is preferred, all we need to do is setting the text overrun behavior to "no trimming".

@timothyqiu timothyqiu added this to the 4.3 milestone Apr 24, 2024
@timothyqiu timothyqiu marked this pull request as ready for review April 24, 2024 02:47
@timothyqiu timothyqiu requested review from a team as code owners April 24, 2024 02:47
@AdriaandeJongh
Copy link
Contributor

AdriaandeJongh commented Apr 24, 2024

If the horizontal scrollbar is preferred, all we need to do is setting the text overrun behavior to "no trimming".

As long as there is a tooltip that displays the full title of a node with a title longer than can be displayed, I think 'no horizontal scrollbar' is the preferred fix. Horizontal scrolling is simply always cumbersome IMO.

EDIT: clarified what I meant with 'this fix'.

min_size.y += bg->get_margin(SIDE_TOP) + bg->get_margin(SIDE_BOTTOM);
}
return Vector2(h_scroll_enabled ? 0 : min_size.x, v_scroll_enabled ? 0 : min_size.y);
Vector2 min_size = Vector2(0, _get_title_button_height());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be ignored when vertical scroll is enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Vertical scrollbar does not scroll the title.

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

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Left some comments, but otherwise looks ok.
(I didn't test extensively though)

@akien-mga
Copy link
Member

Needs rebase to fix CI.

- Take scroll bar space and column title buttons into account
- Fix first column min size sometimes missing one level of indent
- Fix cell min size ignoring text overrun behavior and item inner margin
- Update min size when `hide_root` or `column_title_visible` changes

Wrong description of `item_inner_margin_*` constants is also fixed
@akien-mga akien-mga merged commit ab2daa2 into godotengine:master Apr 25, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Empty Tree ignores column title for minimum size
5 participants