Timeline recycling crash #4790

ouchadam · 2021-12-21T17:33:37Z

Tentatively fixes #4789 Timeline crashes when quickly scrolling or restoring state.

I was unable to reproduce this crash locally, the stack trace looks to be the same as when a TextView is modified after setting a TextFuture

Thread: main, Exception: java.lang.IllegalArgumentException: PrecomputedText's Parameters don't match the parameters of this TextView.
    at android.widget.TextView.setText(TextView.java:6206)
    at android.widget.TextView.setText(TextView.java:6124)
    at android.widget.TextView.setText(TextView.java:6076)
    at androidx.core.widget.TextViewCompat.setPrecomputedText(TextViewCompat.java:907)
    at androidx.appcompat.widget.AppCompatTextView.consumeTextFutureAndSetBlocking(AppCompatTextView.java:542)
    at androidx.appcompat.widget.AppCompatTextView.getText(AppCompatTextView.java:551)
    at androidx.emoji2.viewsintegration.EmojiInputFilter.filter(EmojiInputFilter.java:64)
    at android.widget.TextView.setText(TextView.java:6160)
    at android.widget.TextView.setText(TextView.java:6124)
    at android.widget.TextView.setText(TextView.java:6076)

My theory is that RecyclerView is recycling TextViews with non consumed futures which causes them to throw when the TextView options are rebound. This PR aims to solve the issue by resetting any present futures before calling setText which will allow AppCompatTextView.consumeTextFutureAndSetBlocking to be skipped if the view is rebound with setText directly (such is the case when emojis are present)

ouchadam · 2021-12-21T17:33:54Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt

        } else {
-            holder.messageView.text = if (bindingOptions?.preventMutation.orFalse()) {
+            setTextFuture(null)


this line is the potential fix

github-actions · 2021-12-21T18:00:32Z

Unit Test Results

  66 files ±0   66 suites ±0 56s ⏱️ -2s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 22bab47. ± Comparison against base commit eb9fc57.

♻️ This comment has been updated with latest results.

ganfra

LGTM, will see if we have still the crash after!

bmarty · 2021-12-30T09:40:28Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt

        super.bind(holder)
        holder.messageView.movementMethod = movementMethod
        renderSendState(holder.messageView, holder.messageView)
        holder.messageView.onClick(attributes.itemClickListener)
        holder.messageView.onLongClickIgnoringLinks(attributes.itemLongClickListener)

+        message?.let { holder.messageView.setTextWithEmojiSupport(it, bindingOptions) }


I guess that message should never be null, but the change is not strictly equivalent to what we had before: message ?: "".

Maybe continue to fallback to empty string, to ensure that the text is always updated, or change message to a lateinit CharSequence

(if message is null, the TextView content will not be reset and will display the previous bound value)

ah good catch, resetting to null seems safer, I don't know the epoxy lifecycle well enough to trust using a late init

b5ce8dc

- tentatively fixes IllegalArgumentException when recycling the text views due to AppCompatTextView.consumeTextFutureAndSetBlocking attempting to consuming any futures, even if they may be invalid

…recycling and an item doesn't contain a message

bmarty · 2022-01-05T21:03:05Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt

-        } else {
-            null
-        }
-        markwonPlugins?.forEach { plugin -> plugin.beforeSetText(holder.messageView, message?.charSequence as Spanned) }


This line (markwonPlugins...) has been removed by mistake during the merge I guess

ah 🤦 I got confused by https://github.com/vector-im/element-android/pull/4790/files#diff-473563ebe7dac7363901c094cc094bb438921f76a16bf13a020eefbff15d0c24R100 didn't realise there were two

There are 2 different methods beforeSetText and a afterSetText, I have no idea what they are doing TBH

22bab47

from the default implementations in the markwon library, it seems they're mainly used for apply text movements (links) and measuring lists but custom plugins can make use of them if needed

/** * It can be useful to prepare a TextView for markdown. For example {@code ru.noties.markwon.image.ImagesPlugin} * uses this method to unregister previously registered {@link AsyncDrawableSpan} * (if there are such spans in this TextView at this point). Or {@link CorePlugin} * which measures ordered list numbers **/

bmarty · 2022-01-05T21:03:48Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt

@@ -92,27 +91,22 @@ abstract class MessageTextItem : AbsMessageItem<MessageTextItem.Holder>() {
                it.bind(holder.messageView)
            }
        }
-        val textFuture = if (bindingOptions?.canUseTextFuture.orFalse()) {


I am wondering why we coputed that before the next line... Maybe there were a good reason, I do not remember

ouchadam · 2022-01-06T09:26:04Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt

-        } else {
-            null
-        }
-        markwonPlugins?.forEach { plugin -> plugin.beforeSetText(holder.messageView, message?.charSequence as Spanned) }


also noticed message?.charSequence as Spanned will throw if the message is null, will wrap with a .let

Yes, I'm pretty sure message cannot be null but you are right, better safe than sorry.

… a nullable check to avoid attempt to cast a null to non null

ouchadam commented Dec 21, 2021

View reviewed changes

ouchadam added the Z-Crash label Dec 22, 2021

ganfra approved these changes Dec 22, 2021

View reviewed changes

ouchadam mentioned this pull request Dec 22, 2021

Emojis in code blocks crash #4795

Closed

bmarty requested changes Dec 30, 2021

View reviewed changes

ouchadam added 4 commits January 5, 2022 09:58

extracting the emoji text setting to an extension

4daef60

removing any existing text futures before setting new text

57e0ce2

- tentatively fixes IllegalArgumentException when recycling the text views due to AppCompatTextView.consumeTextFutureAndSetBlocking attempting to consuming any futures, even if they may be invalid

adding changelog entry

0df07d0

allowing nullable message content in order to reset the content when …

6380ee9

…recycling and an item doesn't contain a message

ouchadam force-pushed the feature/adm/emoji-recycling branch from b5ce8dc to 6380ee9 Compare January 5, 2022 10:02

ouchadam requested a review from bmarty January 5, 2022 16:11

bmarty requested changes Jan 5, 2022

View reviewed changes

ouchadam commented Jan 6, 2022

View reviewed changes

adding back the beforeSetText logic for markwon integraton and adding…

22bab47

… a nullable check to avoid attempt to cast a null to non null

bmarty approved these changes Jan 6, 2022

View reviewed changes

bmarty merged commit 296929e into develop Jan 6, 2022

bmarty deleted the feature/adm/emoji-recycling branch January 6, 2022 13:55

ouchadam mentioned this pull request Jan 28, 2022

Timeline recycling crash #5091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeline recycling crash #4790

Timeline recycling crash #4790

ouchadam commented Dec 21, 2021

ouchadam Dec 21, 2021

github-actions bot commented Dec 21, 2021 •

edited

Loading

ganfra left a comment

bmarty Dec 30, 2021

ouchadam Jan 5, 2022

bmarty Jan 5, 2022

ouchadam Jan 5, 2022

bmarty Jan 5, 2022 •

edited

Loading

ouchadam Jan 6, 2022 •

edited

Loading

bmarty Jan 5, 2022

ouchadam Jan 6, 2022

bmarty Jan 6, 2022

Timeline recycling crash #4790

Timeline recycling crash #4790

Conversation

ouchadam commented Dec 21, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2021 • edited Loading

Unit Test Results

ganfra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

ouchadam Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2021 •

edited

Loading

bmarty Jan 5, 2022 •

edited

Loading

ouchadam Jan 6, 2022 •

edited

Loading