-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: crashing message composer input [WPB-8727] #2988
fix: crashing message composer input [WPB-8727] #2988
Conversation
Test Results927 tests 927 ✅ 12m 30s ⏱️ Results for commit bf36f2f. ♻️ This comment has been updated with latest results. |
…age-composer-input-crash
Build 4687 failed. |
APKs built during tests are available here. Scroll down to Artifacts! |
Build 4688 failed. |
APKs built during tests are available here. Scroll down to Artifacts! |
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.
Excellent job 🎸 small comments about dimensions 💪
app/src/main/kotlin/com/wire/android/ui/common/textfield/WireTextField2.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/wire/android/ui/common/textfield/WireTextField2.kt
Outdated
Show resolved
Hide resolved
Build 4690 failed. |
Build 4691 failed. |
APKs built during tests are available here. Scroll down to Artifacts! |
…age-composer-input-crash
Build 4698 failed. |
APKs built during tests are available here. Scroll down to Artifacts! |
It is failing in CI because of
|
APKs built during tests are available here. Scroll down to Artifacts! |
Build 4709 succeeded. The build produced the following APK's: |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
The app is crashing sometimes when clicking on the message composer input.
Causes (Optional)
Changing
readOnly
parameter probably messes something up with the inner text field state so that it ends up withjava.lang.IllegalStateException: LayoutCoordinate operations are only valid when isAttached is true
.Solutions
Getting rid of
readOnly
is not possible because it was introduced some time ago to fix some other issues that we had with message composer and keyboard.New
BasicTextField2
resolves multiple issues that the previous generation of text input had, including this one.BasicTextField2
has been renamed to regularBasicTextField
in newest compose library; the difference is that new one takesTextFieldState
instead ofTextFieldValue
orString
.We decided to use this new version of text field for the message composer. In order to do that, a new hybrid
WireTextField2
is created which allows us to use the new generation of text input but also to still passTextFieldValue
andonValueChanged
callback to make this refactor as minimally invasive as possible. ThisWireTextField2
usesStateSyncingModifier
which is based on composeBasicTextFieldWithValueOnValueChangeSample
which allows to synchronise betweenTextFieldState
andTextFieldValue
withonValueChanged
callback.It's needed to update the
compose-foundation
library to1.7.0-alpha05
, from whichBasicTextField2
has been marked as stable and still olderBasicTextField
is available. In versions prior to chosen one, including current newest release version -1.6.7
,BasicTextField2
has some bad issues with the keyboard (https://issuetracker.google.com/issues/339171226) and interaction source (https://issuetracker.google.com/issues/327665606). To use this version,rememberRipple()
needs to be updated toripple()
andAnchoredDraggableState
also requiresdecayAnimationSpec
.Testing
How to Test
Open conversation screen and click on message composer input right after navigating, preferably on Graphene OS.
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.