From 8560b512e1419a0aa2299205e08bb217340dce6a Mon Sep 17 00:00:00 2001 From: Evgenii Kozlov Date: Thu, 24 Sep 2020 19:54:10 +0300 Subject: [PATCH] Fix/block merge for docs containing divs (#912) * block-merge reworked to take into account document's divs * tests * fixes * updated change log * run ci * fix tests * Update workflow.yml Co-authored-by: Ivanov Konstantin Co-authored-by: Konstantin Ivanov <54908981+konstantiniiv@users.noreply.github.com> --- CHANGELOG.md | 1 + .../presentation/page/PageViewModel.kt | 20 +- .../presentation/page/PageViewModelTest.kt | 98 ----- .../page/editor/EditorMergeTest.kt | 357 ++++++++++++++++++ .../editor/EditorPresentationTestSetup.kt | 17 + 5 files changed, 385 insertions(+), 108 deletions(-) create mode 100644 presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorMergeTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index eeefdda4e4..e7dd575923 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Fixes & tech 🚒 +* Block-merge operations for documents containing sections (aka divs) (#912) * App crashes when opening action menu for link block, which was created by turning a text block into a page (#910) * Should create a new toogle on enter press at the end of the non-empty toggle block (#907) * Should convert toggle block to paragraph on enter press if toggle block's text is empty (#886) diff --git a/presentation/src/main/java/com/agileburo/anytype/presentation/page/PageViewModel.kt b/presentation/src/main/java/com/agileburo/anytype/presentation/page/PageViewModel.kt index 681e1b3449..3bd3bcaace 100644 --- a/presentation/src/main/java/com/agileburo/anytype/presentation/page/PageViewModel.kt +++ b/presentation/src/main/java/com/agileburo/anytype/presentation/page/PageViewModel.kt @@ -822,18 +822,18 @@ class PageViewModel( ) } - // TODO should take into account that previous block could be a Block.Content.Layout! - - val page = blocks.first { it.id == context } - - val index = page.children.indexOf(id) + val index = views.indexOfFirst { it.id == id } if (index > 0) { - val previous = page.children[index.dec()] - proceedWithMergingBlocks( - previous = previous, - target = id - ) + val previous = views[index.dec()] + if (previous is BlockView.Text) { + proceedWithMergingBlocks( + previous = previous.id, + target = id + ) + } else { + Timber.d("Skipping merge because previous block is not a text block") + } } else { Timber.d("Skipping merge on non-empty-block-backspace-pressed event") } diff --git a/presentation/src/test/java/com/agileburo/anytype/presentation/page/PageViewModelTest.kt b/presentation/src/test/java/com/agileburo/anytype/presentation/page/PageViewModelTest.kt index db26e52352..ad3fd03435 100644 --- a/presentation/src/test/java/com/agileburo/anytype/presentation/page/PageViewModelTest.kt +++ b/presentation/src/test/java/com/agileburo/anytype/presentation/page/PageViewModelTest.kt @@ -2143,93 +2143,6 @@ open class PageViewModelTest { } } - @Test - fun `should update text and proceed with merging the first paragraph with the second on non-empty-block-backspace-pressed event`() { - - val root = MockDataFactory.randomUuid() - val firstChild = MockDataFactory.randomUuid() - val secondChild = MockDataFactory.randomUuid() - val thirdChild = MockDataFactory.randomUuid() - - val page = MockBlockFactory.makeOnePageWithThreeTextBlocks( - root = root, - firstChild = firstChild, - secondChild = secondChild, - thirdChild = thirdChild, - firstChildStyle = Block.Content.Text.Style.TITLE, - secondChildStyle = Block.Content.Text.Style.P, - thirdChildStyle = Block.Content.Text.Style.P - ) - - val flow: Flow> = flow { - delay(100) - emit( - listOf( - Event.Command.ShowBlock( - root = root, - blocks = page, - context = root - ) - ) - ) - } - - stubObserveEvents(flow) - stubOpenPage() - stubMergeBlocks(root) - stubUpdateText() - buildViewModel() - - vm.onStart(root) - - coroutineTestRule.advanceTime(100) - - vm.onBlockFocusChanged( - id = thirdChild, - hasFocus = true - ) - - val text = MockDataFactory.randomString() - - vm.onTextChanged( - id = thirdChild, - marks = emptyList(), - text = text - ) - - vm.onNonEmptyBlockBackspaceClicked( - id = thirdChild, - marks = emptyList(), - text = text - ) - - coroutineTestRule.advanceTime(PageViewModel.TEXT_CHANGES_DEBOUNCE_DURATION) - - runBlockingTest { - verify(updateText, times(1)).invoke( - params = eq( - UpdateText.Params( - context = root, - text = text, - marks = emptyList(), - target = thirdChild - ) - ) - ) - } - - runBlockingTest { - verify(mergeBlocks, times(1)).invoke( - params = eq( - MergeBlocks.Params( - context = root, - pair = Pair(secondChild, thirdChild) - ) - ) - ) - } - } - @Test fun `should turn a list item with empty text into a paragraph on endline-enter-pressed event`() { @@ -3975,17 +3888,6 @@ open class PageViewModelTest { } } - private fun stubMergeBlocks(root: String) { - mergeBlocks.stub { - onBlocking { invoke(any()) } doReturn Either.Right( - Payload( - context = root, - events = emptyList() - ) - ) - } - } - fun buildViewModel(urlBuilder: UrlBuilder = builder) { val storage = Editor.Storage() diff --git a/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorMergeTest.kt b/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorMergeTest.kt new file mode 100644 index 0000000000..1c69f1f8ae --- /dev/null +++ b/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorMergeTest.kt @@ -0,0 +1,357 @@ +package com.agileburo.anytype.presentation.page.editor + +import MockDataFactory +import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.agileburo.anytype.domain.block.interactor.MergeBlocks +import com.agileburo.anytype.domain.block.interactor.UpdateText +import com.agileburo.anytype.domain.block.model.Block +import com.agileburo.anytype.domain.ext.content +import com.agileburo.anytype.presentation.MockBlockFactory +import com.agileburo.anytype.presentation.page.PageViewModel +import com.agileburo.anytype.presentation.util.CoroutinesTestRule +import com.nhaarman.mockitokotlin2.eq +import com.nhaarman.mockitokotlin2.times +import com.nhaarman.mockitokotlin2.verifyBlocking +import com.nhaarman.mockitokotlin2.verifyZeroInteractions +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.mockito.MockitoAnnotations + +class EditorMergeTest : EditorPresentationTestSetup() { + + @get:Rule + val rule = InstantTaskExecutorRule() + + @get:Rule + val coroutineTestRule = CoroutinesTestRule() + + @Before + fun setup() { + MockitoAnnotations.initMocks(this) + } + + @Test + fun `should update text and proceed with merging the first paragraph with the second on non-empty-block-backspace-pressed event`() { + + val firstChild = MockDataFactory.randomUuid() + val secondChild = MockDataFactory.randomUuid() + val thirdChild = MockDataFactory.randomUuid() + + val page = MockBlockFactory.makeOnePageWithThreeTextBlocks( + root = root, + firstChild = firstChild, + secondChild = secondChild, + thirdChild = thirdChild, + firstChildStyle = Block.Content.Text.Style.TITLE, + secondChildStyle = Block.Content.Text.Style.P, + thirdChildStyle = Block.Content.Text.Style.P + ) + + stubInterceptEvents() + stubOpenDocument(page) + stubMergeBlocks(root) + stubUpdateText() + + val vm = buildViewModel() + + vm.onStart(root) + + vm.onBlockFocusChanged( + id = thirdChild, + hasFocus = true + ) + + val text = MockDataFactory.randomString() + + vm.onTextChanged( + id = thirdChild, + marks = emptyList(), + text = text + ) + + vm.onNonEmptyBlockBackspaceClicked( + id = thirdChild, + marks = emptyList(), + text = text + ) + + coroutineTestRule.advanceTime(PageViewModel.TEXT_CHANGES_DEBOUNCE_DURATION) + + verifyBlocking(updateText, times(1)) { + invoke( + params = eq( + UpdateText.Params( + context = root, + text = text, + marks = emptyList(), + target = thirdChild + ) + ) + ) + } + + verifyBlocking(mergeBlocks, times(1)) { + invoke( + params = eq( + MergeBlocks.Params( + context = root, + pair = Pair(secondChild, thirdChild) + ) + ) + ) + } + } + + @Test + fun `should merge two text blocks from two different divs`() { + + // SETUP + + val a = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val b = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val div1 = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + children = listOf(a.id), + content = Block.Content.Layout( + type = Block.Content.Layout.Type.DIV + ) + ) + + val div2 = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + children = listOf(b.id), + content = Block.Content.Layout( + type = Block.Content.Layout.Type.DIV + ) + ) + + val page = Block( + id = root, + fields = Block.Fields(emptyMap()), + content = Block.Content.Page( + style = Block.Content.Page.Style.SET + ), + children = listOf(div1.id, div2.id) + ) + + val doc = listOf(page, div1, div2, a, b) + + stubOpenDocument(doc) + stubInterceptEvents() + stubUpdateText() + stubMergeBlocks(root) + + val vm = buildViewModel() + + vm.onStart(root) + + vm.onBlockFocusChanged(b.id, true) + + vm.onNonEmptyBlockBackspaceClicked( + id = b.id, + text = b.content().text, + marks = b.content().marks + ) + + coroutineTestRule.advanceTime(PageViewModel.TEXT_CHANGES_DEBOUNCE_DURATION) + + verifyBlocking(mergeBlocks, times(1)) { + invoke( + params = eq( + MergeBlocks.Params( + context = root, + pair = Pair(a.id, b.id) + ) + ) + ) + } + } + + @Test + fun `should merge two text blocks from the first of two divs`() { + + // SETUP + + val a = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val b = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val c = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val d = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val div1 = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + children = listOf(a.id, b.id), + content = Block.Content.Layout( + type = Block.Content.Layout.Type.DIV + ) + ) + + val div2 = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + children = listOf(c.id, d.id), + content = Block.Content.Layout( + type = Block.Content.Layout.Type.DIV + ) + ) + + val page = Block( + id = root, + fields = Block.Fields(emptyMap()), + content = Block.Content.Page( + style = Block.Content.Page.Style.SET + ), + children = listOf(div1.id, div2.id) + ) + + val doc = listOf(page, div1, div2, a, b, c, d) + + stubOpenDocument(doc) + stubInterceptEvents() + stubUpdateText() + stubMergeBlocks(root) + + val vm = buildViewModel() + + vm.onStart(root) + + vm.onBlockFocusChanged(d.id, true) + + vm.onNonEmptyBlockBackspaceClicked( + id = d.id, + text = d.content().text, + marks = d.content().marks + ) + + coroutineTestRule.advanceTime(PageViewModel.TEXT_CHANGES_DEBOUNCE_DURATION) + + verifyBlocking(mergeBlocks, times(1)) { + invoke( + params = eq( + MergeBlocks.Params( + context = root, + pair = Pair(c.id, d.id) + ) + ) + ) + } + } + + @Test + fun `should not merge text block with the previous block if this previous block is not a text block`() { + + // SETUP + + val a = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Divider, + children = emptyList() + ) + + val b = Block( + id = MockDataFactory.randomUuid(), + fields = Block.Fields(emptyMap()), + content = Block.Content.Text( + text = MockDataFactory.randomString(), + marks = emptyList(), + style = Block.Content.Text.Style.P + ), + children = emptyList() + ) + + val page = Block( + id = root, + fields = Block.Fields(emptyMap()), + content = Block.Content.Page( + style = Block.Content.Page.Style.SET + ), + children = listOf(a.id, b.id) + ) + + val doc = listOf(page, a, b) + + stubOpenDocument(doc) + stubInterceptEvents() + stubUpdateText() + stubMergeBlocks(root) + + val vm = buildViewModel() + + vm.onStart(root) + + vm.onBlockFocusChanged(b.id, true) + + vm.onNonEmptyBlockBackspaceClicked( + id = b.id, + text = b.content().text, + marks = b.content().marks + ) + + coroutineTestRule.advanceTime(PageViewModel.TEXT_CHANGES_DEBOUNCE_DURATION) + + verifyZeroInteractions(mergeBlocks) + } +} \ No newline at end of file diff --git a/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorPresentationTestSetup.kt b/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorPresentationTestSetup.kt index 8150c44f47..ce073723c3 100644 --- a/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorPresentationTestSetup.kt +++ b/presentation/src/test/java/com/agileburo/anytype/presentation/page/editor/EditorPresentationTestSetup.kt @@ -295,4 +295,21 @@ open class EditorPresentationTestSetup { ) } } + + fun stubMergeBlocks(root: String) { + mergeBlocks.stub { + onBlocking { invoke(any()) } doReturn Either.Right( + Payload( + context = root, + events = emptyList() + ) + ) + } + } + + fun stubUpdateText() { + updateText.stub { + onBlocking { invoke(any()) } doReturn Either.Right(Unit) + } + } } \ No newline at end of file