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

EditorResourcePicker: Replace options to load file with button for QuickOpenDialog #97860

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stijn-h
Copy link
Contributor

@stijn-h stijn-h commented Oct 5, 2024

With the new quick open dialog, I think it is helpful to make it more accessible quickly.

afbeelding

2024-10-06.08-52-13.mp4

Additionally, I had to change some code to handle the separators. Now all separators are added in one method: _update_menu_items. Previously the overridable set_create_options would need to do this to ensure proper separation (this wasn't documented), but it could not know whether more properties were going to be added.

Closes godotengine/godot-proposals#10910

@stijn-h stijn-h requested a review from a team as a code owner October 5, 2024 21:59
@RedMser

This comment was marked as outdated.

@stijn-h
Copy link
Contributor Author

stijn-h commented Oct 6, 2024

Is there a proposal for this change?

I added a proposal that answers some of your feedback, I hope.

@npinsker
Copy link

npinsker commented Oct 6, 2024

I love the addition of the icon, but will mention that I occasionally use the slow-load option, when I can't remember exactly what a file is called, but do remember where it is in my directory tree. I would be fine seeing the option removed though -- dragging from FileSystem is just as good.

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.

My only concern is that the short viewable area for the path will become even shorter with the addition of a new button:

image

I don't know a good solution to this. I'd say the saved click is worth losing some space here, since it's often needed, especially when setting up materials or audio samples.

Comment on lines 185 to 187
const Vector<String> &base_types_string = base_type.split(",");

Vector<StringName> base_types;
for (const String &type : base_types_string) {
base_types.push_back(type);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const Vector<String> &base_types_string = base_type.split(",");
Vector<StringName> base_types;
for (const String &type : base_types_string) {
base_types.push_back(type);
Vector<StringName> base_types;
for (String type : base_type.split(",")) {
base_types.push_back(type);

Would be cleaner IMO

@AThousandShips
Copy link
Member

(my bad clicked wrong should have been a simple comment)

…ickOpenDialog

 - Remove option to load files from drop down
 - Make _update_menu_items solely responsible for adding the right separators. Previously
   the overridable set_create_options would need to do this, but it could not know whether
   more properties were going to be added.
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.

EditorResourcePicker: Add quick access for QuickOpenDialog
6 participants