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

[camera] Fix IllegalStateException being thrown in Android implementation when switching activities. #4319

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

BeMacized
Copy link
Contributor

@BeMacized BeMacized commented Sep 7, 2021

This PR removes the lifecycle hooks from the Android implementation. I have a feeling these were added by accident during the recent refactor, as the example app already puts this responsibility on the user of the plugin.
This is also described in the readme: https://github.com/flutter/plugins/tree/master/packages/camera/camera#handling-lifecycle-states

This should fix the issue with the IllegalStateException being thrown when switching to a different activity, for example when using camera in combination with image_picker, as can be seen in the issue linked below:

Relevant issues:

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]. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the plugin 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].
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
    • No new public members to document, or existing ones to update.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

packages/camera/camera/CHANGELOG.md Show resolved Hide resolved
backgroundHandlerThread.start();
} catch (IllegalThreadStateException e) {
// Ignore exception in case the thread has already started.
if (backgroundHandlerThread == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a lifecycle test to avoid re-involved with the same issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a unit-test which tests for the if statement. However I am not sure how we could add a test to verify that we don't add implementation of listening to lifecycle events in the future.

Do you have some insights on how this could be accomplished?

Copy link
Member

Choose a reason for hiding this comment

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

Add an observer WidgetsBindingObserver in a Flutter App, then override the didChangeAppLifecycleState might be a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Capturing from a separate discussion, verifying that the core classes (Camera, CameraPlugin) are not instanceof LifecycleObserver seems like a possible approach, even if it's not foolproof (e.g., a composed object could implement it)

Copy link

Choose a reason for hiding this comment

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

alternatively, it could just return earlier.

Copy link

Choose a reason for hiding this comment

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

if (backgroundHandlerThread  != null) {
  return;
}

@google-cla
Copy link

google-cla bot commented Sep 23, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Sep 23, 2021
@google-cla
Copy link

google-cla bot commented Sep 23, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@BeMacized
Copy link
Contributor Author

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Sep 23, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@mvanbeusekom
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Sep 23, 2021
@stuartmorgan
Copy link
Contributor

@blasten @bparrishMines Could one of you review this? It fixes a P2 regression.

@@ -1,3 +1,8 @@

Copy link

Choose a reason for hiding this comment

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

no need for this line

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

lgtm

BeMacized and others added 6 commits September 27, 2021 23:36
According to feedback from @blasten on PR, the following changes are
made:
- Removed an obsolete empty line in the CHANGELOG.md;
- Return early from the `startBackgroundThread` function when the
  `backgroundHandlerThread is not `null`. This simplifies the method
making it easier to read and understand.
@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 28, 2021
@fluttergithubbot fluttergithubbot merged commit 8a71e0e into flutter:master Sep 28, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2021
@mvanbeusekom mvanbeusekom deleted the issue/89352 branch September 28, 2021 07:19
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 28, 2021
NickalasB added a commit to NickalasB/plugins that referenced this pull request Sep 30, 2021
* master:
  [google_maps_flutter] Add Marker drag events (flutter#2838)
  [flutter_plugin_tools] Validate pubspec description (flutter#4396)
  Add file_selector to the repo list (flutter#4395)
  [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363)
  [google_maps_flutter_web] Add Marker drag events (flutter#4385)
  [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394)
  Handle `PurchaseStatus.restored` correctly in example. (flutter#4393)
  Handle restored purchases in iOS example app (flutter#4392)
  [file_selector] Remove custom analysis options (flutter#4382)
  [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373)
  Fixed _CastError when running example App (flutter#4390)
  [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370)
  Load navigation controls immediately. (flutter#4377)
  [camera] Fix IllegalStateException being thrown in Android implementation when switching activities. (flutter#4319)

# Conflicts:
#	packages/webview_flutter/webview_flutter/CHANGELOG.md
#	packages/webview_flutter/webview_flutter_android/CHANGELOG.md
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
sagargpt pushed a commit to nurture-farm/plugins that referenced this pull request Dec 6, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: camera platform-android waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
6 participants