Merge branch '1289-recycler-view-visible-detection' into 'master'

Prevent RecyclerView's pre-rendering from marking invisible messages as read

Closes #1289

See merge request briar/briar!1061

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java

@@ -47,7 +47,6 @@ import org.briarproject.bramble.api.plugin.ConnectionRegistry;
 import org.briarproject.bramble.api.plugin.event.ContactConnectedEvent;
 import org.briarproject.bramble.api.plugin.event.ContactDisconnectedEvent;
 import org.briarproject.bramble.api.settings.SettingsManager;
-import org.briarproject.bramble.api.sync.GroupId;
 import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.bramble.api.sync.event.MessagesAckedEvent;
 import org.briarproject.bramble.api.sync.event.MessagesSentEvent;
@@ -256,6 +255,9 @@ public class ConversationActivity extends BriarActivity
 		list.setLayoutManager(layoutManager);
 		list.setAdapter(adapter);
 		list.setEmptyText(getString(R.string.no_private_messages));
+		ConversationScrollListener scrollListener =
+				new ConversationScrollListener(adapter, viewModel);
+		list.getRecyclerView().addOnScrollListener(scrollListener);
 
 		textInputView = findViewById(R.id.text_input_container);
 		if (FEATURE_FLAG_IMAGE_ATTACHMENTS) {
@@ -787,23 +789,6 @@ public class ConversationActivity extends BriarActivity
 				.show();
 	}
 
-	@Override
-	public void onItemVisible(ConversationItem item) {
-		if (!item.isRead()) markMessageRead(item.getGroupId(), item.getId());
-	}
-
-	private void markMessageRead(GroupId g, MessageId m) {
-		runOnDbThread(() -> {
-			try {
-				long start = now();
-				messagingManager.setReadFlag(g, m, true);
-				logDuration(LOG, "Marking read", start);
-			} catch (DbException e) {
-				logException(LOG, WARNING, e);
-			}
-		});
-	}
-
 	@UiThread
 	@Override
 	public void respondToRequest(ConversationRequestItem item, boolean accept) {

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationAdapter.java

@@ -14,12 +14,14 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.briar.R;
 import org.briarproject.briar.android.util.BriarAdapter;
+import org.briarproject.briar.android.util.ItemReturningAdapter;
 
 import javax.annotation.Nullable;
 
 @NotNullByDefault
 class ConversationAdapter
-		extends BriarAdapter<ConversationItem, ConversationItemViewHolder> {
+		extends BriarAdapter<ConversationItem, ConversationItemViewHolder>
+		implements ItemReturningAdapter<ConversationItem> {
 
 	private ConversationListener listener;
 	private final RecycledViewPool imageViewPool;
@@ -69,7 +71,6 @@ class ConversationAdapter
 	public void onBindViewHolder(ConversationItemViewHolder ui, int position) {
 		ConversationItem item = items.get(position);
 		ui.bind(item);
-		listener.onItemVisible(item);
 	}
 
 	@Override

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationItem.java

@@ -69,6 +69,10 @@ abstract class ConversationItem {
 		return read;
 	}
 
+	void markRead() {
+		read = true;
+	}
+
 	/**
 	 * Only useful for outgoing messages.
 	 */

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationListener.java

@@ -9,8 +9,6 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 @NotNullByDefault
 interface ConversationListener {
 
-	void onItemVisible(ConversationItem item);
-
 	void respondToRequest(ConversationRequestItem item, boolean accept);
 
 	void openRequestedShareable(ConversationRequestItem item);

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationScrollListener.java

@@ -0,0 +1,26 @@
+package org.briarproject.briar.android.conversation;
+
+import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
+import org.briarproject.briar.android.view.BriarRecyclerViewScrollListener;
+
+@NotNullByDefault
+class ConversationScrollListener extends
+		BriarRecyclerViewScrollListener<ConversationAdapter, ConversationItem> {
+
+	private final ConversationViewModel viewModel;
+
+	protected ConversationScrollListener(ConversationAdapter adapter,
+			ConversationViewModel viewModel) {
+		super(adapter);
+		this.viewModel = viewModel;
+	}
+
+	@Override
+	protected void onItemVisible(ConversationItem item) {
+		if (!item.isRead()) {
+			viewModel.markMessageRead(item.getGroupId(), item.getId());
+			item.markRead();
+		}
+	}
+
+}

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java

@@ -26,6 +26,7 @@ import org.briarproject.bramble.api.settings.Settings;
 import org.briarproject.bramble.api.settings.SettingsManager;
 import org.briarproject.bramble.api.sync.GroupId;
 import org.briarproject.bramble.api.sync.Message;
+import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.briar.android.util.UiUtils;
 import org.briarproject.briar.api.messaging.Attachment;
 import org.briarproject.briar.api.messaging.AttachmentHeader;
@@ -150,6 +151,18 @@ public class ConversationViewModel extends AndroidViewModel {
 		});
 	}
 
+	void markMessageRead(GroupId g, MessageId m) {
+		dbExecutor.execute(() -> {
+			try {
+				long start = now();
+				messagingManager.setReadFlag(g, m, true);
+				logDuration(LOG, "Marking read", start);
+			} catch (DbException e) {
+				logException(LOG, WARNING, e);
+			}
+		});
+	}
+
 	void setContactAlias(String alias) {
 		dbExecutor.execute(() -> {
 			try {

briar-android/src/main/java/org/briarproject/briar/android/threaded/BaseThreadItemViewHolder.java

@@ -51,7 +51,6 @@ public abstract class BaseThreadItemViewHolder<I extends ThreadItem>
 		} else if (!item.isRead()) {
 			layout.setActivated(true);
 			animateFadeOut();
-			listener.onUnreadItemVisible(item);
 		} else {
 			layout.setActivated(false);
 		}

briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadItemAdapter.java

@@ -10,6 +10,7 @@ import android.view.ViewGroup;
 
 import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.briar.R;
+import org.briarproject.briar.android.util.ItemReturningAdapter;
 import org.briarproject.briar.android.util.VersionedAdapter;
 
 import java.util.Collection;
@@ -21,7 +22,7 @@ import static android.support.v7.widget.RecyclerView.NO_POSITION;
 @UiThread
 public class ThreadItemAdapter<I extends ThreadItem>
 		extends RecyclerView.Adapter<BaseThreadItemViewHolder<I>>
-		implements VersionedAdapter {
+		implements VersionedAdapter, ItemReturningAdapter<I> {
 
 	static final int UNDEFINED = -1;
 
@@ -136,30 +137,6 @@ public class ThreadItemAdapter<I extends ThreadItem>
 		return null;
 	}
 
-	/**
-	 * Gets the number of unread items above and below the current view port.
-	 *
-	 * Attention: Do not call this when the list is still scrolling,
-	 *            because then the view port is unknown.
-	 */
-	public UnreadCount getUnreadCount() {
-		int positionTop = layoutManager.findFirstVisibleItemPosition();
-		int positionBottom = layoutManager.findLastVisibleItemPosition();
-		if (positionTop == NO_POSITION && positionBottom == NO_POSITION)
-			return new UnreadCount(0, 0);
-
-		int unreadCounterTop = 0, unreadCounterBottom = 0;
-		for (int i = 0; i < items.size(); i++) {
-			I item = items.get(i);
-			if (i < positionTop && !item.isRead()) {
-				unreadCounterTop++;
-			} else if (i > positionBottom && !item.isRead()) {
-				unreadCounterBottom++;
-			}
-		}
-		return new UnreadCount(unreadCounterTop, unreadCounterBottom);
-	}
-
 	/**
 	 * Returns the position of the first unread item below the current viewport
 	 */
@@ -188,20 +165,7 @@ public class ThreadItemAdapter<I extends ThreadItem>
 		return NO_POSITION;
 	}
 
-	static class UnreadCount {
-
-		final int top, bottom;
-
-		private UnreadCount(int top, int bottom) {
-			this.top = top;
-			this.bottom = bottom;
-		}
-	}
-
 	public interface ThreadItemListener<I> {
-
-		void onUnreadItemVisible(I item);
-
 		void onReplyClick(I item);
 	}
 

briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadListActivity.java

@@ -9,7 +9,6 @@ import android.support.annotation.UiThread;
 import android.support.design.widget.Snackbar;
 import android.support.v7.app.ActionBar;
 import android.support.v7.widget.LinearLayoutManager;
-import android.support.v7.widget.RecyclerView;
 import android.view.MenuItem;
 
 import org.briarproject.bramble.api.contact.ContactId;
@@ -43,10 +42,7 @@ import javax.inject.Inject;
 
 import static android.support.design.widget.Snackbar.make;
 import static android.support.v7.widget.RecyclerView.NO_POSITION;
-import static android.support.v7.widget.RecyclerView.SCROLL_STATE_IDLE;
-import static java.util.logging.Level.INFO;
 import static org.briarproject.bramble.util.StringUtils.isNullOrEmpty;
-import static org.briarproject.briar.android.threaded.ThreadItemAdapter.UnreadCount;
 
 @MethodsNotNullByDefault
 @ParametersNotNullByDefault
@@ -61,11 +57,11 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 			Logger.getLogger(ThreadListActivity.class.getName());
 
 	protected A adapter;
+	private ThreadScrollListener<I> scrollListener;
 	protected BriarRecyclerView list;
 	private LinearLayoutManager layoutManager;
 	protected TextInputView textInput;
 	protected GroupId groupId;
-	private UnreadMessageButton upButton, downButton;
 	@Nullable
 	private MessageId replyId;
 
@@ -76,7 +72,6 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 
 	@CallSuper
 	@Override
-	@SuppressWarnings("ConstantConditions")
 	public void onCreate(@Nullable Bundle state) {
 		super.onCreate(state);
 
@@ -93,37 +88,18 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 				new TextSendController(textInput, this, false);
 		textInput.setSendController(sendController);
 		textInput.setMaxTextLength(getMaxTextLength());
+		UnreadMessageButton upButton = findViewById(R.id.upButton);
+		UnreadMessageButton downButton = findViewById(R.id.downButton);
+
 		list = findViewById(R.id.list);
 		layoutManager = new LinearLayoutManager(this);
-		// FIXME pre-fetching messes with read state, find better solution #1289
-		layoutManager.setItemPrefetchEnabled(false);
 		list.setLayoutManager(layoutManager);
 		adapter = createAdapter(layoutManager);
 		list.setAdapter(adapter);
+		scrollListener = new ThreadScrollListener<>(adapter, getController(),
+				upButton, downButton);
+		list.getRecyclerView().addOnScrollListener(scrollListener);
 
-		list.getRecyclerView().addOnScrollListener(
-				new RecyclerView.OnScrollListener() {
-					@Override
-					public void onScrolled(RecyclerView recyclerView, int dx,
-							int dy) {
-						super.onScrolled(recyclerView, dx, dy);
-						if (dx == 0 && dy == 0) {
-							// scrollToPosition has been called and finished
-							updateUnreadCount();
-						}
-					}
-
-					@Override
-					public void onScrollStateChanged(RecyclerView recyclerView,
-							int newState) {
-						super.onScrollStateChanged(recyclerView, newState);
-						if (newState == SCROLL_STATE_IDLE) {
-							updateUnreadCount();
-						}
-					}
-				});
-		upButton = findViewById(R.id.upButton);
-		downButton = findViewById(R.id.downButton);
 		upButton.setOnClickListener(v -> {
 			int position = adapter.getVisibleUnreadPosTop();
 			if (position != NO_POSITION) {
@@ -211,7 +187,6 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 		MessageId messageId = items.getFirstVisibleItemId();
 		if (messageId != null)
 			adapter.setItemWithIdVisible(messageId);
-		updateUnreadCount();
 		list.showData();
 	}
 
@@ -280,14 +255,6 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 		}
 	}
 
-	@Override
-	public void onUnreadItemVisible(I item) {
-		if (!item.isRead()) {
-			item.setRead(true);
-			getController().markItemRead(item);
-		}
-	}
-
 	@Override
 	public void onReplyClick(I item) {
 		replyId = item.getId();
@@ -403,18 +370,8 @@ public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadI
 		if (isLocal) {
 			scrollToItemAtTop(item);
 		} else {
-			updateUnreadCount();
+			scrollListener.updateUnreadButtons(layoutManager);
 		}
 	}
 
-	private void updateUnreadCount() {
-		UnreadCount unreadCount = adapter.getUnreadCount();
-		if (LOG.isLoggable(INFO)) {
-			LOG.info("Updating unread count: top=" + unreadCount.top +
-					" bottom=" + unreadCount.bottom);
-		}
-		upButton.setUnreadCount(unreadCount.top);
-		downButton.setUnreadCount(unreadCount.bottom);
-	}
-
 }

briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadScrollListener.java

@@ -0,0 +1,86 @@
+package org.briarproject.briar.android.threaded;
+
+import android.support.v7.widget.LinearLayoutManager;
+
+import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
+import org.briarproject.briar.android.view.BriarRecyclerViewScrollListener;
+import org.briarproject.briar.android.view.UnreadMessageButton;
+
+import java.util.logging.Logger;
+
+import static android.support.v7.widget.RecyclerView.NO_POSITION;
+import static java.util.Objects.requireNonNull;
+import static java.util.logging.Level.INFO;
+import static java.util.logging.Logger.getLogger;
+
+@NotNullByDefault
+class ThreadScrollListener<I extends ThreadItem>
+		extends BriarRecyclerViewScrollListener<ThreadItemAdapter<I>, I> {
+
+	private static final Logger LOG =
+			getLogger(ThreadScrollListener.class.getName());
+
+	private final ThreadListController<?, I> controller;
+	private final UnreadMessageButton upButton, downButton;
+
+	protected ThreadScrollListener(ThreadItemAdapter<I> adapter,
+			ThreadListController<?, I> controller,
+			UnreadMessageButton upButton,
+			UnreadMessageButton downButton) {
+		super(adapter);
+		this.controller = controller;
+		this.upButton = upButton;
+		this.downButton = downButton;
+	}
+
+	@Override
+	protected void onItemsVisible(int firstVisible, int lastVisible,
+			int itemCount) {
+		super.onItemsVisible(firstVisible, lastVisible, itemCount);
+		updateUnreadButtons(firstVisible, lastVisible, itemCount);
+	}
+
+	@Override
+	protected void onItemVisible(I item) {
+		if (!item.isRead()) {
+			item.setRead(true);
+			controller.markItemRead(item);
+		}
+	}
+
+	void updateUnreadButtons(LinearLayoutManager layoutManager) {
+		int firstVisible = layoutManager.findFirstVisibleItemPosition();
+		int lastVisible = layoutManager.findLastVisibleItemPosition();
+		int itemCount = adapter.getItemCount();
+		updateUnreadButtons(firstVisible, lastVisible, itemCount);
+	}
+
+	private void updateUnreadButtons(int firstVisible, int lastVisible,
+			int count) {
+		if (firstVisible == NO_POSITION && lastVisible == NO_POSITION) {
+			setUnreadButtons(0, 0);
+			return;
+		}
+		int unreadCounterFirst = 0;
+		for (int i = 0; i < firstVisible; i++) {
+			I item = requireNonNull(adapter.getItemAt(i));
+			if (!item.isRead()) unreadCounterFirst++;
+		}
+		int unreadCounterLast = 0;
+		for (int i = lastVisible + 1; i < count; i++) {
+			I item = requireNonNull(adapter.getItemAt(i));
+			if (!item.isRead()) unreadCounterLast++;
+		}
+		setUnreadButtons(unreadCounterFirst, unreadCounterLast);
+	}
+
+	private void setUnreadButtons(int upCount, int downCount) {
+		if (LOG.isLoggable(INFO)) {
+			LOG.info("Updating unread count: top=" + upCount +
+					" bottom=" + downCount);
+		}
+		upButton.setUnreadCount(upCount);
+		downButton.setUnreadCount(downCount);
+	}
+
+}

briar-android/src/main/java/org/briarproject/briar/android/util/ItemReturningAdapter.java

@@ -0,0 +1,9 @@
+package org.briarproject.briar.android.util;
+
+public interface ItemReturningAdapter<I> {
+
+	I getItemAt(int position);
+
+	int getItemCount();
+
+}

briar-android/src/main/java/org/briarproject/briar/android/view/BriarRecyclerViewScrollListener.java

@@ -0,0 +1,61 @@
+package org.briarproject.briar.android.view;
+
+import android.support.annotation.CallSuper;
+import android.support.v7.widget.LinearLayoutManager;
+import android.support.v7.widget.RecyclerView;
+import android.support.v7.widget.RecyclerView.OnScrollListener;
+
+import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
+import org.briarproject.briar.android.util.ItemReturningAdapter;
+
+import static android.support.v7.widget.RecyclerView.NO_POSITION;
+import static java.util.Objects.requireNonNull;
+
+@NotNullByDefault
+public abstract class BriarRecyclerViewScrollListener<A extends ItemReturningAdapter<I>, I>
+		extends OnScrollListener {
+
+	protected final A adapter;
+
+	private int prevFirstVisible = NO_POSITION;
+	private int prevLastVisible = NO_POSITION;
+	private int prevItemCount = 0;
+
+	protected BriarRecyclerViewScrollListener(A adapter) {
+		this.adapter = adapter;
+	}
+
+	@Override
+	public void onScrolled(RecyclerView recyclerView, int dx, int dy) {
+		LinearLayoutManager layoutManager = (LinearLayoutManager)
+				requireNonNull(recyclerView.getLayoutManager());
+		int firstVisible = layoutManager.findFirstVisibleItemPosition();
+		int lastVisible = layoutManager.findLastVisibleItemPosition();
+		int itemCount = adapter.getItemCount();
+
+		if (firstVisible != prevFirstVisible ||
+				lastVisible != prevLastVisible ||
+				itemCount != prevItemCount) {
+			onItemsVisible(firstVisible, lastVisible, itemCount);
+			prevFirstVisible = firstVisible;
+			prevLastVisible = lastVisible;
+			prevItemCount = itemCount;
+		}
+	}
+
+	@CallSuper
+	protected void onItemsVisible(int firstVisible, int lastVisible,
+			int itemCount) {
+		for (int i = firstVisible; i <= lastVisible; i++) {
+			onItemVisible(i);
+		}
+	}
+
+	private void onItemVisible(int position) {
+		I item = requireNonNull(adapter.getItemAt(position));
+		onItemVisible(item);
+	}
+
+	protected abstract void onItemVisible(I item);
+
+}