-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Add JSLogger class and duplicate SET tags #4675
Conversation
android/src/fabric/java/com/swmansion/reanimated/NativeProxy.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor suggestions but the PR is mergeleable.
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> This PR adds functionality of detecting duplicate Shared Element Transitions tags on the same screen - this is obviously error prone and in current implementation just the first matching tag will be included in transition. To achieve this I've added `JSLogger` class that - currently, it can be expanded - allows to use `console.warn()` function from UI thread in CPP code. I also wrapped all of the code into `ifdef DEBUG` since this behavior shouldn't be present in production build. Since this is my first native PR it might be extraneous. I've added a new _funny_ example that has duplicate shared tags on screens 3 and 4. They are correctly detected on iOS/android simulator and respective physical devices and they are also correctly not detected on Release build. There's an error related to restoring the snapshot. See how transitions behave between screen 1 and screen 2. Especially on android. Also check what happens if you apply transform to the photos themselves, not their container. You can fiddle with image sizes (e.g. make them wider than higher) to notice offset issues.
I'm seeing this warning without having duplicates. The case is as follows: Screen A All 3 have the same image with the same Is this expected? Edit: Actually I think that should be supported, after checking the Example. However, I'm getting this warning without any clear explanation as to why. I wonder if it's a race condition, as the image re-renders quite a few times during mount, as the components hydrate. |
@andreialecu I have modified the DuplicateTags example (I think it's the one you mentioned) to just have a single picture with the same tag on every screen and I don't ever get the warning - might be that you have a lot more complex case. Could you try with current nightly version of Reanimated ( |
Summary
This PR adds functionality of detecting duplicate Shared Element Transitions tags on the same screen - this is obviously error prone and in current implementation just the first matching tag will be included in transition.
To achieve this I've added
JSLogger
class that - currently, it can be expanded - allows to useconsole.warn()
function from UI thread in CPP code.I also wrapped all of the code into
ifdef DEBUG
since this behavior shouldn't be present in production build. Since this is my first native PR it might be extraneous.Test plan
I've added a new funny example that has duplicate shared tags on screens 3 and 4. They are correctly detected on iOS/android simulator and respective physical devices and they are also correctly not detected on Release build.
Note
There's an error related to restoring the snapshot. See how transitions behave between screen 1 and screen 2. Especially on android. Also check what happens if you apply transform to the photos themselves, not their container. You can fiddle with image sizes (e.g. make them wider than higher) to notice offset issues.