-
Notifications
You must be signed in to change notification settings - Fork 673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix default_plugin warnings with recent dart sdk #1567
Conversation
Hey @miguelpruivo, @philenius can you please review this fix and/or approve workflow? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly a comment to see if we can fix a bunch of long standing issues with mis-use of Platform._operatingSystem
in the plugin.
I see you're all actively working on it. "Package file_picker:windows references file_picker:windows as the default plugin, but it does not provide an inline implementation. |
That is indeed what this PR would aim to solve yes. Once we get the required changes in this PR, I'll review it again and then we can release a fix version. |
This file used before Flutter 3.3.0
7f54dda
to
49754d7
Compare
ad00475
to
5bf3db9
Compare
d7baf34
to
cf89df0
Compare
@navaronbracke can you please take another look at this? I think I implemented what you talked about here. Btw, I had hard times reproducing #1343 both based on this PR and master – no success. Either I do something wrong, or, maybe, these issues were fixed earlier somehow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, just some small documentation suggestions.
Once those are addressed, we can land this!
lib/file_picker.dart
Outdated
export './src/file_picker_io.dart'; | ||
// Conditional export needed for web to successfully compile. | ||
export './src/windows/file_picker_windows_stub.dart' | ||
if (dart.library.ffi) './src/windows/file_picker_windows_real.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the file_picker_windows.dart
filename for the real implementation? (rather than file_picker_windows_real.dart
)
The fact that the fake one is called file_picker_windows_stub.dart
clarifies its use in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have no strong opinion about this. So yes, done :)
I also edited the original PR comment to list fixed issues, so these get closed when the PR is merged. As for not being able to reproduce #1343 I'm not sure either. |
cf89df0
to
b2a15c9
Compare
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I also edited out the redundant bullet points (one was an implementation detail, which users don't care about and the other was now a duplicate point since we already mention fixing #1555 in the first point)
It looks like one of the test is failing, because https://github.com/miguelpruivo/flutter_file_picker/pull/1561/files did not update a test expectation. |
This commit fixes a test expectation that was not updated in a previous PR.
Checks are green, so ship it! |
This PR addresses #1555 and removes warning. Implementation is based on this comment from flutter/flutter#152037
Fixes #1484
Fixes #1483
Fixes #1343
Details:
Old implementation with
default_plugin
never did anything over than providing correct list of supported platforms for package page on pub.dev. Since some version of dart sdk, this behaviour has become warning.New implementation provides (arguably) correct usage of
flutter:plugins:
section to avoid warning. Since all logic for choosing current platform and appropriate plugin's implementation is encapsulated inside current code, this PR just provides emptyregisterWith
static methods needed for compilation. Also, it is mandatory for plugin implementations to be public, so we export them.Testing:
Current tests are not affected by this PR. Example app is also functioning (tested on
android
andios
). Since tests on macOS (my dev machine) and example on both emulators works – I believe that functionality is not broken.#1555 provides reproduction with https://github.com/zulip/zulip-flutter repo. This fix can be tested in that repo by adding
dependency_overrides
topubspec.yaml
. This override will point to my fork with fix: