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.

Sign up for GitHub

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

Timeline recycling crash #4790

Merged
merged 5 commits into from
Jan 6, 2022
Merged

Timeline recycling crash #4790

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4daef60
extracting the emoji text setting to an extension
ouchadam Dec 21, 2021
57e0ce2
removing any existing text futures before setting new text
ouchadam Dec 21, 2021
0df07d0
adding changelog entry
ouchadam Dec 21, 2021
6380ee9
allowing nullable message content in order to reset the content when …
ouchadam Jan 5, 2022
22bab47
adding back the beforeSetText logic for markwon integraton and adding…
ouchadam Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/4789.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixing crashes when quickly scrolling or restoring the room timeline
22 changes: 8 additions & 14 deletions ...or/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageTextItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package im.vector.app.features.home.room.detail.timeline.item

import android.text.Spanned
import android.text.method.MovementMethod
import androidx.appcompat.widget.AppCompatTextView
import androidx.core.text.PrecomputedTextCompat
Expand Down Expand Up @@ -92,27 +91,22 @@ abstract class MessageTextItem : AbsMessageItem<MessageTextItem.Holder>() {
it.bind(holder.messageView)
}
}
val textFuture = if (bindingOptions?.canUseTextFuture.orFalse()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PrecomputedTextCompat.getTextFuture(
message?.charSequence ?: "",
TextViewCompat.getTextMetricsParams(holder.messageView),
null)
} else {
null
}
markwonPlugins?.forEach { plugin -> plugin.beforeSetText(holder.messageView, message?.charSequence as Spanned) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bmarty bmarty Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ouchadam ouchadam Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
**/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super.bind(holder)
holder.messageView.movementMethod = movementMethod
renderSendState(holder.messageView, holder.messageView)
holder.messageView.onClick(attributes.itemClickListener)
holder.messageView.onLongClickIgnoringLinks(attributes.itemLongClickListener)
holder.messageView.setTextWithEmojiSupport(message?.charSequence, bindingOptions)
markwonPlugins?.forEach { plugin -> plugin.afterSetText(holder.messageView) }
}

if (bindingOptions?.canUseTextFuture.orFalse()) {
holder.messageView.setTextFuture(textFuture)
private fun AppCompatTextView.setTextWithEmojiSupport(message: CharSequence?, bindingOptions: BindingOptions?) {
if (bindingOptions?.canUseTextFuture.orFalse() && message != null) {
val textFuture = PrecomputedTextCompat.getTextFuture(message, TextViewCompat.getTextMetricsParams(this), null)
setTextFuture(textFuture)
} else {
holder.messageView.text = message?.charSequence
text = message
}
markwonPlugins?.forEach { plugin -> plugin.afterSetText(holder.messageView) }
}

override fun unbind(holder: Holder) {
Expand Down