[path_provider] started supporting background platform channels #4443
Conversation
gaaclarke
force-pushed
the
path_provider_background
branch
4 times, most recently
from
bd74fd4 to
8cc0451
Compare
|
@blasten This works with local testing. Let me know what else you think we need to do. Ideally this would have automated tests. |
| @@ -2,7 +2,7 @@ name: path_provider | |||
| description: Flutter plugin for getting commonly used locations on host platform file systems, such as the temp and app data directories. | |||
| repository: https://github.com/flutter/plugins/tree/master/packages/path_provider/path_provider | |||
| issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+path_provider%22 | |||
| version: 2.0.5 | |||
| version: 2.0.6 | |||
There was a problem hiding this comment.
This will need a higher version constraint on Flutter SDK. I'm not sure if that means it also needs a minor or major version bump.
There was a problem hiding this comment.
Ahh I see, you're using reflection for this.
There was a problem hiding this comment.
It should really have been a minor version update anyway, since this is a change to "add functionality in a backwards compatible manner" (the semver spec description for when to make a minor change), rather than a bug fix.
|
It'd be great if you could test this out in Google's internal repo. Change LGTM though, but would defer to @blasten |
| channel = | ||
| constructor.newInstance(messenger, channelName, StandardMethodCodec.INSTANCE, taskQueue); | ||
| impl = new PathProviderBackgroundThread(); | ||
| Log.d(TAG, "use taskqueues"); |
There was a problem hiding this comment.
if you want to leave this in, make it a full sentence
gaaclarke
force-pushed
the
path_provider_background
branch
from
8cc0451 to
fbf02ac
Compare
|
If we run integration tests against master channel and stable channel the existing tests should be sufficient since the master channel will hit the TaskQueues and the stable will run the old code. I verified locally on the master channel that it is using TaskQueues. |
| .build()); | ||
| private PathProviderImpl impl; | ||
|
|
||
| private interface PathProviderImpl { |
There was a problem hiding this comment.
nit: *Impl is usually used for the implementation of an interface.
There was a problem hiding this comment.
Added docstring to clarify its purpose.
| void getApplicationSupportDirectory(@NonNull Result result); | ||
| } | ||
|
|
||
| private class PathProviderPlatformThread implements PathProviderImpl { |
There was a problem hiding this comment.
If this is the shim, could it have a name that is easier to map to the reason why this class exists?
There was a problem hiding this comment.
Added docstring to clarify its purpose. I couldn't pack all that information into a name. This is something that exists temporarily so between the documentation and the good enough name, hopefully that's satisfactory.
gaaclarke
force-pushed
the
path_provider_background
branch
from
fbf02ac to
9705580
Compare
gaaclarke
added
the
waiting for tree to go green
|
|
||
| public PathProviderPlugin() {} | ||
|
|
||
| private void setup(BinaryMessenger messenger, Context context) { |
There was a problem hiding this comment.
Please remember when naming methods that "setup" is a noun, and "set up" is a verb, so a method (which should be named with a verb) should be called "setUp". I'm aware that Pigeon has this issue, but we shouldn't spread it beyond there.
* master: [webview_flutter] Deprecate evaluateJavascript in favour of runJavaScript and runJavaScriptForResult. (flutter#4403) [webview_flutter] Add interface methods to load local files and HTML strings. (flutter#4446) [ci] add pedantic dep to new in_app_purchase pkgs (flutter#4471) [ci] Remove unused dep from file_selector. (flutter#4468) [ci] update build_runner dep in android_intent and file_selector example (flutter#4467) [webview_flutter] Add platform interface method `loadRequest`. (flutter#4450) Remove -Werror from deprecated plugin Android builds (flutter#4466) [webview_flutter] Update webview packages for Android and iOS to implement `runJavascript` and `runJavascriptReturningResult`. (flutter#4402) [camera] Fix CamcorderProfile Usages (flutter#4423) [google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` (flutter#4391) [in_app_purchase]IAP/platform interface add cancel status (flutter#4093) [image_picker] doc:readme image picker misprints (flutter#4421) Support Hybrid Composition through the GoogleMaps Widget (flutter#4082) [path_provider] started supporting background platform channels (flutter#4443) [device_info] started using Background Platform Channels (flutter#4456) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter/pubspec.yaml
…ter#4443) * [path_provider] started supporting background platform channels * added docstrings
…ter#4443) * [path_provider] started supporting background platform channels * added docstrings
Made
path_providerexecute on background platform channels when it is available instead of managing its own thread.b/203709390
issue: flutter/flutter#91635
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.