From 6d8cfe1a508e448f49fbce41f2af6b49c587e82a Mon Sep 17 00:00:00 2001 From: Kainalu Hagiwara Date: Tue, 4 Aug 2020 16:26:03 -0700 Subject: [PATCH] For #13156, #13280 - Use payloads to bind bookmark viewholders. Change the DiffUtil callback for bookmarks to use the generated equals() method instead of only checking the title and url fields. This prevents the BookmarkNode in our state from getting out of sync with the BookmarkNode the viewholder is bound to. --- .../library/bookmarks/BookmarkAdapter.kt | 42 +++++++++++++++++- .../fenix/library/bookmarks/BookmarkView.kt | 4 -- .../viewholders/BookmarkFolderViewHolder.kt | 31 +++++++++---- .../viewholders/BookmarkItemViewHolder.kt | 44 ++++++++++++++----- .../viewholders/BookmarkNodeViewHolder.kt | 7 +++ .../BookmarkSeparatorViewHolder.kt | 9 ++++ 6 files changed, 111 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt index 8aafaaed1..ffcea7455 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt @@ -59,9 +59,20 @@ class BookmarkAdapter(val emptyView: View, val interactor: BookmarkViewInteracto old[oldItemPosition].guid == new[newItemPosition].guid override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean = + oldMode::class == newMode::class && old[oldItemPosition] in oldMode.selectedItems == new[newItemPosition] in newMode.selectedItems && - old[oldItemPosition].title == new[newItemPosition].title && - old[oldItemPosition].url == new[newItemPosition].url + old[oldItemPosition] == new[newItemPosition] + + override fun getChangePayload(oldItemPosition: Int, newItemPosition: Int): Any? { + val oldItem = old[oldItemPosition] + val newItem = new[newItemPosition] + return BookmarkPayload( + titleChanged = oldItem.title != newItem.title, + urlChanged = oldItem.url != newItem.url, + selectedChanged = oldItem in oldMode.selectedItems != newItem in newMode.selectedItems, + modeChanged = oldMode::class != newMode::class + ) + } override fun getOldListSize(): Int = old.size override fun getNewListSize(): Int = new.size @@ -90,9 +101,36 @@ class BookmarkAdapter(val emptyView: View, val interactor: BookmarkViewInteracto override fun getItemCount(): Int = tree.size + override fun onBindViewHolder( + holder: BookmarkNodeViewHolder, + position: Int, + payloads: MutableList + ) { + if (payloads.isNotEmpty() && payloads[0] is BookmarkPayload) { + holder.bind(tree[position], mode, payloads[0] as BookmarkPayload) + } else { + super.onBindViewHolder(holder, position, payloads) + } + } + override fun onBindViewHolder(holder: BookmarkNodeViewHolder, position: Int) { holder.bind(tree[position], mode) } } +/** + * A RecyclerView Adapter payload class that contains information about changes to a [BookmarkNode]. + * + * @property titleChanged true if there has been a change to [BookmarkNode.title]. + * @property urlChanged true if there has been a change to [BookmarkNode.url]. + * @property selectedChanged true if there has been a change in the BookmarkNode's selected state. + * @property modeChanged true if there has been a change in the state's mode type. + */ +data class BookmarkPayload( + val titleChanged: Boolean, + val urlChanged: Boolean, + val selectedChanged: Boolean, + val modeChanged: Boolean +) + fun BookmarkNode.inRoots() = enumValues().any { it.id == guid } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt index b9f3c6911..6d4b0c690 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt @@ -139,7 +139,6 @@ class BookmarkView( } fun update(state: BookmarkFragmentState) { - val oldMode = mode tree = state.tree if (state.mode != mode) { mode = state.mode @@ -149,9 +148,6 @@ class BookmarkView( } bookmarkAdapter.updateData(state.tree, mode) - if (state.mode != oldMode) { - bookmarkAdapter.notifyDataSetChanged() - } when (mode) { is BookmarkFragmentState.Mode.Normal -> { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt index 5941dc4da..70e418b98 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt @@ -14,6 +14,7 @@ import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.library.LibrarySiteItemView import org.mozilla.fenix.library.SelectionHolder import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState +import org.mozilla.fenix.library.bookmarks.BookmarkPayload import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor import org.mozilla.fenix.library.bookmarks.inRoots @@ -28,28 +29,39 @@ class BookmarkFolderViewHolder( override var item: BookmarkNode? = null + init { + containerView.displayAs(LibrarySiteItemView.ItemType.FOLDER) + } + override fun bind( item: BookmarkNode, mode: BookmarkFragmentState.Mode ) { - this.item = item + bind(item, mode, BookmarkPayload(true, true, true, true)) + } - containerView.displayAs(LibrarySiteItemView.ItemType.FOLDER) + override fun bind(item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload) { + this.item = item setSelectionListeners(item, selectionHolder) if (!item.inRoots()) { setupMenu(item) - if (mode is BookmarkFragmentState.Mode.Selecting) { - containerView.overflowView.hideAndDisable() - } else { - containerView.overflowView.showAndEnable() + if (payload.modeChanged) { + if (mode is BookmarkFragmentState.Mode.Selecting) { + containerView.overflowView.hideAndDisable() + } else { + containerView.overflowView.showAndEnable() + } } } else { containerView.overflowView.visibility = View.GONE } - containerView.changeSelected(item in selectionHolder.selectedItems) + if (payload.selectedChanged) { + containerView.changeSelected(item in selectionHolder.selectedItems) + } + containerView.iconView.setImageDrawable( AppCompatResources.getDrawable( containerView.context, @@ -63,6 +75,9 @@ class BookmarkFolderViewHolder( ) } ) - containerView.titleView.text = item.title + + if (payload.titleChanged) { + containerView.titleView.text = item.title + } } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt index 6ef5f87a3..840b716ef 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt @@ -10,6 +10,7 @@ import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.library.LibrarySiteItemView import org.mozilla.fenix.library.SelectionHolder import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState +import org.mozilla.fenix.library.bookmarks.BookmarkPayload import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor /** @@ -23,27 +24,46 @@ class BookmarkItemViewHolder( override var item: BookmarkNode? = null + init { + containerView.displayAs(LibrarySiteItemView.ItemType.SITE) + } + override fun bind( item: BookmarkNode, mode: BookmarkFragmentState.Mode ) { + bind(item, mode, BookmarkPayload(true, true, true, true)) + } + + override fun bind(item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload) { this.item = item - containerView.displayAs(LibrarySiteItemView.ItemType.SITE) - - if (mode is BookmarkFragmentState.Mode.Selecting) { - containerView.overflowView.hideAndDisable() - } else { - containerView.overflowView.showAndEnable() - } setupMenu(item) - containerView.titleView.text = if (item.title.isNullOrBlank()) item.url else item.title - containerView.urlView.text = item.url + + if (payload.modeChanged) { + if (mode is BookmarkFragmentState.Mode.Selecting) { + containerView.overflowView.hideAndDisable() + } else { + containerView.overflowView.showAndEnable() + } + } + + if (payload.selectedChanged) { + containerView.changeSelected(item in selectionHolder.selectedItems) + } + + if (payload.titleChanged) { + containerView.titleView.text = if (item.title.isNullOrBlank()) item.url else item.title + } else if (payload.urlChanged && item.title.isNullOrBlank()) { + containerView.titleView.text = item.url + } + + if (payload.urlChanged) { + containerView.urlView.text = item.url + setColorsAndIcons(item.url) + } setSelectionListeners(item, selectionHolder) - - containerView.changeSelected(item in selectionHolder.selectedItems) - setColorsAndIcons(item.url) } private fun setColorsAndIcons(url: String?) { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt index 9e6d7caee..751323467 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt @@ -11,6 +11,7 @@ import org.mozilla.fenix.library.LibrarySiteItemView import org.mozilla.fenix.library.SelectionHolder import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu +import org.mozilla.fenix.library.bookmarks.BookmarkPayload import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor /** @@ -28,6 +29,12 @@ abstract class BookmarkNodeViewHolder( mode: BookmarkFragmentState.Mode ) + abstract fun bind( + item: BookmarkNode, + mode: BookmarkFragmentState.Mode, + payload: BookmarkPayload + ) + protected fun setSelectionListeners(item: BookmarkNode, selectionHolder: SelectionHolder) { containerView.setSelectionInteractor(item, selectionHolder, interactor) } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt index daafbf0ba..d1789fa94 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt @@ -7,6 +7,7 @@ package org.mozilla.fenix.library.bookmarks.viewholders import mozilla.components.concept.storage.BookmarkNode import org.mozilla.fenix.library.LibrarySiteItemView import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState +import org.mozilla.fenix.library.bookmarks.BookmarkPayload import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor /** @@ -27,4 +28,12 @@ class BookmarkSeparatorViewHolder( containerView.displayAs(LibrarySiteItemView.ItemType.SEPARATOR) setupMenu(item) } + + override fun bind( + item: BookmarkNode, + mode: BookmarkFragmentState.Mode, + payload: BookmarkPayload + ) { + bind(item, mode) + } }