Archives
/
iceraven-browser
mirror of https://github.com/fork-maintainers/iceraven-browser
2
0
Fork 0
Code Issues Releases Wiki Activity

For #15499 - Smoothly updates topSites list on remove

Browse Source 
To remove the flash on refresh of the topsites list we have to use submitList, however using this too high up in the hierarchy of our listAdapters within listAdapters will cause children to refresh at once. The solution to this is to use submitList lower. Using it in TopSitesPagerAdapter.kt to update the TopSitesAdapter is the way to go. I've also had to use a dummy item for the "removed" Topsite ( with id = -1) so I can manually diff that before using submitList.
upstream-sync
codrut.topliceanu 3 years ago committed by mergify[bot]
parent 55fc014071
commit bc723e0a9b
3 changed files with 195 additions and 33 deletions
Show Stats Download Patch File Download Diff File

20
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt
Unescape Escape View File

@ -16,6 +16,7 @@ import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.feature.tab.collections.TabCollection
import mozilla.components.feature.top.sites.TopSite
import mozilla.components.feature.top.sites.TopSite.Type.FRECENT
import mozilla.components.ui.widgets.WidgetSiteItemView
import org.mozilla.fenix.components.Components
import org.mozilla.fenix.components.tips.Tip
@ -62,8 +63,7 @@ sealed class AdapterItem(@LayoutRes val viewType: Int) {
data class TopSitePager(val topSites: List<TopSite>) :
AdapterItem(TopSitePagerViewHolder.LAYOUT_ID) {
override fun sameAs(other: AdapterItem): Boolean {
val newTopSites = (other as? TopSitePager) ?: return false
return newTopSites.topSites.size == this.topSites.size
return other is TopSitePager
}
override fun contentsSameAs(other: AdapterItem): Boolean {
@ -74,14 +74,24 @@ sealed class AdapterItem(@LayoutRes val viewType: Int) {
return newSitesSequence.zip(oldTopSites).all { (new, old) -> new == old }
}
/**
* Returns a payload if there's been a change, or null if not, but adds a "dummy" item for
* each deleted [TopSite]. This is done in order to more easily identify the actual views
* that need to be removed in [TopSitesPagerAdapter.update].
*
* See https://github.com/mozilla-mobile/fenix/pull/20189#issuecomment-877124730
*/
override fun getChangePayload(newItem: AdapterItem): Any? {
val newTopSites = (newItem as? TopSitePager) ?: return null
val oldTopSites = (this as? TopSitePager) ?: return null
val changed = mutableSetOf<Pair<Int, TopSite>>()
for ((index, item) in newTopSites.topSites.withIndex()) {
if (oldTopSites.topSites.getOrNull(index) != item) {
changed.add(Pair(index, item))
for ((index, item) in oldTopSites.topSites.withIndex()) {
val changedItem =
newTopSites.topSites.getOrNull(index) ?: TopSite(-1, "REMOVED", "", 0, FRECENT)
if (changedItem != item) {
changed.add((Pair(index, changedItem)))
}
}
return if (changed.isNotEmpty()) TopSitePagerPayload(changed) else null

36
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesPagerAdapter.kt
Unescape Escape View File

@ -6,6 +6,7 @@ package org.mozilla.fenix.home.sessioncontrol.viewholders.topsites
import android.view.LayoutInflater
import android.view.ViewGroup
import androidx.annotation.VisibleForTesting
import androidx.recyclerview.widget.DiffUtil
import androidx.recyclerview.widget.ListAdapter
import kotlinx.android.synthetic.main.component_top_sites.view.*
@ -42,21 +43,42 @@ class TopSitesPagerAdapter(
}
}
private fun update(
@VisibleForTesting
internal fun update(
payload: TopSitePagerPayload,
position: Int,
adapter: TopSitesAdapter
) {
// Only currently selected page items need to be updated.
for (item in payload.changed) {
if (item.first < TOP_SITES_PER_PAGE && position == 0) {
adapter.notifyItemChanged(item.first, item.second)
} else if (item.first >= TOP_SITES_PER_PAGE && position == 1) {
adapter.notifyItemChanged(item.first - TOP_SITES_PER_PAGE, item.second)
}
val currentPageChangedItems = getCurrentPageChanges(payload, position)
// Build the new list
val refreshedItems: MutableList<TopSite> = mutableListOf()
refreshedItems.addAll(adapter.currentList)
// Update new list based on changed items. Mark any removed items for deletion
val itemsToRemove: MutableList<Int> = mutableListOf()
currentPageChangedItems.forEach { item ->
if (item.second.id == -1L)
itemsToRemove.add(item.first - (position * TOP_SITES_PER_PAGE))
refreshedItems[item.first - (position * TOP_SITES_PER_PAGE)] = item.second
}
// Delete any items marked as such and submit the list, then notify adapter of deletions
itemsToRemove.forEach { refreshedItems.removeAt(it) }
adapter.submitList(refreshedItems)
}
/**
* @returns the changed only items for the currently specified page in [position]
*/
@VisibleForTesting
internal fun getCurrentPageChanges(payload: TopSitePagerPayload, position: Int) =
payload.changed.filter { changedPair ->
if (position == 0) changedPair.first < TOP_SITES_PER_PAGE
else changedPair.first >= TOP_SITES_PER_PAGE
}
override fun onBindViewHolder(holder: TopSiteViewHolder, position: Int) {
val adapter = holder.itemView.top_sites_list.adapter as TopSitesAdapter
adapter.submitList(getItem(position))

172
app/src/test/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesPagerAdapterTest.kt
Unescape Escape View File

@ -4,44 +4,174 @@
package org.mozilla.fenix.home.sessioncontrol.viewholders.topsites
import io.mockk.Runs
import io.mockk.every
import io.mockk.just
import io.mockk.mockk
import io.mockk.spyk
import io.mockk.verify
import mozilla.components.feature.top.sites.TopSite
import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.mozilla.fenix.home.sessioncontrol.AdapterItem
import org.mozilla.fenix.home.sessioncontrol.AdapterItem.TopSitePagerPayload
class TopSitesPagerAdapterTest {
private lateinit var topSitesPagerAdapter: TopSitesPagerAdapter
private val topSite = TopSite(
id = 1L,
title = "Title1",
url = "https://mozilla.org",
null,
DEFAULT
)
private val topSite2 = TopSite(
id = 2L,
title = "Title2",
url = "https://mozilla.org",
null,
DEFAULT
)
private val topSite3 = TopSite(
id = 3L,
title = "Title3",
url = "https://firefox.org",
null,
DEFAULT
)
private val topSite4 = TopSite(
id = 4L,
title = "Title4",
url = "https://firefox.org",
null,
DEFAULT
)
@Before
fun setup() {
topSitesPagerAdapter = spyk(TopSitesPagerAdapter(mockk()))
}
@Test
fun testDiffCallback() {
val topSite = TopSite(
id = 1L,
title = "Title1",
url = "https://mozilla.org",
assertEquals(
TopSitesPagerAdapter.TopSiteListDiffCallback.getChangePayload(
listOf(topSite, topSite3),
listOf(topSite, topSite2)
),
TopSitePagerPayload(setOf(Pair(1, topSite2)))
)
}
@Test
fun `GIVEN a payload with topSites for both pages WHEN getCurrentPageChanges THEN return topSites only for current page`() {
val payload = TopSitePagerPayload(
setOf(
Pair(0, topSite),
Pair(1, topSite2),
Pair(2, topSite3),
Pair(8, topSite4)
)
)
val resultPage1: List<Pair<Int, TopSite>> =
topSitesPagerAdapter.getCurrentPageChanges(payload, 0)
val resultPage2: List<Pair<Int, TopSite>> =
topSitesPagerAdapter.getCurrentPageChanges(payload, 1)
assertEquals(
listOf(Pair(0, topSite),
Pair(1, topSite2),
Pair(2, topSite3)),
resultPage1
)
assertEquals(
listOf(Pair(8, topSite4)),
resultPage2
)
}
@Test
fun `WHEN update is called to delete the 1st of 4 topSites THEN submitList will update 3 topSites`() {
val currentList = mutableListOf(topSite, topSite2, topSite3, topSite4)
val topSitesAdapter: TopSitesAdapter = mockk()
every { topSitesAdapter.currentList } returns currentList
every { topSitesAdapter.submitList(any()) } just Runs
val removedTopSite = TopSite(
id = -1L,
title = "REMOVED",
url = "https://firefox.org",
null,
TopSite.Type.DEFAULT
DEFAULT
)
val payload = TopSitePagerPayload(
setOf(
Pair(0, removedTopSite),
Pair(1, topSite2),
Pair(2, topSite3),
Pair(3, topSite4)
)
)
val topSite2 = TopSite(
id = 2L,
title = "Title2",
url = "https://mozilla.org",
topSitesPagerAdapter.update(payload, 0, topSitesAdapter)
val expected = mutableListOf(topSite2, topSite3, topSite4)
verify { topSitesAdapter.submitList(expected) }
}
@Test
fun `WHEN update is called to delete the 4th of 4 topSites THEN submitList will update 1 topSite`() {
val currentList = mutableListOf(topSite, topSite2, topSite3, topSite4)
val topSitesAdapter: TopSitesAdapter = mockk()
every { topSitesAdapter.currentList } returns currentList
every { topSitesAdapter.submitList(any()) } just Runs
val removedTopSite = TopSite(
id = -1L,
title = "REMOVED",
url = "https://firefox.org",
null,
TopSite.Type.DEFAULT
DEFAULT
)
val payload = TopSitePagerPayload(
setOf(
Pair(3, removedTopSite)
)
)
val topSite3 = TopSite(
topSitesPagerAdapter.update(payload, 0, topSitesAdapter)
val expected = mutableListOf(topSite, topSite2, topSite3)
verify { topSitesAdapter.submitList(expected) }
}
@Test
fun `WHEN update is called to update the 3rd of 4 topSites THEN submitList will contain 4 items`() {
val currentList = mutableListOf(topSite, topSite2, topSite3, topSite4)
val topSitesAdapter: TopSitesAdapter = mockk()
every { topSitesAdapter.currentList } returns currentList
every { topSitesAdapter.submitList(any()) } just Runs
val changedTopSite = TopSite(
id = 3L,
title = "Title2",
title = "CHANGED",
url = "https://firefox.org",
null,
TopSite.Type.DEFAULT
DEFAULT
)
assertEquals(
TopSitesPagerAdapter.TopSiteListDiffCallback.getChangePayload(
listOf(topSite, topSite3),
listOf(topSite, topSite2)
),
AdapterItem.TopSitePagerPayload(setOf(Pair(1, topSite2)))
val payload = TopSitePagerPayload(
setOf(
Pair(2, changedTopSite)
)
)
topSitesPagerAdapter.update(payload, 0, topSitesAdapter)
val expected = mutableListOf(topSite, topSite2, changedTopSite, topSite4)
verify { topSitesAdapter.submitList(expected) }
}
}

Write Preview
Loading…
Cancel
Save