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 item positioning, text alignment & unwanted clipping of ItemList items #88577

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

davthedev
Copy link
Contributor

@davthedev davthedev commented Feb 20, 2024

Implements part of the fixes mentioned in #88217

The following points are fixed:

  • Clipping of selection indicator at the top or bottom of the list
  • Gap at the bottom of the list in rows mode when the end of the scrollbar is reached
  • In grid display, labels were not properly centered, a little offset to the side was happening
  • Separators overlapping some items by a pixel or two beyond the border
  • Inconsistent scale of vertical separation, which is double in comparison to horizontal separation (if you put 10px vertically, it displays like 20px, while if you put 10px horizontally, it remains 10px). Default & editor themes adjusted to compensate.

For the sake of consistency, the horizontal separation which did nothing on list rows (non-grid) mode is now acting like an internal padding, the same way it does for grid items.

This is a rewrite of the positioning algorithm so that internal calculation of item positions now corresponds to the actual screen rendering.

In the base situation before this fix, items positions are calculated with a gap in mind then grown on the four sides to cover that gap. This is responsible of the clipped selection indicators.

With this fix applied, the position calculation takes into account an appropriate padding to give the same effect as in the initial situation, and positions them correctly to align with the container borders without unwanted clippings and paddings.

No more clipping at the top and left

Capture d’écran du 2024-02-20 03-49-31
Capture d’écran du 2024-02-20 03-49-12

No more extra gap at the bottom

Capture d’écran du 2024-02-20 03-49-51

Text is correctly centered within grid items

Capture d’écran du 2024-02-20 03-48-53
Capture d’écran du 2024-02-20 03-42-02

Row items can have horizontal padding, leveraging the h_separation parameter that was not taken into account in this mode

Capture d’écran du 2024-02-20 04-12-07

@Mickeon Mickeon requested review from Mickeon and KoBeWi February 22, 2024 16:58
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

When making #88370 I encountered several minor nuisances that I feel like this PR addresses.
The code also looks cleaner and I feel like users would appreciate this, even if it would ever-so-slightly change how ItemLists look.

Still requires testing to make sure nothing silly is overlooked.

scene/gui/item_list.cpp Outdated Show resolved Hide resolved
scene/gui/item_list.cpp Outdated Show resolved Hide resolved
scene/gui/item_list.cpp Outdated Show resolved Hide resolved
@davthedev davthedev force-pushed the itemlist-light-refactor branch from f22f48b to 4e9abb9 Compare February 25, 2024 20:09
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally in the editor and control gallery demo project, it works as expected. However, in the default project theme used by the control gallery demo project, vertical spacing is now reduced, which makes it harder to click individual items:

Before After
Screenshot_20240226_192641 Screenshot_20240226_192631

scene/resources/default_theme.cpp needs to be modified to increase ItemList's v_separation constant to get a similar appearance to before.

We should make sure the change in how ItemList's v_separation theme item is interpreted is clearly documented in the release notes though, as existing themes will need to double the value for the current appearance to be retained. As such, we should avoid cherry-picking this PR to 4.2.

@davthedev
Copy link
Contributor Author

Indeed, I forgot to adjust the default theme after the vertical spacing fix. Will have a look.

I don't think some smaller item height makes the thing harder to click, on the opposite it becomes less small-screen-friendly and requires more work when using the default theme in a low-res, scaled-up game. But I agree with your point that we should avoid breaking existing projects appearance as much as possible and limit those to more important version upgrades.

I have additional fixes and improvements that I plan to build on top of this work, btw.

@davthedev davthedev force-pushed the itemlist-light-refactor branch 2 times, most recently from b0007e6 to eb65b28 Compare February 27, 2024 00:17
@davthedev davthedev requested a review from a team as a code owner February 27, 2024 00:17
@davthedev davthedev force-pushed the itemlist-light-refactor branch from eb65b28 to f63728c Compare February 27, 2024 19:30
@davthedev
Copy link
Contributor Author

I limited the blast radius of this refactoring so that we can merge it in 4.2. Perhaps I can revert the v_separation to use the old 2x behavior to avoid breaking current themes. What do you think?

Saying this because I have other points to refactor that can break existing themes, and I want to deliver those in a different PR that would wait for the correct timing for such changes. This PR would have not point then as it is considered breaking in the current state.

@akien-mga akien-mga merged commit da91622 into godotengine:master Feb 29, 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.

5 participants