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 context support for editor property name i18n #82852

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Oct 5, 2023

Fixes #82788

  • Two optional params are added to process_name(): full property path & class name
    • Full property path is enough for most use cases, e.g. constant_force vs force/8_bit
    • Class name is used when full property paths are the same, but have different meanings, e.g. Normal in Decal.texture_normal and TextureButton.texture_normal
  • Context is assigned inside EditorPropertyNameProcessor
    • Context is rarely needed.
    • Easy to parse when extracting for POT.
  • Context can be specified either for full/property/path or for Class::full/property/path
    • Class name is rarely needed. I think this kind of properties should be renamed when possible.
    • Simplifies theme_overrides_style/normal as it applies to many UI nodes

@Wuzzy2
Copy link

Wuzzy2 commented Oct 5, 2023

Using "Noun" or "Verb" as context is a bad idea because sometimes even identical nouns or verbs can have different meanings.

The Gettext context entry is supposed to describe something like a topic or category that fits to that string or some other identifier that clearly helps to fix the ambiguity.

See also: https://www.gnu.org/software/gettext/manual/gettext.html#Contexts

@AThousandShips
Copy link
Member

AThousandShips commented Oct 5, 2023

Using "Noun" or "Verb" as context is a bad idea because sometimes even identical nouns or verbs can have different meanings.

Granted but it refers to how English uses it, i.e. what word to translate from

The difference between "A Constant Force" and "To Be Constant Forcing"

@timothyqiu
Copy link
Member Author

Using "Noun" or "Verb" as context is a bad idea because sometimes even identical nouns or verbs can have different meanings.

I think you might be worried that "Noun" and "Verb" won't resolve all the possible ambiguities in the project? These contexts are not part of a fixed set of labels that would be picked from when a new ambiguous string appears. They are chosen on a case-by-case basis for each ambiguous string independently.

In this case, the two existing usages are "Constant Force Applied to the Object" and "Force the Audio File to be Imported as 8-Bit". I thought about "Physics" vs "Audio" too, but it feels weird because the word "force" itself has nothing to do with audio.

@Wuzzy2
Copy link

Wuzzy2 commented Oct 5, 2023

Yes, technically the context could be anything to just resolve the collision. I was just arguing it could be a little bit more descriptive. But technically this is not 100% required as long all collisions are resolved.

@timothyqiu
Copy link
Member Author

@Wuzzy2 Any suggestions?

@Wuzzy2
Copy link

Wuzzy2 commented Oct 6, 2023

For the physical "Force", name the context "physics".
For the concept of forcing a setting, name the context "forcing a setting".

@timothyqiu
Copy link
Member Author

Changed to "Physics" and "Enforce". I think "forcing a setting" is a bit specific.

@timothyqiu timothyqiu force-pushed the property-context branch 2 times, most recently from 7555e44 to ae2b902 Compare October 6, 2023 11:33
@timothyqiu timothyqiu marked this pull request as ready for review October 6, 2023 11:48
@timothyqiu timothyqiu requested review from a team as code owners October 6, 2023 11:48
@Chaosus Chaosus added this to the 4.3 milestone Oct 15, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Oct 18, 2023

I tested the context for Normal and it works for "Geometry", but not for "Ordinary". Theme overrides translate the names without using context.

@timothyqiu
Copy link
Member Author

Good catch! I tested with Ordinary one and no context one being the same, so I didn't spot that bug. I'll work on that.

@timothyqiu timothyqiu requested a review from a team as a code owner October 19, 2023 02:26
Comment on lines -934 to -940
theme->set_font("normal", "Fonts", Ref<Font>());
theme->set_font("large", "Fonts", Ref<Font>());
Copy link
Member Author

@timothyqiu timothyqiu Oct 19, 2023

Choose a reason for hiding this comment

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

Note: These two are removed because

  • They were used to store default theme fonts when Godot was open sourced. But fonts were changed to a dedicated theme member later, so these entries were abandoned.
  • They add unnecessary strings to translate. Not worth adding exceptions for.

editor/editor_property_name_processor.cpp Outdated Show resolved Hide resolved
editor/editor_property_name_processor.cpp Outdated Show resolved Hide resolved
editor/editor_property_name_processor.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 84b3d14 into godotengine:master Apr 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the property-context branch April 6, 2024 07:03
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.

Word "Force" is used ambiguously, needs translation context
6 participants