Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(storage): move Storage to use Pigeon for platform channels #11521

Merged
merged 36 commits into from
Oct 22, 2023
Merged

feat(storage): move Storage to use Pigeon for platform channels #11521

merged 36 commits into from
Oct 22, 2023

Conversation

cynthiajoan
Copy link
Collaborator

@cynthiajoan cynthiajoan commented Aug 27, 2023 •
edited
Loading

Description

Replace this paragraph with a description of what this PR is doing. If you're modifying existing behavior, describe the existing behavior, how this PR is changing it, and what motivated the change.

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.

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. Updating the pubspec.yaml and changelogs is not required.

  • 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 (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • 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.
  • No, this is not a breaking change.

@cynthiajoan cynthiajoan marked this pull request as ready for review October 12, 2023 06:54
@Lyokone
Copy link
Contributor

Lyokone commented Oct 17, 2023

Hey @cynthiajoan, the code LGTM for the migration waiting for the tests to go green

russellwheatley
russellwheatley requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@russellwheatley russellwheatley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the PR, @cynthiajoan! I hope you don't mind, but I found some issue that ought to be addressed for updating. Also - in the FlutterFirebaseStorageTask.java, FlutterFirebaseStoragePlugin.java & TaskStateChannelStreamHandler.java, there are a number of warnings that probably should be fixed. For example, methods that are not used can be removed if no longer needed.

I'm going to look at the Apple side of things next, and see what might need addressing there 😄

.../android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java Outdated Show resolved Hide resolved
...android/src/main/java/io/flutter/plugins/firebase/storage/TaskStateChannelStreamHandler.java Outdated
Comment on lines 26 to 31
private static final Executor taskExecutor = Executors.newSingleThreadExecutor();
private Boolean destroyed = false;

private final Object pauseSyncObject = new Object();
private final Object resumeSyncObject = new Object();
private final Object cancelSyncObject = new Object();
Copy link
Member

Choose a reason for hiding this comment

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

These need removing, use the instances from the FlutterFirebaseStorageTask created. i.e pass them into the constructor for this class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, although I feel these sync object is actually not necessary in the new way.

...android/src/main/java/io/flutter/plugins/firebase/storage/TaskStateChannelStreamHandler.java Outdated Show resolved Hide resolved
...android/src/main/java/io/flutter/plugins/firebase/storage/TaskStateChannelStreamHandler.java Show resolved Hide resolved
...android/src/main/java/io/flutter/plugins/firebase/storage/TaskStateChannelStreamHandler.java Show resolved Hide resolved
.../android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java Show resolved Hide resolved
@russellwheatley
Copy link
Member

I think we should bring back these tests for this PR: https://github.com/firebase/flutterfire/blob/master/tests/integration_test/firebase_storage/task_e2e.dart

We skip because of how flakey they are on the emulator but worth checking for this as the internal implementation has now changed for tasks that pause/resume/cancel.

@cynthiajoan
Copy link
Collaborator Author

I think we should bring back these tests for this PR: https://github.com/firebase/flutterfire/blob/master/tests/integration_test/firebase_storage/task_e2e.dart

We skip because of how flakey they are on the emulator but worth checking for this as the internal implementation has now changed for tasks that pause/resume/cancel.

I brought part of the tests back, it's still flaky for some of them, I'm letting them running to see what the results would be.

russellwheatley
russellwheatley requested changes Oct 20, 2023
View reviewed changes
Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

I've made one comment here but other than that, LGTM

@cynthiajoan
Copy link
Collaborator Author

I've made one comment here but other than that, LGTM

Thanks @russellwheatley, addressed the comment.

@cynthiajoan cynthiajoan merged commit edddc1d into master Oct 22, 2023
18 checks passed
@cynthiajoan cynthiajoan deleted the feat/storage_pigeon branch October 22, 2023 21:26
LocLt-Mobile pushed a commit to guide-inc-org/guide-flutter_fire that referenced this pull request Oct 29, 2023
@cynthiajoan @a-maurice @LocLt-Mobile
feat(storage): move Storage to use Pigeon for platform channels (fire…
d9647b8 
…base#11521)

* pigeon template setup

* make storage macOS running

* method channel implementation update

* android compilable

* android emulator upload succeed

* add more task listener

* Get event channel working

* pause iOS for now

* update message.dart async

* Implement more Storage for iOS functionality

* make ios e2e test build

* more e2e test fix

* make e2e test pass (android)

* android e2e test

* make storage iOS pass e2e

* Add license header

* code format

* Update unit test, remove ones that's not work with pigeon

* add symbolic link for macos files

* make pigeon generated compatible with both iOS and macOS

* fix for analyze check

* add missing analyze fix

* more touch

* address part of review comments, majorly android ones

* format changes

* addressing the iOS feedback

* update with cleanup

* address the analyze check

* add retry to flaky pause/resume test

* clean up the android task destroy logic

* need to address the flakiness of this specific task

* format

---------

Co-authored-by: Cynthia Jiang <cynthiajiang@google.com>
Co-authored-by: a-maurice <amaurice@google.com>
@firebase firebase locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Lyokone Lyokone Lyokone approved these changes

@russellwheatley russellwheatley russellwheatley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cynthiajoan @Lyokone @russellwheatley @a-maurice