[devtools_app] Integrate the file_selector plugin #2506

tugorez · 2020-11-16T23:45:01Z

kenzieschmoll · 2020-11-30T20:36:55Z

tugorez · 2021-01-14T17:00:26Z

@kenzieschmoll I added support for web, linux and macOS. I manually tested web and it works.
It should be the same for linux and macOS: Can you help me to try those platforms integration?

kenzieschmoll · 2021-01-14T17:37:54Z

packages/devtools_app/lib/src/app_size/file_import_container.dart

+  }
+
+  // Only show the import button in linux, macOS and web.
+  bool get _shouldShowImportButton =>


can this just be true? what will happen if someone tries to build devtools and run on windows? defaultTargetPlatform == TargetPlatform.macOS is false when I tested on my mac.

can this just be true? what will happen if someone tries to build devtools and run on windows?

If someone presses the import button in a platform that is not supported by file_selector then an Unimplemented error will be thrown.

defaultTargetPlatform == TargetPlatform.macOS is false when I tested on my mac.

Oh then I should find another way to discover the platform the project is running on.

Is there some reason you're not using file_selector_windows?

Is there some reason you're not using file_selector_windows?

That's a good point. Let me add it and remove the platform validation. @kenzieschmoll does that sounds good to you?

making it work on all platforms sgtm 👍

end users won't be hit by this anyway - they use DevTools on web. We mostly develop on Mac and Linux and none of us to my knowledge develop on windows.

I just added windows and removed the platform validation. Thanks to both :)!

kenzieschmoll · 2021-01-14T17:38:56Z

Tested on MacOs and it appears that only directories are selectable when finder is launched. See this screenshot with all the .json files disabled.

tugorez · 2021-01-14T17:59:19Z

Tested on MacOs and it appears that only directories are selectable when finder is launched. See this screenshot with all the .json files disabled.

@stuartmorgan do you know why this might be happening? I'm providing this XTypeGroup to the plugin.

stuartmorgan-g · 2021-01-14T18:11:01Z

I'm providing this XTypeGroup to the plugin.

You want XTypeGroup(extensions: ['json']). The extension of 'foo.json' is json, not .json; . is a separator.

I'm surprised it worked on web. Maybe there's fixup happening there?

Edit: Looks like there is:
https://github.com/flutter/plugins/blob/master/packages/file_selector/file_selector_web/lib/src/utils.dart#L35-L38
as a side-effect of converting to the format expected by the underlying web specification. Filed flutter/flutter#73959

tugorez · 2021-01-14T18:18:31Z

I'm providing this XTypeGroup to the plugin.

You want XTypeGroup(extensions: ['json']). The extension of 'foo.json' is json, not .json; . is a separator.

I'm surprised it worked on web. Maybe there's fixup happening there?

Oh you are right. Prepending a "." to the extensions is a web only thing. It should be fine to change ".json" to "json" as the "normalization" should be handled by this.

packages/devtools_app/lib/src/app_size/file_import_container.dart

tugorez · 2021-01-14T18:24:22Z

I'm providing this XTypeGroup to the plugin.

You want XTypeGroup(extensions: ['json']). The extension of 'foo.json' is json, not .json; . is a separator.
I'm surprised it worked on web. Maybe there's fixup happening there?

Oh you are right. Prepending a "." to the extensions is a web only thing. It should be fine to change ".json" to "json" as the "normalization" should be handled by this.

I changed the extension, @kenzieschmoll PTAL :)

tugorez · 2021-01-14T18:24:32Z

cc @ditman

kenzieschmoll

tested manually on Mac and it works great! I assume linux would work too - maybe @jacob314 or @terrylucas could do a quick check since you already have a linux dev environment set up?

kenzieschmoll · 2021-01-14T23:21:39Z

packages/devtools_app/lib/src/app_size/file_import_container.dart

