[path_provider] added android support for getDownloadsDirectory #4167
Conversation
| if (paths != null && paths.isNotEmpty) { | ||
| return paths.first; | ||
| } else { | ||
| throw UnsupportedError('getDownloadsPath is not supported on Android'); |
There was a problem hiding this comment.
Maybe change the error to something that references the empty path since it is now supported on Android?
| if (paths != null && paths.isNotEmpty) { | ||
| return paths.first; | ||
| } else { | ||
| throw UnsupportedError('getDownloadsPath is not supported on Android'); |
There was a problem hiding this comment.
UnsupportedError is incorrect here; the function is supported. This would kill someone's app (because it's an Error, which isn't meant to be caught) if the underlying API call failed, which is not a programming error by the caller.
Why isn't this just returning null?
There was a problem hiding this comment.
in hindsight, i was thinking to notify the user that downloads path is not found. but the function returns a nullable string and the main function getDownloadsDirectory also returns a nullable directory, hence returning null should be correct.
There was a problem hiding this comment.
in hindsight, i was thinking to notify the user that downloads path is not found.
In Dart, an Error is never the correct mechanism for that. Per the docs: "An Error object represents a program failure that the programmer should have avoided." A programmer cannot avoid Android not finding the download folder, so that's not a case that should ever be represented by an Error.
| } | ||
| test('getDownloadsPath succeeds', () async { | ||
| final String? path = await pathProvider.getDownloadsPath(); | ||
| expect(path, kExternalStoragePaths); |
There was a problem hiding this comment.
There are two code paths through the new method, success and failure. Each path needs a unit test.
There was a problem hiding this comment.
i've updated the new method to return a nullable path
There was a problem hiding this comment.
You still need a test for the case where it fails (e.g., to catch the fact that your current code will, incorrectly, throw if no external storage paths are returned).
There was a problem hiding this comment.
The current function now only returns a path, one code path, so I think 1 test is enough? (getDownloadsPath succeed)
Cause it no longer throws an error, hence in test we can't expect it to throw something.
There was a problem hiding this comment.
The current function now only returns a path, one code path, so I think 1 test is enough? (getDownloadsPath succeed)
As my comment above says, there is a bug in your current implementation, which is compelling evidence that the current test is insufficient.
Cause it no longer throws an error
Please test your code with an empty return array if you don't believe me that your code can still throw.
| @@ -81,7 +81,9 @@ class PathProviderAndroid extends PathProviderPlatform { | |||
| } | |||
|
|
|||
| @override | |||
| Future<String?> getDownloadsPath() { | |||
| throw UnsupportedError('getDownloadsPath is not supported on Android'); | |||
There was a problem hiding this comment.
removed incorrect throw error
Followed step 3. of this approach flutter/plugins#4559 (review)
Fixes flutter/flutter#93198
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.