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

Redesign Quick Open #56772

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Conversation

stijn-h
Copy link
Contributor

@stijn-h stijn-h commented Jan 13, 2022

Closes godotengine/godot-proposals#3788

2024-10-1

Features:

  • dynamic switching between list and grid view
  • support for filtering files from addons
  • history of recently opened files

@Calinou
Copy link
Member

Calinou commented Jan 14, 2022

Still not decided on what kind of design to go with. Current one is most standard, but might have problems with long file paths.

For long file paths, ellipsis () can be used, with only the end of the path showing. Label has built-in support for automatic ellipsis in master, but I'm not sure if it can be used with right-aligned text to show only the end of a string. If this doesn't work, then you'll have to implement ellipsis manually.

@stijn-h
Copy link
Contributor Author

stijn-h commented Feb 6, 2022

This PR is ready for review! There are a few gui/theme issues I can't figure out, but these aren't blocking issues and fixing it could increase the scope too much (for me at least), so I think it's better if they are be fixed later.

I also looked into making the dialog a singleton. It makes some sense given that we allocate all these nodes for the results on startup. However, that only works if only one object subscribed to the quick open signal at any moment.

@stijn-h stijn-h marked this pull request as ready for review February 6, 2022 16:11
@stijn-h stijn-h requested review from a team as code owners February 6, 2022 16:11
@YeldhamDev
Copy link
Member

YeldhamDev commented Feb 6, 2022

The current looks of this dialog goes very against the overall visual style of the Godot editor:

  • The place holding the main contents should be the dark area, but instead it's the bottom part.
  • The icon for changing the view mode is at the bottom, when it would better placed to the right of the search bar.
  • The text of the items in list mode seem to be truncated for no good reason.
  • I think the bottom text should rather be a tooltip when hovering an item.
  • Not really a big fan of windows resizing by themselves.
  • There are no borders.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 8, 2022

What's the status of this? The remarks from the previous comment should be addressed. Also the PR needs rebase.

@stijn-h stijn-h force-pushed the quick-open-redesign branch 2 times, most recently from a00f209 to c00370c Compare April 30, 2022 17:09
@stijn-h
Copy link
Contributor Author

stijn-h commented Apr 30, 2022

What's the status of this?

The new dialog is functional, although there are still some things I don't like in the code. I've already mentioned a few styling issues, and I don't know how to fix them. Any help with theming/UI issues, or code review would be appreciated.

@stijn-h
Copy link
Contributor Author

stijn-h commented Apr 30, 2022

The current looks of this dialog goes very against the overall visual style of the Godot editor:

To me, it's kind of clear that Godot's design might be lacking, which is why I redesigned this dialog in the first place. I designed it precisely without caring about what 'style' Godot has, only caring about what a Quick Open dialog needs.

  • The place holding the main contents should be the dark area, but instead it's the bottom part.

I'm okay with making the results part darker, but I think the bottom part should still be even darker. Here is what it looks like (ignore the white dialog border, I think it happened after rebasing, maybe a theme update?).

image

  • The icon for changing the view mode is at the bottom, when it would better placed to the right of the search bar.

No, that would not be better. It would no longer be a simple search bar, and even on it's own, that place is not a better location. There is free space at the bottom, and putting it there also doesn't break the simple design of search bar + search results.

  • The text of the items in list mode seem to be truncated for no good reason.

There is a good reason, as I've explained in the post above.

  • I think the bottom text should rather be a tooltip when hovering an item.

No.

  • Not really a big fan of windows resizing by themselves.

I don't think it works well with Godot's separate windows, but I have seen this style work in other software. It's up for debate whether we want to keep this or not.

  • There are no borders.

So?

@YeldhamDev
Copy link
Member

To me, it's kind of clear that Godot's design might be lacking, which is why I redesigned this dialog in the first place. I designed it precisely without caring about what 'style' Godot has, only caring about what a Quick Open dialog needs.

If you think Godot's overall UI design needs changes that's completely fine, but this sort of thing needs to be done as a whole and with proper discussion on what said changes should be (the proposals repository would be the perfect place for that).

Having a section of the editor look and act differently from the rest just adds confusion to the user, who expects things to behave consistently across the editor.

@stijn-h
Copy link
Contributor Author

stijn-h commented Apr 30, 2022

If you think Godot's overall UI design needs changes that's completely fine, but this sort of thing needs to be done as a whole and with proper discussion on what said changes should be (the proposals repository would be the perfect place for that).

Top down, bottom up. Both is possible, but I have no interest in getting bogged down in discussions over what 'design rules' Godot should follow. I think it's more effective to try to design a small isolated component, and maybe others can take that further.

Actually, I think that because of this top down approach, Godot's style (which was supposed to have gotten a complete overhaul in 4.0 btw) has remained so stagnant, even when there is so much room for improvement.

@stijn-h stijn-h force-pushed the quick-open-redesign branch 2 times, most recently from 4fa91dd to 9cb4f5f Compare November 23, 2022 23:18
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Some new constants

editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Outdated Show resolved Hide resolved
editor/gui/editor_quick_open_dialog.cpp Show resolved Hide resolved
@stijn-h
Copy link
Contributor Author

stijn-h commented Sep 30, 2024

Some updates

  • Added back support for page up/down and left right keys. I removed this after using the FlowContainer, but I found a way to do it. The one caveat (still) is that left/right keys are taken by the grid view, so you cant move the search box cursor in grid view. But I think the moving the selection index is preferable.
  • Added history on a type basis. Only works if you open the dialog with one base type (which is most of the time I think). Open Scene -> history of opened scene files. Open script -> history of opened scripts. And if the number of possible results is low we still just show all possible options.

@stijn-h stijn-h force-pushed the quick-open-redesign branch 3 times, most recently from c0f1b1d to da5d417 Compare October 1, 2024 17:51
@stijn-h
Copy link
Contributor Author

stijn-h commented Oct 1, 2024

I made the grid view more compact, and use aspect ratio of items again. With that I think this PR is ready for review

2024-10-1

  - Updated list view with thumbnails, and separate file name.
  - Added a grid view which has larger icons.
  - Added toggle to filter out files from addons.
  - Store history for each opened resource type.

New Editor settings for Quick Open:
  - Startup display mode (grid or list):
      - Determined by the requested resource type.
      - Whatever was last used.
  - Toggle to filter out files from addons (for persistence).

Notes
  - The dialog is now created once in EditorNode, and globally available for other components.
  - A fixed number of result scenes are instantiated, and reused based on query.
  - Drop support for multiselect.
@stijn-h
Copy link
Contributor Author

stijn-h commented Oct 1, 2024

Figured out why separation wasn't working... I think this got renamed at some point in 4.0... that's what I deserve for having this PR open for two years I suppose.

afbeelding

afbeelding

@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2024

Well I already approved it; just gave a quick test now and it's still fine. We can still improve it after merging.

@akien-mga
Copy link
Member

akien-mga commented Oct 2, 2024

I know @samsface and @a-johnston have been working on improving the fuzzy matching for the Quick Open dialog in #82200.

Does this PR also impact the fuzzy matching, or only improves the UI side of things?

CC @MajorMcDoom too.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2024

Does this PR also impact the fuzzy matching, or only improves the UI side of things?

From my testing, it allows non-tokenized matching, like before #88660 (so now both are possible). idk if it affects results, they look same/similar as before.

@akien-mga
Copy link
Member

Alright, if contributors interested in the search/fuzzy matching behavior are fine with merging this as a first step, and then doing further work on top of it, this should be good to go.

@MajorMcDoom
Copy link
Contributor

Alright, if contributors interested in the search/fuzzy matching behavior are fine with merging this as a first step, and then doing further work on top of it, this should be good to go.

In my view, the work I did is outdated from a UX needs perspective and thus if anything supersedes it, it's a good thing. So I'm totally good with this.

@akien-mga akien-mga merged commit b070f4a into godotengine:master Oct 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks @stijn-h for this great overhaul, almost 3 years in the making! 🎉

@a-johnston
Copy link

Alright, if contributors interested in the search/fuzzy matching behavior are fine with merging this as a first step, and then doing further work on top of it, this should be good to go.

Yeah when @samsface linked this I think we both expected to just rebase on top whenever it merged. The match highlighting logic might be a bit funky but ultimately the change sets are pretty separate.

BTW very cool changes! I really like the look of the new popup.

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.

Improve Quick Open dialog design