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

Use QuickOpen to load resources in the inspector. #37228

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Mar 22, 2020

When you click the "Load" button to populate a Resource field in the
inspector dock, the editor currently creates a file dialog. Navigating
through folders to find the file you want is tedious.

This replaces the file dialog with the same QuickOpen dialog used for
the "Quick Run" and "Quick Open" scene actions. The result list is
filtered to only resources of the appropriate type, and you can type
I find that this is much faster and more intuitive than the file dialog.

As far as I'm aware, the only "feature" you lose compared to the old
FileDialog is the ability to show all types of files by changing the
extension filter to "All Files" in the lower right. This seems like an
unnecessary feature anyways, as selecting a file that is not of the
appropriate resource type would just result in an error like so:

The selected resource (StreamTexture) does not match any type expected for this property (AudioStream).

Relates to godotengine/godot-proposals#346.

The old way:
filedialog

The new way:
quickopen

@rcorre
Copy link
Contributor Author

rcorre commented Mar 22, 2020

I should probably call set_title on the QuickOpen to say something like "Select a Resource" instead of "Please Confirm". I figure that should be wrapped in a TTR, but what else do I need to do so it gets translated?

@Calinou
Copy link
Member

Calinou commented Mar 22, 2020

I figure that should be wrapped in a TTR, but what else do I need to do so it gets translated?

Wrapping it in a TTR() call should be enough.

@rcorre
Copy link
Contributor Author

rcorre commented Jul 8, 2020

Rebased and squashed. FWIW I'm also playing with using quickopen for the "New Inherited Scene" command at rcorre@55e953a but haven't added it to this PR.

@akien-mga
Copy link
Member

As far as I'm aware, the only "feature" you lose compared to the old
FileDialog is the ability to show all types of files by changing the
extension filter to "All Files" in the lower right.

You also lose the resource preview that the FileDialog shows. In your GIF it's arguably not the most important as they're sound waves, but for images or scenes, the preview can be quite useful to pick the relevant asset. With QuickOpen you need to know what's the exact name of the resource you want to load, without any option to preview it.

@rcorre
Copy link
Contributor Author

rcorre commented Jul 8, 2020

I guess I tend to focus on names rather than images, but fair point. Do you think it is feasible/useful to add a preview to quickopen similar to fzf's preview window? Or make this a separate menu option (load vs quickload)? Or configuration?

@Calinou Calinou added this to the 4.0 milestone Jan 5, 2021
@Calinou
Copy link
Member

Calinou commented Mar 20, 2021

Given the current lack of preview in the QuickOpen dialog, this would likely be better as a opt-in editor setting for power users.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 20, 2021

I'd be happy with this as an opt-in. I can also look into adding preview if that seems plausible -- if we can make this work for all cases and avoid having two code paths that seems ideal.

I use this feature all the time on my personal editor build, so I'd love to see it in the mainline.

@YuriSizov YuriSizov requested a review from a team August 24, 2021 23:00
@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 14, 2021

This looks interesting, but if it's still desired it needs to be remade for the current EditorResourcePicker control. To address the concerns voiced by others I would suggest adding another menu option besides "Load" – "Quick Load" – that would bring up that dialog. No editor setting needed, both options always handy.

@rcorre
Copy link
Contributor Author

rcorre commented Sep 15, 2021

Thanks @pycbouh, I've updated it to 4.0 and made it a separate option from the regular load dialog.
In 4.0, I've found that the QuickOpen dialog does not automatically grab focus, which makes it much less useful (you still have to grab your mouse and mouse over it). That happens in the "Quick Open Scene" dialog as well though, so I don't think it is a problem with this PR.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 15, 2021

In 4.0, I've found that the QuickOpen dialog does not automatically grab focus, which makes it much less useful (you still have to grab your mouse and mouse over it).

Hmm, it grabs focus for me. Can be a platform-specific problem due to dialogs being real windows now. But it is indeed out of scope here, if that's the case.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

It works well, I like the feature and that it's side by side with the regular "Load" now. Code also looks good and simple, easy to maintain in future. I would probably go for a new icon here, maybe the same folder with a looking glass superimposed, but it can be done in another PR.

You just need to fix the include order to make clang-format happy, and should be good to merge (if there are no objections).

Here's how it currently works:

2021-09-15_15-49-38.mp4

@Calinou
Copy link
Member

Calinou commented Sep 16, 2021

Should Quick Load be listed before Load in the dropdown? I think most people will prefer to use Quick Load as a "default" option, and only use Load when they actually need to see thumbnails.

@YuriSizov
Copy link
Contributor

Should Quick Load be listed before Load in the dropdown? I think most people will prefer to use Quick Load as a "default" option, and only use Load when they actually need to see thumbnails.

Semantically it's probably reasonable to have it before, but from the usability perspective I don't see much difference as they are next to each other and it's a matter of a few pixels.

@rcorre
Copy link
Contributor Author

rcorre commented Sep 18, 2021

In 4.0, I've found that the QuickOpen dialog does not automatically grab focus, which makes it much less useful (you still have to grab your mouse and mouse over it).

Hmm, it grabs focus for me. Can be a platform-specific problem due to dialogs being real windows now. But it is indeed out of scope here, if that's the case.

Ah, "single window" got unset for me somehow, and I use focus-follows-mouse in my window manager.

@YuriSizov YuriSizov added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 18, 2021
@YuriSizov
Copy link
Contributor

Ah, "single window" got unset for me somehow, and I use focus-follows-mouse in my window manager.

👍 As long as it's not on Godot 🙂


Can you reverse the order of the options so that they make more sense semantically (quick before regular)?

When clicking on a resource field in the inspector dock, you now have
the "Quick Load" option in addition to "Load". This opens a QuickOpen
dialog allowing the user to type in a phrase to quickly locate the
desired resource (similar to "Quick Open Scene").

In my experience, this is much faster than clicking through the File
Dialog.

Relates to godotengine/godot-proposals#346.
@rcorre
Copy link
Contributor Author

rcorre commented Sep 19, 2021

Can you reverse the order of the options so that they make more sense semantically (quick before regular)?

Done

@akien-mga akien-mga merged commit 4d9b585 into godotengine:master Sep 20, 2021
@akien-mga
Copy link
Member

Thanks!

@rcorre rcorre deleted the rcorre/better-load-dialog branch September 20, 2021 11:16
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 21, 2021
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

5 participants