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

Jump to bottom

Safe epoxy char sequence #4837

Merged
merged 6 commits into from
Jan 4, 2022
Merged

Safe epoxy char sequence #4837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d7c9e15
Create EpoxyCharSequence: wrapper for a CharSequence, which support m…
bmarty Jan 3, 2022
ca44d8f
Use EpoxyCharSequence for all Epoxy items
bmarty Jan 3, 2022
27161bf
preventMutation is not needed anymore, since we now uses EpoxyCharSeq…
bmarty Jan 3, 2022
a38f5f9
Format
bmarty Jan 3, 2022
4642299
Changelog
bmarty Jan 3, 2022
e03c806
Split long line
bmarty Jan 3, 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/4837.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stop using CharSequence as EpoxyAttribute because it can lead to crash if the CharSequence mutates during rendering.
3 changes: 0 additions & 3 deletions vector/src/androidTest/java/im/vector/app/features/html/SpanUtilsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ class SpanUtilsTest : InstrumentedTest {
val string = SpannableString("Text")
val result = spanUtils.getBindingOptions(string)
result.canUseTextFuture shouldBeEqualTo true
result.preventMutation shouldBeEqualTo false
}

@Test
Expand All @@ -117,15 +116,13 @@ class SpanUtilsTest : InstrumentedTest {
string.setSpan(StrikethroughSpan(), 10, 23, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE)
val result = spanUtils.getBindingOptions(string)
result.canUseTextFuture shouldBeEqualTo false
result.preventMutation shouldBeEqualTo false
}

@Test
fun testGetBindingOptionsMetricAffectingSpan() {
val string = SpannableString("Emoji \uD83D\uDE2E\u200D\uD83D\uDCA8")
val result = spanUtils.getBindingOptions(string)
result.canUseTextFuture shouldBeEqualTo false
result.preventMutation shouldBeEqualTo true
}

private fun trueIfAlwaysAllowed() = Build.VERSION.SDK_INT < Build.VERSION_CODES.P
Expand Down
19 changes: 7 additions & 12 deletions vector/src/main/java/im/vector/app/core/epoxy/bottomsheet/BottomSheetMessagePreviewItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,14 @@ import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.charsequence.EpoxyCharSequence
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.epoxy.util.preventMutation
import im.vector.app.core.extensions.setTextOrHide
import im.vector.app.features.displayname.getBestName
import im.vector.app.features.home.AvatarRenderer
import im.vector.app.features.home.room.detail.timeline.item.BindingOptions
import im.vector.app.features.home.room.detail.timeline.tools.findPillsAndProcess
import im.vector.app.features.media.ImageContentRenderer
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.util.MatrixItem

/**
Expand All @@ -50,13 +49,13 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
lateinit var matrixItem: MatrixItem

@EpoxyAttribute
lateinit var body: CharSequence
lateinit var body: EpoxyCharSequence

@EpoxyAttribute
var bindingOptions: BindingOptions? = null

@EpoxyAttribute
var bodyDetails: CharSequence? = null
var bodyDetails: EpoxyCharSequence? = null

@EpoxyAttribute
var imageContentRenderer: ImageContentRenderer? = null
Expand All @@ -65,7 +64,7 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
var data: ImageContentRenderer.Data? = null

@EpoxyAttribute
var time: CharSequence? = null
var time: String? = null

@EpoxyAttribute
var movementMethod: MovementMethod? = null
Expand All @@ -84,13 +83,9 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
}
holder.imagePreview.isVisible = data != null
holder.body.movementMethod = movementMethod
holder.body.text = if (bindingOptions?.preventMutation.orFalse()) {
body.preventMutation()
} else {
body
}
holder.bodyDetails.setTextOrHide(bodyDetails)
body.findPillsAndProcess(coroutineScope) { it.bind(holder.body) }
holder.body.text = body.charSequence
holder.bodyDetails.setTextOrHide(bodyDetails?.charSequence)
body.charSequence.findPillsAndProcess(coroutineScope) { it.bind(holder.body) }
holder.timestamp.setTextOrHide(time)
}

Expand Down
4 changes: 2 additions & 2 deletions vector/src/main/java/im/vector/app/core/epoxy/bottomsheet/BottomSheetRadioActionItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import im.vector.app.core.extensions.setTextOrHide
abstract class BottomSheetRadioActionItem : VectorEpoxyModel<BottomSheetRadioActionItem.Holder>() {

@EpoxyAttribute
var title: CharSequence? = null
var title: String? = null

@StringRes
@EpoxyAttribute
Expand All @@ -47,7 +47,7 @@ abstract class BottomSheetRadioActionItem : VectorEpoxyModel<BottomSheetRadioAct
var selected = false

@EpoxyAttribute
var description: CharSequence? = null
var description: String? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
lateinit var listener: ClickListener
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/core/epoxy/bottomsheet/BottomSheetSendStateItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ abstract class BottomSheetSendStateItem : VectorEpoxyModel<BottomSheetSendStateI
var showProgress: Boolean = false

@EpoxyAttribute
lateinit var text: CharSequence
lateinit var text: String

@EpoxyAttribute
@DrawableRes
Expand Down
27 changes: 27 additions & 0 deletions vector/src/main/java/im/vector/app/core/epoxy/charsequence/EpoxyCharSequence.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package im.vector.app.core.epoxy.charsequence

/**
* Wrapper for a CharSequence, which support mutation of the CharSequence, which can happen during rendering
*/
class EpoxyCharSequence(val charSequence: CharSequence) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let the EpoxyCharSequence inherits CharSequence like this, to avoid accessing charSequence value everywhere

class EpoxyCharSequence(val charSequence: CharSequence) : CharSequence by charSequence

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, let me try it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not work, the TextView displays the Object and not the CharSequence... Weird

image

Copy link
Member Author

Choose a reason for hiding this comment

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

According to @ganfra investigation, it can work using

class EpoxyCharSequence(val charSequence: CharSequence) : Spannable by SpannableString(charSequence) {...}

but I prefer not to create a new SpannableString for each charSequence.
Using .charSequence whenever it is necesssary is probably better, because it forces the developer to do it (no "hidden magic")

private val hash = charSequence.toString().hashCode()

override fun hashCode() = hash
override fun equals(other: Any?) = other is EpoxyCharSequence && other.hash == hash
Copy link
Member Author

Choose a reason for hiding this comment

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

We do not compare the String content, since the risk of collision is low enough, and Epoxy is mainly using hashCode() to compare items

}
11 changes: 6 additions & 5 deletions .../vector/app/core/epoxy/util/Extensions.kt → ...app/core/epoxy/charsequence/Extensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021 New Vector Ltd
* Copyright (c) 2022 New Vector Ltd
Copy link
Member Author

Choose a reason for hiding this comment

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

Happy new year!

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,8 +14,9 @@
* limitations under the License.
*/

package im.vector.app.core.epoxy.util
package im.vector.app.core.epoxy.charsequence

import android.text.SpannableString

