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

Bind missing default value for RichTextLabel methods. #79053

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Jul 5, 2023

Bind missing default value for RichTextLabel methods:
void push_font()
void set_table_column_expand()

I found that void push_paragraph()'s default value of argument p_direction in c++ is Control::TEXT_DIRECTION_INHERITED, but use TextServer::DIRECTION_AUTO to bind.
I'm not sure this need to be fixed or not.

@YuriSizov YuriSizov added this to the 4.2 milestone Jul 5, 2023
@Daylily-Zeleen Daylily-Zeleen marked this pull request as ready for review July 5, 2023 11:16
@Daylily-Zeleen Daylily-Zeleen requested review from a team as code owners July 5, 2023 11:16
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/rich_text_label_missing_default_method_value branch from d958404 to dd9e2b7 Compare July 5, 2023 11:27
scene/gui/rich_text_label.cpp Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/rich_text_label_missing_default_method_value branch 2 times, most recently from 4ef0c4e to 7fdaa1c Compare July 7, 2023 07:53
@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner July 7, 2023 07:53
@akien-mga
Copy link
Member

akien-mga commented Jul 7, 2023

CC @RedworkDE - We should think about how we want to handle compatibility now that 4.1 is released.
We still run checks against the 4.0-stable API, we should start checking 4.1-stable (and see if we also still want to check 4.0, or only 4.1).

Edit: I wouldn't block this PR either way, if we add the 4.1-stable checks in a follow-up PR we can move the expected errors added here to the 4.1 file.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/rich_text_label_missing_default_method_value branch from 7fdaa1c to 8d54d7e Compare July 7, 2023 08:37
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

This needs GDExtension compat methods a-la #76577, or this will break compat between 4.1 and 4.2 (we skipped that for 4.1 because of the more fundamental breaks) and then the validation error will go away as well.

@bruvzg
Copy link
Member

bruvzg commented Jul 7, 2023

This needs GDExtension compat methods a-la #76577, or this will break compat between 4.1 and 4.2 (we skipped that for 4.1 because of the more fundamental breaks) and then the validation error will go away as well.

Adding default values should not break anything, call arguments won't change. If it does, it probably should be fixed on godot-cpp side instead.

@RedworkDE
Copy link
Member

The default values are part of the method hash tho, so a build looking for the old hash will no longer find these methods, which is what the compat methods then fix.

While it can be argued that the method hash should not depend on the default values, that is an entirely different issue and changing that is way beyond the scope of this PR (it would need to be done here in core tho)

@Daylily-Zeleen
Copy link
Contributor Author

I think we should convert this pr to draft, and wait a pr to implement generate method hash without default values.
Or just bind compatible methods in this pr?

@RedworkDE , @bruvzg , @akien-mga what do you think?

@RedworkDE
Copy link
Member

I would just add the compat methods here, as changing how the hash is computed comes with a whole bunch of complications, which adding the compat methods is pretty straight forward and quickly unblocks this PR.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/rich_text_label_missing_default_method_value branch from 8d54d7e to eb34579 Compare July 7, 2023 12:49
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/rich_text_label_missing_default_method_value branch from eb34579 to 4029a05 Compare July 11, 2023 03:18
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

Seems fine to me

@YuriSizov YuriSizov merged commit 5167bed into godotengine:master Jul 12, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/rich_text_label_missing_default_method_value branch August 31, 2024 11:01
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