[file_selector] initial implementation #3140

tugorez · 2020-10-12T00:06:45Z

Description

Add the API for the file_selector plugin.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

Change some variables to final, condense for loops and some if statements.

…Testing)

Also some refactoring of plugin code to accomplish tests

new fromData constructor in XFile class

tugorez · 2020-10-30T22:02:18Z

Some very high level comments based on an initial skim.

In addition to the comments below: why isn't there a driver/integration test as part of this?

Those tests should and will be added as part as each platform implementation.

packages/file_selector/file_selector/test/file_selector_test.dart

stuartmorgan-g · 2020-11-01T19:38:08Z

Some very high level comments based on an initial skim.
In addition to the comments below: why isn't there a driver/integration test as part of this?

Those tests should and will be added as part as each platform implementation.

Every plugin I've implemented for desktop here has integration tests at both layers. Why would we do less testing in this plugin?

Examples:
https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher/example/integration_test
https://github.com/flutter/plugins/tree/master/packages/path_provider/path_provider/example/integration_test
https://github.com/flutter/plugins/tree/master/packages/shared_preferences/shared_preferences/example/integration_test

ditman · 2020-11-02T19:30:05Z

Every plugin I've implemented for desktop here has integration tests at both layers.

@stuartmorgan most plugins so far have a purely programmatic API that never leaves the Flutter App under test. In this case, the test would need to drive native OS dialogs (I don't know if that's doable?) or bypass all that altogether by adding a mock method channel handler (which wouldn't work for the web case).

I don't see how this end of the plugin can be integration-tested in a way that is somewhat platform agnostic. Do you have any idea about that?

As a bad example, look at the tests for the image picker ;)

stuartmorgan-g · 2020-11-02T19:45:00Z

Oh, right. I wasn't thinking about the fact that this is UI based; sorry about that. Yes, that does probably mean integration tests can't live at this layer.

(We should still unit test the app-facing API though, using a mock platform interface implementaiton, per my other comments.)

ditman · 2020-11-02T19:47:28Z

We should still unit test the app-facing API though, using a mock platform interface implementaiton, per my other comments.

@stuartmorgan yes, those should be easy to add; it's just verify that the mock platform methods are being called (if done with Mockito and a Mock platform implementation) or by doing the method logging technique.

ditman

Thanks for addressing this! I think it's ready to go, even though, there's a small change incoming regarding the cross_file package (we should be able to hide that fact from the platform_interface package, possibly!)

If Stuart is happy with the current implementation, let's get this merged!

ditman · 2020-11-18T01:46:42Z

packages/file_selector/file_selector/example/lib/get_directory_page.dart

+import 'package:file_selector/file_selector.dart';
+import 'package:flutter/material.dart';
+
+/// Screen that shows an example of openFile(s)


This screen shows an example of getDirectoryPath, it seems :P

Great catch! Fixed here and (hopefully) everywhere.

ditman · 2020-11-19T22:03:23Z

Note to all: package: cross_file is now a thing:

https://pub.dev/packages/cross_file

stuartmorgan-g

Thanks for the expanded test coverage and examples! LGTM with some minor nits.

stuartmorgan-g · 2020-11-20T01:41:22Z

packages/file_selector/file_selector/example/lib/get_directory_page.dart

+class GetDirectoryPage extends StatelessWidget {
+  void _getDirectoryPath(BuildContext context) async {
+    final String initialDirectory = '~/Desktop';
+    final String confirmButtonText = 'Choose a cool directory';


Nit: This is a lot of text for a button. Maybe pick something more realistic to demonstrate, like "Choose"?

stuartmorgan-g · 2020-11-20T01:42:43Z

packages/file_selector/file_selector/example/lib/get_directory_page.dart

+/// Screen that shows an example of openFile(s)
+class GetDirectoryPage extends StatelessWidget {
+  void _getDirectoryPath(BuildContext context) async {
+    final String initialDirectory = '~/Desktop';


Do the various OS file dialogs actually expand "~"? The robust way to do this would be to use path_provider; if we don't want to depend on that I would suggest we just don't set a starting directory in the example.

That is a good point. I'm not sure if ~ gets actually expanded so I will play safe here and remove it.
Thanks @stuartmorgan :)!

Note that path_provider doesn't work in web at all AFAIK (U_U)

stuartmorgan-g · 2020-11-20T01:45:43Z

packages/file_selector/file_selector/example/lib/open_multiple_images_page.dart

+      extensions: ['jpg', 'jpeg'],
+    );
+    final XTypeGroup pngTypeGroup = XTypeGroup(
+      extensions: ['png'],


Why no label?

stuartmorgan-g · 2020-11-20T01:46:02Z

packages/file_selector/file_selector/example/lib/open_multiple_images_page.dart

+class OpenMultipleImagesPage extends StatelessWidget {
+  void _openImageFile(BuildContext context) async {
+    final XTypeGroup jpgsTypeGroup = XTypeGroup(
+      label: 'images',


It seems odd to call this "images" rather than "JPEGs" given that the other option is also an image type.

tugorez · 2020-11-20T17:20:28Z

Note to all: package: cross_file is now a thing:

https://pub.dev/packages/cross_file

flutter/flutter#70950 to track the integration. Thanks for the heads up :)!

ditman · 2020-11-20T21:48:11Z

Let's go with this one! Thanks all for the hard work!

stuartmorgan-g · 2020-11-24T21:39:09Z

While doing manual testing on Windows with the example I noticed that none of the example calls handle the user pressing cancel (in which case the returned path will be null). Right now canceling any dialog in the example will throw an exception.

ditman · 2020-11-24T22:39:45Z

@stuartmorgan that's an interesting "edge" case for the web; in web you never get called back if the user cancels the selection (no events, null path... nothing!)

Co-authored-by: Jason Panelli <[email protected]>

jasonpanelli added 30 commits July 15, 2020 18:35

"Hello World" file_picker plugin

ec70f70

Add DOM Initialization and Test a[download]

390c1fe

Add FilePicker Type + Add TextField to Example

234c811

Test Blob URL Instead of Data Scheme

4fd9c14

Rename PickedFile to XFile; Implement saveFile with Uint8List

2d06300

PoC loadFile text only

2d924da

Add suggestedName to saveFile

fccf520

Use file name in loadFile example

0a40194

Load Multiple Files, Save As a Type

30bae1d

Add Method Channel Implementation + Some Refactor

06cf4fc

Initial Refactor

3303db7

Change some variables to final, condense for loops and some if statements.

Basic Method Channel Tests

2b21ba4

Break Down loadFile Into Smaller Parts

9b7d3fe

Use FileTypeFilterGroup class

5f5f9aa

Bug Fix (No Input to File Extension)

a73f088

More Refactor of loadFile and saveFile into Smaller Pieces (Prep for …

e8e9f41

…Testing)

Initial tests for web implementation

2dc5059

Also some refactoring of plugin code to accomplish tests

Delete original test file

73c4b62

Test Loading Multiple Files + saveFile tests

3b3ab63

Fix Example Text Overflow

d24b55a

Add XPath class and New API to interface

727c752

Add XPath and begin getReadPath(s)

d0855d9

Functional getReadPath

7398fff

Do Nothing in getSavePath web

4eccd88

Refactor XFile Class to use XPath

c995259

Remove XPath, update API

dc400aa

Move Save Logic to XFile and Remove XPath

2999129

new fromData constructor in XFile class

Add XType and XTypeGroup

1abe106

Remove Named Constructors

52a17fe

Bug Fix, 'accept' attribute requires extensions start with '.'

ee291fc

tugorez added 4 commits October 30, 2020 13:31

Improve tests

8e98c95

Merge branch 'master' into file_selector

e907949

Add open multiple images example

4d8bd68

Add documentation to public field

2284daa

stuartmorgan-g suggested changes Nov 1, 2020

View reviewed changes

packages/file_selector/file_selector/test/file_selector_test.dart Show resolved Hide resolved

tugorez added 2 commits November 6, 2020 15:03

Add get directory example

208e504

Merge branch 'master' into file_selector

5a0c109

ditman mentioned this pull request Nov 11, 2020

[webview_flutter] Add file chooser on Android #3225

Closed

13 tasks

stuartmorgan-g mentioned this pull request Nov 11, 2020

[file_selector] Implement for Windows #3265

Closed

13 tasks

Improve tests

3d2cf35

This was referenced Nov 16, 2020

P2: [devtools_app] Make the "Import File" button works. flutter/devtools#2505

Closed

[file_selector_web]: Add initial implementation #3141

Merged

ditman approved these changes Nov 18, 2020

View reviewed changes

stuartmorgan-g approved these changes Nov 20, 2020

View reviewed changes

Fix the example's class comments

113c756

tugorez added 2 commits November 20, 2020 11:21

Change button's label

8fb1fcc

Address nits

ea11561

ditman merged commit 576f04a into flutter:master Nov 20, 2020

amantoux pushed a commit to amantoux/plugins that referenced this pull request Feb 8, 2021

[file_selector] Initial implementation (flutter#3140)

dc26706

Co-authored-by: Jason Panelli <[email protected]>

adsonpleal pushed a commit to nubank/plugins that referenced this pull request Feb 26, 2021

[file_selector] Initial implementation (flutter#3140)

646117e

Co-authored-by: Jason Panelli <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[file_selector] initial implementation #3140

[file_selector] initial implementation #3140

tugorez commented Oct 12, 2020 •

edited

Loading

tugorez commented Oct 30, 2020

stuartmorgan-g commented Nov 1, 2020 •

edited

Loading

ditman commented Nov 2, 2020

stuartmorgan-g commented Nov 2, 2020

ditman commented Nov 2, 2020

ditman left a comment

ditman Nov 18, 2020

tugorez Nov 20, 2020

ditman commented Nov 19, 2020

stuartmorgan-g left a comment

stuartmorgan-g Nov 20, 2020

tugorez Nov 20, 2020

stuartmorgan-g Nov 20, 2020

tugorez Nov 20, 2020

ditman Nov 20, 2020

stuartmorgan-g Nov 20, 2020

tugorez Nov 20, 2020

stuartmorgan-g Nov 20, 2020

tugorez Nov 20, 2020

tugorez commented Nov 20, 2020

ditman commented Nov 20, 2020

stuartmorgan-g commented Nov 24, 2020

ditman commented Nov 24, 2020

[file_selector] initial implementation #3140

[file_selector] initial implementation #3140

Conversation

tugorez commented Oct 12, 2020 • edited Loading

Description

Related Issues

Checklist

Breaking Change

tugorez commented Oct 30, 2020

stuartmorgan-g commented Nov 1, 2020 • edited Loading

ditman commented Nov 2, 2020

stuartmorgan-g commented Nov 2, 2020

ditman commented Nov 2, 2020

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Nov 19, 2020

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tugorez commented Nov 20, 2020

ditman commented Nov 20, 2020

stuartmorgan-g commented Nov 24, 2020

ditman commented Nov 24, 2020

tugorez commented Oct 12, 2020 •

edited

Loading

stuartmorgan-g commented Nov 1, 2020 •

edited

Loading