From 239c4a27adab6036411f12354e3b6acc590ddd53 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 26 Jan 2021 16:56:43 -0300 Subject: [PATCH] Address first round of review feedback for thread list view model migration --- .../briar/android/forum/ForumActivity.java | 16 ++++++------ .../conversation/GroupActivity.java | 14 ++++++----- .../conversation/GroupViewModel.java | 3 +++ .../conversation/JoinMessageItem.java | 2 +- .../android/threaded/ThreadItemAdapter.java | 13 ++++------ .../android/threaded/ThreadListViewModel.java | 25 ++++++++++--------- .../PrivateGroupManagerIntegrationTest.java | 13 ---------- 7 files changed, 38 insertions(+), 48 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/forum/ForumActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/forum/ForumActivity.java index 5cf01d818..abb30abc5 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/forum/ForumActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/forum/ForumActivity.java @@ -103,16 +103,16 @@ public class ForumActivity extends // Handle presses on the action bar items int itemId = item.getItemId(); if (itemId == R.id.action_forum_share) { - Intent i2 = new Intent(this, ShareForumActivity.class); - i2.setFlags(FLAG_ACTIVITY_CLEAR_TOP); - i2.putExtra(GROUP_ID, groupId.getBytes()); - startActivityForResult(i2, REQUEST_SHARE_FORUM); + Intent i = new Intent(this, ShareForumActivity.class); + i.setFlags(FLAG_ACTIVITY_CLEAR_TOP); + i.putExtra(GROUP_ID, groupId.getBytes()); + startActivityForResult(i, REQUEST_SHARE_FORUM); return true; } else if (itemId == R.id.action_forum_sharing_status) { - Intent i3 = new Intent(this, ForumSharingStatusActivity.class); - i3.setFlags(FLAG_ACTIVITY_CLEAR_TOP); - i3.putExtra(GROUP_ID, groupId.getBytes()); - startActivity(i3); + Intent i = new Intent(this, ForumSharingStatusActivity.class); + i.setFlags(FLAG_ACTIVITY_CLEAR_TOP); + i.putExtra(GROUP_ID, groupId.getBytes()); + startActivity(i); return true; } else if (itemId == R.id.action_forum_delete) { showUnsubscribeDialog(); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupActivity.java index 053358caa..93b9cbd23 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupActivity.java @@ -25,6 +25,7 @@ import androidx.lifecycle.ViewModelProvider; import static android.view.View.GONE; import static android.view.View.VISIBLE; +import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; import static org.briarproject.briar.android.activity.RequestCodes.REQUEST_GROUP_INVITE; import static org.briarproject.briar.android.util.UiUtils.observeOnce; import static org.briarproject.briar.api.privategroup.PrivateGroupConstants.MAX_GROUP_POST_TEXT_LENGTH; @@ -110,26 +111,26 @@ public class GroupActivity extends startActivity(i); return true; } else if (itemId == R.id.action_group_reveal) { - if (viewModel.isCreator().getValue()) + if (requireNonNull(viewModel.isCreator().getValue())) throw new IllegalStateException(); Intent i = new Intent(this, RevealContactsActivity.class); i.putExtra(GROUP_ID, groupId.getBytes()); startActivity(i); return true; } else if (itemId == R.id.action_group_invite) { - if (!viewModel.isCreator().getValue()) + if (!requireNonNull(viewModel.isCreator().getValue())) throw new IllegalStateException(); Intent i = new Intent(this, GroupInviteActivity.class); i.putExtra(GROUP_ID, groupId.getBytes()); startActivityForResult(i, REQUEST_GROUP_INVITE); return true; } else if (itemId == R.id.action_group_leave) { - if (viewModel.isCreator().getValue()) + if (requireNonNull(viewModel.isCreator().getValue())) throw new IllegalStateException(); showLeaveGroupDialog(); return true; } else if (itemId == R.id.action_group_dissolve) { - if (!viewModel.isCreator().getValue()) + if (!requireNonNull(viewModel.isCreator().getValue())) throw new IllegalStateException(); showDissolveGroupDialog(); return true; @@ -152,7 +153,8 @@ public class GroupActivity extends @Override public void onReplyClick(GroupMessageItem item) { - if (!viewModel.isDissolved().getValue()) super.onReplyClick(item); + Boolean isDissolved = viewModel.isDissolved().getValue(); + if (isDissolved != null && !isDissolved) super.onReplyClick(item); } private void setGroupEnabled(boolean enabled) { @@ -195,7 +197,7 @@ public class GroupActivity extends viewModel.deletePrivateGroup(); } - public void onGroupDissolved() { + private void onGroupDissolved() { AlertDialog.Builder builder = new AlertDialog.Builder(this, R.style.BriarDialogTheme); builder.setTitle(getString(R.string.groups_dissolved_dialog_title)); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupViewModel.java index ada485e47..557eb2865 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/GroupViewModel.java @@ -104,6 +104,9 @@ class GroupViewModel extends ThreadListViewModel { LOG.info("Group message received, adding..."); GroupMessageItem item = buildItem(g.getHeader(), g.getText()); addItem(item); + // In case the join message comes from the creator, + // we need to reload the sharing contacts + // in case it was delayed and the sharing count is wrong (#850). if (item instanceof JoinMessageItem && (((JoinMessageItem) item).isInitial())) { loadSharingContacts(); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/JoinMessageItem.java b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/JoinMessageItem.java index bb92d8310..234fcccfb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/JoinMessageItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/privategroup/conversation/JoinMessageItem.java @@ -16,7 +16,7 @@ class JoinMessageItem extends GroupMessageItem { JoinMessageItem(JoinMessageHeader h, String text) { super(h, text); - this.isInitial = h.isInitial(); + isInitial = h.isInitial(); } @Override diff --git a/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadItemAdapter.java b/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadItemAdapter.java index a66969592..7e5bf01ae 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadItemAdapter.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadItemAdapter.java @@ -4,8 +4,7 @@ import android.view.LayoutInflater; import android.view.View; import android.view.ViewGroup; -import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; -import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault; +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.ItemReturningAdapter; @@ -21,8 +20,7 @@ import androidx.recyclerview.widget.ListAdapter; import static androidx.recyclerview.widget.RecyclerView.NO_POSITION; @UiThread -@MethodsNotNullByDefault -@ParametersNotNullByDefault +@NotNullByDefault public class ThreadItemAdapter extends ListAdapter> implements ItemReturningAdapter { @@ -41,7 +39,7 @@ public class ThreadItemAdapter @Override public boolean areContentsTheSame(I a, I b) { return a.isHighlighted() == b.isHighlighted() && - a.isRead() && b.isRead(); + a.isRead() == b.isRead(); } }); this.listener = listener; @@ -63,7 +61,7 @@ public class ThreadItemAdapter ui.bind(item, listener); } - public int findItemPosition(MessageId id) { + int findItemPosition(MessageId id) { for (int i = 0; i < getItemCount(); i++) { if (id.equals(getItem(i).getId())) return i; } @@ -91,8 +89,7 @@ public class ThreadItemAdapter @Nullable I getHighlightedItem() { - for (int i = 0; i < getItemCount(); i++) { - I item = getItem(i); + for (I item : getCurrentList()) { if (item.isHighlighted()) return item; } return null; diff --git a/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadListViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadListViewModel.java index 946629760..a655dc605 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadListViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/threaded/ThreadListViewModel.java @@ -67,7 +67,6 @@ public abstract class ThreadListViewModel private final MessageTracker messageTracker; private final EventBus eventBus; - @DatabaseExecutor private final MessageTree messageTree = new MessageTreeImpl<>(); private final MutableLiveData>> items = new MutableLiveData<>(); @@ -126,12 +125,12 @@ public abstract class ThreadListViewModel protected abstract void clearNotifications(); - public void blockAndClearNotifications() { + void blockAndClearNotifications() { notificationManager.blockNotification(groupId); clearNotifications(); } - public void unblockNotifications() { + void unblockNotifications() { notificationManager.unblockNotification(groupId); } @@ -221,24 +220,26 @@ public abstract class ThreadListViewModel PostHeader header) throws DbException; @UiThread - public void setReplyId(@Nullable MessageId id) { + void setReplyId(@Nullable MessageId id) { replyId = id; } @UiThread @Nullable - public MessageId getReplyId() { + MessageId getReplyId() { return replyId; } void storeMessageId(@Nullable MessageId messageId) { - if (messageId != null) runOnDbThread(() -> { - try { - messageTracker.storeMessageId(groupId, messageId); - } catch (DbException e) { - logException(LOG, WARNING, e); - } - }); + if (messageId != null) { + runOnDbThread(() -> { + try { + messageTracker.storeMessageId(groupId, messageId); + } catch (DbException e) { + logException(LOG, WARNING, e); + } + }); + } } protected abstract void markItemRead(I item); diff --git a/briar-core/src/test/java/org/briarproject/briar/privategroup/PrivateGroupManagerIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/privategroup/PrivateGroupManagerIntegrationTest.java index 9dae9d6b5..8dd6e855b 100644 --- a/briar-core/src/test/java/org/briarproject/briar/privategroup/PrivateGroupManagerIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/privategroup/PrivateGroupManagerIntegrationTest.java @@ -356,19 +356,6 @@ public class PrivateGroupManagerIntegrationTest } } - @Test - public void testJoinMessages() throws Exception { - addGroup(); - - Collection headers0 = - groupManager0.getHeaders(groupId0); - assertEquals(2, headers0.size()); - - Collection headers1 = - groupManager1.getHeaders(groupId0); - assertEquals(2, headers1.size()); - } - @Test public void testRevealingRelationships() throws Exception { addGroup();