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

Move registration of fallbacks property in the base Font class #78266

Merged
merged 1 commit into from Aug 7, 2023

Conversation

Faolan-Rad
Copy link
Contributor

No description provided.

@Faolan-Rad Faolan-Rad requested a review from a team as a code owner June 15, 2023 09:08
@Faolan-Rad
Copy link
Contributor Author

Put the property define where the get setter are defined

@akien-mga
Copy link
Member

akien-mga commented Jun 15, 2023

Thanks for your first contribution.

Could you clarify what motivates the change? It doesn't seem wrong per se but it's unclear what it's fixing, if anything.

You would also need to update the class reference to match the change of inheritance. See https://docs.godotengine.org/en/latest/contributing/documentation/updating_the_class_reference.html

@akien-mga akien-mga requested a review from bruvzg June 15, 2023 09:11
@akien-mga akien-mga added this to the 4.x milestone Jun 15, 2023
@AThousandShips
Copy link
Member

You're also making each case storage, I don't think FontVariation and SystemFont should store these, and that this setup is by design for that very reason

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

It was defined in the subclasses for the reason, fallback has, PROPERTY_USAGE_STORAGE flag for FontFile and default flags for the rest of fonts.

This change will hide fallback from the editor inspector UI for all types of fonts, which is wrong (it should be only hidden for FontFile since it's an imported resource and should not be modified manually).

@KoBeWi
Copy link
Member

KoBeWi commented Jun 15, 2023

We usually handle such cases with _validate_property().
So fallbacks should be defined with the default usage and FontFile should have _validate_property() that unexposes it. (see box_container.cpp for example)

@Faolan-Rad
Copy link
Contributor Author

Faolan-Rad commented Jun 15, 2023

The reason I noticed I am working on a code generator for a networking system that adds code into every set method and my code could not find the set method which seemed odd to me because this problem did show up else where in the code base and this is the only case I can find properties definitions exciting separate from where the methods are defined.

Also I added _validate_property to font file so it well tell it to be storage

I am doing this because it seems be a consistency issue

@AThousandShips
Copy link
Member

The documentation needs to be updated for variation as well, it doesn't have that property any longer locally

scene/resources/font.cpp Outdated Show resolved Hide resolved
@Faolan-Rad
Copy link
Contributor Author

@AThousandShips I do not see what you are talking about or has this been already dealt with with my last force push

@AThousandShips
Copy link
Member

AThousandShips commented Jun 15, 2023

The file FontVariation.xml still contains documentation for fallbacks, this wouldn't be generated so CI will fail, you'll need to remove this property from there, to verify please compile your code and run --doctool on the built editor

You also have to update your branch to the latest upstream version or CI will fail

@Faolan-Rad
Copy link
Contributor Author

I just realized I forgot to save the file @AThousandShips that is why it was still there

@Faolan-Rad
Copy link
Contributor Author

@AThousandShips so I should rebase it?

@AThousandShips
Copy link
Member

My bad you were more out of date than I thought so it should be safe

@KoBeWi
Copy link
Member

KoBeWi commented Jun 15, 2023

You should also squash the commits into 1 and remove me from co-authors.

@akien-mga akien-mga requested a review from bruvzg July 2, 2023 18:37
@AThousandShips
Copy link
Member

AThousandShips commented Jul 2, 2023

Still need to fix the documentation, methods for properties aren't exposed in the documentation

@YuriSizov
Copy link
Contributor

By the way, this would technically be a breaking change for C#, at least.

@akien-mga akien-mga changed the title Moved property fallbacks to where it is created in font Move registration of fallbacks property in the base Font class Aug 7, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 7, 2023
@akien-mga akien-mga merged commit 5fc0d71 into godotengine:master Aug 7, 2023
13 of 14 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member

AThousandShips commented Aug 7, 2023

Broke API compat, was merged before the CI could report this, needs followup, let's make sure these specific CI pass before merging

@akien-mga
Copy link
Member

akien-mga commented Aug 7, 2023

My bad, CI was taking forever and I overlooked that it hadn't passed yet here.

Edit: Fixup: #80374.

akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 7, 2023
akien-mga added a commit that referenced this pull request Aug 7, 2023
IntangibleMatter pushed a commit to IntangibleMatter/godot that referenced this pull request Aug 12, 2023
IntangibleMatter pushed a commit to IntangibleMatter/godot that referenced this pull request Aug 13, 2023
WhalesState pushed a commit to WhalesState/godot that referenced this pull request Aug 13, 2023
wscalf pushed a commit to wscalf/godot that referenced this pull request Aug 13, 2023
mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
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.

None yet

6 participants