-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Shorten too-long exported resource dropdown menus and switch to CreateDialog #43
Comments
I'd rename "Open Create Dialog" to "Browse" and move to the top, unless populating numerous resource items is severe performance-wise, because then you'd have to account for window size for relatively small number of maximum elements, and you'd still have to scroll down if the popup doesn't fit the window. |
Also searchbox at the top of that dropdown would be useful, that way you don't have to click on open dialog button if you know the name you're looking for, so it will just show that filtered part of the list. |
@Razzeeyy Edit: Although, maybe I could just add a method that exposes the VBoxContainer itself and the external context can add in a search filter all by itself(?). That would be a very minor change to the engine's @Xrayez What if instead, we just add an ellipses to the bottom of the list and clicking on it would open the CreateDialog? Would you guys rather have that, or a disabled ellipses (just there to indicate that there are more, unlisted entries) AND a "Browse..." at the top that only appears when there are too many, just like the ellipses at the bottom? |
As for me I actually meant we can do both. If it doesn't hurt anyone's sense of style and not a burder to implement. ^^' Though I'm not sure if that search bar would come in handy in different areas. |
If I "hope" that I'll find a resource manually by scrolling but eventually scroll down to bottom anyway, then I wouldn't need to scroll up/re-popup the menu again just to "Browse...", so adding ellipses would make sense here as well I guess. |
I think adding a popup would be really bad UX-wise, complexifying the process of creating a new resource. Either a search bar, or basically making the popup menu scrollable are better ideas. |
@groud We'd need core engine changes for that, wouldn't we (to add an optional search bar filter for a subset of PopupMenu items, the updating of which does not affect the search bar menu item)? I guess I'm good with taking that approach, but then we would also be duplicating a lot of ClassDB/script-class management logic and inheritance caching between EditorPropertyResource and CreateDialog whenever #22, point 4 is addressed. In which case, I think we'd need to refactor out inheritance hierarchy caches into their own third-party class which EditorPropertyResource and CreateDialog can reference on their own; something like an EditorInheritanceCache class in EditorData or something. Otherwise we'll start seeing the same slowdowns currently present in the CreateDialog inside of the EditorPropertyResource's PopupMenu. Do you see that being approved? |
If something is needed twice, it can indeed be made common (if it's complex enough obviously). If the code is clean I don't see any problem with that. |
this is true! I support this! |
Would be partially fixed by godotengine/godot#36687.
BTW, I think bumping issues without information isn't that welcome. |
Note that PopupMenu already has a allow_search property, which enables navigation with the letter keys. |
@Jummit Is there a reason why |
I don't think so. |
Alternative idea: Implement the search bar into the dropdown menu itself, as the 'allow_search' might be too hidden for beginners: Now by typing something in the search bar it would automatically limit to the options that contain the typed in string (i chose the search bar over the implicit typing in the keyboard, as it might be something that people new to godot might miss) The "Edit etc." options are hidden below, in a collapse menu, so they are always available (not just at the very bottom of the context) but hideable. This menu might remember if the user had it open the last time the user was in a resource context (or had it collapsed) Mockup-Project: The feature proposal: #18 might help to limit this by a lot for most of my cases, but i still think its much better for everyone if the resource context got improved in its useability. The mockup is focused on rough layout or estimate functionality and is not a specific dictate. Its mainly meant to communicate my idea and not represent the final expectations. As this is mainly about the functionality i didnt bother with the icons |
@Jummit I noticed it's allowed by default in OptionButton but not PopupMenu. Here's a pull request where it's enabled by default in PopupMenu: godotengine/godot#38811 |
Would be nice to have this in 4.0 or 4.x, since the usability of this context menu is - to put it mildly - minimal, compared to the rest of the editor. |
@ExquisiterEmil This is less of an issue in 4.0+ because you can export any custom resource you have, so you practically never need to export just |
Is that possible in beta 10 already? I am exporting a custom Resource (in C#) and still get the abysmal drop down... |
It's possible, but maybe not in C#. See godotengine/godot#63126 |
Describe the project you are working on:
Describe how this feature / enhancement will help your project:
When one does
export(Resource) var res = null
, they are presented with an empty exported resource that can be supplied with any type of resource. The "valid_inheritors" of this resource includes all possible resource types in the engine as well as all possible Custom Type or Script Class resources.When you click the dropdown menu, the EditorPropertyResource class calls
_update_menu_items
and populates the dropdown with oneNew <type>
menu item for every valid inheritor of the exported type. For Resources (and probably a few others), this floods the UI with a list that is obscenely long to navigate.What's worse is that users who operate on a touchpad-equipped laptop, and who don't have a mouse with a scroll wheel, CANNOT scroll this list at all (or if there is a way, it is not intuitive as I cannot figure out how to do it). Using a keyboard to move the selection down the list will make it move down, but the list will not scroll up to continue displaying the moving-down selector to you, so you are blind.
Either way, the UX in this scenario is abysmal. I propose that we assign a maximum number of allowed menu items to be generated, and when the size has been reached, we instead add an "Open Create Dialog" button and stop generating menu items. If the user selects the Create Dialog, the EditorPropertyResource would configure it for the currently exported type and then open the dialog for the user to select the type.
Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
This is what exporting a Resource looks like before clicking the dropdown menu:
This is what it looks like afterwards:
This is what exporting an AudioEffect looks like:
Assuming the number of entries had been reached at something like 23 (I was gonna say 20), the AudioEffect dropdown POST the suggested changes might look like this:
Clicking the button would prompt the opening of the CreateDialog (but configured for AudioEffect in this example, not for Resource which is what the image below shows):
Describe implementation detail for your proposal (in code), if possible:
editor/editor_properties.h
editor/editor_properties.cpp, EditorPropertyResource::_update_menu_items
, keep a running count of which menu item index you are adding. When it becomes equal to the maximum allowed count AND there are still more to render, then add a menu item with no functionality that is just an ellipses (to indicate that there are more that exist) and then add a "Open Create Dialog..." menu item.If this enhancement will not be used often, can it be worked around with a few lines of script?:
It is used very often for data-driven games on small screens / laptops. It is built into the editor source code and is not very pluggable via the EditorPlugin's available API, so no, it cannot be worked around.
Is there a reason why this should be core and not an add-on in the asset library?:
The only code capable of making the necessary changes is direct alterations to the EditorPropertyResource class in the editor source code.
The text was updated successfully, but these errors were encountered: