[file_selector] Implement for Windows

[stuartmorgan-g]

Description

Adds a Windows implementation of file_selector, using platform channels. This is heavily based on the existing FDE file_chooser plugin, modified to reflect the different platform channel API.

(In the future, this could potentially be replaced by an FFI-based implementation, but this version allows us to leverage existing, field-tested code rather than starting from scratch.)

Related Issues

Windows portion of flutter/flutter#70221

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.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

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.

[stuartmorgan-g]

I'm marking this as a draft for now because I wrote this before #3140 was posted, and so the example app is completely different; once that's finalized I'll replace the example with a version of that example that's implement using the platform package so there aren't totally different example styles for no reason.

@cbracken If you could start reviewing the Windows plugin code in the meantime, that would be great. Most of this was previously reviewed as FDE's file_chooser, but there were modifications (and extra review eyes are always helpful 😃)

@ditman we should discuss whether or not you are comfortable moving forward with the desktop implementations without integration tests for now; see flutter/flutter#70221 and flutter/flutter#70233 for background. While we definitely need to build the test infrastructure, the reality is that I'm already maintaining essentially this exact plugin as file_chooser in the FDE repo (also without integration tests), and people writing desktop applications are already using it (since there's no alternative), so my view is that moving it here without blocking on integration test infrastructure--which would push this to sometime in 2021--is still an incremental improvement. Either way I'm committed to maintaining the code already.

[stuartmorgan-g]

/cc @amirh to weigh in on the test infrastructure question as well; see previous comment.

[cbracken]

@cbracken If you could start reviewing the Windows plugin code in the meantime, that would be great.

Will do!

[ditman]

@stuartmorgan I'm OK with not having great integration tests for the plugin, as long as we're reasonably happy with the unit tests on a per-package basis (I know it's not the same, but at least we have a mechanism to assert behavior with different inputs).

my view is that moving it here without blocking on integration test infrastructure--which would push this to sometime in 2021--is still an incremental improvement.

100% agree, I'd love that this gets released!

Either way I'm committed to maintaining the code already.

Do you mind adding yourself to the root OWNERS file for file_selector? That way you get to see the incoming PRs to the plugin automatically (for example, XFile is getting extracted to a reusable package, so it can be used in other plugins that handle "files").

[stuartmorgan-g]

as long as we're reasonably happy with the unit tests on a per-package basis

Sorry, to clarify: we don't currently have any infrastructure for running tests that interact with UI elements at all on any of the desktop platforms. So there wouldn't be any unit tests in the desktop implementation packages, because we can't drive the dialog. I.e., the only automated testing of this package would be that it builds; ensuring that it works would, currently, be a completely manual test process. (As it currently is for FDE.)

[ditman]

@stuartmorgan ahh gotcha, because all this does is implement the platform interface on the desktop side, no Dart. I guess that for some bugs we'll still be able to add some tests to the method_channel class (regarding the API across the channel), but for internal issues we might have some trouble.

This is marked as 0.0.1, and we can not endorse it after publishing (so people must opt-in to get the functionality). Google Maps was marked as "Developer Preview" for a long while, I don't see why this one couldn't be the same.

(Do you mind adding yourself here?)

[amirh]

@ditman we should discuss whether or not you are comfortable moving forward with the desktop implementations without integration tests for now; see flutter/flutter#70221 and flutter/flutter#70233 for background. While we definitely need to build the test infrastructure, the reality is that I'm already maintaining essentially this exact plugin as file_chooser in the FDE repo (also without integration tests), and people writing desktop applications are already using it (since there's no alternative), so my view is that moving it here without blocking on integration test infrastructure--which would push this to sometime in 2021--is still an incremental improvement. Either way I'm committed to maintaining the code already.

I can see the reasoning for landing this without a test as it is already maintained this way in the FDE repo.
Per #5 in https://github.com/flutter/flutter/wiki/Tree-hygiene#overview we should get approval from @Hixie to land this without a test.

[Hixie]

IMHO we should first land the infrastructure to run tests, because in the past when we have "pragmatically" decided that we couldn't write tests because there was not yet any infrastructure what that has really meant is that we went years without creating the infrastructure and by the time we added it we had dozens if not hundreds (if you count the wider community) of plugins with zero testing, and we had not really saved any time since we still had to create the infrastructure.

[stuartmorgan-g]

@Hixie I agree with all of your points. Some specific considerations here though:

by the time we added it we had dozens

This is the only native-UI-based plugin in flutter/plugins that I expect us to want to implement on desktop until we have PlatformView support (and those are on a long timeline; unlike this, blocking those on having native UI tests shouldn't appreciably impact anyone, so would be a good place to draw a line in the sand).

if not hundreds (if you count the wider community) of plugins with zero testing

To clarify: this plugin isn't on the critical path, so the impact of blocking this PR on having native-UI plugin tests is not that we'll get those tests sooner, it's that this PR won't land for, realistically, several quarters. I would not expect us landing or not landing one plugin here to have any impact on the number of third-party plugins that use native UI.

and we had not really saved any time since we still had to create the infrastructure.

This isn't about saving time, it's about taking a desktop-specific FDE plugin I already maintain (file_chooser) and replacing it with desktop implementations of this new cross-platform plugin, so that people who want file selection across desktop+mobile and/or desktop+web don't have to write their own adaptors to multiple plugins.

tl;dr, the tradeoff here is not "write tests now or write tests later", it's "maintain file_chooser in FDE without tests for several quarters and have no cross-platform 1P file chooser plugin during that time, or maintain file_selector here without tests for several quarters, and have the cross-platform version".

(Writing that, I realized there's a third-option: I replace FDE's file_chooser with FDE unendorsed federated implementations of file_selector. That would mean we'd maintain the test hygiene policy for flutter/* repos, while making it possible for people to use file_selector across platforms--although with extra hoops to jump through. Which is better than keeping file_chooser as-is, but still means we're making file_selector harder to use for the short-to-medium term for a distinction--FDE vs Flutter--that's pretty obscure to most people at this point.)

[Hixie]

I'm not going to stand in the way here if you think it needs to land. But I will say that all those arguments are more or less the same arguments we had a few years ago, and I regret not being more vehement then about the need for testing first.

[stuartmorgan-g]

if you think it needs to land

It doesn't; as I said, this isn't on any critical paths, which is why sinking a lot of time into test infrastructure right now to land it with integration tests isn't one of the viable options.

I'll just morph the FDE plugins into unendorsed federated implementations, and revisit moving them here someday in the future when we have eng resources to invest in desktop native UI plugin unit tests.