Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@audkar
Copy link
Contributor

@audkar audkar commented Oct 4, 2019

Description

Migrate android path_provider plugin to the new embedding method.

Related Issues

flutter/flutter#41844

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@audkar
Copy link
Contributor Author

audkar commented Oct 4, 2019

Currently existing drive test fails because of exception (before my changes):

Expected: throws <Instance of 'UnsupportedError'>
    Actual: <Instance of 'Future<Directory>'>
    Which: threw MissingPluginException:<MissingPluginException(No implementation 
                found for method getLibraryDirectory on channel plugins.flutter.io/path_provider)>

this test expects UnsupportedError. Problem is that thrown error is different. UnsupportedError vs MissingPluginException. Is this changed how platform is dealing with result.notImplemented()?

@audkar
Copy link
Contributor Author

audkar commented Oct 5, 2019

Also drive tests are currently executed only for old Plugin implementation. Is there any way to make them run on a new plugin also? (I tested them manually by switching MainActivity in AndroidManifest) but what permanent solution could be used to test both plugin implementations with drive test?

@audkar audkar requested a review from collinjackson as a code owner October 14, 2019 07:06
@collinjackson
Copy link
Contributor

You can include an EmbeddingV1ActivityTest.java for the old embedder and MainActivityTest.java for the new embedder and CI will run both of them. See #2160 for an example.

@audkar audkar force-pushed the path_provider_migrate branch from 8ff2b11 to b746aed Compare October 16, 2019 10:55
@audkar
Copy link
Contributor Author

audkar commented Oct 16, 2019

@collinjackson when I add e2e tests I get:

/home/audrius/.pub-cache/hosted/pub.dartlang.org/e2e-0.2.1/android/src/main/java/dev/flutter/plugins/e2e/E2EPlugin.java:35: error: cannot access LifecycleOwner
        binding.getApplicationContext(), binding.getFlutterEngine().getDartExecutor());
               ^
  class file for android.arch.lifecycle.LifecycleOwner not found

Is current e2e version compatible with stable channel and Lifecycle dependency problem?

@audkar audkar changed the title [WIP][path_provider] Migrate to the new embedding [path_provider] support the v2 embedder Oct 16, 2019
}

// TODO(amirh): Remove this hack once androidx.lifecycle is included on stable. https://github.com/flutter/flutter/issues/42348
afterEvaluate {
Copy link
Member

Choose a reason for hiding this comment

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

with this temporary tweak, the error you were pointing at should be gone on stable. Would you like to sync your PR up to master and try the tests again?

@audkar
Copy link
Contributor Author

audkar commented Nov 12, 2019

Currently I have no time to work on this PR. Feel free to close or modify it.

@cyanglaz
Copy link
Contributor

v2 embedding has been supported in path provider from #2284. I'm closing this PR

@cyanglaz cyanglaz closed this May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants