[file_selector_windows] Migrate native implementation to Dart.

[juandausa]

Migrate native implementation to Dart, including the following changes:

• Migrate openFile.
• Migrate openFiles.
• Migrate getSavePath.
• Migrate getDirectoryPath.
• Remove CPP implementation.
• Update package version.

We based our development on the available filepicker_windows

[file_seletor_windows] Migrate CPP implementation to Dart. #112212

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.

[yaakovschectman]

Have these changes had any effect on existing dll files affecting the autoroll tests?

[cbracken]

A few quick doc/comment-related comments to start with. This is purely documentation related and I've only glanced through the first few files.

[cbracken]

Here and a few other places: avoid adding docs that add no details over the method name.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-useless-documentation

[stuartmorgan-g]

I did an initial skim, and left some inline comments, but as a high-level comment: I find this difficult to review because all of the classes have names that don't clearly distinguish them from each other, and then the comments for each class are a single vague sentence. Please expand on the class documentation so that each class has a clear description of what that class's responsibility is in the overall structure, so that readers (including us as reviewers) can get a clear sense of the overall structure of this code without having to reverse-engineer it from the details.

I'm also concerned about the fact that this is an all-at-once rewrite, which due to the lack of integration tests (since we can't do integration tests for native modal UI at the moment for Windows) means that all the tests are new. So in one PR we're discarding the entire real-world-tested implementation for a completely new implementation, and also discarding all of the unit tests. Could this instead be done as a series of incremental PRs, adding new Dart code piece by piece with (when possible) a clear mapping to both the native code it's replacing and a clear mapping between the unit tests? That would make this much easier to review as a delta to the existing implementation, instead of as a green-field implementation that may have regressions.

[stuartmorgan-g]

Why is the implementation of the class an argument?

[juandausa]

It is used for testing purposes.

[stuartmorgan-g]

If this is intended to wrap IFileOpenDialog, why is it taking the dialog as an argument to every method instead of that being a constructor argument?

[juandausa]

Hi Stuart, like this class is also used to create new instances, we prefer passing IFileOpenDialog as a parameter.

[stuartmorgan-g]

This doesn't match the extends, which makes this comment very confusing to me.

[juandausa]

Hi Stuart, it was an error, we updated the hierarchy and the comments.

[stuartmorgan-g]

I haven't gone through this in detail yet, but it seems concerning that there are six different implementation files but only one test file. Why doesn't the test structure match the implementation structure?

[juandausa]

Hi everyone, thanks for the feedback. We are taking care of the suggested changes but, we don't want to wait till cover all the comments before submitting them.
@stuartmorgan @loic-sharma @cbracken do you want us to answer each comment or do you prefer a consolidated message?

The remaining uncovered comments are the following:

• Have these changes had any effect on existing dll files affecting the autoroll tests?
• Useless comments.
• hResult usage.
• Add tests to the wrapper classes.

Cheers,

[juandausa]

Hi @yaakovschectman, I don't follow you, what do you mean by this?

Have these changes had any effect on existing dll files affecting the autoroll tests?

Thanks,

[yaakovschectman]

Previously, tests were failing due to a missing .dll file. Now it looks like they're failing for a different reason.

[stuartmorgan-g]

The remaining uncovered comments are the following

Was there a response to my structural comment about how the PR is being done somewhere?

[juandausa]

Hi Stuart, about your question:

The remaining uncovered comments are the following

Was there a response to my structural comment about how the PR is being done somewhere?

We could add the whole implementation but instead of removing the CPP code, use it for just three out of four functions, and make the other one uses the new Dart implementation. What do you think about this?

Thanks,

[stuartmorgan-g]

That doesn't address my concerns with reviewability or test equivalence since the "add the whole implementation" part is the problem. It appear that this was a from-scratch rewrite, rather than an incremental conversion, and that makes comparing quite difficult.

As an example: in looking at this again, it appear that you completely dropped the window parenting. Code to pass a parent is there, but it's never populated as far as I can tell. Which means that you didn't preserve the logic from the C++ implementation, but importantly also that you didn't replicate the test coverage of file_selector_plugin_test.cpp. This is why all-at-once rewrites are problematic; there's no clear way to tell how many other similar regressions there might be here, or what test coverage was lost.

At a minimum, since we can't do end-to-end integration tests (due to the modal native UI) the starting point for a completely new implementation would need to be a direct conversion of the existing C++ pseudo-inegration tests (which start at the native entry point, and only mock Windows APIs, to test all of the native code end to end) to Dart, so that we can compare the tests and easily see that we have identical coverage to what's already there.

[juandausa]

Hi Stuart, so we should migrate the existing code to Dart, without changing its logic. We are going to do this in parts, as you told us.

The plan following:

• Create a bunch of PR migrating the current CPP implementation to Dart following a TDD approach using the CPP tests as a base.
• Once we have a Dart codebase, we can compare this implementation to it for an easier code review. In the meantime, we can close this PR and eventually use its codebase against the newly translated code, now that we have applied the feedback.

Thanks.