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

fix(android): prevent shutting down camera when re-mounting children #1995

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

thymikee
Copy link
Contributor

Fixes #1311

There's a bug where re-mounting children without transparent background shuts down the camera on Android (adb on LG G2 shows that stopPreview from QCamera2HWI gets called, but I couldn't set up debugging to see what's calling it, help appreciated).

By accident I've found a workaround, which involves wrapping children in a transparent view (not sure why backgroundColor: 'transparent' plays role here but it does).

As a manual test I've added a previously buggy behavior to example app – pressing zoom in / zoom out will mount/unmount a Text node which is a direct child of RNCamera.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 13, 2018

I think we should fix the root cause of this issue instead of some temporary hack

@sibelius
Copy link
Collaborator

Does this happen on Expo câmera module as well?

I think we can use this hacky for now

@sibelius
Copy link
Collaborator

@sibelius
Copy link
Collaborator

I think we can try to solve this using react native lifecycles

https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/LifecycleEventListener.java#L27

onHostResume
onHostPause
onHostDestroy

my guess is that onPause or onResume is not being called properly by react-native

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 14, 2018

by adding this code to onViewAdded the issue is also "solved". Anyway why is the view removed an readded? When you start zooming the first time this behaviour occures. 0 -> 0.1. However on further renders this issue is not present anymore.

   if (hasCameraPermissions()) {
      start();
    }

Maybe your solution is the right solution 😕

@thymikee
Copy link
Contributor Author

thymikee commented Dec 14, 2018

Anyway why is the view removed an readded?

Without this method overrriden the React injected UI (children) flash for a split of second and once the component mounts, the camera is put on top. So the solution is to remove and add views with 0 index, because that's supposedly how you can be sure about the view (camera) can be on the very bottom, and React UI is on still on top. Not sure why this happens, maybe because of how the camera screen lays out?
Here's reference to where it was added: #308

However on further renders this issue is not present anymore

The camera instance is still present and isCameraOpen reports false after the first children re-render (changing zoom from 0 to 0.1)

I'm not experienced in Android dev (just did some basics years ago), so it's all new to me and I'm just trying to get my head around it 😅

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 14, 2018

react-native-linear-gradient avoids passing in children into the NativeView

https://github.com/react-native-community/react-native-linear-gradient/blob/master/index.android.js#L84

https://github.com/react-native-community/react-native-linear-gradient/blob/master/index.ios.js

Other libraries from react-native-community also do not have this issue since they do not allow passing children to the "NativeView" and force users to utilize absolute positioning.

@thymikee
Copy link
Contributor Author

So, maybe we should follow this guidance? Should be possible to remove that hack from 2yrs ago altogether.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 14, 2018

@thymikee I agree.

I would prefer an implementation like this one: https://github.com/react-native-community/react-native-linear-gradient/blob/master/index.android.js#L84

@thymikee
Copy link
Contributor Author

Gonna work on it over the weekend then! Thanks for helping out

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 14, 2018

@thymikee Thank you for getting this finally fixed 🎉

@sibelius
Copy link
Collaborator

I prefer to avoid absolute position

why should we stop camera when changing children?

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 14, 2018

@sibelius Do you know any project that implemented passing children to the native component directly (without the issues we are experiencing)?

@sibelius
Copy link
Collaborator

we can check on github https://github.com/search?l=Java&q=ViewGroupManager&type=Code

maybe someone in the react native core could help us on this

cc @axe-fb @cpojer

@thymikee
Copy link
Contributor Author

@n1ru4l addressed your feedback, so it still works well and we got rid of a hacky behavior on Android.

@miracle2k
Copy link
Contributor

The pull request will fix this, but for completeness sake, I want to point to #1862 (comment) which I believe is the reason why the issue exists when children are added inside the native view.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 23, 2018

@sibelius Do you want to investigate more or accept this solution? I am fine with this "workaround".

@sibelius
Copy link
Collaborator

I’m fine with this workaround, we can accept a native solution later on if someone fixes it

@sibelius sibelius merged commit 5695f5d into react-native-camera:master Jan 8, 2019
@thymikee thymikee deleted the fix/1311 branch January 8, 2019 10:50
n1ru4l pushed a commit that referenced this pull request Jan 8, 2019
# [1.7.0](v1.6.6...v1.7.0) (2019-01-08)

### Bug Fixes

* **android:** prevent shutting down camera when re-mounting children ([#1995](#1995)) ([5695f5d](5695f5d))

### Features

* **tidelift:** add tidelift badge on readme ([#2038](#2038)) ([5768d30](5768d30))
@n1ru4l
Copy link
Collaborator

n1ru4l commented Jan 8, 2019

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@n1ru4l n1ru4l added the released label Jan 8, 2019
sunil-dev7 added a commit to sunil-dev7/react-native-camera that referenced this pull request Jun 11, 2021
# [1.7.0](react-native-camera/react-native-camera@v1.6.6...v1.7.0) (2019-01-08)

### Bug Fixes

* **android:** prevent shutting down camera when re-mounting children ([#1995](react-native-camera/react-native-camera#1995)) ([5695f5d](react-native-camera/react-native-camera@5695f5d))

### Features

* **tidelift:** add tidelift badge on readme ([#2038](react-native-camera/react-native-camera#2038)) ([5768d30](react-native-camera/react-native-camera@5768d30))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants