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

Expose and rename ItemList's _check_shape_changed to force_update_list_size #63634

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

Spartan322
Copy link
Contributor

@Spartan322 Spartan322 commented Jul 29, 2022

Closes godotengine/godot-proposals#254

Exposes force_update_list_size to allow users to update auto_height_value early before draw if shape_changed is true, can be retrieved via get_minimum_size().

Exposed Additions

ItemList:

func force_update_list_size()

@Spartan322 Spartan322 requested review from a team as code owners July 29, 2022 17:04
@Calinou Calinou added this to the 4.0 milestone Jul 29, 2022
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from 4c09602 to 4f1b4a4 Compare July 29, 2022 22:10
@akien-mga akien-mga modified the milestones: 4.0, 4.x Sep 1, 2022
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch 2 times, most recently from 9b31eb8 to 569f778 Compare October 20, 2022 07:37
@Spartan322
Copy link
Contributor Author

Spartan322 commented Oct 20, 2022

Its probably better with this PR to also expose the auto_height_value as a getter, as in cases where auto_height is false its impossible to tell what the current is still without introducing code smells: (and it still calculates it regardless of auto_height)
Current solution this provides:

# changed list without auto_height enabled
force_update_list_size()
auto_height = true
var height = get_minimum_size().y
auto_height = false
# ...

But if auto_height_value is exposed and updates could be forced then both the code and documentation becomes more readable and doesn't require this janky solution:

# changed list without auto_height enabled
force_update_list_size()
var height = get_auto_height()
# ...

@KoBeWi
Copy link
Member

KoBeWi commented Dec 13, 2022

The PR looks ok. I guess it doesn't hurt to add the new function if it has some uses (and it's huge, so replicating it isn't exactly simple). The other function you mention has easy workaround and is rather specific, so it should be added in another PR if anything (and probably needs separate proposal).

I'm wondering if the name can't be better though. Maybe it should be update_auto_height().

doc/classes/ItemList.xml Outdated Show resolved Hide resolved
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch 2 times, most recently from aa48ad5 to 7e45c7e Compare December 13, 2022 20:57
@Spartan322 Spartan322 changed the title Move and expose item update behavior to update_for_items Move and expose item auto height update behavior to update_auto_height Dec 13, 2022
scene/gui/item_list.cpp Outdated Show resolved Hide resolved
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from 7e45c7e to c51d5e1 Compare December 13, 2022 22:35
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.

I think the method description might still need an improvement, but I don't have any suggestion. Other than that the PR looks ok.

doc/classes/ItemList.xml Outdated Show resolved Hide resolved
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch 2 times, most recently from e24bad9 to 02f2fc5 Compare December 14, 2022 14:04
@akien-mga
Copy link
Member

akien-mga commented Dec 14, 2022

The CI failure was my fault (now fixed), will need a rebase to solve it.

@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from 02f2fc5 to 033ce9e Compare December 14, 2022 16:12
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from 033ce9e to d143fa3 Compare December 21, 2022 20:16
@akien-mga akien-mga requested a review from YuriSizov August 28, 2023 10:12
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
scene/gui/item_list.cpp Outdated Show resolved Hide resolved
scene/gui/item_list.cpp Outdated Show resolved Hide resolved
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from d143fa3 to 3cde7cd Compare September 2, 2023 22:10
@Spartan322
Copy link
Contributor Author

Spartan322 commented Sep 2, 2023

I'm not exactly sure what would be the best recommendation for this as of a2a524d, as that accomplishes majorly the same functionality except it doesn't expose it, which is fine, but would it be better to rename _check_shape_changed to update_auto_height or merely expose it as such in the bindings? Or should the underscore in the name simply be removed and expose it as that, the name makes it sound kinda incorrect for a regular user, its kinda like a try_update type of behavior. Regardless I could also use this PR to update the comments to reflect the comment style.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 14, 2023

@Spartan322 If we're exposing it, it can be named force_update_list_size. Calling it anything auto_height would be a misnomer. Note that "list" is there so that the method is not named too generically to avoid conflicts with classes higher up the inheritance chain.

There is no reason to have different names publicly and privately, so you can can just rename the method and make it public, and expose it with the same name to the API.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 14, 2023
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

See above.

@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch 3 times, most recently from 29a926c to 11e3490 Compare September 14, 2023 23:01
@Spartan322 Spartan322 changed the title Move and expose item auto height update behavior to update_auto_height Rename ItemList::_check_shape_changed to force_update_list_size and expose Sep 14, 2023
@Spartan322 Spartan322 changed the title Rename ItemList::_check_shape_changed to force_update_list_size and expose Expose and rename ItemList's _check_shape_changed to force_update_list_size Sep 14, 2023
doc/classes/ItemList.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Other than the docs, this seems fine to me.

Rename ItemList::_check_shape_changed to force_update_list_size

`force_update_list_size` is especially useful for updating the auto_height_value early
@Spartan322 Spartan322 force-pushed the itemlist-predraw-size branch from 11e3490 to 421e8c5 Compare September 15, 2023 17:18
@akien-mga akien-mga merged commit 4a48fd4 into godotengine:master Sep 17, 2023
@akien-mga
Copy link
Member

Thanks!

@Spartan322 Spartan322 deleted the itemlist-predraw-size branch September 22, 2023 01:44
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.

Implement ItemList Pre-Draw Size Calculation
6 participants