fun CharSequence?.preventMutation(): CharSequence? = this?.let { SpannableString(it) }
/**
* Extensions to wrap CharSequence to EpoxyCharSequence
*/
fun CharSequence.toEpoxyCharSequence() = EpoxyCharSequence(this)
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/core/epoxy/profiles/notifications/RadioButtonItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import im.vector.app.core.extensions.setAttributeTintedImageResource
abstract class RadioButtonItem : VectorEpoxyModel<RadioButtonItem.Holder>() {

@EpoxyAttribute
var title: CharSequence? = null
var title: String? = null
Copy link
Member Author

Choose a reason for hiding this comment

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

Whenever a CharSequence is not required, it simpler to use an immutable String


@StringRes
@EpoxyAttribute
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/core/ui/bottomsheet/BottomSheetGenericRadioAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import im.vector.app.core.platform.VectorSharedAction
* Parent class for a bottom sheet action
*/
open class BottomSheetGenericRadioAction(
open val title: CharSequence?,
open val title: String?,
open val description: String? = null,
open val isSelected: Boolean
) : VectorSharedAction {
Expand Down
4 changes: 2 additions & 2 deletions vector/src/main/java/im/vector/app/core/ui/list/GenericEmptyWithActionItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ import im.vector.app.core.extensions.setTextOrHide
abstract class GenericEmptyWithActionItem : VectorEpoxyModel<GenericEmptyWithActionItem.Holder>() {

@EpoxyAttribute
var title: CharSequence? = null
var title: String? = null

@EpoxyAttribute
var description: CharSequence? = null
var description: String? = null

@EpoxyAttribute
@DrawableRes
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/core/ui/list/GenericFooterItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import im.vector.app.features.themes.ThemeUtils
abstract class GenericFooterItem : VectorEpoxyModel<GenericFooterItem.Holder>() {

@EpoxyAttribute
var text: CharSequence? = null
var text: String? = null

@EpoxyAttribute
var style: ItemStyle = ItemStyle.NORMAL_TEXT
Expand Down
9 changes: 5 additions & 4 deletions vector/src/main/java/im/vector/app/core/ui/list/GenericItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.charsequence.EpoxyCharSequence
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.extensions.setTextOrHide

Expand All @@ -41,10 +42,10 @@ import im.vector.app.core.extensions.setTextOrHide
abstract class GenericItem : VectorEpoxyModel<GenericItem.Holder>() {

@EpoxyAttribute
var title: CharSequence? = null
var title: EpoxyCharSequence? = null

@EpoxyAttribute
var description: CharSequence? = null
var description: EpoxyCharSequence? = null

@EpoxyAttribute
var style: ItemStyle = ItemStyle.NORMAL_TEXT
Expand All @@ -71,7 +72,7 @@ abstract class GenericItem : VectorEpoxyModel<GenericItem.Holder>() {

override fun bind(holder: Holder) {
super.bind(holder)
holder.titleText.setTextOrHide(title)
holder.titleText.setTextOrHide(title?.charSequence)

if (titleIconResourceId != -1) {
holder.titleIcon.setImageResource(titleIconResourceId)
Expand All @@ -82,7 +83,7 @@ abstract class GenericItem : VectorEpoxyModel<GenericItem.Holder>() {

holder.titleText.textSize = style.toTextSize()

holder.descriptionText.setTextOrHide(description)
holder.descriptionText.setTextOrHide(description?.charSequence)

if (hasIndeterminateProcess) {
holder.progressBar.isVisible = true
Expand Down
5 changes: 3 additions & 2 deletions vector/src/main/java/im/vector/app/core/ui/list/GenericPillItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.charsequence.EpoxyCharSequence
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.extensions.setTextOrHide
import im.vector.app.features.themes.ThemeUtils
Expand All @@ -39,7 +40,7 @@ import im.vector.app.features.themes.ThemeUtils
abstract class GenericPillItem : VectorEpoxyModel<GenericPillItem.Holder>() {

@EpoxyAttribute
var text: CharSequence? = null
var text: EpoxyCharSequence? = null

@EpoxyAttribute
var style: ItemStyle = ItemStyle.NORMAL_TEXT
Expand All @@ -60,7 +61,7 @@ abstract class GenericPillItem : VectorEpoxyModel<GenericPillItem.Holder>() {
override fun bind(holder: Holder) {
super.bind(holder)

holder.textView.setTextOrHide(text)
holder.textView.setTextOrHide(text?.charSequence)
holder.textView.typeface = style.toTypeFace()
holder.textView.textSize = style.toTextSize()
holder.textView.gravity = if (centered) Gravity.CENTER_HORIZONTAL else Gravity.START
Expand Down
7 changes: 4 additions & 3 deletions vector/src/main/java/im/vector/app/core/ui/list/GenericWithValueItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.charsequence.EpoxyCharSequence
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.extensions.setTextOrHide
import im.vector.app.features.themes.ThemeUtils
Expand All @@ -41,10 +42,10 @@ import im.vector.app.features.themes.ThemeUtils
abstract class GenericWithValueItem : VectorEpoxyModel<GenericWithValueItem.Holder>() {

@EpoxyAttribute
var title: CharSequence? = null
var title: EpoxyCharSequence? = null

@EpoxyAttribute
var value: CharSequence? = null
var value: String? = null

@EpoxyAttribute
@ColorInt
Expand All @@ -62,7 +63,7 @@ abstract class GenericWithValueItem : VectorEpoxyModel<GenericWithValueItem.Hold

override fun bind(holder: Holder) {
super.bind(holder)
holder.titleText.setTextOrHide(title)
holder.titleText.setTextOrHide(title?.charSequence)

if (titleIconResourceId != -1) {
holder.titleIcon.setImageResource(titleIconResourceId)
Expand Down
6 changes: 3 additions & 3 deletions vector/src/main/java/im/vector/app/features/autocomplete/command/AutocompleteCommandItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ import im.vector.app.core.epoxy.onClick
abstract class AutocompleteCommandItem : VectorEpoxyModel<AutocompleteCommandItem.Holder>() {

@EpoxyAttribute
var name: CharSequence? = null
var name: String? = null

@EpoxyAttribute
var parameters: CharSequence? = null
var parameters: String? = null

@EpoxyAttribute
var description: CharSequence? = null
var description: String? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
var clickListener: ClickListener? = null
Expand Down
Loading