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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Create Theme menu in Theme Editor #46808

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Mar 9, 2021

This PR continues with the enhancements from #46593, after which some options were still left as they were:

image

First problem with them is that this actions is not about creating a Theme resource. Instead it's about filling the one you are currently editing with items from another Theme resource, be it the default theme or the editor theme. Editor theme also comes with an option to copy all the content from the current editor theme, otherwise only item definitions are copied.

This looks like a very limited way to interact with this system, so I propose to make it a more flexible and generalized solution. As a part of the new popup we introduce a new "Import Items" tab that handles importing items from a number of other themes. To the old options for the default and the editor theme we also add a tab for an arbitrary theme that can be selected from a file. Tabs themselves are similar and present a complete list of all available definitions where the user can filter them, select them by type group, data type or individually.

version 2
Here's the original layout before some adjustments were made; you can also see it in the clips below.

Users are free to select with high granularity whether to copy only a definition, or items content as well. While granularity is there, I don't expect it to be the main way to interact with this UI. Bulk options should be the prevalent way. Granularity will be helpful for people who want to add only a single item from a type. This feature was temporarily lost in #46593, so this is the way to restore it.

Items then can be imported with a nice progress popup (previously editor was just hanging for a bit as the items were copied).

Here's some of this in action:

2021-04-26_03-39-07.mp4

And here's some filtering:

2021-04-26_03-40-20.mp4

You may notice that the first tab is slightly different now. Outside of some small enhancements the main difference is the omission of the "Add Type from Class" option. Its functionality overlaps with the new tab, but in a very limited way. I believe the new UI provides a more approachable way to achieve the same result, plus it allows to select a type from a theme other than the default one.


This PR was heavily reworked. For posterity, here's the original description:

Old description of this PR This PR replaces these options ![image](https://user-images.githubusercontent.com/11782833/109666854-a50c2900-7b80-11eb-8000-f5b0cc66d86b.png)

With these options
image

To begin with, this action was never about creating a Theme resource it was about filling it with items from another Theme resource, be it the default theme or the editor theme, with an option to actually copy all definitions and their content from the editor one.

Now you pick either the default theme, or the editor theme, or a completely arbitrary theme stored in a file, and you can import it as granularly as you want:

godot windows tools 64_2021-03-09_00-49-49

Selecting "include" options will copy definitions of the items, while also selecting "import data" options will copy their content as well. And this can be done with any base Theme resource, not just with the editor theme.

The process now also has a progress bar, as importing some of the bigger themes, such as the editor theme, can hang the editor for some time. Unfortunately, with the progress bar the process of importing takes more time. But at least the UI is not dead in the meantime. If anyone knows a good trick to reduce the number of progress bar updates, so it doesn't wait unnecessary ticks, please advise 馃檭

And here's the entire process:

2021-03-09_03-01-30.mp4

@YuriSizov YuriSizov added this to the 4.0 milestone Mar 9, 2021
@YuriSizov YuriSizov requested a review from a team as a code owner March 9, 2021 00:14
@YuriSizov YuriSizov marked this pull request as draft March 9, 2021 00:14
@Calinou
Copy link
Member

Calinou commented Mar 9, 2021

Unfortunately, with the progress bar the process of importing takes more time. But at least the UI is not dead in the meantime. If anyone knows a good trick to reduce the number of progress bar updates, so it doesn't wait unnecessary ticks, please advise 馃檭

If it only takes 3-4 seconds at most, I'd just display a dialog without any progress reporting. You can do this by calling EditorProgressDialog as usual, but only step once in the beginning.

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch 2 times, most recently from 9a95969 to 8c55881 Compare March 17, 2021 15:38
@akien-mga akien-mga requested a review from a team March 17, 2021 15:44
@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch from 8c55881 to 2698f92 Compare March 17, 2021 17:33
@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch 2 times, most recently from b2ce17e to d723b11 Compare March 25, 2021 12:28
@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch 2 times, most recently from b172109 to 76b6b1d Compare April 25, 2021 21:31
@YuriSizov YuriSizov marked this pull request as ready for review April 25, 2021 21:57
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 25, 2021

I've reworked this PR to reflect discussion in the previous issue. This should be a more complete solution now.

As for the slow import when the progress bar is visible, this is related to the Inspector updates, which seem to create a lot of hog on master currently. If you switch the tab to "Node" for example, import goes through almost instanteneously. Regardless, I've adjusted it so the bar is updated only every 10 items.

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch 2 times, most recently from 6b50241 to 0258aca Compare April 25, 2021 22:22
@YuriSizov
Copy link
Contributor Author

Okay, compiler warnings are gone now. Should be good to review. 馃

@KoBeWi
Copy link
Member

KoBeWi commented Apr 25, 2021

The right-side panel listing items would be probably better with icons
image

Also maybe it could tell you how many elements are selected? 馃 E.g. "148 Colors (40 selected)". Unless it's difficult to implement.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 26, 2021

The right-side panel listing items would be probably better with icons

I can certainly try that, but I was afraid it may look too cryptic. A series of identical icons may also look more cluttered than a series of identical texts. I have icons ready nonetheless, as I use them for selecting and deselecting all items already.

Also maybe it could tell you how many elements are selected? 馃 E.g. "148 Colors (40 selected)". Unless it's difficult to implement.

It's trivial to implement, all selected items are tracked independently from the Tree as I need to restore them when filter is changed. That filter would be a problem here, though, as the numbers reflect the amount of items of the data type currently matching the filter (and buttons only work on what is visible as well). So I'm not sure how well a selected counter would fit there.

I do think we can work on some way to represent the number of selected items for some sanity check. One idea I have is to show a confirmation screen before actually starting the import. Such screen can list that you have X colors, Y icons and Z styleboxes ready to be imported.

Otherwise, we can indeed display more text in the side panel, especially if we change buttons to use icons. There will then be more space to separately list total, matching and selected number of items. Whether it is actually useful though, I don't know. I've decided to display number in the first place as a warning to users that there is a lot of items of a particular data type. Supposedly after that they should be roughly aware what they've selected. But I'm open to changes.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 26, 2021

I can certainly try that, but I was afraid it may look too cryptic. A series of identical icons may also look more cluttered than a series of identical texts. I have icons ready nonetheless, as I use them for selecting and deselecting all items already.

But they wouldn't be identical. Also I mean text with icon, so they are easier to distinguish on quick glance.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 26, 2021

Oh, you mean icons corresponding to each data type?

image


Edit: I've pushed it, because, yeah, it's nice like that. I've also noticed and fixed an issue with comparison when restoring checkboxes after the filter is changed. It was right in my clip 馃う

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch from 0258aca to 9ca805a Compare April 26, 2021 00:36
@KoBeWi
Copy link
Member

KoBeWi commented Apr 26, 2021

I actually meant something like this
image
but that works too I guess.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 26, 2021

Did a deeper review of functionality.

  • Filter textbox is missing the X button for clearing.
  • The only way to expand item groups is doing it manually
    image
    It's rather inconvenient if you want to select specific items from multiple groups. Maybe there should be a button to expand everything? Also filtering doesn't expand either.
  • "Collapse/expand type items" sounds a bit confusing. Maybe simply "Collapse types" would be better?
  • This part is a bit confusing too
    image
    It's not immediately clear that the warning applies to icons. I wonder if putting separators here would make it any better.
  • Maybe double-clicking an item name in the tree should select it? (but not sure if it should select with data or not)

The first point would be the most important to fix, others are mostly for consideration.

Overall the import tab feels a bit overwhelming 馃 But maybe it's not really possible to make it simpler and we just need a good tutorial? I imagine a complete newbie might just get lost with all the options.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 26, 2021

The only way to expand item groups is doing it manually
It's rather inconvenient if you want to select specific items from multiple groups. Maybe there should be a button to expand everything? Also filtering doesn't expand either.
"Collapse/expand type items" sounds a bit confusing. Maybe simply "Collapse types" would be better?

Filtering does actually expand, but only to the level of the data type. I.e. if you look for font_color it will expand matching types up to the second level. I find it a bit too much to expand down to the items, but it can be arranged. Otherwise, I'm not really sure how people would use this new UI to predict which actions we'd need to optimize. I've added toggle for the first level mainly because I couldn't decide if it's better to have them collapsed or to show data types from the getgo.

This part is a bit confusing too
It's not immediately clear that the warning applies to icons. I wonder if putting separators here would make it any better.

Well, it speaks specifically about "icon data" there. I've actually amended the text from the old version of the PR to make sure it specifically refers to icons. But I can add some separation there. I also though that "X colors" etc can be made bolder or bigger, or maybe we could give them background. Something to highlight that those labels start sections. But I don't have a good reference for such case in our existing UI, so not sure which would be the best.

Maybe double-clicking an item name in the tree should select it?

Not sure how it would be more convenient than a checkbox, but sure, I can do that. There is no other functionality related to the name that we need, so it can be used for selection.

Overall the import tab feels a bit overwhelming 馃 But maybe it's not really possible to make it simpler and we just need a good tutorial? I imagine a complete newbie might just get lost with all the options.

Yes, it's definitely a lot to take in. But IMO what we have now is deceptively simple. My hope is that while it could take some time to learn, it should provide more use once you do. Though I could assume that newbies would want the old behavior more than anything else, which means hitting "Select All" button at the bottom and importing that. And that button is not very prominent. Hmm...

Documentation on skinning/theming could definitely be improved.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 26, 2021

Filtering does actually expand, but only to the level of the data type. I.e. if you look for font_color it will expand matching types up to the second level.

The problem arises when you search for a specific element. You get multiple datatypes as a result and you don't know where is your element until you expand them all.

Well, it speaks specifically about "icon data" there.

Yeah, the text is clear and all. But if you ignore the text, the warning is mildly confusing at first look. Maybe it's not really a problem though.

Not sure how it would be more convenient than a checkbox, but sure, I can do that.

Dunno, when I was clicking through the list I somewhat instinctively tried to double-click something to select it. But then again, double-click for expand would make as much sense, which is a Tree problem not related to this PR.

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch 2 times, most recently from 6b26a4c to a3e5873 Compare April 27, 2021 14:31
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 27, 2021

Okay, some changes to the layout to improve workflow accents and UX.

godot windows tools 64_2021-04-27_17-30-03

Right side is more compact and less intimidating. Buttons below the Tree now have labels and not just icons. That puts more attention to them, as they are expected to be the main way to interact with this UI from the standpoint of a new/more casual user. Both icons and texts create some familiarity between icons and actions which helps to make the right side panel more compact by relying on the same icons for similar tasks.

Here's how it looks when some data types are empty.

I've added a clear button to the filter as requested. Filtering itself now expands to the level of the first match. So if we match the type, it will remain closed. If we don't match the type, but we match items within it, we expand to those items. I think it makes the most sense this way.

godot windows tools 64_2021-04-27_17-30-15

@KoBeWi
Copy link
Member

KoBeWi commented Apr 28, 2021

The new tab is much better now.

I still think that information on how many items are selected would be useful.

Also maaybe some confirmation dialog when you click Close with items selected could be useful too (as closing the dialog clears selection, and can be done accidentally). Similar thing with the Import Selected button - if you don't have anything selected the button does nothing with no feedback.

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch from a3e5873 to 47a999d Compare May 6, 2021 14:49
@YuriSizov
Copy link
Contributor Author

Not sure if it's the best way to display the amount of selected items, but I think it's an okay way. I've aligned it to the right because on the left it blends with the rest of the texts and looks like a mess.

godot windows tools 64_2021-05-06_17-47-15

I've also added the confirmation and the warning you've suggested. There is a problem with the confirmation though. You can still close the dialog window using the "X" OS button, and I see no way to capture that. We also don't clear the selection after successfully importing items, so the confirmation will always be there, even if you've already imported what you wanted. Maybe it's a small price to pay for saving other people from misclicks.

Here's all of this in action:

2021-05-06_17-45-32.mp4

@KoBeWi
Copy link
Member

KoBeWi commented May 6, 2021

We also don't clear the selection after successfully importing items

Is there no way to do that? (I mean clear the selection)

@YuriSizov
Copy link
Contributor Author

Is there no way to do that? (I mean clear the selection)

Sure there is, but I think it may be useful if we didn't. I can imagine someone importing, going to the first tab, doing something there, then coming back to the import tab to repeat the same import. This is obviously a speculation, but at least this flow is currently possible. Those who need a reset can already press "Deselect All".

@YuriSizov YuriSizov force-pushed the theme-editor-better-create-ui branch from 47a999d to 912420b Compare May 9, 2021 00:13
@akien-mga akien-mga merged commit a8a51cd into godotengine:master May 9, 2021
@akien-mga
Copy link
Member

Thanks!

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

4 participants