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.

Sign up for GitHub

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

Jump to bottom

GUI: Fix text overlapping icon in Tree #78756

Merged
merged 1 commit into from
Aug 25, 2023
Merged

GUI: Fix text overlapping icon in Tree #78756

merged 1 commit into from
Aug 25, 2023

Conversation

dalexeev
Copy link
Member

  1. Fix icon (left) size not being taken into account which causes the button (right) to overlap the text.
  2. Restore old default text_overrun_behavior. TextLine defaults to OVERRUN_TRIM_ELLIPSIS and TextParagraph defaults to OVERRUN_NO_TRIMMING.
  3. Make text_overrun_behavior customizable.

@dalexeev dalexeev requested review from a team as code owners June 27, 2023 15:31
@akien-mga akien-mga added bug topic:gui cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jun 27, 2023
@akien-mga akien-mga added this to the 4.2 milestone Jun 27, 2023
@akien-mga akien-mga requested a review from bruvzg June 27, 2023 15:36
capnm pushed a commit to capnm/godot4 that referenced this pull request Aug 8, 2023
@dalexeev @capnm
GUI: Fix text overlapping icon in Tree
f34025b 
godotengine#78756
?? 2 months ago
@dalexeev dalexeev force-pushed the gui-tree-fix-text-overlapping-icon branch from 602ddf7 to 4a526cf Compare August 9, 2023 14:39
@YuriSizov
Copy link
Contributor

I guess that would be an alternative to #71667 ? How does your solution handle deep nested trees? Ideally I'd like something close to the clip in #71667 (comment) (unfortunately, the sources of this approach have been lost to time, so it would need to be reproduced).

@dalexeev
Copy link
Member Author

dalexeev commented Aug 9, 2023 •
edited
Loading

No, I don't think it's related. This PR fixes a regression that came later than the last update #71667. Fixed only overlapping of the right icons (buttons) on the text, not on the left icon.

How does your solution handle deep nested trees?

1.mp4

The horizontal scroll bar remains, sorry for not capturing it on video.

akien-mga
akien-mga reviewed Aug 17, 2023
View reviewed changes
scene/gui/tree.cpp Outdated Show resolved Hide resolved
akien-mga
akien-mga approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga 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 overall, but I'll let @bruvzg review implementation details.

bruvzg
bruvzg approved these changes Aug 17, 2023
View reviewed changes
scene/gui/tree.cpp Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the gui-tree-fix-text-overlapping-icon branch from 4a526cf to f2cbd64 Compare August 17, 2023 17:38
@dalexeev
Copy link
Member Author

I noticed that after rebase the text doesn't fit anymore. Looks like some other PR broke this recently.

OP:

Now:

YuriSizov
YuriSizov suggested changes Aug 24, 2023
View reviewed changes
scene/gui/tree.cpp Outdated
@@ -2096,7 +2117,12 @@ int Tree::draw_item(const Point2i &p_pos, const Point2 &p_draw_ofs, const Size2
buttons_width += button_size.width + theme_cache.button_margin;
}

p_item->cells.write[i].text_buf->set_width(item_width);
int text_width = item_width;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int text_width = item_width;
int text_width = item_width - theme_cache.inner_item_margin_left - theme_cache.inner_item_margin_right;

This should fix the issue with the text overflow. It's already accounted for when drawing the rectangle, so we only need to add it to the text width. I've tested it and it seems to work fine.

See #75460.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've noticed a new bug, but it's probably not related to this PR.

1.mp4

Copy link
Contributor

@YuriSizov YuriSizov Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can also be related to these new margin properties, I guess.

@dalexeev dalexeev force-pushed the gui-tree-fix-text-overlapping-icon branch from f2cbd64 to 07d2348 Compare August 24, 2023 19:09
@YuriSizov YuriSizov merged commit 017b196 into godotengine:master Aug 25, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@dalexeev dalexeev deleted the gui-tree-fix-text-overlapping-icon branch August 25, 2023 13:12
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruvzg bruvzg bruvzg approved these changes

@YuriSizov YuriSizov YuriSizov approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
bug regression topic:gui
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Scene Panel: node names are overlapping icons
4 participants
@dalexeev @YuriSizov @akien-mga @bruvzg