Skip to content

[path_provider] added android support for getDownloadsDirectory #4317

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/path_provider/path_provider_android/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
## 2.0.28

* Updates minimum supported SDK version to Flutter 3.3/Dart 2.18.
* Adds support for getDownloadsDirectory in Android

## 2.0.27

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ class PathProviderAndroid extends PathProviderPlatform {
}

@override
Future<String?> getDownloadsPath() {
throw UnsupportedError('getDownloadsPath is not supported on Android');
Future<String?> getDownloadsPath() async {
final List<String>? paths =
await getExternalStoragePaths(type: StorageDirectory.downloads);
if (paths != null && paths.isNotEmpty) {
return paths.first;
}
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one I've updated to catch the stateError scenario when externalStoragePaths returns empty list.

}
4 changes: 3 additions & 1 deletion packages/path_provider/path_provider_android/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: path_provider_android
description: Android implementation of the path_provider plugin.
repository: https://github.com/flutter/packages/tree/main/packages/path_provider/path_provider_android
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+path_provider%22
version: 2.0.27
version: 2.0.28

environment:
sdk: ">=2.18.0 <4.0.0"
Expand All @@ -23,9 +23,11 @@ dependencies:
path_provider_platform_interface: ^2.0.1

dev_dependencies:
build_runner: ^2.3.2
flutter_test:
sdk: flutter
integration_test:
sdk: flutter
mockito: 5.4.1
pigeon: ^9.2.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartmorgan I assume these should be removed before being merged?

test: ^1.16.0
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
// found in the LICENSE file.

import 'package:flutter_test/flutter_test.dart';
import 'package:mockito/annotations.dart';
import 'package:mockito/mockito.dart';
import 'package:path_provider_android/messages.g.dart' as messages;
import 'package:path_provider_android/path_provider_android.dart';
import 'package:path_provider_platform_interface/path_provider_platform_interface.dart';
import 'messages_test.g.dart';
import 'path_provider_android_test.mocks.dart';

const String kTemporaryPath = 'temporaryPath';
const String kApplicationSupportPath = 'applicationSupportPath';
Expand Down Expand Up @@ -37,14 +40,17 @@ class _Api implements TestPathProviderApi {
String? getTemporaryPath() => kTemporaryPath;
}

@GenerateMocks(<Type>[TestPathProviderApi])
void main() {
TestWidgetsFlutterBinding.ensureInitialized();

group('PathProviderAndroid', () {
late MockTestPathProviderApi mockApi;
late PathProviderAndroid pathProvider;

setUp(() async {
pathProvider = PathProviderAndroid();
mockApi = MockTestPathProviderApi();
TestPathProviderApi.setup(_Api());
});

Expand Down Expand Up @@ -89,13 +95,20 @@ void main() {
});
} // end of for-loop

test('getDownloadsPath fails', () async {
try {
await pathProvider.getDownloadsPath();
fail('should throw UnsupportedError');
} catch (e) {
expect(e, isUnsupportedError);
}
test('getDownloadsPath succeeds', () async {
when(mockApi.getExternalStoragePaths(messages.StorageDirectory.downloads))
.thenReturn(<String?>[kDownloadsPath]);
final List<String?> path =
mockApi.getExternalStoragePaths(messages.StorageDirectory.downloads);
expect(path.first, kDownloadsPath);
});

test('getDownloadsPath null', () async {
when(mockApi.getExternalStoragePaths(messages.StorageDirectory.downloads))
.thenReturn(<String?>[null]);
final List<String?> path =
mockApi.getExternalStoragePaths(messages.StorageDirectory.downloads);
expect(path.first, null);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can check this one if its going in the right direction? I'm trying to add a case for the null externalStoragePaths, but if I override it to empty list then it will affect other tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll either have to modify kExternalStoragePaths and set it back or override _Api or TestPathProviderApi with the desired behavior and set your test to use that implementation like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the null test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems okay to me but perhaps you can move this test to a different group and set up the TestPathProviderApi with TestPathProviderApi.setup(_ApiNull()) just for that group of tests, so that it doesn't conflict with any tests in this group?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had an answer to the original question that I didn't realize I'd never actually published, and was still Pending. It would be much simpler to make the test implementation return configurable values, rather than have two different implementations and test groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the getDownloads test to use mockito, so we can configure values for normal path and null path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Stuart was suggesting making _Api return a configurable value (i.e. modifying kExternalStoragePaths) versus mocking TestPathProviderApi, but please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was suggesting making the values returned by _Api configurable rather than hard-coded constants. Usually we do that by making the return values public fields of the fake implementation.

I'm okay with mockito instead (although I think it's overkill here), but not with having two different ways of structuring these tests; if you really want to use mockito then all of the tests need to be rewritten to use mockito and _Api removed.

});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Mocks generated by Mockito 5.4.1 from annotations
// in path_provider_android/test/path_provider_android_test.dart.
// Do not manually edit this file.

// @dart=2.19

// ignore_for_file: no_leading_underscores_for_library_prefixes
import 'package:mockito/mockito.dart' as _i1;
import 'package:path_provider_android/messages.g.dart' as _i3;

import 'messages_test.g.dart' as _i2;

// ignore_for_file: type=lint
// ignore_for_file: avoid_redundant_argument_values
// ignore_for_file: avoid_setters_without_getters
// ignore_for_file: comment_references
// ignore_for_file: implementation_imports
// ignore_for_file: invalid_use_of_visible_for_testing_member
// ignore_for_file: prefer_const_constructors
// ignore_for_file: unnecessary_parenthesis
// ignore_for_file: camel_case_types
// ignore_for_file: subtype_of_sealed_class

/// A class which mocks [TestPathProviderApi].
///
/// See the documentation for Mockito's code generation for more information.
class MockTestPathProviderApi extends _i1.Mock
implements _i2.TestPathProviderApi {
MockTestPathProviderApi() {
_i1.throwOnMissingStub(this);
}

@override
List<String?> getExternalCachePaths() => (super.noSuchMethod(
Invocation.method(
#getExternalCachePaths,
[],
),
returnValue: <String?>[],
) as List<String?>);
@override
List<String?> getExternalStoragePaths(_i3.StorageDirectory? directory) =>
(super.noSuchMethod(
Invocation.method(
#getExternalStoragePaths,
[directory],
),
returnValue: <String?>[],
) as List<String?>);
}