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 Project Settings' Plugin display #88308

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Feb 14, 2024

Partially addresses godotengine/godot-proposals#8438

May provide more screenshots at a later time.
image

This PR makes a few simple yet notable adjustments to the Project Setting's Plugin section (the Addons):

  • The "Enabled" section has been moved to the far left, and has been shortened.
  • Enabled addons display their name in bold;
  • The Version section is displayed in monospace;
  • Added a bit of padding columns to the left and right;
  • The warning message for missing keys in the .cfg file is more concise.
    • Silly warning, by the way. The only truly necessary key is script, I can't think of a reason to require all other fields, as well.

Internally the EditorPluginSettings' code has been restructured a tad, as well.

@vvvvvvitor
Copy link

Imo, addons which are enabled should not make their names bold as it's inconsistent with other elements from the editor, I don't remember that happening anywhere else in the editor.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 17, 2024

See proposal. While it is "inconsistent" there's desire to make enabled addons more noticeable. It does help.
Could you provide other lists in the editor where something similar could be done?

@vvvvvvitor
Copy link

See proposal. While it is "inconsistent" there's desire to make enabled addons more noticeable. It does help.
Could you provide other lists in the editor where something similar could be done?

I have read the proposal before posting my comment, I do still think it's weird to include it. I'm not on my computer rn, I'll look for other components asap.

@vvvvvvitor
Copy link

vvvvvvitor commented Feb 17, 2024

Could you provide other lists in the editor where something similar could be done?

I did a quick look through the editor and the only other one I could find was Shader Globals, that page is already pretty similar to the current plugins tab. This is wrong.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 17, 2024

But Shader Globals cannot be enabled or disabled, they can only be added or removed from the list, so they don't need their name in bold. There may be no precedent to something like this, hence why it looks weird to you?

Autoloads feature a similar list and the logic is similar, too.

The alternative to bold enabled addons could be to... darken disabled addons, instead?

@vvvvvvitor
Copy link

But Shader Globals cannot be enabled or disabled, they can only be added or removed from the list, so they don't need their name in bold. There may be no precedent to something like this, hence why it looks weird to you?

I'm pretty sure there were toggles on the Shader Globals tab, let me double check

@vvvvvvitor
Copy link

vvvvvvitor commented Feb 17, 2024

The alternative to bold enabled addons could be to... darken disabled addons, instead?

That could work too, it's even consistent to when you disable a node's process.

@vvvvvvitor
Copy link

vvvvvvitor commented Feb 17, 2024

But Shader Globals cannot be enabled or disabled, they can only be added or removed from the list, so they don't need their name in bold. There may be no precedent to something like this, hence why it looks weird to you?

I'm pretty sure there were toggles on the Shader Globals tab, let me double check

Screenshot_20240217_140815_Godot Editor 4.jpg

Ah, I see, I mistakenly assumed the variable's bool toggle was a enable/disable toggle.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks great otherwise!

editor/editor_plugin_settings.cpp Outdated Show resolved Hide resolved
editor/editor_plugin_settings.cpp Outdated Show resolved Hide resolved
editor/editor_plugin_settings.cpp Outdated Show resolved Hide resolved
item->add_button(4, get_editor_theme_icon(SNAME("Edit")), BUTTON_PLUGIN_EDIT, false, TTR("Edit Plugin"));
item->set_text(COLUMN_NAME, name);
if (is_active) {
item->set_custom_font(COLUMN_NAME, get_theme_font("bold", EditorStringName(EditorFonts)));
Copy link
Member

@KoBeWi KoBeWi Feb 17, 2024

Choose a reason for hiding this comment

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

You could fetch the font before the loop. Same below (font and icon).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is whether to do this or make the name more transparent (although in the original proposal no one seemed to mind the bold)

Copy link
Member

@KoBeWi KoBeWi Feb 17, 2024

Choose a reason for hiding this comment

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

You'd still need to fetch a "disabled" color, no? I mean, it's better to use a defined color than just decrease opacity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ever since the massive editor theme refactoring I'm not sure how. Do you have a hint?

Copy link
Member

Choose a reason for hiding this comment

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

e.g. get_theme_color(SNAME("font_disabled_color"), EditorStringName(Editor))

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I think it's worth to try using a different color instead of bold font to differentiate enabled and disabled plugins. Currently it doesn't look that readable with a longer list.

@Mickeon Mickeon force-pushed the editor-plugin-settings-list-display-woohoo branch from 2627ed1 to e8d8cfd Compare February 17, 2024 20:54
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 17, 2024

Here's a screenshot on how it'd look with the above suggestion
image

@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2024

Looks better IMO.
Though the disabled text looks smaller for some reason? 🤔

@Mickeon Mickeon force-pushed the editor-plugin-settings-list-display-woohoo branch from e8d8cfd to 5080c62 Compare February 17, 2024 22:21
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 17, 2024

I updated the PR to use the disabled color.

Though the disabled text looks smaller for some reason

That's the drawback. The font size is not changed, it may be just the antialiasing.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Code looks good to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 17, 2024
@akien-mga akien-mga merged commit d318177 into godotengine:master Feb 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the editor-plugin-settings-list-display-woohoo branch February 18, 2024 11:37
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.

6 participants