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

Change themes font_color_selected to font_selected_color #44194

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 8, 2020

To make it consistent with all the other Theme's color properties i.e. they end in color, this PR renames font_color_selected to font_selected_color.
Part of #16863.

Edit: Updated to include:

  • font_color_accel -> font_accelerator_color
  • font_color_bg -> font_unselected_color
  • font_color_disabled -> font_disabled_color
  • font_color_fg -> font_selected_color
  • font_color_hover -> font_hover_color
  • font_color_hover_pressed -> font_hover_pressed_color
  • font_color_pressed -> font_pressed_color
  • font_color_readonly -> font_readonly_color
  • font_color_separator -> font_separator_color
  • font_color_shadow -> font_shadow_color
  • font_color_uneditable -> font_uneditable_color
  • font_outline_modulate -> font_outline_color
  • icon_color_disabled -> icon_disabled_color
  • icon_color_hover -> icon_hover_color
  • icon_color_hover_pressed -> icon_hover_pressed_color
  • icon_color_normal -> icon_normal_color
  • icon_color_pressed -> icon_pressed_color
  • tab_fg -> tab_selected
  • tab_bg -> tab_unselected

@@ -181,7 +181,7 @@
</theme_item>
<theme_item name="font_color_readonly" type="Color" default="Color( 0.88, 0.88, 0.88, 0.5 )">
Copy link
Member

Choose a reason for hiding this comment

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

This one was likely added later on to match the font_color_* construct, but it should likely be renamed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to also rename:

  • font_color_accel -> font_accelerator_color
  • font_color_bg -> font_unselected_color
  • font_color_disabled -> font_disabled_color
  • font_color_fg -> font_selected_color
  • font_color_hover -> font_hover_color
  • font_color_hover_pressed -> font_hover_pressed_color
  • font_color_pressed -> font_pressed_color
  • font_color_readonly -> font_readonly_color
  • font_color_shadow -> font_shadow_color
  • font_color_uneditable -> font_uneditable_color
  • font_outline_modulate -> font_outline_color
  • icon_color_disabled -> icon_disabled_color
  • icon_color_hover -> icon_hover_color
  • icon_color_hover_pressed -> icon_hover_pressed_color
  • icon_color_normal -> icon_normal_color
  • icon_color_pressed -> icon_pressed_color
  • tab_fg -> tab_selected
  • tab_bg -> tab_unselected

Note 1: I've renamed font_outline_modulate -> font_outline_color. However, let me know if it should remain font_outline_modulate. My thoughts are that although it actually just modulates the outline colour, this name makes more sense and we can easily change the documentation to reflect that.
Note 2: I've changed fg to selected and bg to unselected, which apply to tabs, and better reflects what they refer to.

@madmiraal madmiraal force-pushed the fix-font_selected_color branch 3 times, most recently from b57a3c9 to 533658b Compare December 15, 2020 17:04
@madmiraal
Copy link
Contributor Author

Rebased following merge of #44070 and #42595.

Now includes font_color_separator -> font_separator_color

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44487 and #44605.

@akien-mga
Copy link
Member

I'm not sure that it's worth it to change all color constants to make a single odd one consistent with the others - it feels like it should maybe be done the other way around.

But let's see what others think about it.

@akien-mga
Copy link
Member

We discussed this quickly in PR review and nobody has a strong preference either way.
What do you think @Calinou @KoBeWi @YeldhamDev @groud @volzhs as folks who work with theming?

@YeldhamDev
Copy link
Member

@akien-mga I find the new names better, both for consistency and for the better naming in the case of the tab themes.

@Calinou
Copy link
Member

Calinou commented Dec 28, 2020

@akien-mga I think the new names are better as well.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 28, 2020

I actually prefer old names, but the new ones are fine too.

Changed:
font_color_accel -> font_accelerator_color
font_color_bg -> font_unselected_color
font_color_disabled -> font_disabled_color
font_color_fg -> font_selected_color
font_color_hover -> font_hover_color
font_color_hover_pressed -> font_hover_pressed_color
font_color_pressed -> font_pressed_color
font_color_readonly -> font_readonly_color
font_color_selected -> font_selected_color
font_color_shadow -> font_shadow_color
font_color_uneditable -> font_uneditable_color
icon_color_disabled -> icon_disabled_color
icon_color_hover -> icon_hover_color
icon_color_hover_pressed -> icon_hover_pressed_color
icon_color_normal -> icon_normal_color
icon_color_pressed -> icon_pressed_color

Also includes:
font_outline_modulate -> font_outline_color
tab_fg -> tab_selected
tab_bg -> tab_unselected
@madmiraal
Copy link
Contributor Author

Rebased following merge of #45128.

@akien-mga akien-mga merged commit 6cba658 into godotengine:master Jan 25, 2021
@akien-mga
Copy link
Member

Thanks!

@madmiraal madmiraal deleted the fix-font_selected_color branch January 26, 2021 06:55
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

5 participants