-        color: Theme.of(context).textTheme.headline1.color,
-      ),
-      textAlign: TextAlign.left,
+  Widget _buildImportedFileDisplay() => Text(


can you add overflow: TextOverflow.ellipsis, to this text widget? thanks.

kenzieschmoll · 2021-01-14T23:23:19Z

packages/devtools_app/lib/src/app_size/file_import_container.dart

+            final acceptedTypeGroups = [
+              XTypeGroup(extensions: ['json']),
+            ];


can you make List<String> acceptedTypeGroups an optional parameter in the FileImportContainer constructor that defaults to ['json'], and use that here instead?

kenzieschmoll · 2021-01-14T23:24:30Z

packages/devtools_app/pubspec.yaml

@@ -11,23 +11,27 @@ version: 0.9.6
 homepage: https://github.com/flutter/devtools

 environment:
-  sdk: '>=2.10.0 <3.0.0'
+  sdk: ">=2.10.0 <3.0.0"


any reason for changing from ' to " ? if not, can we leave as single quotes? here, and elsewhere in pubspec.yaml

kenzieschmoll · 2021-01-14T23:25:18Z

packages/devtools_app/pubspec.yaml

 dependency_overrides:
-# The "#OVERRIDE_FOR_DEVELOPMENT" lines are stripped out when we publish.
-# All overriden dependencies are published together so there is no harm
-# in treating them like they are part of a mono-repo while developing.
+  # The "#OVERRIDE_FOR_DEVELOPMENT" lines are stripped out when we publish.
+  # All overriden dependencies are published together so there is no harm
+  # in treating them like they are part of a mono-repo while developing.
  devtools_server: #OVERRIDE_FOR_DEVELOPMENT
-    path: ../devtools_server  #OVERRIDE_FOR_DEVELOPMENT
-  devtools_shared:  #OVERRIDE_FOR_DEVELOPMENT
-    path: ../devtools_shared  #OVERRIDE_FOR_DEVELOPMENT
-  devtools_testing:  #OVERRIDE_FOR_DEVELOPMENT
-    path: ../devtools_testing  #OVERRIDE_FOR_DEVELOPMENT
+    path: ../devtools_server #OVERRIDE_FOR_DEVELOPMENT
+  devtools_shared: #OVERRIDE_FOR_DEVELOPMENT
+    path: ../devtools_shared #OVERRIDE_FOR_DEVELOPMENT
+  devtools_testing: #OVERRIDE_FOR_DEVELOPMENT
+    path: ../devtools_testing #OVERRIDE_FOR_DEVELOPMENT


please revert these white space changes as well. Thank you.

ditman · 2021-01-16T02:32:33Z

packages/devtools_app/lib/src/app_size/file_import_container.dart

+    final acceptedTypeGroups = [
+      XTypeGroup(extensions: ['json'])
+    ];


Echoing kenz's earlier review:

can you make List<String> acceptedTypeGroups an optional parameter in the FileImportContainer constructor that defaults to ['json'], and use that here instead?

acceptedTypeGroups should be defined in line ~20 of this file, and be an optional named parameter to FileImportContainer, which defaults to the current XTypeGroup of json, so they can use the FileImportContainer in other places of the App (to select TXT files or PNG or whatever) :)

packages/devtools_app/lib/src/app_size/file_import_container.dart

kenzieschmoll

One nit - then LGTM. Thanks for your work on this!

ditman · 2021-01-22T21:00:19Z

👏 bravo!

tugorez mentioned this pull request Dec 17, 2020

[file_selector_web]: Add initial implementation flutter/plugins#3141

Merged

13 tasks

tugorez force-pushed the file_selector branch 3 times, most recently from b9fa43c to 451f60a Compare January 14, 2021 16:58

tugorez force-pushed the file_selector branch from 451f60a to b618a3f Compare January 14, 2021 17:01

kenzieschmoll reviewed Jan 14, 2021

View reviewed changes

stuartmorgan-g mentioned this pull request Jan 14, 2021

[file_selector] Make treatment of extensions given with with '.' consistent flutter/flutter#73959

Closed

kenzieschmoll reviewed Jan 14, 2021

View reviewed changes

packages/devtools_app/lib/src/app_size/file_import_container.dart Show resolved Hide resolved

kenzieschmoll reviewed Jan 14, 2021

View reviewed changes

ditman reviewed Jan 16, 2021

View reviewed changes

tugorez added 8 commits January 21, 2021 21:10

Render the "Import File" button.

12a0b8a

Fix the extension.

016fd27

Add support for windows

744a9df

Change _buildImportedFileDisplay format

4cb0f2e

Reorganize code

b7bd935

Restore pubspec format

9ec5329

Refactor the file importer

d38453b

Add a different overflow strategy

3b0ba83

tugorez added 2 commits January 21, 2021 21:10

Revert formatting

e802e71

Make extensions an optional argument.

6f1081f

tugorez force-pushed the file_selector branch from d0dcc51 to 6f1081f Compare January 22, 2021 03:12

kenzieschmoll reviewed Jan 22, 2021

View reviewed changes

packages/devtools_app/lib/src/app_size/file_import_container.dart Outdated Show resolved Hide resolved

kenzieschmoll approved these changes Jan 22, 2021

View reviewed changes

Fix the doc comment.

566bad8

tugorez marked this pull request as ready for review January 22, 2021 17:05

kenzieschmoll approved these changes Jan 22, 2021

View reviewed changes

kenzieschmoll merged commit 22c4657 into flutter:master Jan 22, 2021

[devtools_app] Integrate the file_selector plugin #2506

[devtools_app] Integrate the file_selector plugin #2506

Uh oh!

Conversation

tugorez commented Nov 16, 2020

Uh oh!

kenzieschmoll commented Nov 30, 2020

Uh oh!

tugorez commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Jan 14, 2021

Uh oh!

tugorez commented Jan 14, 2021

Uh oh!

stuartmorgan-g commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tugorez commented Jan 14, 2021

Uh oh!

Uh oh!

tugorez commented Jan 14, 2021

Uh oh!

tugorez commented Jan 14, 2021

Uh oh!

kenzieschmoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kenzieschmoll left a comment

Choose a reason for hiding this comment

Uh oh!

ditman commented Jan 22, 2021

Uh oh!

Uh oh!

tugorez commented Jan 14, 2021 •

edited

Loading

stuartmorgan-g commented Jan 14, 2021 •

edited

Loading

kenzieschmoll Jan 14, 2021 •

edited

Loading

ditman Jan 16, 2021 •

edited

Loading