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 ItemList text trimming #97439

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

havi05
Copy link
Contributor

@havi05 havi05 commented Sep 25, 2024

This fixes #97438

The left content margin was probably calculated as an offset by the get_size() function in the line above, but I couldn't check that, because I haven't found the function. If this needs to be changed please tell me where to find it.

Video showing the resolved issue

2024-09-25.13-04-00.mp4

@havi05 havi05 requested a review from a team as a code owner September 25, 2024 10:15
@AThousandShips AThousandShips added bug topic:gui cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 25, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 25, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Sep 25, 2024

The minimal right margin is smaller than the left one. Shouldn't they be symmetrical? 🤔
image
EDIT:
It's symmetrical when h_separation is 0. Looks like the left side uses half of the separation, the right one should too probably.


Also the autowrap margin with multiple columns seems to be the same as before:

godot.windows.editor.dev.x86_64_WBVtklIQcZ.mp4

But this can be considered a separate issue.

@havi05
Copy link
Contributor Author

havi05 commented Sep 25, 2024

If the minimum right margin is the same size as the left margin, the text is cut off as it cannot be displayed completely in the center of the ItemList at this size. (see image)

Image

Screenshot 2024-09-25 165548

If you set the h_separation larger than the size of the ItemList, only the left size of the h_separation is taken into account and the text is displayed in full. Theoretically, if the h_separation is the size of the ItemList (or larger), it would fill the entire space and hide the text, leaving an empty item. (see video 1)

Therefore, I thought if there is enough space for the text, it should be displayed and it would be symmetrical if you enable the auto_width function (#93270). (see video 2)

Video 1

2024-09-25.17-04-24.mp4

Video 2

2024-09-25.17-12-36.mp4

@havi05
Copy link
Contributor Author

havi05 commented Sep 25, 2024

I think the autowrap margin should be considered as a separate issue. Is there a bug report already?

The autowrap margin takes the scroll bar into account, even if it is not visible.
Here are the videos with and without the fix.

Before

2024-09-25.17-38-37.mp4

After

2024-09-25.18-05-42.mp4

@havi05 havi05 force-pushed the itemlist-fix-texttrimming branch 3 times, most recently from 17ee4f4 to 554f73a Compare September 26, 2024 15:25
@WhalesState
Copy link
Contributor

It's symmetrical when h_separation is 0. Looks like the left side uses half of the separation, the right one should too probably.

This happens when adding panel->get_margin(SIDE_LEFT) to a position.x and subtracting panel->get_minimum_size().width from width instead of panel->get_margin(SIDE_RIGHT).

@havi05
Copy link
Contributor Author

havi05 commented Sep 29, 2024

@WhalesState I don't think I quite understood your comment.
Do you think it should stay the way I did it in the pr (and it's just the explanation to the problem), or should it be changed back to the previous state?

@WhalesState
Copy link
Contributor

WhalesState commented Sep 29, 2024

Seems like you already have fixed that, but i have found a regression.

  • The icons are not aligned correctly when using top icons.

Already an existing issue:

  • The h_separation is not clamped.
  • The separation line is not drawn when aligning the icons on top.
ItemList.mp4

@havi05 havi05 force-pushed the itemlist-fix-texttrimming branch from 554f73a to ed4d2bd Compare September 29, 2024 16:27
@havi05
Copy link
Contributor Author

havi05 commented Sep 29, 2024

The icons are not aligned correctly when using top icons.

Fixed

The h_separation is not clamped.

I have added if statements so that v_separation and h_separation can no longer be negative.
Is there a better way to disable negative numbers for Theme::DATA_TYPE_CONSTANT than checking each usage?

The separation line is not drawn when aligning the icons on top.

This was apparently intentionally removed. (#82236)

@WhalesState
Copy link
Contributor

WhalesState commented Sep 29, 2024

I have added if statements so that v_separation and h_separation can no longer be negative. Is there a better way to disable negative numbers for Theme::DATA_TYPE_CONSTANT than checking each usage?

I think we can't use property hints or setters for theme constants, you will have to clamp them everytime if the negative values will cause issues. you can use MAX(theme_cache.h_separation, 0), or maybe caching it int h_sep = MAX(theme_cache.h_separation, 0); if it will be used many times.

@havi05 havi05 force-pushed the itemlist-fix-texttrimming branch from ed4d2bd to 5e2e140 Compare September 29, 2024 16:59
@havi05
Copy link
Contributor Author

havi05 commented Oct 1, 2024

@KoBeWi What you think of my comment on the symmetry of the minimum margin? (#97439 (comment))
I think a new user could get confused that the text is truncated when the next entry to be separated is already on the next row.

@havi05 havi05 force-pushed the itemlist-fix-texttrimming branch from 5e2e140 to b0bf9d2 Compare October 1, 2024 08:41
@havi05 havi05 force-pushed the itemlist-fix-texttrimming branch from b0bf9d2 to 6a9e50b Compare October 1, 2024 15:49
@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2024

So I did an experiment to test the new behavior. I made a StyleBox with 0 content margins and borders equal to h_separation. This way it's easy to visualize the actual margins imposed by separation.

Here's old behavior:

godot_TeUIK8bHkk.mp4

It uses half separation on the left and full separation on the right.

Here's new behavior:

godot.windows.editor.dev.x86_64_XuH2iyyjRB.mp4

Half separation on the left, right separation is ignored.

I think both are sort of inconsistent, but the new one is more friendly, as there is more space.

@havi05
Copy link
Contributor Author

havi05 commented Oct 1, 2024

So it would be good for now, and we could open a discussion on better consistency if that is needed?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2024

Yeah, overall it's an improvement.

@havi05
Copy link
Contributor Author

havi05 commented Oct 1, 2024

Should I rebase the branch or does this happen automatically when merging?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2024

Rebasing is only needed if there are conflicts, or sometimes when a PR is very old and wasn't recently updated.

@akien-mga akien-mga merged commit 89febc5 into godotengine:master Oct 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release regression topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ItemList - Text is cut off even if there is enough space
5 participants