Add method for external storage #28

natelust · 2017-05-09T19:00:14Z

Android supports accessing the "external" storage medium. Support
finding this path in flutter with a method call in path_provider

I am interested in writing a flutter app that will access some files a user may have put on their device manually (android). This necessitates looking up the path for the external storage. Though I could do the same thing in my own package, I figured it would be more useful to have this functionality in the path provider package so it can be more broadly used. Sadly this functionality is restricted to android only, as iOS does not let you escape the app sandbox. If there is some other mechanism you would prefer me to go through instead of just creating a pull request, please let me know.

googlebot · 2017-05-09T19:00:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

natelust · 2017-05-09T19:02:02Z

I signed it!

googlebot · 2017-05-09T19:02:04Z

CLAs look good, thanks!

mit-mit · 2017-05-11T08:17:07Z

packages/path-provider/lib/path_provider.dart

+
+  /// Path to a directory where the application may access top level storage
+  ///
+  /// On iOS, This function returns null as it is not possible to access outside


Sentences in comments are missing terminating .

Few typos in iOS sentence, change to:

/// On iOS, this function returns null, as it is not possible to access the /// file system outside the apps sandbox.

szakarias

Thank you very much for your interest! :-)

Please update the README.md with this new method and increase the version number (update puspec.yaml and CHANGELOG.md).

Also, it would be great if the example app could display the new method as well.

szakarias · 2017-05-11T08:18:30Z

packages/path-provider/lib/path_provider.dart

@@ -40,3 +40,16 @@ const _channel = const MethodChannel('plugins.flutter.io/path_provider');
      return null;
    return new Directory(path);
  }
+
+  /// Path to a directory where the application may access top level storage


Please add dot at the end.

szakarias · 2017-05-11T08:19:00Z

packages/path-provider/lib/path_provider.dart

+
+  /// Path to a directory where the application may access top level storage
+  ///
+  /// On iOS, This function returns null as it is not possible to access outside


On iOS, This -> On iOS, this

szakarias · 2017-05-11T08:19:19Z

packages/path-provider/lib/path_provider.dart

+  /// Path to a directory where the application may access top level storage
+  ///
+  /// On iOS, This function returns null as it is not possible to access outside
+  /// the apps sandbox


Again, a dot at the end of the sentence.

szakarias · 2017-05-11T08:20:30Z

packages/path-provider/lib/path_provider.dart

+  /// On iOS, This function returns null as it is not possible to access outside
+  /// the apps sandbox
+  ///
+  /// On Android this returns getExternalStorageDirectory


Again a dot at the end of the sentence.

szakarias · 2017-05-11T12:06:08Z

...path-provider/android/src/main/java/io/flutter/plugins/path_provider/PathProviderPlugin.java

