From 05fc30e6e8cf149313c7b8123530937fe842dadc Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Mon, 24 Jul 2023 12:46:17 -0400 Subject: [PATCH] Fix CFv2 initial scrolling bugs. --- .../widget/ConversationLayoutManager.kt | 76 +++++++++++++++++++ .../conversation/MarkReadHelper.java | 36 +++++++-- .../conversation/v2/ConversationFragment.kt | 52 ++++++++++--- 3 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 app/src/main/java/androidx/recyclerview/widget/ConversationLayoutManager.kt diff --git a/app/src/main/java/androidx/recyclerview/widget/ConversationLayoutManager.kt b/app/src/main/java/androidx/recyclerview/widget/ConversationLayoutManager.kt new file mode 100644 index 0000000000..cd1b252301 --- /dev/null +++ b/app/src/main/java/androidx/recyclerview/widget/ConversationLayoutManager.kt @@ -0,0 +1,76 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package androidx.recyclerview.widget + +import android.content.Context +import org.signal.core.util.logging.Log + +/** + * Variation of a vertical, reversed [LinearLayoutManager] that makes specific assumptions in how it will + * be used by Conversation view to support easier scrolling to the initial start position. + * + * Primarily, it assumes that an initial scroll to position call will always happen and that the implementation + * of [LinearLayoutManager] remains unchanged with respect to how it assigns [mPendingScrollPosition] and + * [mPendingScrollPositionOffset] in [LinearLayoutManager.scrollToPositionWithOffset] and how it always clears + * the pending state variables in every call to [LinearLayoutManager.onLayoutCompleted]. + * + * The assumptions are necessary to force the requested scroll position/layout to occur even if the request + * happens prior to the data source populating the recycler view/adapter. + */ +class ConversationLayoutManager(context: Context) : LinearLayoutManager(context, RecyclerView.VERTICAL, true) { + + private var afterScroll: (() -> Unit)? = null + + override fun supportsPredictiveItemAnimations(): Boolean { + return false + } + + /** + * Scroll to the desired position and be notified when the layout manager has completed the request + * via [afterScroll] callback. + */ + fun scrollToPositionWithOffset(position: Int, offset: Int, afterScroll: () -> Unit) { + this.afterScroll = afterScroll + super.scrollToPositionWithOffset(position, offset) + } + + /** + * If a scroll to position request is made and a layout pass occurs prior to the list being populated with via the data source, + * the base implementation clears the request as if it was never made. + * + * This override will capture the pending scroll position and offset, determine if the scroll request was satisfied, and + * re-request the scroll to position to force another attempt if not satisfied. + * + * A pending scroll request will be re-requested if the pending scroll position is outside the bounds of the current known size of + * items in the list. + */ + override fun onLayoutCompleted(state: RecyclerView.State?) { + val pendingScrollPosition = mPendingScrollPosition + val pendingScrollOffset = mPendingScrollPositionOffset + + val reRequestPendingPosition = pendingScrollPosition >= (state?.mItemCount ?: 0) + + // Base implementation always clears mPendingScrollPosition+mPendingScrollPositionOffset + super.onLayoutCompleted(state) + + // Re-request scroll to position request if necessary thus forcing mPendingScrollPosition+mPendingScrollPositionOffset to be re-assigned + if (reRequestPendingPosition) { + Log.d(TAG, "Re-requesting pending scroll position: $pendingScrollPosition offset: $pendingScrollOffset") + if (pendingScrollOffset != INVALID_OFFSET) { + scrollToPositionWithOffset(pendingScrollPosition, pendingScrollOffset) + } else { + scrollToPosition(pendingScrollPosition) + } + } else { + afterScroll?.invoke() + afterScroll = null + } + } + + companion object { + private val TAG = Log.tag(ConversationLayoutManager::class.java) + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/MarkReadHelper.java b/app/src/main/java/org/thoughtcrime/securesms/conversation/MarkReadHelper.java index 223e7da35e..d7bd3b3e4d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/MarkReadHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/MarkReadHelper.java @@ -8,6 +8,7 @@ package org.thoughtcrime.securesms.conversation; import android.content.Context; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import androidx.lifecycle.Lifecycle; import androidx.lifecycle.LifecycleOwner; import androidx.recyclerview.widget.LinearLayoutManager; @@ -39,9 +40,10 @@ public class MarkReadHelper { private final ConversationId conversationId; private final Context context; - private final LifecycleOwner lifecycleOwner; - private final Debouncer debouncer = new Debouncer(DEBOUNCE_TIMEOUT); - private long latestTimestamp; + private final LifecycleOwner lifecycleOwner; + private final Debouncer debouncer = new Debouncer(DEBOUNCE_TIMEOUT); + private long latestTimestamp; + private boolean ignoreViewReveals = false; public MarkReadHelper(@NonNull ConversationId conversationId, @NonNull Context context, @NonNull LifecycleOwner lifecycleOwner) { this.conversationId = conversationId; @@ -50,7 +52,7 @@ public class MarkReadHelper { } public void onViewsRevealed(long timestamp) { - if (timestamp <= latestTimestamp || lifecycleOwner.getLifecycle().getCurrentState() != Lifecycle.State.RESUMED) { + if (timestamp <= latestTimestamp || lifecycleOwner.getLifecycle().getCurrentState() != Lifecycle.State.RESUMED || ignoreViewReveals) { return; } @@ -69,11 +71,33 @@ public class MarkReadHelper { }); } + /** + * Prevent calls to {@link #onViewsRevealed(long)} from causing messages to be marked read. + *

+ * This is particularly useful when the conversation could move around after views are + * displayed (e.g., initial scrolling to start position). + */ + public void ignoreViewReveals() { + ignoreViewReveals = true; + } + + /** + * Stop preventing calls to {@link #onViewsRevealed(long)} from not marking messages as read. + * + * @param timestamp Timestamp of most recent reveal messages, same as usually provided to {@code onViewsRevealed} + */ + public void stopIgnoringViewReveals(@Nullable Long timestamp) { + this.ignoreViewReveals = false; + if (timestamp != null) { + onViewsRevealed(timestamp); + } + } + /** * Given the adapter and manager, figure out the timestamp to mark read up to. * - * @param conversationAdapter The conversation thread's adapter - * @param layoutManager The conversation thread's layout manager + * @param conversationAdapter The conversation thread's adapter + * @param layoutManager The conversation thread's layout manager * @return A Present(Long) if there's a timestamp to proceed with, or Empty if this request should be ignored. */ @SuppressWarnings("resource") diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt index b72f78bae8..f4c482c6ab 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt @@ -60,6 +60,7 @@ import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.Observer +import androidx.recyclerview.widget.ConversationLayoutManager import androidx.recyclerview.widget.LinearLayoutManager import androidx.recyclerview.widget.RecyclerView import com.google.android.material.dialog.MaterialAlertDialogBuilder @@ -118,7 +119,6 @@ import org.thoughtcrime.securesms.components.location.SignalPlace import org.thoughtcrime.securesms.components.mention.MentionAnnotation import org.thoughtcrime.securesms.components.menu.ActionItem import org.thoughtcrime.securesms.components.menu.SignalBottomActionBar -import org.thoughtcrime.securesms.components.recyclerview.SmoothScrollingLinearLayoutManager import org.thoughtcrime.securesms.components.settings.app.subscription.donate.DonateToSignalFragment import org.thoughtcrime.securesms.components.settings.app.subscription.donate.DonateToSignalType import org.thoughtcrime.securesms.components.settings.conversation.ConversationSettingsActivity @@ -442,7 +442,7 @@ class ConversationFragment : private val textDraftSaveDebouncer = Debouncer(500) private val recentEmojis: RecentEmojiPageModel by lazy { RecentEmojiPageModel(ApplicationDependencies.getApplication(), TextSecurePreferences.RECENT_STORAGE_KEY) } - private lateinit var layoutManager: LinearLayoutManager + private lateinit var layoutManager: ConversationLayoutManager private lateinit var markReadHelper: MarkReadHelper private lateinit var giphyMp4ProjectionRecycler: GiphyMp4ProjectionRecycler private lateinit var addToContactsLauncher: ActivityResultLauncher @@ -519,6 +519,7 @@ class ConversationFragment : FullscreenHelper(requireActivity()).showSystemUI() markReadHelper = MarkReadHelper(ConversationId.forConversation(args.threadId), requireContext(), viewLifecycleOwner) + markReadHelper.ignoreViewReveals() initializeConversationThreadUi() @@ -778,7 +779,6 @@ class ConversationFragment : binding.conversationItemRecycler.doAfterNextLayout { SignalLocalMetrics.ConversationOpen.onRenderFinished() doAfterFirstRender() - animationsAllowed = true } } } @@ -1395,7 +1395,7 @@ class ConversationFragment : private fun getVoiceNoteMediaController() = requireListener().voiceNoteMediaController private fun initializeConversationThreadUi() { - layoutManager = SmoothScrollingLinearLayoutManager(requireContext(), true) + layoutManager = ConversationLayoutManager(requireContext()) binding.conversationItemRecycler.setHasFixedSize(false) binding.conversationItemRecycler.layoutManager = layoutManager binding.conversationItemRecycler.addOnScrollListener(ScrollListener()) @@ -2105,15 +2105,43 @@ class ConversationFragment : //region Scroll Handling private fun moveToStartPosition(meta: ConversationData) { - scrollToPositionDelegate.requestScrollPosition( - position = meta.getStartPosition(), - smooth = true, - scrollStrategy = if (meta.shouldJumpToMessage()) { - jumpAndPulseScrollStrategy - } else { - ScrollToPositionDelegate.DefaultScrollStrategy + if (meta.getStartPosition() == 0) { + layoutManager.scrollToPositionWithOffset(0, 0) { + animationsAllowed = true + markReadHelper.stopIgnoringViewReveals(MarkReadHelper.getLatestTimestamp(adapter, layoutManager).orNull()) } - ) + } else { + binding.toolbar.viewTreeObserver.addOnGlobalLayoutListener(StartPositionScroller(meta)) + } + } + + /** Helper to scroll the conversation to the correct position and offset based on toolbar height and the type of position */ + private inner class StartPositionScroller(private val meta: ConversationData) : ViewTreeObserver.OnGlobalLayoutListener { + + override fun onGlobalLayout() { + val rect = Rect() + binding.toolbar.getGlobalVisibleRect(rect) + val toolbarOffset = rect.bottom + binding.toolbar.viewTreeObserver.removeOnGlobalLayoutListener(this) + + val offset = when { + meta.getStartPosition() == 0 -> 0 + meta.shouldJumpToMessage() -> (binding.conversationItemRecycler.height - toolbarOffset) / 4 + meta.shouldScrollToLastSeen() -> binding.conversationItemRecycler.height - toolbarOffset + else -> binding.conversationItemRecycler.height + } + + Log.d(TAG, "Scrolling to start position ${meta.getStartPosition()}") + layoutManager.scrollToPositionWithOffset(meta.getStartPosition(), offset) { + animationsAllowed = true + markReadHelper.stopIgnoringViewReveals(MarkReadHelper.getLatestTimestamp(adapter, layoutManager).orNull()) + if (meta.shouldJumpToMessage()) { + binding.conversationItemRecycler.post { + adapter.pulseAtPosition(meta.getStartPosition()) + } + } + } + } } /**