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

Merged
merged 1 commit into from
Dec 23, 2024

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.
@Repiteo Repiteo merged commit 3f74cc2 into godotengine:master Dec 23, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 23, 2024

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Dec 24, 2024

This makes an inconsistency between the buttons.
Arrow button opens the same menu as clicking the resource field, while open button does not appear in the menu.
The menu could be different for arrow button and include load options. But maybe it's fine, idk. I just find this odd.

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
8 participants