Refactor Edit Theme menu in Theme Editor

[YuriSizov]

This PR attempts to make the experience of editing the set of Theme resource items a bit more organized. It replaces this series of unfortunate menus with a single window.

In this window the user, hopefully, gets a better at-a-glance overview of the items, contributing to the theme. It has unified UI for all the previous operations, plus additional actions which were made possible due to this single window approach.

Earlier version with little badges

On the left side, there is a list of all item types available on the edited theme. On the right side, there is a tree of items of the selected item type. You can add items of a specific variant from the top toolbar, which only requires to enter the item's name. You can delete individual items, sets of items by variant or by affiliation (from base class, custom, all). Items can also be renamed. Like before, you can add all items of a specific base class by selecting it from a list (bottom left).

You can also create empty types. They are not stored and only exist in memory, but this flow makes more sense from the UX standpoint. Previously you'd enter the custom type name during the item creation, but now you'd create a type and then fill it with items.

I'm not sure how good I made the icons, because for some reason all editor icons on buttons in master are blurry for me. So probably disregard that.

And you can still create a theme from templates or the editor theme. I also want to add the ability to create a theme based on another theme, but I feel it's better left for a separate PR.

---

If this change is controversial, I can open a proposal. However, I felt that the current UI is inadequate at its task, confusing and not easy to use. As this is mostly a visual change, not a functionality change, I hope a long discussion is not required. I'm also fine if this all will be gone later, when a better theme editor arrives.

Porting this to 3.2 seems pretty straightforward, but I don't think that cherrypicking would be wise here. I can make a separate PR for 3.2 once this one is stabilized.

---

Incidentally, fixes godotengine/godot-proposals#2251, also closes #29038, supersedes #29057.

[YeldhamDev]

I don't think the new icons are really necessary. The "Add:" and "Remove:" labels should be enough.

[YuriSizov]

I don't think the new icons are really necessary. The "Add:" and "Remove:" labels should be enough.

Well, I can see it working for "add" (though FontSize is still new), but for "remove" I'm not sure how we can differentiate the actions using only existing icons. They are, as displayed:

• Remove Class Items (i.e. taken from the base class)
• Remove Custom Items (i.e. not defined on the base class)
• Remove All Items

[YeldhamDev]

I'm talking about the little plus and clear symbols in the icons. Specially the clear ones, as in that size, they look like mush.

[YuriSizov]

Yes, I got your meaning 🙂 I'm saying that for "add", indeed, we can reuse the same icons without the little green pluses, but for "remove" there are no currently available icons that I find fitting. For "Remove Class Items" I'm using Control's icon, but for the other two the images are new. "Remove Custom Items" is based on Control, but has our rainbow colors applied to it, and for "all" I use an asterics with a similar color adjustment.

As for looking like mush, I'm not sure this is true, but as I've noted, all icons on buttons look mushy in master currently. Compare to how crispy the icons are on TreeItems, for example.

But at any rate, if you want to propose different icons for anything, I don't mind replacing.

[YeldhamDev]

I'm talking about the little symbols on the bottom right of each icon, the icons themselves are fine.

[YuriSizov]

Okay, removed the little badges

[YuriSizov]

I've added a way to rename theme items:

I also restructured the code so that everything related to the Edit Theme Items window is in its own class. This also allowed to pick better names for class members, because I was starting to go mad from all the variations of "edit", "item" and "type". I hope nobody started reviewing the code yet 🙏

The code can be much cleaner if we move the DataType enum to the Theme resource and add methods for adding, checking and removing theme items based on its value instead of explicitly typing out every type for every operation.

So this:

switch (p_data_type) {
	case DATA_TYPE_ICON:
		edited_theme->clear_icon(p_item_name, p_item_type);
		break;
	case DATA_TYPE_STYLEBOX:
		edited_theme->clear_stylebox(p_item_name, p_item_type);
		break;
	case DATA_TYPE_FONT:
		edited_theme->clear_font(p_item_name, p_item_type);
		break;
	case DATA_TYPE_FONT_SIZE:
		edited_theme->clear_font_size(p_item_name, p_item_type);
		break;
	case DATA_TYPE_COLOR:
		edited_theme->clear_color(p_item_name, p_item_type);
		break;
	case DATA_TYPE_CONSTANT:
		edited_theme->clear_constant(p_item_name, p_item_type);
		break;
}

Can be turned into this:

edited_theme->clear_theme_item(p_data_type, p_item_name, p_item_type);

But this is a task for another day and for another PR.

[ultramarinebicycle]

Can we also do something about making the process of changing like one item somewhere less confusing? I know not everything is meant for beginners, but when you have to go through a sequence of steps this long to change something like the Button color or font, you just ask yourself if you should be spending that much time on all of this. For me, I literally go to Inkscape and whip up a button there and use a TextureButton in Godot instead of bothering with the whole theme process. Can this be made any simpler for smaller tasks?

[YuriSizov]

Can we also do something about making the process of changing like one item somewhere less confusing?

Not in this PR, but of course we need to improve the accessibility of this feature. One of the reasons I've started with it is because I believe this feature underutilized because people are scared off by bad UX. There are a couple of proposals talking about possible improvements godotengine/godot-proposals#1438 and godotengine/godot-proposals#1791.

That said, if you want to change the color of a single button, you don't need to bother with a Theme. Every control has a way to locally override its style properties with "Custom ..." sections in the Inspector. You can create a font or a color override and apply to an individual item with not much bother. Theme is useful when you want your style to be applied to a lot of items, possibly different items.

[golddotasksquestions]

Ah ok, now I got it! I guess I got hung up on the word "default" here. Thanks for the patience, my bad.
How would I use the default Theme then; not of the Editor, but what Godot uses by default on the Control nodes? Is this already build in and I just can't seem to find it?

By default I mean this:

As a beginner I would like to work from that, make little changes here until I understand the system and decide to start to make a completely new Theme from scratch

[YuriSizov]

How would I use the default Theme then; not of the Editor, but what Godot uses by default on the Control nodes? Is this already build in and I just can't seem to find it?

Current options are as follows:

• Create Empty Template - Makes a copy of the default theme definitions, but doesn't copy any content.
• Create Empty Editor Template - Makes a copy of the Editor's theme definitions, but doesn't copy content.
• Create from Current Editor Theme - Makes a copy of the current Editor's theme definitions and copies some content (but not StyleBoxes).

#46808 Makes it possible to pick the default theme, the editor's theme or an arbitrary Theme resource, and manually select which definitions to copy and whether to copy content of them too. It gives you all the control.

PS. I actually forgot a bit about what I did there, but it should actually copy StyleBoxes as well in #46808 (if you pick it), so your wish should be granted 🙂

[KoBeWi]

I tried the new edit menu and:

• There doesn't seem to be any way to remove a type from the left column
• Remove Custom Items button removes some class elements, mostly fonts and font sizes
• I wonder if it wouldn't be better if empty categories on style list didn't appear. They don't serve any purpose
• Can this column be removed?
  
• Style renaming could be done inline, like in e.g. scene tree dock
• I don't see any way to add a single style from a class. I can add only all of them
• Not sure if it's a problem on my side or with debug build, but most operations on styles cause significant freeze

[YuriSizov]

There doesn't seem to be any way to remove a type from the left column

That's correct, types are never removed when all of their items are removed, it's the existing behavior. I simply allow empty types to be visible because I use it to allow users to create empty custom styles. Empty styles are not saved, however, so there is no bloat in the resource itself. It does help with the UX though. I guess a method to forcefully remove a type from all theme item sets can be introduced, if it in any way confuses users.

Remove Custom Items button removes some class elements, mostly fonts and font sizes

I've noticed that too, and "Remove Class Items" doesn't remove some that it's supposed to either. The code itself is sound, so I'm not exactly sure why this happens. I can look more into it, but I think it boils down to how values are stored/copied between Themes.

I wonder if it wouldn't be better if empty categories on style list didn't appear. They don't serve any purpose

Can be done, though at some point I was thinking about adding an "Add Item" button there as well. But since I've never added this button, I can remove empty groups.

Can this column be removed?

That was a weird one, and I've looked into it already. But I dunno what's happening, because I only ever have 2 columns, but it does look like there are three. I wasn't able to pinpoint the issue. You can see in code that I set columns to 2 (edit_items_tree->set_columns(2);) and only ever use indices 0 and 1.

Style renaming could be done inline, like in e.g. scene tree dock

It can, but I already have the window for creating them and it supports hitting Enter to confirm, so it made sense to reuse it. And hitting Esc should already close it too, I think?

I don't see any way to add a single style from a class. I can add only all of them

Indeed, I've missed that case. Do you have a proposal how to restore it? Like, on hitting "Add" a new window can appear with the list, or something like that? Keep in mind, that in the new Theme Editor this can be achieved in a way more intuitive flow. But it does make this PR a bit of a regress, I guess. Though I've never figured that the current UI can do that to begin with, so there is that 🙃

Not sure if it's a problem on my side or with debug build, but most operations on styles cause significant freeze

I don't think it's anything new, though from my humble attempts to look into performance the most taxing part seems to be a redraw of the editor UI after Theme changes. But I'm not sure if it's my UI somehow, or the Inspector UI. Inspector in master currently has weird issues, so I wouldn't rule it out. Any help to profile this is appreciated, as I have no experience profiling C++ applications.

[Calinou]

Any help to profile this is appreciated, as I have no experience profiling C++ applications.

My go-to choices for CPU profiling are HotSpot on Linux and VerySleepy on Windows. Make sure to compile a binary that contains debug symbols so that the profiler can display them.

You can also use the microbenchmarking snippet in specific parts of your code.

[KoBeWi]

That's correct, types are never removed when all of their items are removed, it's the existing behavior. I simply allow empty types to be visible because I use it to allow users to create empty custom styles. Empty styles are not saved, however, so there is no bloat in the resource itself. It does help with the UX though. I guess a method to forcefully remove a type from all theme item sets can be introduced, if it in any way confuses users.

The most weird part is that the classes disappear when you reload the theme. But maybe it's ok 🤔

Like, on hitting "Add" a new window can appear with the list, or something like that?

Maybe you could have two buttons: "Add All" and "Add Styles..." and the latter would open some style list with checkboxes.

[YuriSizov]

• I've remembered what the problem with Remove Custom/Class Items was. The problem was that has_* methods sometimes do additional checks on the items beyond just their existence. For example, has_font_size will also double check if the font size is greater than 0. It will return false, if it exists but 0 or below. And all resource types are checked for validity.

  • I guess there is a reason for this to be the case, so to avoid breaking anything I've added has_*_nocheck methods. They don't try to validate the theme item, just to check if it is defined. This solves the problem.

• I've hidden empty categories in the item tree.

• The extra column in the tree was not extra at all. It was the second column that contained buttons, it's just that the highlight is not drawn behind buttons, which is why it looked like an extra column. Maybe you've already known that. I've removed it and put everything into the same, single column.

• I've looked a bit more into performance and indeed it seems to be related to the Inspector. Merely hiding the dock makes all operations instantaneous. It gets worse when doing bulk operations with multiple calls to set_*/clear_* methods, because each call emits the resource has changed signal, and the dock reacts. Maybe there is a way to halt it from updates?

---

Still looking into being able to add individual items from a class type. Worst case scenario, it's easy to remove the unneeded parts in the new UI. But in the overall scope of things it got me to thinking if this can somehow be moved to the Import menu from #46808. Otherwise it's weird that we can granularly add items and maybe their values from a number of options (default, editor, a file), but we can only copy individual definitions here from the default theme.

Or maybe it's not weird, but this whole theming mess is pretty confusing.

[KoBeWi]

I think adding single items could be done in another PR, unless it requires some significant changes.

But I found a bug. You can add an item even if you don't have any type added. This is not that bad, but after you add a type, your type list will suddenly have 2 types, one being empty.

[YuriSizov]

But I found a bug. You can add an item even if you don't have any type added. This is not that bad, but after you add a type, your type list will suddenly have 2 types, one being empty.

Empty name type is actually a valid case, but you are correct, buttons should be disabled unless a type is selected. Empty name type should be explicitly created by user (by clicking Add next to Add Custom Type without typing any name in). I'll look into fixing it tomorrow.

I think adding single items could be done in another PR, unless it requires some significant changes.

I think I have an idea I want to try about it, but it will make the next PR (#46808) obsolete. I'm just being slow implementing it because I only spare time on weekends to work on Godot 🙂

What I'm imagining is that instead of an OptionButton with a list of classes from the default theme we add a button that brings up another screen/popup. On that screen user can pick from 3 tabs: default theme, editor theme and custom theme (which allows selecting an arbitrary Theme resource). On each tab there is a tree of types existing in that theme and beneath each type there is a list of properties. There is probably another level of depth in that tree for data types, similar to the right side of the popup from this PR, or data types are grouped in some other way.

Within this tree users can find the item they want to add specifically, but they also can select several items and import all of them at the same time. They can select whole types and they can select items of specific data types. Basically, there are checkboxes and then there are buttons that allow to select items in bulk to achieve the same functionality as we have with #46808. Somehow this should also have the option to import only definitions or import definitions with data.

You may think this is a bit of an overkill to replace the old UI for adding individual items of existing types. It is, but I hope that combining it with the other UI from Create/Import Theme menus would reduce confusion for users struggling to figure out what all those options do exactly. To streamline the process of adding individual items I think that the tree in each tab should have a search field above it, providing filtering to the tree.

---

I'll try to provide visual aids when I have time to work on it. I'm also fine with moving this particular problem to #46808. I think they are related, essentially. And since this is not a PR that I propose we immediatelly cherry-pick, we can lose some functionality for now and re-add it later, of course.

[YuriSizov]

The issue with an empty type should be resolved now.

---

Now about the missing feature and #46808. I propose that we remove "Add Type from Class" button and instead create a dedicated tab for importing class items. This tab would allow adding individual items of types as well as whole groups of items, similar to how #46808 is supposed to work. I intend it as a solution to the missing feature you've mentioned and to the "Create Theme" UI. Because of that I would propose we do this in #46808, and in this PR we keep "Add Type from Class" as is, adding all items and whatnot.

Manage Items tab
Import Items tab

I'm not a big fan of tabs within tabs, but I'm not sure what else can we use here from Godot's UI toolkit. Hopefully something like different alignment breaks it visually enough to not be confusing. The filter field is supposed to work in a similar manner to how help search or create node dialogs work.

On "Another Theme" tab (or however we choose to call it) there will also be a file selector button and dialog, but otherwise all three tabs would work the same.

[KoBeWi]

Seems like most obvious bugs are fixed. The code looks ok too (although I'm unsure about the sub-scope thing I commented above. It's probably a style preference though).

This is definitely an improvement from the current editor and many people like it already.

[akien-mga]

Looks good to me 👍

[akien-mga]

Thanks!