Skip to content

Adds a basic dialog for opening a file #3342

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

Merged
merged 16 commits into from
Sep 8, 2021
Merged

Conversation

elliette
Copy link
Member

@elliette elliette commented Sep 4, 2021

This is working towards #3075.

The dialog is currently hidden behind a hard coded boolean until it's more polished and has tests.

The dialog can be opened with ⌘ P or from the codeview menu.

Demo opening the dialog with ⌘ P:

hotkey_file_opener

Demo opening the dialog from the menu:

menu_file_opener

@elliette
Copy link
Member Author

elliette commented Sep 4, 2021

One thing I'm still trying to figure out is why I'm seeing the error (below). It shows up when the dialog is first opened, and the autocomplete overlay is opened. Everything still seems to work as intended despite the error. If I only show the autocomplete results once a user has entered a query, then there is no error.

This Overlay widget cannot be marked as needing to build because the framework is already in the process of building widgets. A widget can be marked as needing to be built during the build phase only if one of its ancestors is currently building. This exception is allowed because the framework builds parent widgets before children, which means a dirty descendant will always be built. Otherwise, the framework might not visit this widget during this build phase.
The widget on which setState() or markNeedsBuild() was called was:
  Overlay-[LabeledGlobalKey<OverlayState>#447a2]
The widget which was currently being built when the offending call was made was:
  Container

@elliette elliette marked this pull request as ready for review September 4, 2021 00:25
@kenzieschmoll
Copy link
Member

One thing I'm still trying to figure out is why I'm seeing the error (below). It shows up when the dialog is first opened, and the autocomplete overlay is opened. Everything still seems to work as intended despite the error. If I only show the autocomplete results once a user has entered a query, then there is no error.

This means that setState is being called (or a ValueNotifer's value changed, which inherently calls setState), while the tree is building. This is only legal if the widget trying to be marked as needing to build (Overlay) has an ancestor that is already building. The Overlay however, is probably very high up in the widget tree, so it does not have any ancestors that are building in the current build phase. So I'd try to track down which setState call or value notifier change is occurring while a build is in progress. Looks like it is whatever is triggering the the Overlay widget to be marked as needing to build while the Container widget is already building.

searchFieldKey: fileSearchFieldKey,
searchFieldEnabled: true,
shouldRequestFocus: true,
closeOverlayOnEscape: false,
Copy link
Member

Choose a reason for hiding this comment

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

should this be true? I would think we'd want to give the user the ability to close the dialog from keyboard. Closing the dialog on Escape matches IntelliJ's open file dialog behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

This skips closing the autocomplete overlay. So ESC will close the dialog (otherwise a user first has to click ESC to close the autocomplete results, then click ESC again to close the dialog)

List<ScriptRef> scriptRefs,
) {
if (query.isEmpty) {
takeTopMatches(scriptRefs);
Copy link
Member

Choose a reason for hiding this comment

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

should we be returning this value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the idea is to imitate Chrome DevTools. So if the query is empty, we show a list of files. If there is a query and it doesn't match, we show no files:

chrome file opener

.where((scriptRef) => scriptRef.uri.caseInsensitiveFuzzyMatch(query))
.toList();

return takeTopMatches([...exactMatches, ...fuzzyMatches, ...scriptRefs]);
Copy link
Member

Choose a reason for hiding this comment

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

should we still be including scriptRefs here? If none match the query, I think we should have empty results. This matches the open file behavior in IntelliJ.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, changed so that we return don't return any files if there is no match


_autoCompleteController.selectTheSearch = true;
SchedulerBinding.instance.addPostFrameCallback((_) => _handleSearch());
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment explaining why this needs to be in a post frame callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

@elliette elliette merged commit 33421dd into flutter:master Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants