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 Auto width behavior to ItemList #93270

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

havi05
Copy link
Contributor

@havi05 havi05 commented Jun 17, 2024

I have rewritten the PR #39848 which implemented a Auto width functionality in the ItemList node, but never got rebased. It should be the same behavior like using the Auto height feature.

I had the problem that the last 2 letters of a word were not shown. That's why I added 6 pixels to the calculation in line 1481 (item_list.cpp) Please let me know if it is acceptable to hard-code this value.

Closes #31504


This is my first PR at the GodotEngine. Please tell me if I have done any mistakes or if there is anything I can improve.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2024

Please ask if the original author has abandoned their work, otherwise there's no reason to take it over, please communicate with other developers

Regardless you need to cite them as a coauthor as you've copied their code, like so:


Co-authored-by: Craig Hupin <althar93@hotmail.com>

The empty line matters

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from c9da14a to 09785a5 Compare June 17, 2024 17:17
@havi05
Copy link
Contributor Author

havi05 commented Jun 17, 2024

Please ask if the original author has abandoned their work, otherwise there's no reason to take it over, please communicate with other developers

@AThousandShips Thank you for bringing this to my attention and asking Althar93 for me. I'll keep it in mind for next time.

@havi05
Copy link
Contributor Author

havi05 commented Jun 17, 2024

Am I right that the auto_width documentation must be written below auto_height to solve the failling check?

@AThousandShips
Copy link
Member

Yes it has to be alphabetical, use --doctool on the built binary to generate automatically

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 09785a5 to 33100c3 Compare June 17, 2024 17:47
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 33100c3 to 7a1bb4c Compare June 29, 2024 21:18
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 7a1bb4c to 6637a5c Compare August 15, 2024 18:58
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 6637a5c to 12e1a10 Compare September 9, 2024 14:45
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 12e1a10 to 0bf1a5d Compare September 19, 2024 14:36
@havi05
Copy link
Contributor Author

havi05 commented Sep 19, 2024

Since Althar93 has not responded to the question 5 months ago about continuing his work (#39848 (comment)) and his last commit in #39848 was 4 years ago, would it be possible to review these changes already, or would you prefer to wait?

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 0bf1a5d to c10879f Compare September 24, 2024 10:54
@KoBeWi
Copy link
Member

KoBeWi commented Sep 24, 2024

The feature is implemented correctly, but it has problems with fitting text. See the above comment.

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch 2 times, most recently from a39901d to d532eee Compare September 24, 2024 21:28
@havi05 havi05 marked this pull request as draft September 24, 2024 21:30
@KoBeWi
Copy link
Member

KoBeWi commented Sep 24, 2024

The text is no longer cut, but there is some excessive margin on the right.

godot.windows.editor.dev.x86_64_5SeNSEtuhg.mp4

@havi05
Copy link
Contributor Author

havi05 commented Sep 24, 2024

I noticed I had theme_cache.panel_style->get_minimum_size().width two times in my calculation. I'm checking if thats the problem.

@havi05

This comment was marked as off-topic.

@havi05
Copy link
Contributor Author

havi05 commented Sep 25, 2024

This depends on this pr #97439 for this issue #97438.

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from d532eee to 7331249 Compare October 2, 2024 14:11
@havi05 havi05 marked this pull request as ready for review October 2, 2024 14:12
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from 7331249 to ce9ec99 Compare October 2, 2024 15:09
@havi05
Copy link
Contributor Author

havi05 commented Oct 2, 2024

I applied the requested changes and fixed a bug that caused the right margin to be too wide when more than one column was used.

@havi05 havi05 requested a review from KoBeWi October 2, 2024 15:32
@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2024

The margin problem is fixed.
Though there is a problem where if you increase column count, the size doesn't update immediately:

godot.windows.editor.dev.x86_64_8g3HH9T339.mp4

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from ce9ec99 to aa4824e Compare October 3, 2024 06:55
@havi05
Copy link
Contributor Author

havi05 commented Oct 3, 2024

I fixed the column update problem. I have noticed that after hiding the visibility of the scrollbar, the size is updated about one pixel too late. I could not figure out what is causing this problem.

2024-10-03.08-50-46.mp4

Thank you for your quick replies and your patience testing my code so many times!

@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from aa4824e to ac1dbc7 Compare October 3, 2024 15:08
Co-authored-by: Craig Hupin <althar93@hotmail.com>
@havi05 havi05 force-pushed the feature-itemlist-autowidth branch from ac1dbc7 to 1e1dbd8 Compare October 4, 2024 06:44
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 4, 2024
@akien-mga akien-mga merged commit 058f06c into godotengine:master Oct 4, 2024
19 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.

ItemList doesn't expand for text elements
4 participants