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

Debugger file picker #1652

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Conversation

DaveShuckerow
Copy link
Contributor

Branch based on #1649

Introduce a picker with search functionality for files.

image

Add search functionality

Filter by default to the main package
@DaveShuckerow DaveShuckerow changed the title Debug file picker Debugger file picker Feb 21, 2020
@kenzieschmoll
Copy link
Member

DBC: can you put a border and padding around the two sections to break up the view? It is a bit difficult to visually parse the UI and the two sections are too close to the splitter grippy.

Example of what we do for the flame chart:

    Padding(
      padding: const EdgeInsets.only(bottom: 8.0),
      child: Container(
        key: TimelineScreen.flameChartSectionKey,
        decoration: BoxDecoration(
          border: Border.all(color: Theme.of(context).focusColor),
        ),
        child: content,
      ),
    );

@jacob314 jacob314 requested a review from terrylucas February 24, 2020 16:35
@jacob314
Copy link
Contributor

Added @terrylucas as a reviewer as he wrote the equivalent dart:html code.

scrollDirection: Axis.horizontal,
child: SizedBox(
width: MediaQuery.of(context).size.width,
child: ListView.builder(
Copy link
Contributor

Choose a reason for hiding this comment

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

@grouma your concerns about slow file pickers when a project has 1000s of files are about to be solved.... :)

color: ref == widget.selected ? selectedColor : null,
child: InkWell(
onTap: () => widget.onSelected(ref),
child: Padding(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you have Padding + Align why not use Container
@pq this is an example where I think we could have a nice quick fix to convert this code to a Container if we thought that was the cleaner way to write this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting!

If we can generalize this at all, it would be nice to add some layout related refactorings to server.

/fyi @bwilkerson.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a reasonable refactoring to apply here.

I don't think it's a particularly high-value lint or quick fix. It's not a very common problem, and there can be cases where the transformation between Padding(child: Align)) to Container() is not possible.

} else {
_filtered = widget.scripts.scripts
.where(
(ref) => ref.uri.contains(value.toLowerCase()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider removing this trailing comma.

Copy link
Contributor

Choose a reason for hiding this comment

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

this logic should be extended to match what was in html_scripts_view.
specifically, use getShortScriptName or equivalent before calling contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left a TODO to do this. Currently debugger_state is not importable in flutter code.

Copy link
Contributor

Choose a reason for hiding this comment

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

sg

padding: const EdgeInsets.all(4.0),
child: Align(
alignment: Alignment.centerLeft,
child: Text('${ref?.uri?.split('/')?.last} (${ref?.uri})'),
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems a bit different than what html_scripts_view.dart is currently showing. Can you make it consistent or add a TODO to make it consistent with a followup cl?
Specifically: matching text should be in bold and we should be showing the entire uri.
What we have currently in the html app isn't perfect so it could also be reasonable to iterate with UXR to figure out what we really want to show here.

Copy link
Contributor Author

@DaveShuckerow DaveShuckerow Feb 25, 2020

Choose a reason for hiding this comment

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

I came to this design from meeting with internal users last week. They explained that one of the problems with the existing debugger page is that they can't see the names of files in the list because there is too much boilerplate in the long g3 package paths. To remedy this, I've adopted the form:

foo.dart (package:my_long_package_name/lib/src/long/path/to/foo.dart)

Copy link
Contributor

Choose a reason for hiding this comment

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

I had also hoped users would want to use package: for this but that isn't what our users say overall.
we did a user survey on this last summer and 38% of users want to see package: and 39% of users prefer package: while 56% percent prefer file paths relative to their app route.
in bazel projects the package: urls are particularly bad and counterintuitive. Rather than
package:my_long_package_name/lib/src/long/path/to/foo.dart
you get
package:my_long_package_name.src.long.path.to/foo.dart

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just add a TODO resolve this. For example, you could wrap each file its own scrollable so the filename was always visible and the start of the path was truncated. Would be good to get UXR involved on this as there are some tradeoffs and there is a lot of plcaes in the tool where we would like to do a better job of displaying file names.

@DaveShuckerow
Copy link
Contributor Author

DBC: can you put a border and padding around the two sections to break up the view? It is a bit difficult to visually parse the UI and the two sections are too close to the splitter grippy.

Example of what we do for the flame chart:

    Padding(
      padding: const EdgeInsets.only(bottom: 8.0),
      child: Container(
        key: TimelineScreen.flameChartSectionKey,
        decoration: BoxDecoration(
          border: Border.all(color: Theme.of(context).focusColor),
        ),
        child: content,
      ),
    );

Good idea; added this as a common widget.

@jacob314
Copy link
Contributor

lgtm

@DaveShuckerow DaveShuckerow merged commit 44b53e2 into flutter:master Feb 25, 2020
@DaveShuckerow DaveShuckerow deleted the debug-file-picker branch February 25, 2020 18:36
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.

4 participants