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.

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

Merged
merged 19 commits into from
Jul 11, 2023
Merged

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Jul 5, 2023

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 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.

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.

@tjzel tjzel requested a review from piaskowyk July 6, 2023 13:52
@tjzel tjzel marked this pull request as ready for review July 6, 2023 13:52
scripts/cpplint.sh Outdated Show resolved Hide resolved
Common/cpp/JSLogger/JSLogger.h Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.h Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.h Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.h Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Outdated Show resolved Hide resolved
ios/native/NativeProxy.mm Outdated Show resolved Hide resolved
ios/native/NativeProxy.mm Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Outdated Show resolved Hide resolved
Copy link
Member

@tomekzaw tomekzaw left a 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.

Common/cpp/LayoutAnimations/LayoutAnimationsManager.cpp Outdated Show resolved Hide resolved
Common/cpp/LayoutAnimations/LayoutAnimationsManager.h Outdated Show resolved Hide resolved
Common/cpp/Tools/JSLogger.cpp Outdated Show resolved Hide resolved
@tjzel tjzel added this pull request to the merge queue Jul 11, 2023
Merged via the queue into main with commit c710190 Jul 11, 2023
@tjzel tjzel deleted the @tjzel/SET/find-duplicate-tags branch July 11, 2023 16:09
Latropos pushed a commit that referenced this pull request Jul 13, 2023
<!-- 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.
@andreialecu
Copy link
Contributor

andreialecu commented Aug 10, 2023

I'm seeing this warning without having duplicates.

The case is as follows:

Screen A
Screen B
Screen C

All 3 have the same image with the same sharedTransitionTag, navigating A->B then to C triggers this warning.

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.

@tjzel
Copy link
Collaborator Author

tjzel commented Aug 18, 2023

@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 (yarn add react-native-reanimated@nightly) and see if the errors persists? If yes, then feel free to open an issue with a reproduction 🙏.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants