[file_selector] Migrate Windows implementation from CPP to Dart

[adpinola]

This PR migrates the Windows CPP implementation to Dart.
These changes have no impact on the API.

We also kept consistency along method signatures and return values across all classes between CPP and Dart versions.

We followed the styling guide from these recommendations, but it is important to note that in order for the format check to pass, some of those recommendations can't be followed, like this one.

This PR includes the changes in:

• #6518
• #6539
• #6546

which are partial migrations from involved classes. (feedback and last updates were not applied there)

Issue: [file_selector_windows] Migrate CPP implementation to Dart. #112212

The following images show the current functionality tested against the example app located at file_selector_windows/example

Open File

Open Image

Open Multiple Images

Save a File

Get Directory Path

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[adpinola]

I've seen that this check legacy-version-analyze CHANNEL:2.10.5 is failing, and as far as I understand, It is related to new dependencies like win32 and ffi; both of them declare in their own pubspec.yml file the following:

environment:
  sdk: '>=2.17.0 <3.0.0'

I'm not sure how to address this situation, But I'm wondering if we should change the environment settings in the package

from:

environment:
  sdk: ">=2.12.0 <3.0.0"
  flutter: ">=2.10.0"

to:

environment:
  sdk: ">=2.17.0 <3.0.0"
  flutter: ">=3.0.0"

Will that avoid running the legacy-version-analyze check?

[adpinola]

Hi @timsneath thanks for your feedback, we applied some changes, can you take a look again?

What we did specifically was:

• Replaced TEXT() usage with ToNativeUtf16(allocator: arena) to handle allocations as we did previously in other parts of the code.
• Applied the styling guide from these recommendations, but it is important to note that in order for the format check to pass, some of those recommendations can't be followed, like this one.

[adpinola]

Hi @loic-sharma, thanks for your feedback.
I'll be attending to your comments today.

At a glance, there are some cases when we are returning an empty string or not throwing an exception. The reason for this was to maintain the exact implementation that was written in C++. If you agree, I'll make the changes to throw a WindowsException in those cases.

Thanks!!

[loic-sharma]

At a glance, there are some cases when we are returning an empty string or not throwing an exception. The reason for this was to maintain the exact implementation that was written in C++.

Gotcha, that makes perfect sense! This Dart migration is already a major change; I agree that minimizing the differences between the two implementations is valuable.

If you agree, I'll make the changes to throw a WindowsException in those cases.

I'm not very familiar with the existing C++ implementation, I'll take a closer look at that and get back to you.

[adpinola]

Thanks, @loic-sharma.

Regarding my previous comment, I applied this one:

As per FileSelectorPlatform's documentation, this should return null if the user cancels the operation. However, a WindowsException(E_FAIL) exception is thrown instead in this scenario by FileSelectorApi._showDialog.

to match the API expectation, regardless of the previous C++ implementation. (Pigeons used to handle this in the top level as I recently double-checked)

Moreover, since the ShellWin32Api wrapper is new, It's better to actually throw the exception instead of the empty string, as you suggested. So I applied that, also.

I think we are now in better shape.

Thanks!!

[stuartmorgan]

From triage: what's the status of this PR? Is it waiting for new updates, or for re-review?

[timsneath]

I'll take a look after Flutter Forward if you don't hear from anyone else.

…

On Tue, Jan 17, 2023 at 10:16 PM stuartmorgan ***@***.***> wrote: From triage: what's the status of this PR? Is it waiting for new updates, or for re-review? — Reply to this email directly, view it on GitHub <#6568 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWL67KY7LVHAHCSHKTGPLWS3V2HANCNFSM6AAAAAARDQOI6A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[adpinola]

From triage: what's the status of this PR? Is it waiting for new updates, or for re-review?

@stuartmorgan It is ready for re-review.

[stuartmorgan]

Apologies if I missed or forgot some discussion elsewhere, but what are the changes in approach here relative to https://github.com/flutter/plugins/pull/6467to address #6467 (comment)? There were incremental PRs, but those got re-combined, so it seems like we're back to a full from-scratch rewrite here.

[adpinola]

Hi, the main difference is that we kept a 1:1 relation between Dart and Cpp classes. Morevorer, method's were implemented following the same logic as they used to have in old Cpp implementation, like input and returned values.

[stuartmorgan]

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!