Fix unread divider placement and scroll-to-unread on conversation open.

app/src/main/java/androidx/recyclerview/widget/ConversationLayoutManager.kt

@@ -24,6 +24,11 @@ class ConversationLayoutManager(context: Context) : LinearLayoutManager(context,
 
   private var afterScroll: (() -> Unit)? = null
 
+  // Backing state for scrollToPositionTopAligned; alignTopCorrected guards the one-shot corrective re-scroll.
+  private var alignTopPosition: Int = RecyclerView.NO_POSITION
+  private var alignTopInset: Int = 0
+  private var alignTopCorrected: Boolean = false
+
   override fun supportsPredictiveItemAnimations(): Boolean {
     return false
   }
@@ -34,9 +39,23 @@ class ConversationLayoutManager(context: Context) : LinearLayoutManager(context,
    */
   fun scrollToPositionWithOffset(position: Int, offset: Int, afterScroll: () -> Unit) {
     this.afterScroll = afterScroll
+    alignTopPosition = RecyclerView.NO_POSITION
     super.scrollToPositionWithOffset(position, offset)
   }
 
+  /**
+   * Scroll so [position]'s decorated top (including any top decoration, e.g. the unread divider) lands [topInset] px
+   * below the top of the recycler. [afterScroll] fires once the alignment settles.
+   */
+  fun scrollToPositionTopAligned(position: Int, topInset: Int, afterScroll: () -> Unit) {
+    this.afterScroll = afterScroll
+    alignTopPosition = position
+    alignTopInset = topInset
+    alignTopCorrected = false
+    // Rough first pass: the exact offset needs the item's height, which isn't known until it's laid out (see onLayoutCompleted).
+    super.scrollToPositionWithOffset(position, height - topInset)
+  }
+
   /**
    * 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.
@@ -64,10 +83,26 @@ class ConversationLayoutManager(context: Context) : LinearLayoutManager(context,
       } else {
         scrollToPosition(pendingScrollPosition)
       }
-    } else {
-      afterScroll?.invoke()
-      afterScroll = null
+      return
     }
+
+    // The target is now laid out, so its height is known. Correct the offset once so the decorated top sits at the
+    // requested inset, then let the next layout settle before notifying via afterScroll.
+    if (alignTopPosition != RecyclerView.NO_POSITION && !alignTopCorrected) {
+      val target = findViewByPosition(alignTopPosition)
+      if (target != null) {
+        alignTopCorrected = true
+        if (getDecoratedTop(target) != alignTopInset) {
+          val correctedOffset = (height - paddingBottom) - alignTopInset - getDecoratedMeasuredHeight(target)
+          super.scrollToPositionWithOffset(alignTopPosition, correctedOffset)
+          return
+        }
+      }
+    }
+
+    afterScroll?.invoke()
+    afterScroll = null
+    alignTopPosition = RecyclerView.NO_POSITION
   }
 
   companion object {

app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationData.kt

@@ -9,8 +9,8 @@ import org.thoughtcrime.securesms.recipients.Recipient
 data class ConversationData(
   val threadRecipient: Recipient,
   val threadId: Long,
-  val lastSeen: Long,
-  val lastSeenPosition: Int,
+  val firstUnreadId: Long,
+  val firstUnreadPosition: Int,
   val lastScrolledPosition: Int,
   val jumpToPosition: Int,
   val threadSize: Int,
@@ -24,14 +24,14 @@ data class ConversationData(
     return jumpToPosition >= 0
   }
 
-  fun shouldScrollToLastSeen(): Boolean {
-    return lastSeenPosition > 0
+  fun shouldScrollToFirstUnread(): Boolean {
+    return firstUnreadPosition > 0
   }
 
   fun getStartPosition(): Int {
     return when {
       shouldJumpToMessage() -> jumpToPosition
-      messageRequestData.isMessageRequestAccepted && shouldScrollToLastSeen() -> lastSeenPosition
+      messageRequestData.isMessageRequestAccepted && shouldScrollToFirstUnread() -> firstUnreadPosition
       messageRequestData.isMessageRequestAccepted -> lastScrolledPosition
       else -> threadSize
     }

app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationRepository.java

@@ -50,8 +50,10 @@ public class ConversationRepository {
   public @NonNull ConversationData getConversationData(long threadId, @NonNull Recipient conversationRecipient, int jumpToPosition) {
     ThreadTable.ConversationMetadata    metadata                       = SignalDatabase.threads().getConversationMetadata(threadId);
     int                                 threadSize                     = SignalDatabase.messages().getMessageCountForThread(threadId);
-    long                                lastSeen                       = metadata.getLastSeen();
-    int                                 lastSeenPosition               = 0;
+    MessageTable.OldestUnread           oldestUnread                   = metadata.getUnreadCount() > 0 ? SignalDatabase.messages().getOldestUnread(threadId) : null;
+    long                                firstUnreadId                  = oldestUnread != null ? oldestUnread.getId() : -1;
+    long                                firstUnreadDateReceived        = oldestUnread != null ? oldestUnread.getDateReceived() : 0;
+    int                                 firstUnreadPosition            = 0;
     long                                lastScrolled                   = metadata.getLastScrolled();
     int                                 lastScrolledPosition           = 0;
     boolean                             isMessageRequestAccepted       = RecipientUtil.isMessageRequestAccepted(threadId);
@@ -59,15 +61,16 @@ public class ConversationRepository {
     ConversationData.MessageRequestData messageRequestData             = new ConversationData.MessageRequestData(isMessageRequestAccepted, isConversationHidden);
     boolean                             showUniversalExpireTimerUpdate = false;
 
-    if (lastSeen > 0) {
-      lastSeenPosition = SignalDatabase.messages().getMessagePositionByDateReceivedTimestamp(threadId, lastSeen, false);
+    if (firstUnreadDateReceived > 0) {
+      firstUnreadPosition = SignalDatabase.messages().getMessagePositionByDateReceivedTimestamp(threadId, firstUnreadDateReceived, false);
     }
 
-    if (lastSeenPosition <= 0) {
-      lastSeen = 0;
+    if (firstUnreadPosition <= 0) {
+      firstUnreadId           = -1;
+      firstUnreadDateReceived = 0;
     }
 
-    if (lastSeen == 0 && lastScrolled > 0) {
+    if (firstUnreadDateReceived == 0 && lastScrolled > 0) {
       lastScrolledPosition = SignalDatabase.messages().getMessagePositionByDateReceivedTimestamp(threadId, lastScrolled, true);
     }
 
@@ -108,7 +111,7 @@ public class ConversationRepository {
       showUniversalExpireTimerUpdate = true;
     }
 
-    return new ConversationData(conversationRecipient, threadId, lastSeen, lastSeenPosition, lastScrolledPosition, jumpToPosition, threadSize, messageRequestData, showUniversalExpireTimerUpdate, metadata.getUnreadCount(), groupMemberAcis);
+    return new ConversationData(conversationRecipient, threadId, firstUnreadId, firstUnreadPosition, lastScrolledPosition, jumpToPosition, threadSize, messageRequestData, showUniversalExpireTimerUpdate, metadata.getUnreadCount(), groupMemberAcis);
   }
 
   public void markGiftBadgeRevealed(long messageId) {

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt

@@ -1172,7 +1172,7 @@ class ConversationFragment :
       .doOnSuccess { state ->
         SignalLocalMetrics.ConversationOpen.onDataLoaded()
         conversationItemDecorations.selfRecipientId = Recipient.self().id
-        conversationItemDecorations.setFirstUnreadCount(state.meta.unreadCount)
+        conversationItemDecorations.setUnreadState(state.meta.unreadCount, state.meta.firstUnreadId)
         colorizer.onGroupMembershipChanged(state.meta.groupMemberAcis)
       }
       .observeOn(AndroidSchedulers.mainThread())
@@ -3238,20 +3238,28 @@ class ConversationFragment :
       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
-      }
+      val startPosition = meta.getStartPosition()
+      Log.d(TAG, "Scrolling to start position $startPosition")
 
-      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())
+      if (meta.shouldScrollToFirstUnread()) {
+        // Land the divider just below the toolbar.
+        layoutManager.scrollToPositionTopAligned(startPosition, toolbarOffset) {
+          animationsAllowed = true
+          markReadHelper.stopIgnoringViewReveals(MarkReadHelper.getLatestTimestamp(adapter, layoutManager).orNull())
+        }
+      } else {
+        val offset = when {
+          startPosition == 0 -> 0
+          meta.shouldJumpToMessage() -> (binding.conversationItemRecycler.height - toolbarOffset) / 4
+          else -> binding.conversationItemRecycler.height
+        }
+        layoutManager.scrollToPositionWithOffset(startPosition, offset) {
+          animationsAllowed = true
+          markReadHelper.stopIgnoringViewReveals(MarkReadHelper.getLatestTimestamp(adapter, layoutManager).orNull())
+          if (meta.shouldJumpToMessage()) {
+            binding.conversationItemRecycler.post {
+              adapter.pulseAtPosition(startPosition)
+            }
           }
         }
       }

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationItemDecorations.kt

@@ -10,6 +10,7 @@ import android.graphics.Rect
 import android.view.LayoutInflater
 import android.view.View
 import android.widget.TextView
+import androidx.annotation.VisibleForTesting
 import androidx.core.content.ContextCompat
 import androidx.recyclerview.widget.RecyclerView
 import org.thoughtcrime.securesms.R
@@ -45,6 +46,11 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
       unreadViewHolder?.bind()
     }
 
+  /** The current unread-divider state. Exposed for instrumentation tests asserting end-to-end divider behavior. */
+  @get:VisibleForTesting
+  val unreadStateForTesting: UnreadState
+    get() = unreadState
+
   var currentItems: List<ConversationElement?> = emptyList()
     set(value) {
       field = value
@@ -119,31 +125,24 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
     }
   }
 
-  /** Must be called before first setting of [currentItems] */
-  fun setFirstUnreadCount(unreadCount: Int) {
-    if (unreadState == UnreadState.None && unreadCount > 0) {
-      unreadState = UnreadState.InitialUnreadState(unreadCount)
+  /**
+   * Must be called before first setting of [currentItems]. [firstUnreadId] is the row id of the oldest unread message,
+   * used as the unread divider's anchor.
+   */
+  fun setUnreadState(unreadCount: Int, firstUnreadId: Long) {
+    if (unreadState == UnreadState.None && unreadCount > 0 && firstUnreadId > 0) {
+      unreadState = UnreadState.CompleteUnreadState(unreadCount = unreadCount, firstUnreadId = firstUnreadId)
     }
   }
 
   /**
-   * If [unreadState] is [UnreadState.InitialUnreadState] we need to determine the first unread timestamp based on
-   * initial unread count.
-   *
-   * Once in [UnreadState.CompleteUnreadState], need to update the unread count based on new incoming messages since
-   * the first unread timestamp. If an outgoing message is found in this range the unread state is cleared completely,
-   * which causes the unread divider to be removed.
+   * Recomputes the unread count from newer messages up to the first unread message. If an outgoing message is found in
+   * that range the unread state is cleared, removing the divider.
    */
   private fun updateUnreadState(items: List<ConversationElement?>) {
     val state: UnreadState = unreadState
 
-    if (state is UnreadState.InitialUnreadState) {
-      val firstUnread: ConversationMessageElement? = findFirstUnreadStartingAt(items, (state.unreadCount - 1).coerceIn(items.indices), state.unreadCount)
-      val timestamp = firstUnread?.timestamp()
-      if (timestamp != null) {
-        unreadState = UnreadState.CompleteUnreadState(unreadCount = state.unreadCount, firstUnreadTimestamp = timestamp)
-      }
-    } else if (state is UnreadState.CompleteUnreadState) {
+    if (state is UnreadState.CompleteUnreadState) {
       var newUnreadCount = 0
       for (element in items) {
         if (element is ConversationMessageElement) {
@@ -155,7 +154,7 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
               newUnreadCount++
             }
 
-            if (element.timestamp() == state.firstUnreadTimestamp) {
+            if (element.conversationMessage.messageRecord.id == state.firstUnreadId) {
               unreadState = state.copy(unreadCount = max(state.unreadCount, newUnreadCount))
               break
             }
@@ -165,30 +164,6 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
     }
   }
 
-  /**
-   * Attempt to find the "first" unread message, searching a range of 20 items in the list starting at index `unreadCount - 1`. The
-   * search helps us skip over interspersed read messages like chat events that could mess up the location of the header.
-   */
-  private fun findFirstUnreadStartingAt(items: List<ConversationElement?>, startingIndex: Int, unreadCount: Int): ConversationMessageElement? {
-    val endingIndex = (startingIndex + 20).coerceAtMost(items.lastIndex)
-    var targetUnread: ConversationMessageElement? = null
-    var runningUnreadCount = 0
-
-    for (index in startingIndex..endingIndex) {
-      val item = items[index] as? ConversationMessageElement
-      if ((item?.conversationMessage?.messageRecord as? MmsMessageRecord)?.isRead == false) {
-        targetUnread = item
-        runningUnreadCount++
-      }
-
-      if (runningUnreadCount >= unreadCount) {
-        break
-      }
-    }
-
-    return targetUnread ?: items[startingIndex] as? ConversationMessageElement
-  }
-
   /**
    * Only include message that would normally count towards unread count when updating the banner while new messages
    * come in while viewing the chat.
@@ -201,6 +176,9 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
    * Note 2: The caller should've already checked [MmsMessageRecord.isOutgoing] before calling this but some outgoing
    * messages don't use the outgoing types like an outgoing group call, so filter on the [MmsMessageRecord.fromRecipient]
    * here as well.
+   *
+   * Note 3: Only actually-unread rows count -- some inbox-type events are inserted already-read (e.g. identity updates),
+   * and counting them would inflate the banner past the thread's stored unread count.
    */
   private fun MmsMessageRecord.countsTowardsUnread(): Boolean {
     val likelyIncoming = MessageTypes.isInboxType(this.type) ||
@@ -208,16 +186,15 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
       MessageTypes.isIncomingAudioCall(this.type) ||
       MessageTypes.isIncomingVideoCall(this.type)
 
-    return likelyIncoming && !MessageTypes.isGroupUpdate(this.type) && this.fromRecipient.id != selfRecipientId
+    return likelyIncoming && !this.isRead && !MessageTypes.isGroupUpdate(this.type) && this.fromRecipient.id != selfRecipientId
   }
 
   private fun isFirstUnread(bindingAdapterPosition: Int): Boolean {
     val state = unreadState
 
     return state is UnreadState.CompleteUnreadState &&
-      state.firstUnreadTimestamp != null &&
       bindingAdapterPosition in currentItems.indices &&
-      (currentItems[bindingAdapterPosition] as? ConversationMessageElement)?.timestamp() == state.firstUnreadTimestamp
+      (currentItems[bindingAdapterPosition] as? ConversationMessageElement)?.conversationMessage?.messageRecord?.id == state.firstUnreadId
   }
 
   /**
@@ -365,10 +342,7 @@ class ConversationItemDecorations(hasWallpaper: Boolean = false, private val sch
     /** Unread state hasn't been initialized or there are 0 unreads upon entering the conversation */
     object None : UnreadState()
 
-    /** On first load of data, there is at least 1 unread message but we don't know the 'position' in the list yet */
-    data class InitialUnreadState(val unreadCount: Int) : UnreadState()
-
-    /** We have at least one unread and know the timestamp of the first unread message and thus 'position' for the header */
-    data class CompleteUnreadState(val unreadCount: Int, val firstUnreadTimestamp: Long? = null) : UnreadState()
+    /** We have at least one unread and know the row id of the first unread message, used to position the header */
+    data class CompleteUnreadState(val unreadCount: Int, val firstUnreadId: Long) : UnreadState()
   }
 }

app/src/main/java/org/thoughtcrime/securesms/database/MessageTable.kt

@@ -2670,6 +2670,30 @@ open class MessageTable(context: Context?, databaseHelper: SignalDatabase) : Dat
       }
   }
 
+  /**
+   * The oldest unread message as displayed in the thread (latest revision, not collapsed, not pinned), or null if there
+   * are none. Anchors the unread divider ([OldestUnread.id]) and its scroll position ([OldestUnread.dateReceived]); this
+   * is a separate query from the unread count and is not expected to select an identical row set.
+   */
+  fun getOldestUnread(threadId: Long): OldestUnread? {
+    val pinnedMessageClause = "($TYPE & ${MessageTypes.SPECIAL_TYPES_MASK}) != ${MessageTypes.SPECIAL_TYPE_PINNED_MESSAGE}"
+    // The redundant "($READ = 0 OR $REACTIONS_UNREAD = 1 OR $VOTES_UNREAD = 1)" term lets the planner use the partial
+    // index to satisfy ORDER BY $DATE_RECEIVED without a sort (same trick as setMessagesReadSince).
+    return readableDatabase
+      .select(ID, DATE_RECEIVED)
+      .from("$TABLE_NAME INDEXED BY $INDEX_THREAD_DATE_RECEIVED_UNREAD")
+      .where("$THREAD_ID = ? AND $STORY_TYPE = 0 AND $PARENT_STORY_ID <= 0 AND ($READ = 0 OR $REACTIONS_UNREAD = 1 OR $VOTES_UNREAD = 1) AND $READ = 0 AND $SCHEDULED_DATE = -1 AND $LATEST_REVISION_ID IS NULL AND $COLLAPSED_STATE != ${CollapsedState.COLLAPSED.id} AND $pinnedMessageClause", threadId)
+      .orderBy("$DATE_RECEIVED ASC")
+      .limit(1)
+      .run()
+      .readToSingleObject { cursor ->
+        OldestUnread(
+          id = cursor.requireLong(ID),
+          dateReceived = cursor.requireLong(DATE_RECEIVED)
+        )
+      }
+  }
+
   fun getUnreadMentionCount(threadId: Long): Int {
     return readableDatabase
       .count()
@@ -6557,6 +6581,11 @@ open class MessageTable(context: Context?, databaseHelper: SignalDatabase) : Dat
     val threadId: Long
   )
 
+  data class OldestUnread(
+    val id: Long,
+    val dateReceived: Long
+  )
+
   data class Duplicate(
     val id: Long,
     val dateSent: Long,