From fdfd8a488f3e8a1ed06acd71b9159a68e57b275f Mon Sep 17 00:00:00 2001 From: Evgenii Kozlov Date: Wed, 5 Feb 2020 18:16:56 +0300 Subject: [PATCH] Refactor BlockViewDiffUtil (#190) --- CHANGELOG.md | 24 ++- .../anytype/core_ui/common/Checkable.kt | 8 + .../core_ui/features/page/BlockAdapter.kt | 13 +- .../core_ui/features/page/BlockView.kt | 10 +- .../features/page/BlockViewDiffUtil.kt | 99 +++++------- .../core_ui/features/page/BlockViewHolder.kt | 141 ++++-------------- .../anytype/core_ui/BlockViewDiffUtilTest.kt | 40 ++++- .../anytype/core_utils/ext/Extensions.kt | 10 ++ .../presentation/mapper/MapperExtension.kt | 2 +- 9 files changed, 153 insertions(+), 194 deletions(-) create mode 100644 core-ui/src/main/java/com/agileburo/anytype/core_ui/common/Checkable.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e5be0431e..8073b8593c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,31 @@ # Change log for Android @Anytype app. +## Version 0.0.18 (WIP) + +### New features + +* + +### Design + +* + +### Fixes && tech + +* Refactored `BlockViewDiffUtil` (processing change payload) (#164) + +### Middleware + +* + ## Version 0.0.17 ### New features * User can now use the color toolbar to change the text color of the whole text block (#153) -* User can now user the markup-color toolbar to change the background color of the selected text (#111) -* Create checkbox-list item on enter-pressed-event (instead of a simple paragraph) (#155) -* Create bulleted-list item on enter-pressed-event (instead of a simple paragraph) (#154) +* User can now use the markup-color toolbar to change the background color of the selected text (#111) +* Create a checkbox-list item on enter-pressed-event (instead of a simple paragraph) (#155) +* Create a bulleted-list item on enter-pressed-event (instead of a simple paragraph) (#154) * `Block.Content.Text` model now has optional `color` property (#153). * Added documentation engine (`DOKKA`) for `domain` module: documentation is generated automatically from KDoc (#168). * Added new content model: `Block.Content.Link` (#173) diff --git a/core-ui/src/main/java/com/agileburo/anytype/core_ui/common/Checkable.kt b/core-ui/src/main/java/com/agileburo/anytype/core_ui/common/Checkable.kt new file mode 100644 index 0000000000..8010bfc6b4 --- /dev/null +++ b/core-ui/src/main/java/com/agileburo/anytype/core_ui/common/Checkable.kt @@ -0,0 +1,8 @@ +package com.agileburo.anytype.core_ui.common + +/** + * Defines a view that can be checked + */ +interface Checkable { + val isChecked: Boolean +} \ No newline at end of file diff --git a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockAdapter.kt b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockAdapter.kt index 23dc9dcea0..25c12829b8 100644 --- a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockAdapter.kt +++ b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockAdapter.kt @@ -24,6 +24,7 @@ import com.agileburo.anytype.core_ui.features.page.BlockViewHolder.Companion.HOL import com.agileburo.anytype.core_ui.features.page.BlockViewHolder.Companion.HOLDER_TASK import com.agileburo.anytype.core_ui.features.page.BlockViewHolder.Companion.HOLDER_TITLE import com.agileburo.anytype.core_ui.features.page.BlockViewHolder.Companion.HOLDER_TOGGLE +import com.agileburo.anytype.core_utils.ext.typeOf import timber.log.Timber /** @@ -230,20 +231,20 @@ class BlockAdapter( when (holder) { is BlockViewHolder.Paragraph -> { holder.processChangePayload( - payloads = payloads, - item = blocks[position] as BlockView.Paragraph + payloads = payloads.typeOf(), + item = blocks[position] ) } is BlockViewHolder.Bulleted -> { holder.processChangePayload( - payloads = payloads, - item = blocks[position] as BlockView.Bulleted + payloads = payloads.typeOf(), + item = blocks[position] ) } is BlockViewHolder.Checkbox -> { holder.processChangePayload( - payloads = payloads, - item = blocks[position] as BlockView.Checkbox + payloads = payloads.typeOf(), + item = blocks[position] ) } else -> TODO() diff --git a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockView.kt b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockView.kt index 0a27e1930a..572dee5727 100644 --- a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockView.kt +++ b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockView.kt @@ -1,5 +1,6 @@ package com.agileburo.anytype.core_ui.features.page +import com.agileburo.anytype.core_ui.common.Checkable import com.agileburo.anytype.core_ui.common.Focusable import com.agileburo.anytype.core_ui.common.Markup import com.agileburo.anytype.core_ui.common.ViewType @@ -152,15 +153,16 @@ sealed class BlockView : ViewType { * UI-model for checkbox blocks. * @property id block's id * @property text checkbox's content text - * @property checked immutable checkbox state (whether this checkbox is checked or not) + * @property isChecked immutable checkbox state (whether this checkbox is checked or not) */ data class Checkbox( override val id: String, override val marks: List = emptyList(), override val focused: Boolean = false, - val text: String, - val checked: Boolean = false - ) : BlockView(), Markup, Focusable { + override val text: String, + override val color: String? = null, + override val isChecked: Boolean = false + ) : BlockView(), Markup, Focusable, Text, Checkable { override fun getViewType() = HOLDER_CHECKBOX override val body: String = text } diff --git a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewDiffUtil.kt b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewDiffUtil.kt index 6255498e84..fc951c15c2 100644 --- a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewDiffUtil.kt +++ b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewDiffUtil.kt @@ -1,6 +1,8 @@ package com.agileburo.anytype.core_ui.features.page import androidx.recyclerview.widget.DiffUtil +import com.agileburo.anytype.core_ui.common.Focusable +import com.agileburo.anytype.core_ui.common.Markup class BlockViewDiffUtil( private val old: List, @@ -22,81 +24,48 @@ class BlockViewDiffUtil( override fun getChangePayload(oldItemPosition: Int, newItemPosition: Int): Any? { - // TODO refactoring needed. Return list of changes instead of one change. - val oldBlock = old[oldItemPosition] val newBlock = new[newItemPosition] - return if (oldBlock is BlockView.Paragraph && newBlock is BlockView.Paragraph) { - if (oldBlock.text != newBlock.text) { - if (oldBlock.marks != newBlock.marks) - if (oldBlock.color != newBlock.color) { - TEXT_MARKUP_AND_COLOR_CHANGED - } else { - TEXT_AND_MARKUP_CHANGED - } - else if (oldBlock.color != newBlock.color) { - TEXT_AND_COLOR_CHANGED - } else - TEXT_CHANGED - } else { - when { - oldBlock.marks != newBlock.marks && oldBlock.color != newBlock.color -> MARKUP_AND_COLOR_CHANGED - oldBlock.focused != newBlock.focused && oldBlock.color != newBlock.color -> FOCUS_AND_COLOR_CHANGED - oldBlock.marks != newBlock.marks -> MARKUP_CHANGED - oldBlock.focused != newBlock.focused -> FOCUS_CHANGED - oldBlock.color != newBlock.color -> TEXT_COLOR_CHANGED - else -> throw IllegalStateException("Unexpected change payload scenario:\n$oldBlock\n$newBlock") - } - } - } else if (oldBlock is BlockView.Bulleted && newBlock is BlockView.Bulleted) { - if (oldBlock.text != newBlock.text) { - if (oldBlock.marks != newBlock.marks) - if (oldBlock.color != newBlock.color) { - TEXT_MARKUP_AND_COLOR_CHANGED - } else { - TEXT_AND_MARKUP_CHANGED - } - else if (oldBlock.color != newBlock.color) { - TEXT_AND_COLOR_CHANGED - } else - TEXT_CHANGED - } else { - when { - oldBlock.marks != newBlock.marks && oldBlock.color != newBlock.color -> MARKUP_AND_COLOR_CHANGED - oldBlock.focused != newBlock.focused && oldBlock.color != newBlock.color -> FOCUS_AND_COLOR_CHANGED - oldBlock.marks != newBlock.marks -> MARKUP_CHANGED - oldBlock.focused != newBlock.focused -> FOCUS_CHANGED - oldBlock.color != newBlock.color -> TEXT_COLOR_CHANGED - else -> throw IllegalStateException("Unexpected change payload scenario:\n$oldBlock\n$newBlock") - } - } - } else if (oldBlock is BlockView.Checkbox && newBlock is BlockView.Checkbox) { - if (oldBlock.text != newBlock.text) { - if (oldBlock.marks != newBlock.marks) - TEXT_AND_MARKUP_CHANGED - else - TEXT_CHANGED - } else { - when { - oldBlock.marks != newBlock.marks -> MARKUP_CHANGED - oldBlock.focused != newBlock.focused -> FOCUS_CHANGED - else -> throw IllegalStateException("Unexpected change payload scenario:\n$oldBlock\n$newBlock") - } - } - } else + if (newBlock::class != oldBlock::class) + return super.getChangePayload(oldItemPosition, newItemPosition) + + val changes = mutableListOf() + + if (newBlock is BlockView.Text && oldBlock is BlockView.Text) { + if (newBlock.text != oldBlock.text) + changes.add(TEXT_CHANGED) + if (newBlock.color != oldBlock.color) + changes.add(TEXT_COLOR_CHANGED) + } + + if (newBlock is Markup && oldBlock is Markup) { + if (newBlock.marks != oldBlock.marks) + changes.add(MARKUP_CHANGED) + } + + if (newBlock is Focusable && oldBlock is Focusable) { + if (newBlock.focused != oldBlock.focused) + changes.add(FOCUS_CHANGED) + } + + return if (changes.isNotEmpty()) + Payload(changes) + else super.getChangePayload(oldItemPosition, newItemPosition) } + /** + * Payload of changes to apply to a block view. + */ + data class Payload( + val changes: List + ) + companion object { const val TEXT_CHANGED = 0 const val MARKUP_CHANGED = 1 - const val TEXT_AND_MARKUP_CHANGED = 2 const val FOCUS_CHANGED = 3 const val TEXT_COLOR_CHANGED = 4 - const val TEXT_MARKUP_AND_COLOR_CHANGED = 5 - const val TEXT_AND_COLOR_CHANGED = 6 - const val FOCUS_AND_COLOR_CHANGED = 7 - const val MARKUP_AND_COLOR_CHANGED = 8 } } \ No newline at end of file diff --git a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewHolder.kt b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewHolder.kt index 4dc824f273..bcf2a761b5 100644 --- a/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewHolder.kt +++ b/core-ui/src/main/java/com/agileburo/anytype/core_ui/features/page/BlockViewHolder.kt @@ -9,14 +9,11 @@ import com.agileburo.anytype.core_ui.R import com.agileburo.anytype.core_ui.common.* import com.agileburo.anytype.core_ui.extensions.color import com.agileburo.anytype.core_ui.extensions.tint -import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.FOCUS_AND_COLOR_CHANGED import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.FOCUS_CHANGED import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.MARKUP_CHANGED -import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_AND_COLOR_CHANGED -import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_AND_MARKUP_CHANGED import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_CHANGED import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_COLOR_CHANGED -import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_MARKUP_AND_COLOR_CHANGED +import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Payload import com.agileburo.anytype.core_ui.tools.DefaultSpannableFactory import com.agileburo.anytype.core_ui.tools.DefaultTextWatcher import com.agileburo.anytype.core_ui.widgets.text.TextInputWidget @@ -111,6 +108,33 @@ sealed class BlockViewHolder(view: View) : RecyclerView.ViewHolder(view) { ) } } + + fun processChangePayload( + payloads: List, + item: BlockView + ) = payloads.forEach { payload -> + + Timber.d("Processing $payload") + + if (item is BlockView.Text) { + if (payload.changes.contains(TEXT_CHANGED)) + if (content.text.toString() != item.text) + content.setText(item.text) + + if (payload.changes.contains(TEXT_COLOR_CHANGED)) + item.color?.let { setTextColor(it) } + } + + if (item is Markup) { + if (payload.changes.contains(MARKUP_CHANGED)) + setMarkup(item) + } + + if (item is Focusable) { + if (payload.changes.contains(FOCUS_CHANGED)) + setFocus(item) + } + } } class Paragraph(view: View) : BlockViewHolder(view), TextHolder { @@ -155,53 +179,6 @@ sealed class BlockViewHolder(view: View) : RecyclerView.ViewHolder(view) { } content.selectionDetector = { onSelectionChanged(item.id, it) } } - - fun processChangePayload( - payloads: List, - item: BlockView.Paragraph - ) = payloads.forEach { payload -> - - Timber.d("Applying change payload: $payload") - - when (payload) { - MARKUP_CHANGED -> { - if (item.marks.isLinksPresent()) { - content.setLinksClickable() - } - setMarkup(markup = item) - } - TEXT_CHANGED -> { - if (content.text.toString() != item.text) - content.setText(item.text) - } - TEXT_AND_MARKUP_CHANGED -> { - if (item.marks.isLinksPresent()) { - content.setLinksClickable() - } - if (content.text.toString() != item.text) - content.setText(item.text) - setMarkup(markup = item) - } - TEXT_MARKUP_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - if (content.text.toString() != item.text) content.setText(item.text) - setMarkup(markup = item) - } - FOCUS_CHANGED -> { - setFocus(item) - } - TEXT_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - } - TEXT_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - if (content.text.toString() != item.text) content.setText(item.text) - } - FOCUS_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - } - } - } } class Title(view: View) : BlockViewHolder(view), TextHolder { @@ -344,7 +321,7 @@ sealed class BlockViewHolder(view: View) : RecyclerView.ViewHolder(view) { onFocusChanged: (String, Boolean) -> Unit ) { - checkbox.isSelected = item.checked + checkbox.isSelected = item.isChecked checkbox.setOnClickListener { checkbox.isSelected = !checkbox.isSelected @@ -379,27 +356,6 @@ sealed class BlockViewHolder(view: View) : RecyclerView.ViewHolder(view) { content.selectionDetector = { onSelectionChanged(item.id, it) } } - - fun processChangePayload( - payloads: List, - item: BlockView.Checkbox - ) = payloads.forEach { payload -> - when (payload) { - MARKUP_CHANGED -> setMarkup(markup = item) - TEXT_CHANGED -> { - if (content.text.toString() != item.text) - content.setText(item.text) - } - TEXT_AND_MARKUP_CHANGED -> { - if (content.text.toString() != item.text) - content.setText(item.text) - setMarkup(markup = item) - } - FOCUS_CHANGED -> { - setFocus(item) - } - } - } } class Task(view: View) : BlockViewHolder(view) { @@ -473,45 +429,6 @@ sealed class BlockViewHolder(view: View) : RecyclerView.ViewHolder(view) { super.setTextColor(color) bullet.tint(content.context.color(R.color.black)) } - - fun processChangePayload( - payloads: List, - item: BlockView.Bulleted - ) = payloads.forEach { payload -> - - Timber.d("Applying change payload: $payload") - - when (payload) { - MARKUP_CHANGED -> setMarkup(markup = item) - TEXT_CHANGED -> { - if (content.text.toString() != item.text) - content.setText(item.text) - } - TEXT_AND_MARKUP_CHANGED -> { - if (content.text.toString() != item.text) - content.setText(item.text) - setMarkup(markup = item) - } - TEXT_MARKUP_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - if (content.text.toString() != item.text) content.setText(item.text) - setMarkup(markup = item) - } - FOCUS_CHANGED -> { - setFocus(item) - } - FOCUS_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - } - TEXT_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - } - TEXT_AND_COLOR_CHANGED -> { - if (item.color != null) setTextColor(item.color) - if (content.text.toString() != item.text) content.setText(item.text) - } - } - } } class Numbered(view: View) : BlockViewHolder(view) { diff --git a/core-ui/src/test/java/com/agileburo/anytype/core_ui/BlockViewDiffUtilTest.kt b/core-ui/src/test/java/com/agileburo/anytype/core_ui/BlockViewDiffUtilTest.kt index b1abaf7c47..cada87a398 100644 --- a/core-ui/src/test/java/com/agileburo/anytype/core_ui/BlockViewDiffUtilTest.kt +++ b/core-ui/src/test/java/com/agileburo/anytype/core_ui/BlockViewDiffUtilTest.kt @@ -3,8 +3,12 @@ package com.agileburo.anytype.core_ui import com.agileburo.anytype.core_ui.common.Markup import com.agileburo.anytype.core_ui.features.page.BlockView import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil +import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.MARKUP_CHANGED +import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Companion.TEXT_CHANGED +import com.agileburo.anytype.core_ui.features.page.BlockViewDiffUtil.Payload import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertNull class BlockViewDiffUtilTest { @@ -121,7 +125,7 @@ class BlockViewDiffUtilTest { assertEquals( actual = payload, - expected = BlockViewDiffUtil.MARKUP_CHANGED + expected = Payload(listOf(MARKUP_CHANGED)) ) } @@ -162,7 +166,7 @@ class BlockViewDiffUtilTest { assertEquals( actual = payload, - expected = BlockViewDiffUtil.TEXT_CHANGED + expected = Payload(listOf(TEXT_CHANGED)) ) } @@ -201,7 +205,37 @@ class BlockViewDiffUtilTest { assertEquals( actual = payload, - expected = BlockViewDiffUtil.TEXT_AND_MARKUP_CHANGED + expected = Payload(listOf(TEXT_CHANGED, MARKUP_CHANGED)) ) } + + @Test + fun `should return empty payload if types differ`() { + + val index = 0 + + val id = MockDataFactory.randomUuid() + + val text = MockDataFactory.randomString() + + val oldBlock: BlockView = BlockView.HeaderOne( + id = id, + text = text + ) + + val newBlock: BlockView = BlockView.HeaderOne( + id = id, + text = text + ) + + val old = listOf(oldBlock) + + val new = listOf(newBlock) + + val diff = BlockViewDiffUtil(old = old, new = new) + + val payload = diff.getChangePayload(index, index) + + assertNull(actual = payload) + } } \ No newline at end of file diff --git a/core-utils/src/main/java/com/agileburo/anytype/core_utils/ext/Extensions.kt b/core-utils/src/main/java/com/agileburo/anytype/core_utils/ext/Extensions.kt index 1f4679bf59..5dcdd5f2b5 100644 --- a/core-utils/src/main/java/com/agileburo/anytype/core_utils/ext/Extensions.kt +++ b/core-utils/src/main/java/com/agileburo/anytype/core_utils/ext/Extensions.kt @@ -34,5 +34,15 @@ inline fun MutableList.shiftDown(srcIndex: Int, dstIndex: Int) = } } +inline fun List<*>.typeOf(): List { + val retlist = mutableListOf() + this.forEach { + if (it is T) { + retlist.add(it) + } + } + return retlist +} + fun Context.toast(msg: CharSequence) = Toast.makeText(this, msg, Toast.LENGTH_LONG).show() fun Fragment.toast(msg: CharSequence) = requireActivity().toast(msg) \ No newline at end of file diff --git a/presentation/src/main/java/com/agileburo/anytype/presentation/mapper/MapperExtension.kt b/presentation/src/main/java/com/agileburo/anytype/presentation/mapper/MapperExtension.kt index 7a9966ce4a..6819056593 100644 --- a/presentation/src/main/java/com/agileburo/anytype/presentation/mapper/MapperExtension.kt +++ b/presentation/src/main/java/com/agileburo/anytype/presentation/mapper/MapperExtension.kt @@ -62,7 +62,7 @@ fun Block.toView(focused: Boolean = false): BlockView = when (val content = this id = id, text = content.text, marks = mapMarks(content), - checked = content.isChecked == true, + isChecked = content.isChecked == true, focused = focused ) Style.TOGGLE -> BlockView.Toggle(