Skip to content

Conversation

@bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Apr 25, 2023

Android implementation of the file_selector package

Related Links:
flutter/plugins#6468
flutter/flutter#25659
Part of flutter/flutter#110098

Useful Resources:
https://developer.android.com/guide/topics/providers/document-provider
https://developer.android.com/training/data-storage/shared/documents-files

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

apply plugin: 'com.android.library'

android {
namespace 'io.flutter.plugins.file_selector_android'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartmorgan Is there a preference for the org of new Android implementations? This is the standard, but I think new ones should be dev.flutter.packages.<pluginname>.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 26, 2023

Choose a reason for hiding this comment

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

Yes, we should definitely use dev.flutter going forward. A while ago I fixed all the legacy references to io.flutter in the plugins repo, except for Android because changing namespaces is non-trivial so I punted on that part.

@bparrishMines
Copy link
Contributor Author

@stuartmorgan This is ready for another review. I was able to find the problem with integration tests on Android. We use different entry-point target files when we run the tests locally vs when we run it on FTL. I was able to get a proper run on FTL and locally by adding

/// Entry point for integration tests that require espresso.
void integrationTestMain() {
  enableFlutterDriverExtension();
  app.main();
}

to both example/lib/main.dart and example/integration_test/file_selector_android_test.dart. And setting the entry-point for DriverExtensionActivity to integrationTestMain.

I was thinking of landing this PR as is and following up with a fix to the tooling that will set the entry-point target file based on whether we are running the Dart integration tests vs the Java integration tests.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Just small small things, LGTM with those changes!

# Name/Organization <email address>

Google Inc.
The Chromium Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line can be removed.

// If the size is unknown, the value stored is null. But because an
// int can't be null, the behavior is implementation-specific,
// and unpredictable. So as
// a rule, check if it's null before assigning to an int. This will
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is pretty verbose, can we just remove these two sentences? Keeping just the first and last one seems like plenty of explanation.

}
}

if (bytes == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can only happen if size was null, so it would be simpler to just early return above if size is null, and then initialize bytes directly to a value instead of having it be possibly null.

String? initialDirectory,
String? confirmButtonText,
}) async {
assert(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 26, 2023
@auto-submit auto-submit bot merged commit b9935d1 into flutter:main Jun 26, 2023
@bparrishMines bparrishMines deleted the file_selector_android branch June 26, 2023 18:29
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2023
stuartmorgan-g pushed a commit to flutter/flutter that referenced this pull request Jun 27, 2023
flutter/packages@6b70804...f89ce02

2023-06-27 hansmuller@google.com Updated
rfw/test/material_widgets_test.dart for M3 (flutter/packages#4316)
2023-06-27 tarrinneal@gmail.com [shared_preferences] Adds new
`clearWithParameters` and `getAllWithParameters` methods to platform
interface. (flutter/packages#4261)
2023-06-26 engine-flutter-autoroll@skia.org Roll Flutter from
042c036 to 96a2c05 (60 revisions) (flutter/packages#4313)
2023-06-26 10687576+bparrishMines@users.noreply.github.com
[file_selector_android] Create initial Android implementation of the
file_selector package (flutter/packages#3814)
2023-06-26 49699333+dependabot[bot]@users.noreply.github.com
[in_app_pur]: Bump org.json:json from 20230227 to 20230618 in
/packages/in_app_purchase/in_app_purchase/example/android/app
(flutter/packages#4244)
2023-06-26 me@ghyeok.io [webview_flutter_wkwebview] Adds the
`isInspectable` to `WebKitWebViewController`. (flutter/packages#3984)
2023-06-26 stuartmorgan@google.com [ci] Remove jcenter from legacy
project (flutter/packages#4306)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert
to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: file_selector platform-android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants