refactor(Android): replace inset listeners on decor with multiple insets listeners on screen #2938
+145
−28
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maciekstosio
commented
May 27, 2025
android/src/main/java/com/swmansion/rnscreens/InsetsObserver.kt
Outdated
Show resolved
Hide resolved
## Description Review notes for #2938. Please note that I've described each change in commit details. ## Changes - **refactor insets managment for sheets** - **Update android/src/main/java/com/swmansion/rnscreens/InsetsObserver.kt** - **Remove secondary constructor & add context as first parameter** - **Update callsite** - **Make use of ctor param instead of introducing additional variable** - **Rename systemListener -> externalListener** - **Add a comment explaining semantics of `Screen.setOnApplyWindowInsetsListener`** - **Renmae setOnApplyWindowListener -> setExternalOnApplyWindowInsetsListener** - **Rename rollingInsets -> currentInsets & assert nullability** - **Add comment explaining what is being returned there** ## Test code and steps to reproduce <!-- Please include code that can be used to test this change and short description how this example should work. This snippet should be as minimal as possible and ready to be pasted into editor (don't exclude exports or remove "not important" parts of reproduction example) --> ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes
|
Found a bug on API 29, that does not happen on API 35. Dunno on what version it starts. I also don't know whether this is a regression or it also occurs in main. Will check this. Video recorded on Screen.Recording.2025-05-28.at.12.05.19.movCaution This works fine on main. |
|
Found another bug on API 29, that does not happen on API 35 Screen.Recording.2025-05-28.at.12.06.46.movVideo recorded on Caution This works on main. |
kkafar
requested changes
May 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently we have one singleton proxy listener that is attached to decor and we use that to listen for keyboard events. That sometimes may leads to problems, that some underlying view will consume insets and we won't get notified about this. This PR fixes is by allowing to add multiple events listeners on screen and using it for foresheets.
We keep the systemListener and ownListeeners separately, as the first one can be attached temporarily but we wouldn't be able to distinguish it and our own. Additional "own" listeners suppose to be attache through the insetObserver
Test code and steps to reproduce
Checklist