Skip to content

Commit

Permalink
Improve and debug ghost swipe situation
Browse files Browse the repository at this point in the history 
Change-Id: Ic6f2956a139516050e2b7e5ec248969326b7ec43
  • Loading branch information
@SpiritCroc
SpiritCroc committed May 11, 2022
1 parent b2a6252 commit 8855665
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 27 deletions.
35 changes: 27 additions & 8 deletions vector/src/main/java/im/vector/app/AppStateHandler.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.asFlow
import arrow.core.Option
import arrow.core.getOrElse
import de.spiritcroc.matrixsdk.util.DbgUtil
import im.vector.app.core.di.ActiveSessionHolder
import im.vector.app.core.utils.BehaviorDataSource
import im.vector.app.features.analytics.AnalyticsTracker
Expand All @@ -45,6 +46,7 @@ import org.matrix.android.sdk.api.session.getRoomSummary
import org.matrix.android.sdk.api.session.group.model.GroupSummary
import org.matrix.android.sdk.api.session.initsync.SyncStatusService
import org.matrix.android.sdk.api.session.room.model.RoomSummary
import timber.log.Timber
import javax.inject.Inject
import javax.inject.Singleton

Expand All @@ -53,6 +55,17 @@ sealed class RoomGroupingMethod {
data class BySpace(val spaceSummary: RoomSummary?) : RoomGroupingMethod()
}

enum class SelectSpaceFrom {
// Initialized / uiStateRepository
INIT,
// Swiped in home pager
SWIPE,
// Persisted after swipe in home pager
PERSIST_SWIPE,
// Selected from non-pager UI
SELECT,
}

fun RoomGroupingMethod.space() = (this as? RoomGroupingMethod.BySpace)?.spaceSummary
fun RoomGroupingMethod.group() = (this as? RoomGroupingMethod.ByLegacyGroup)?.groupSummary

Expand All @@ -74,10 +87,11 @@ class AppStateHandler @Inject constructor(

// SchildiChat: the boolean means pendingSwipe and defaults to false. Set to true for swiping spaces, so you want to ignore updates which have pendingSwipe = true.
// Call it different then the upstream one so we don't forget adding `first` to upstream's logic.
private val selectedSpaceDataSourceSc = BehaviorDataSource<Option<Pair<RoomGroupingMethod, Boolean>>>(Option.empty())
private val selectedSpaceDataSourceSc = BehaviorDataSource<Option<Pair<RoomGroupingMethod, SelectSpaceFrom>>>(Option.empty())

val selectedRoomGroupingFlow = selectedSpaceDataSourceSc.stream().map { it.map { it.first } }
val selectedRoomGroupingFlowIgnoreSwipe = selectedSpaceDataSourceSc.stream().filter { it.getOrElse{ null }?.second != true }.map { it.map { it.first } }
val selectedRoomGroupingFlowIgnoreSwipe = selectedSpaceDataSourceSc.stream()
.filter { it.getOrElse{ null }?.second != SelectSpaceFrom.SWIPE }

fun getCurrentRoomGroupingMethod(): RoomGroupingMethod? {
// XXX we should somehow make it live :/ just a work around
Expand All @@ -94,7 +108,12 @@ class AppStateHandler @Inject constructor(
}
}

fun setCurrentSpace(spaceId: String?, session: Session? = null, persistNow: Boolean = false, pendingSwipe: Boolean = false) {
fun setCurrentSpace(spaceId: String?, session: Session? = null, persistNow: Boolean = false, from: SelectSpaceFrom = SelectSpaceFrom.SELECT) {
if (DbgUtil.isDbgEnabled(DbgUtil.DBG_VIEW_PAGER) && from == SelectSpaceFrom.SELECT) {
// We want a stack trace
Timber.w(Exception("Home pager: setCurrentSpace/SELECT"))
}

val uSession = session ?: activeSessionHolder.getSafeActiveSession() ?: return
if (selectedSpaceDataSourceSc.currentValue?.orNull()?.first is RoomGroupingMethod.BySpace &&
spaceId == selectedSpaceDataSourceSc.currentValue?.orNull()?.first?.space()?.roomId) return
Expand All @@ -105,7 +124,7 @@ class AppStateHandler @Inject constructor(
uiStateRepository.storeSelectedSpace(spaceSum?.roomId, uSession.sessionId)
}

selectedSpaceDataSourceSc.post(Option.just(Pair(RoomGroupingMethod.BySpace(spaceSum), pendingSwipe)))
selectedSpaceDataSourceSc.post(Option.just(Pair(RoomGroupingMethod.BySpace(spaceSum), from)))
if (spaceId != null) {
uSession.coroutineScope.launch(Dispatchers.IO) {
tryOrNull {
Expand All @@ -120,7 +139,7 @@ class AppStateHandler @Inject constructor(
if (selectedSpaceDataSourceSc.currentValue?.orNull()?.first is RoomGroupingMethod.ByLegacyGroup &&
groupId == selectedSpaceDataSourceSc.currentValue?.orNull()?.first?.group()?.groupId) return
val activeGroup = groupId?.let { uSession.groupService().getGroupSummary(groupId) }
selectedSpaceDataSourceSc.post(Option.just(Pair(RoomGroupingMethod.ByLegacyGroup(activeGroup), false)))
selectedSpaceDataSourceSc.post(Option.just(Pair(RoomGroupingMethod.ByLegacyGroup(activeGroup), SelectSpaceFrom.SELECT)))
if (groupId != null) {
uSession.coroutineScope.launch {
tryOrNull {
Expand All @@ -137,7 +156,7 @@ class AppStateHandler @Inject constructor(
// sessionDataSource could already return a session while activeSession holder still returns null
it.orNull()?.let { session ->
if (uiStateRepository.isGroupingMethodSpace(session.sessionId)) {
setCurrentSpace(uiStateRepository.getSelectedSpace(session.sessionId), session)
setCurrentSpace(uiStateRepository.getSelectedSpace(session.sessionId), session, from = SelectSpaceFrom.INIT)
} else {
setCurrentGroup(uiStateRepository.getSelectedGroup(session.sessionId), session)
}
Expand Down Expand Up @@ -193,8 +212,8 @@ class AppStateHandler @Inject constructor(
val uSession = activeSessionHolder.getSafeActiveSession() ?: return

// We want to persist it, so we also want to remove the pendingSwipe status
if (currentValue.second) {
selectedSpaceDataSourceSc.post(Option.just(Pair(currentMethod, false)))
if (currentValue.second == SelectSpaceFrom.SWIPE) {
selectedSpaceDataSourceSc.post(Option.just(Pair(currentMethod, SelectSpaceFrom.PERSIST_SWIPE)))
}

// Persist it across app restarts
Expand Down
52 changes: 35 additions & 17 deletions vector/src/main/java/im/vector/app/features/home/HomeDetailFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ import com.airbnb.mvrx.fragmentViewModel
import com.airbnb.mvrx.withState
import com.google.android.material.badge.BadgeDrawable
import de.spiritcroc.matrixsdk.util.DbgUtil
import de.spiritcroc.matrixsdk.util.Dimber
import de.spiritcroc.viewpager.reduceDragSensitivity
import im.vector.app.AppStateHandler
import im.vector.app.R
import im.vector.app.RoomGroupingMethod
import im.vector.app.SelectSpaceFrom
import im.vector.app.core.extensions.restart
import im.vector.app.core.extensions.toMvRxBundle
import im.vector.app.core.platform.StateView
Expand Down Expand Up @@ -83,6 +85,7 @@ class HomeDetailFragment @Inject constructor(
CurrentCallsView.Callback {

private val DEBUG_VIEW_PAGER = DbgUtil.isDbgEnabled(DbgUtil.DBG_VIEW_PAGER)
private val viewPagerDimber = Dimber("Home pager", DbgUtil.DBG_VIEW_PAGER)

private val viewModel: HomeDetailViewModel by fragmentViewModel()
private val unknownDeviceDetectorSharedViewModel: UnknownDeviceDetectorSharedViewModel by activityViewModel()
Expand Down Expand Up @@ -153,7 +156,7 @@ class HomeDetailFragment @Inject constructor(
// space pager: update appStateHandler's current page to update rest of the UI accordingly
views.roomListContainerPager.registerOnPageChangeCallback(object : ViewPager2.OnPageChangeCallback() {
override fun onPageSelected(position: Int) {
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: selected page $position $initialPageSelected")
viewPagerDimber.i{"Home pager: selected page $position $initialPageSelected"}
super.onPageSelected(position)
if (!initialPageSelected) {
// Do not apply before we have restored the previous value
Expand All @@ -164,13 +167,7 @@ class HomeDetailFragment @Inject constructor(
initialPageSelected = true
}
}
val selectedId = getSpaceIdForPageIndex(position)
appStateHandler.setCurrentSpace(selectedId, pendingSwipe = true)
if (pagerPagingEnabled) {
onSpaceChange(
selectedId?.let { viewModel.getRoom(it)?.roomSummary() }
)
}
selectSpaceFromSwipe(position)
}
})

Expand Down Expand Up @@ -255,6 +252,16 @@ class HomeDetailFragment @Inject constructor(
}
}

private fun selectSpaceFromSwipe(position: Int) {
val selectedId = getSpaceIdForPageIndex(position)
appStateHandler.setCurrentSpace(selectedId, from = SelectSpaceFrom.SWIPE)
if (pagerPagingEnabled) {
onSpaceChange(
selectedId?.let { viewModel.getRoom(it)?.roomSummary() }
)
}
}

private fun handleCallStarted() {
dismissLoadingDialog()
val fragmentTag = HomeTab.DialPad.toFragmentTag()
Expand Down Expand Up @@ -542,17 +549,18 @@ class HomeDetailFragment @Inject constructor(
}
*/

private fun setupViewPager(roomGroupingMethod: RoomGroupingMethod, spaces: List<RoomSummary>?, tab: HomeTab) {
private fun setupViewPager(roomGroupingMethodPair: Pair<RoomGroupingMethod, SelectSpaceFrom>, spaces: List<RoomSummary>?, tab: HomeTab) {
val roomGroupingMethod = roomGroupingMethodPair.first
val oldAdapter = views.roomListContainerPager.adapter as? FragmentStateAdapter
val pagingAllowed = vectorPreferences.enableSpacePager() && tab is HomeTab.RoomList
if (pagingAllowed && spaces == null) {
Timber.i("Home pager: Skip initial setup, root spaces not known yet")
viewPagerDimber.i{"Home pager: Skip initial setup, root spaces not known yet"}
views.roomListContainerStateView.state = StateView.State.Loading
return
} else {
views.roomListContainerStateView.state = StateView.State.Content
}
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: setup, old adapter: $oldAdapter")
viewPagerDimber.i{"Home pager: setup, old adapter: $oldAdapter"}
val unsafeSpaces = spaces?.map { it.roomId } ?: listOf()
val selectedSpaceId = (roomGroupingMethod as? RoomGroupingMethod.BySpace)?.spaceSummary?.roomId
val selectedIndex = getPageIndexForSpaceId(selectedSpaceId, unsafeSpaces)
Expand All @@ -561,10 +569,20 @@ class HomeDetailFragment @Inject constructor(
// Check if we need to recreate the adapter for a new tab
if (oldAdapter != null) {
val changed = pagerTab != tab || pagerSpaces != safeSpaces || pagerPagingEnabled != pagingEnabled
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: has changed: $changed (${pagerTab != tab} ${pagerSpaces != safeSpaces} ${pagerPagingEnabled != pagingEnabled} $selectedIndex ${views.roomListContainerPager.currentItem}) | $safeSpaces")
viewPagerDimber.i{ "has changed: $changed (${pagerTab != tab} ${pagerSpaces != safeSpaces} ${pagerPagingEnabled != pagingEnabled} $selectedIndex ${roomGroupingMethodPair.second} ${views.roomListContainerPager.currentItem}) | $safeSpaces" }
if (!changed) {
// No need to re-setup pager, just check for selected page
if (pagingEnabled) {
// No need to re-setup pager, just check for selected page
// Prioritize the actually displayed space for all space changes except for SELECT ones, to avoid
// unexpected page changes onResume for old updates
if (roomGroupingMethodPair.second != SelectSpaceFrom.SELECT) {
// Tell the rest of the UI that we want to keep displaying the current space
viewPagerDimber.i { "Discard space change from ${roomGroupingMethodPair.second} (${selectedSpaceId}/$selectedIndex), persist own (${views.roomListContainerPager.currentItem})" }
if (views.roomListContainerPager.currentItem != selectedIndex) {
selectSpaceFromSwipe(views.roomListContainerPager.currentItem)
}
return
}
if (selectedIndex != null) {
if (selectedIndex != views.roomListContainerPager.currentItem) {
// post() mitigates a case where we could end up in an endless loop circling around the same few spaces
Expand Down Expand Up @@ -613,16 +631,16 @@ class HomeDetailFragment @Inject constructor(
}

override fun createFragment(position: Int): Fragment {
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: create fragment for position $position")
viewPagerDimber.i{"Home pager: create fragment for position $position"}
return when (tab) {
is HomeTab.DialPad -> createDialPadFragment()
is HomeTab.RoomList -> {
val params = if (pagingEnabled) {
val spaceId = getSpaceIdForPageIndex(position)
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: position $position -> space $spaceId")
viewPagerDimber.i{"Home pager: position $position -> space $spaceId"}
RoomListParams(tab.displayMode, spaceId).toMvRxBundle()
} else {
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: paging disabled; position $position -> follow")
viewPagerDimber.i{"Home pager: paging disabled; position $position -> follow"}
RoomListParams(tab.displayMode, SPACE_ID_FOLLOW_APP).toMvRxBundle()
}
childFragmentManager.fragmentFactory.instantiate(activity!!.classLoader, RoomListFragment::class.java.name).apply {
Expand All @@ -645,7 +663,7 @@ class HomeDetailFragment @Inject constructor(
return
}
try {
if (DEBUG_VIEW_PAGER) Timber.i("Home pager: set initial page $selectedIndex")
viewPagerDimber.i{"Home pager: set initial page $selectedIndex"}
views.roomListContainerPager.setCurrentItem(selectedIndex ?: 0, false)
initialPageSelected = true
} catch (e: Exception) {
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class HomeDetailViewModel @AssistedInject constructor(
appStateHandler.selectedRoomGroupingFlowIgnoreSwipe
.setOnEach {
copy(
roomGroupingMethodIgnoreSwipe = it.orNull() ?: RoomGroupingMethod.BySpace(null)
roomGroupingMethodIgnoreSwipe = it.orNull()
)
}
}
Expand Down
3 changes: 2 additions & 1 deletion vector/src/main/java/im/vector/app/features/home/HomeDetailViewState.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,15 @@ import com.airbnb.mvrx.MavericksState
import com.airbnb.mvrx.Uninitialized
import im.vector.app.R
import im.vector.app.RoomGroupingMethod
import im.vector.app.SelectSpaceFrom
import org.matrix.android.sdk.api.session.initsync.SyncStatusService
import org.matrix.android.sdk.api.session.room.model.RoomSummary
import org.matrix.android.sdk.api.session.sync.SyncState
import org.matrix.android.sdk.api.util.MatrixItem

data class HomeDetailViewState(
val roomGroupingMethod: RoomGroupingMethod = RoomGroupingMethod.BySpace(null),
val roomGroupingMethodIgnoreSwipe: RoomGroupingMethod? = null,
val roomGroupingMethodIgnoreSwipe: Pair<RoomGroupingMethod, SelectSpaceFrom>? = null,
val myMatrixItem: MatrixItem? = null,
val asyncRooms: Async<List<RoomSummary>> = Uninitialized,
val currentTab: HomeTab = HomeTab.RoomList(RoomListDisplayMode.ALL),
Expand Down

0 comments on commit 8855665

Please sign in to comment.