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

Improve naming of theme properties throughout GUI code #65437

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 6, 2022

I really wanted to introduce some sensible naming scheme for theme items by Godot 4, but ran out of time. So in this, and a couple follow-up PRs I'll try to clean up some names that are the most problematic for one reason or another. This PR also includes a second commit that fixes some random theming issues I've noticed while working on this. Put it in a dedicated commit so that it's clear which changes are not related to renames directly.

The list of changes:

  • ItemList:
    • bg -> panel
    • bg_focus -> focus

The name "bg" is used across several different controls, but is not consistent with other controls which call the main stylebox "panel". Calling it "bg" is a misnomer, as it's not just providing a background, but also adds offsets. Also we frown upon shortened names, and rename those occurrences for 4.0.
As for "bg_focus", here it is technically not too wrong, since it is drawn below the items (unlike Tree, where it is drawn above). However, the name doesn't fit the rest of the controls that have a dedicated focus style, and I feel like the order of drawing is an implementation detail that should not be reflected in the name. We may even want to introduce a theme constant to control whether it should be drawn above all or below, since there have been a few issues with that. So anyway, for consistency, this is now called "focus".

  • Tree:
    • bg -> panel
    • bg_focus -> focus

Same as ItemList, but also here focus is indeed drawn on top of everything, making it a completely wrong name.

  • ProgressBar:
    • bg -> background
    • fg -> fill

This should make the semantic meaning clearer, instead of referring to their draw order.

  • ScrollContainer:
    • bg -> panel

Same as previous entries, this is more consistent, not a shortened name, etc.

  • FileDialog:
    • *_icon_modulate -> *_icon_color
    • files_disabled -> file_disabled_color

Technically, colors assigned to icons are used for modulation, but we don't call them that in most theme properties, hence the rename. "files_disabled" is just weird and I think is an obvious one.

  • CheckButton:
    • on*/off* -> checked*/unchecked*

This makes the names consistent with CheckBox, and in general less awkward.

  • check_v_adjust -> check_v_offset

A couple of classes have this. We typically call adjustments like that offsets, hence the rename.


As for fixes, Panel had an unused style, RichTextLabel had an extra definition for the text outline which was never actually set, and SplitContainer had a regression in theme caching from a recent fix-up PR.


I've added the necessary conversion rules to the tool, but it will probably misbehave on some "bg" properties which were used elsewhere as well. The ones I've renamed are probably the most relevant cases, so it's better to include a rename, I think.

I also don't know if theme renames should be added to the C# list, as I don't see under what circumstances they would be found in C# code as regular camelCase properties. They should only be found in strings or in scene files, so the already include rules seem wrong to me, and I didn't add the new ones there.

Rename ItemList's bg -> panel
Rename ItemList's bg_focus -> focus
Rename ProgressBar's bg -> background
Rename ProgressBar's fg -> fill
Rename Tree's bg -> panel
Rename Tree's bg_focus -> focus
Rename ScrollContainer's bg -> panel
Rename FileDialog's *_icon_modulate -> *_icon_color
Rename FileDialog's files_disabled -> file_disabled_color
Rename CheckButton's on/off -> checked/unchecked
Rename check_v_adjust -> check_v_offset
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems great 👍

@akien-mga akien-mga merged commit 80dacac into godotengine:master Sep 7, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon
Copy link
Contributor

Mickeon commented Sep 7, 2022

Thank you personally! There were just too many inconsistencies and I'm glad it's being swiftly addressed in time!

@YuriSizov YuriSizov deleted the theme-gui-renames branch September 7, 2022 10:06
@YuriSizov
Copy link
Contributor Author

There were just too many inconsistencies and I'm glad it's being swiftly addressed in time!

Well, some of it. I'll do another PR for buttons, but it may be controversial and it's not as bad currently as these properties were. There may be still some in GraphEdit and ColorPicker, as I didn't touch those to avoid conflicts with their refactoring PRs.

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.

3 participants