@@ -46,4 +51,8 @@ private String getPathProviderTemporaryDirectory() {
  private String getPathProviderApplicationDocumentsDirectory() {
    return PathUtils.getDataDirectory(activity);
  }
+
+  private String getPathProviderStorageDirectory() {
+    return Environment.getExternalStorageDirectory().getAbsolutePath();


What is the use case for this? The documentation for getExternalDirectory() says not to use this directly but instead use getExternalStoragePublicDirectory(String) .

The use case I had in mind for this was to get the root of the available file system, such that a file browser could be created within an app. I wanted to create a toy audio book app that would allow the user to navigate to a directory and mark it as the location all audio books could be found under. The latter API call you bring up is not quite targeted for this use case, as it expects you to specify what type of media you are interested in (i.e. for finding the location to save pictures). In my case the user may have located their audio books at any place accessible on the external storage.

szakarias · 2017-05-11T12:16:12Z

packages/path-provider/lib/path_provider.dart

+  /// the apps sandbox
+  ///
+  /// On Android this returns getExternalStorageDirectory
+  Future<Directory> getExternalStorageDirectory() async {


On iOS, this will result in a MissingPluginException since there is no handling of a method with that name. To avoid this, you could check the OS for iOS here and throw a UnsupportedError. Remember to document also, that the OS needs to be determined before calling this method.

I added a check for iOS and documented in the comments above the function

natelust · 2017-05-11T18:44:02Z

I have added to the example to make use of this new function, but I unfortunately could not test the example, as it seems to be broken by a change in flutter itself (most likely due to moving this stuff out to stand alone). I followed the structure of the existing example, so I don't expect any issues. I would be happy to look into fixing the example as a new small project.

I appreciate you considering this pull request. I am quite new to mobile app development. (I am coming from a background of python/c++ for high performance computing in astrophysics). Flutter is the first framework that has gotten my genuinely interested in the mobile ecosystem. I really like the design choices and am enjoying Dart as a language. I appreciate the opportunity to contribute to this project.

natelust · 2017-05-11T18:49:23Z

Also, I don't know the etiquette on this project in regards to cleaning up commits, and commit history in general. Is it advisable to squash the commits down like I did?

szakarias

We are very happy to get contributions so thanks for taking you time to do so.

Please remember to compile and run the code before submitting. I was able to run the example by fixing the compile errors :-)

If you are not using IntelliJ as your IDE, you could consider it. We recommend using an IntelliJ IDE with Flutter for code completion, inline error checking, and visual debugging features.

You don't need to squash the commits here as you can squash when you merge the PR. Also it makes it much easier to review the changes if you keep separate commits when you address comments. :-)

szakarias · 2017-05-15T10:18:31Z

packages/path-provider/example/lib/main.dart

@@ -105,6 +113,24 @@ class _MyHomePageState extends State<MyHomePage> {
              child: new FutureBuilder<Directory>(
                  future: _appDocumentsDirectory, builder: _buildDirectory),
            ),
+            new Cloumn(


Cloumn -> Column

szakarias · 2017-05-15T10:19:30Z

packages/path-provider/example/lib/main.dart

+            new Cloumn(
+              children : <Widget>[
+                new Padding(
+                  padding: const EdgeInserts.add(16.0),


EdgeInserts.add(16.0) -> EdgeInsets.all(16.0)

szakarias · 2017-05-15T10:20:41Z

packages/path-provider/example/lib/main.dart

+                new Padding(
+                  padding: const EdgeInserts.add(16.0),
+                  child: new RaisedButton(
+                    child: const Test('${Platform.operatingSystem != "ios" ?


const Test -> New Text

You cannot have const here as the expression is not constant.

szakarias · 2017-05-15T10:23:34Z

packages/path-provider/example/lib/main.dart

+                  child: new RaisedButton(
+                    child: const Test('${Platform.operatingSystem != "ios" ?
+                                      "Get External Storage Directory" :
+                                      "External Directories are unavailable " +


Directories -> directories

szakarias · 2017-05-15T10:24:18Z

packages/path-provider/example/lib/main.dart

+                                      "Get External Storage Directory" :
+                                      "External Directories are unavailable " +
+                                      "on iOS"}'),
+                    onPressed: _requestExternalStorageDirectory,


Let onPressed be null on iOS. This disables the button.

szakarias · 2017-05-15T10:26:05Z

packages/path-provider/example/lib/main.dart

@@ -65,6 +66,13 @@ class _MyHomePageState extends State<MyHomePage> {
    });
  }

+  void _requestExternalStorageDirectory() {
+    setState(() {
+      if (Platform.operatingSystem != "ios")


Please check the OS with Platform.isIOS instead, here and below as well.

szakarias · 2017-05-15T10:27:08Z

packages/path-provider/CHANGELOG.md

@@ -1,3 +1,7 @@
+## [0.2.1] - 2017-05-11


Once we reach the final version this date should be updated accordingly.

szakarias · 2017-05-15T10:28:52Z

packages/path-provider/CHANGELOG.md

@@ -1,3 +1,7 @@
+## [0.2.1] - 2017-05-11
+
+* Add function to determine external storage directory


Missing terminating .

szakarias · 2017-05-15T10:36:18Z

packages/path-provider/lib/path_provider.dart

+  /// On Android this returns getExternalStorageDirectory.
+  Future<Directory> getExternalStorageDirectory() async {
+    if (Platform.operatingSystem == "ios")
+      throw new UnimplementedError("Functionality not available on iOS");


Please use UnsupportedError instead.

natelust · 2017-05-15T15:42:05Z

I have addressed the issues mentioned above (except updating the final date), and rebased my fork onto master. I must say I am actually quite embarrassed at some of the mistakes I had. Previously I had assumed the build failures I was seeing in the platform specific libraries were related to API changes, but it was my own silly bugs causing it to fail. I must spend more time looking over things in the future. You have been quite patient in working with me, and I thank you for that. I am sorry that this has caused so much work for you. I have two commits related to the changes requested, which can be squashed prior to merging in whatever manor you prefer. Please let me know if there is anything else to be done.

szakarias

LGTM when the empty lines are removed.

szakarias · 2017-05-16T09:23:22Z

...path-provider/android/src/main/java/io/flutter/plugins/path_provider/PathProviderPlugin.java

 import io.flutter.plugin.common.MethodCall;
 import io.flutter.plugin.common.MethodChannel;
 import io.flutter.plugin.common.MethodChannel.MethodCallHandler;
 import io.flutter.plugin.common.MethodChannel.Result;
 import io.flutter.plugin.common.PluginRegistry.Registrar;
 import io.flutter.util.PathUtils;

+
+


Remove empty lines

Should I squash down to one commit?

You can use the 'Squash and merge' button, and remove the extra commit messages.

I have no write access to the github repository, so I can not actually do the merge myself. I have squashed all the commits on my fork, so it can be merged at any point.

Android supports accessing the "external" storage medium. Support finding this path in flutter with a method call in path_provider

mit-mit · 2017-05-16T14:50:51Z

Yup, we will handle the merge. Thanks so much for contributing to Flutter, @natelust !

FAILURE: Build failed with an exception. * Where: Build file 'C:\Users\srburton\AppData\Roaming\Pub\Cache\hosted\pub.dartlang.org\shared_preferences_macos-0.0.1+6\android\build.gradle' line: 22 * What went wrong: A problem occurred evaluating root project 'shared_preferences_macos'. > Failed to apply plugin [id 'com.android.library'] > Minimum supported Gradle version is 5.4.1. Current version is 5.1.1. If using the gradle wrapper, try editing the distributionUrl in C:\Users\srburton\AppData\Roaming\Pub\Cache\hosted\pub.dartlang.org\shared_preferences_macos-0.0.1+6\android\gradle\wrapper\gradle-wrapper.properties to gradle-5.4.1-all.zip * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights. * Get more help at https://help.gradle.org BUILD FAILED in 1s [ +1 ms] Running Gradle task 'assembleAarRelease'... (completed in 2,0s) [ ] FAILURE: Build failed with an exception. * Where: Build file 'C:\Users\srburton\AppData\Roaming\Pub\Cache\hosted\pub.dartlang.org\shared_preferences_macos-0.0.1+6\android\build.gradle' line: 22 * What went wrong: A problem occurred evaluating root project 'shared_preferences_macos'. > Failed to apply plugin [id 'com.android.library'] > Minimum supported Gradle version is 5.4.1. Current version is 5.1.1. If using the gradle wrapper, try editing the distributionUrl in C:\Users\srburton\AppData\Roaming\Pub\Cache\hosted\pub.dartlang.org\shared_preferences_macos-0.0.1+6\android\gradle\wrapper\gradle-wrapper.properties to gradle-5.4.1-all.zip * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights. * Get more help at https://help.gradle.org BUILD FAILED in 1s [ +5 ms] "flutter apk" took 179.497ms. The plugin shared_preferences_macos could not be built due to the issue above. #0 throwToolExit (package:flutter_tools/src/base/common.dart:28:3) flutter#1 buildPluginsAsAar (package:flutter_tools/src/android/gradle.dart:726:7) flutter#2 _asyncErrorWrapperHelper.<anonymous closure> (dart:async-patch/async_patch.dart:80:45) #3 _rootRunBinary (dart:async/zone.dart:1146:38) flutter#4 _CustomZone.runBinary (dart:async/zone.dart:1039:19) flutter#5 _FutureListener.handleError (dart:async/future_impl.dart:153:20) flutter#6 Future._propagateToListeners.handleError (dart:async/future_impl.dart:692:47) flutter#7 Future._propagateToListeners (dart:async/future_impl.dart:713:24) flutter#8 Future._completeError (dart:async/future_impl.dart:532:5) flutter#9 _AsyncAwaitCompleter.completeError (dart:async-patch/async_patch.dart:38:15) flutter#10 buildGradleAar (package:flutter_tools/src/android/gradle.dart) flutter#11 _asyncThenWrapperHelper.<anonymous closure> (dart:async-patch/async_patch.dart:73:64) flutter#12 _rootRunUnary (dart:async/zone.dart:1134:38) flutter#13 _CustomZone.runUnary (dart:async/zone.dart:1031:19) flutter#14 _FutureListener.handleValue (dart:async/future_impl.dart:139:18) flutter#15 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:680:45) flutter#16 Future._propagateToListeners (dart:async/future_impl.dart:709:32) #17 Future._completeWithValue (dart:async/future_impl.dart:524:5) flutter#18 _AsyncAwaitCompleter.complete (dart:async-patch/async_patch.dart:32:15) flutter#19 _completeOnAsyncReturn (dart:async-patch/async_patch.dart:290:13) flutter#20 _DefaultProcessUtils.run (package:flutter_tools/src/base/process.dart) flutter#21 _asyncThenWrapperHelper.<anonymous closure> (dart:async-patch/async_patch.dart:73:64) flutter#22 _rootRunUnary (dart:async/zone.dart:1134:38) flutter#23 _CustomZone.runUnary (dart:async/zone.dart:1031:19) flutter#24 _FutureListener.handleValue (dart:async/future_impl.dart:139:18) flutter#25 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:680:45) flutter#26 Future._propagateToListeners (dart:async/future_impl.dart:709:32) flutter#27 Future._completeWithValue (dart:async/future_impl.dart:524:5) flutter#28 Future.wait.<anonymous closure> (dart:async/future.dart:400:22) flutter#29 _rootRunUnary (dart:async/zone.dart:1134:38) flutter#30 _CustomZone.runUnary (dart:async/zone.dart:1031:19) flutter#31 _FutureListener.handleValue (dart:async/future_impl.dart:139:18) flutter#32 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:680:45) flutter#33 Future._propagateToListeners (dart:async/future_impl.dart:709:32) flutter#34 Future._completeWithValue (dart:async/future_impl.dart:524:5) flutter#35 Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:554:7) flutter#36 _rootRun (dart:async/zone.dart:1126:13) flutter#37 _CustomZone.run (dart:async/zone.dart:1023:19) flutter#38 _CustomZone.runGuarded (dart:async/zone.dart:925:7) flutter#39 _CustomZone.bindCallbackGuarded.<anonymous closure> (dart:async/zone.dart:965:23) flutter#40 _microtaskLoop (dart:async/schedule_microtask.dart:43:21) flutter#41 _startMicrotaskLoop (dart:async/schedule_microtask.dart:52:5) flutter#42 _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:118:13) flutter#43 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:175:5)

[firebase_remote_config] Fix Future already completed RemoteConfig.instance

mit-mit requested a review from szakarias May 11, 2017 08:11

mit-mit reviewed May 11, 2017

View reviewed changes

szakarias suggested changes May 11, 2017

View reviewed changes

natelust force-pushed the master branch 4 times, most recently from f390673 to 282d8d2 Compare May 11, 2017 18:31

szakarias suggested changes May 15, 2017

View reviewed changes

natelust force-pushed the master branch from 8caf5dc to 33b2a6c Compare May 15, 2017 15:35

szakarias approved these changes May 16, 2017

View reviewed changes

Add method for external storage

487ce83

Android supports accessing the "external" storage medium. Support finding this path in flutter with a method call in path_provider

natelust force-pushed the master branch from 33b2a6c to 487ce83 Compare May 16, 2017 13:48

mit-mit merged commit 91d6430 into flutter:master May 16, 2017

srburton mentioned this pull request Feb 21, 2020

[FIX] - Gradle #2546

Closed

13 tasks

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

Merge pull request flutter#28 from collinjackson/flutter#1966

08dad0c

[firebase_remote_config] Fix Future already completed RemoteConfig.instance

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for external storage #28

Add method for external storage #28

natelust commented May 9, 2017

googlebot commented May 9, 2017

natelust commented May 9, 2017

googlebot commented May 9, 2017

mit-mit May 11, 2017

szakarias left a comment

szakarias May 11, 2017

szakarias May 11, 2017

szakarias May 11, 2017

szakarias May 11, 2017

szakarias May 11, 2017

natelust May 11, 2017

szakarias May 11, 2017 •

edited

Loading

natelust May 11, 2017

natelust commented May 11, 2017

natelust commented May 11, 2017

szakarias left a comment •

edited

Loading

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

szakarias May 15, 2017

natelust commented May 15, 2017

szakarias left a comment

szakarias May 16, 2017

natelust May 16, 2017

szakarias May 16, 2017

natelust May 16, 2017

mit-mit commented May 16, 2017

		@@ -1,3 +1,7 @@
		## [0.2.1] - 2017-05-11

		* Add function to determine external storage directory

Add method for external storage #28

Add method for external storage #28

Conversation

natelust commented May 9, 2017

googlebot commented May 9, 2017

natelust commented May 9, 2017

googlebot commented May 9, 2017

Choose a reason for hiding this comment

szakarias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szakarias May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natelust commented May 11, 2017

natelust commented May 11, 2017

szakarias left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natelust commented May 15, 2017

szakarias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mit-mit commented May 16, 2017

szakarias May 11, 2017 •

edited

Loading

szakarias left a comment •

edited

Loading