[Android] Add workarounds for ignoring state restoration and invalid line count #864
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
- Coverage 89.40% 89.35% -0.06%
==========================================
Files 118 86 -32
Lines 16845 15271 -1574
Branches 635 0 -635
==========================================
- Hits 15061 13646 -1415
+ Misses 1760 1625 -135
+ Partials 24 0 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Ignoring state restoration in Compose. - Invalid line count causing recompositions.
jmartinesp
force-pushed
the
fix/jme/android-crash-selection-on-view-restoration
branch
from
9a1ce4e to
3a78de1
Compare
jmartinesp
force-pushed
the
fix/jme/android-crash-selection-on-view-restoration
branch
from
3a78de1 to
cc467da
Compare
|
Can I get another review for this PR? I added tests for checking the text + selection restoration, as well as keeping the right line count. I also tested it on EXA and it seems to work fine. |
...y-compose/src/androidTest/java/io/element/android/wysiwyg/compose/RichTextEditorStateTest.kt
Outdated
Show resolved
Hide resolved
...y-compose/src/androidTest/java/io/element/android/wysiwyg/compose/RichTextEditorStateTest.kt
Outdated
Show resolved
Hide resolved
| // Set initial HTML and selection, state will be restored automatically in the view | ||
| if (editableText.isEmpty()) { | ||
| setHtml(state.internalHtml) | ||
| setSelection(state.selection.first, state.selection.second) | ||
| } |
There was a problem hiding this comment.
In EXA, the selection state is no longer restored when toggling the text formatting options:
Screen_recording_20231115_162355.webm
There was a problem hiding this comment.
You're right. I'm reverting this code! It's weird, because it contradicts some of my previous tests while debugging, but maybe fixing the previous bug also changed that behaviour.
There was a problem hiding this comment.
I've added an extra assertion for this case to help catch this in future: #868
There was a problem hiding this comment.
Oh cool, I was adding something similar but your tests are more thorough so let's use them instead 👍 .
There was a problem hiding this comment.
Actually, I accidentally uploaded some changes, but those also verify line count is not 1 by default, so maybe we can change the #868 PR to be based on these? I'll squash the commits anyway.
There was a problem hiding this comment.
Sure that makes sense - I can update #868 to use two lines of text.
...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -135,7 +138,7 @@ private fun RealEditor( | |||
|
|
|||
| applyDefaultStyle() | |||
|
|
|||
| // Restore the state of the view with the saved state | |||
| // Set initial HTML and selection based on the provided state | |||
There was a problem hiding this comment.
This is less confusing, thanks!
jmartinesp
deleted the
fix/jme/android-crash-selection-on-view-restoration
branch
I found 2 more bugs in the interop between Compose and the AndroidView:
When restoring state in- this was fixed in Rust. In any case, I believe this state restoration is not needed because the state is kept at theRealEditor'sAndroidView, for some reason if there is a single NSBP char at the end of the HTML it'll just remove it, and it can cause crashes because the selection may become invalid.ComposerModeland the text and selection are restored in theEditorEditText.onRestoreInstanceStatemethod. This should fix the crashes we're seeing in EXA when a mention is added.factorycontents run before thisonRestoreInstanceStateruns, so there's no way to know if we're dealing with a restoration, AFAICT, so by the time we're adding the text changed listener on the factory the state hasn't been restored and thelineCountwill always be 0, which in turn will trigger a recomposition of the whole component, and then another one once we have the rightlineCount = 1value. I changed the code to only take into accountlineCountif it's > 0, which should fit our usage since we don't care about value 0 (the edittext being empty).