From 0523c4e718b0ca9d4b6f339c64773b4904cd6147 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 18 Oct 2016 15:50:05 -0200 Subject: [PATCH] Address issues found in code review --- briar-android/res/values/strings.xml | 1 + .../android/forum/ForumActivity.java | 70 ++++++++++----- .../android/forum/ForumControllerImpl.java | 28 +++--- .../conversation/GroupActivity.java | 41 +++++---- .../conversation/GroupControllerImpl.java | 36 ++++---- .../android/threaded/ThreadItemAdapter.java | 7 +- .../android/threaded/ThreadListActivity.java | 44 ++++----- .../threaded/ThreadListController.java | 9 +- .../threaded/ThreadListControllerImpl.java | 89 ++++++++----------- .../src/org/briarproject/api/blogs/Blog.java | 11 ++- .../briarproject/api/clients/BaseGroup.java | 13 ++- .../briarproject/api/clients/NamedGroup.java | 29 ++++++ .../src/org/briarproject/api/forum/Forum.java | 9 +- .../api/privategroup/PrivateGroup.java | 9 +- .../briarproject/forum/ForumManagerImpl.java | 2 +- 15 files changed, 235 insertions(+), 163 deletions(-) create mode 100644 briar-api/src/org/briarproject/api/clients/NamedGroup.java diff --git a/briar-android/res/values/strings.xml b/briar-android/res/values/strings.xml index 0f541d6fb..b1652f5fb 100644 --- a/briar-android/res/values/strings.xml +++ b/briar-android/res/values/strings.xml @@ -75,6 +75,7 @@ Send No data … + The entered text is too long It seems that you are new here and have no contacts yet.\n\nTap the + icon at the top and follow the instructions to add some friends to your list.\n\nPlease remember: You can only add new contacts face-to-face to prevent anyone from impersonating you or reading your messages in the future. diff --git a/briar-android/src/org/briarproject/android/forum/ForumActivity.java b/briar-android/src/org/briarproject/android/forum/ForumActivity.java index 74537eed7..eafa66098 100644 --- a/briar-android/src/org/briarproject/android/forum/ForumActivity.java +++ b/briar-android/src/org/briarproject/android/forum/ForumActivity.java @@ -2,7 +2,9 @@ package org.briarproject.android.forum; import android.content.DialogInterface; import android.content.Intent; +import android.os.Bundle; import android.support.annotation.LayoutRes; +import android.support.annotation.StringRes; import android.support.v4.app.ActivityCompat; import android.support.v4.app.ActivityOptionsCompat; import android.support.v7.app.AlertDialog; @@ -29,6 +31,7 @@ import static android.content.Intent.FLAG_ACTIVITY_CLEAR_TOP; import static android.content.Intent.FLAG_ACTIVITY_SINGLE_TOP; import static android.support.v4.app.ActivityOptionsCompat.makeCustomAnimation; import static android.widget.Toast.LENGTH_SHORT; +import static org.briarproject.api.forum.ForumConstants.MAX_FORUM_POST_BODY_LENGTH; public class ForumActivity extends ThreadListActivity { @@ -36,7 +39,7 @@ public class ForumActivity extends private static final int REQUEST_FORUM_SHARED = 3; @Inject - protected ForumController forumController; + ForumController forumController; @Override public void injectActivity(ActivityComponent component) { @@ -49,7 +52,23 @@ public class ForumActivity extends } @Override - protected @LayoutRes int getLayout() { + public void onCreate(Bundle state) { + super.onCreate(state); + + Intent i = getIntent(); + String groupName = i.getStringExtra(GROUP_NAME); + if (groupName != null) setTitle(groupName); + else loadNamedGroup(); + } + + @Override + protected void onNamedGroupLoaded(Forum forum) { + setTitle(forum.getName()); + } + + @Override + @LayoutRes + protected int getLayout() { return R.layout.activity_forum; } @@ -110,11 +129,18 @@ public class ForumActivity extends } @Override + protected int getMaxBodyLength() { + return MAX_FORUM_POST_BODY_LENGTH; + } + + @Override + @StringRes protected int getItemPostedString() { return R.string.forum_new_entry_posted; } @Override + @StringRes protected int getItemReceivedString() { return R.string.forum_new_entry_received; } @@ -125,24 +151,7 @@ public class ForumActivity extends @Override public void onClick(final DialogInterface dialog, int which) { - forumController.deleteGroupItem( - new UiResultExceptionHandler( - ForumActivity.this) { - @Override - public void onResultUi(Void v) { - Toast.makeText(ForumActivity.this, - R.string.forum_left_toast, - LENGTH_SHORT) - .show(); - } - - @Override - public void onExceptionUi( - DbException exception) { - // TODO proper error handling - dialog.dismiss(); - } - }); + deleteNamedGroup(); } }; AlertDialog.Builder builder = @@ -155,4 +164,25 @@ public class ForumActivity extends builder.show(); } + private void deleteNamedGroup() { + forumController.deleteNamedGroup( + new UiResultExceptionHandler( + ForumActivity.this) { + @Override + public void onResultUi(Void v) { + Toast.makeText(ForumActivity.this, + R.string.forum_left_toast, + LENGTH_SHORT) + .show(); + } + + @Override + public void onExceptionUi( + DbException exception) { + // TODO proper error handling + finish(); + } + }); + } + } diff --git a/briar-android/src/org/briarproject/android/forum/ForumControllerImpl.java b/briar-android/src/org/briarproject/android/forum/ForumControllerImpl.java index 9375d0a35..9ec21e76c 100644 --- a/briar-android/src/org/briarproject/android/forum/ForumControllerImpl.java +++ b/briar-android/src/org/briarproject/android/forum/ForumControllerImpl.java @@ -15,11 +15,12 @@ import org.briarproject.api.forum.ForumManager; import org.briarproject.api.forum.ForumPost; import org.briarproject.api.forum.ForumPostHeader; import org.briarproject.api.lifecycle.LifecycleManager; -import org.briarproject.api.sync.GroupId; import org.briarproject.api.sync.MessageId; import org.briarproject.util.StringUtils; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.Executor; import java.util.logging.Logger; @@ -48,7 +49,7 @@ public class ForumControllerImpl @Override public void onActivityResume() { super.onActivityResume(); - notificationManager.clearForumPostNotification(groupId); + notificationManager.clearForumPostNotification(getGroupId()); } @Override @@ -57,7 +58,7 @@ public class ForumControllerImpl if (e instanceof ForumPostReceivedEvent) { final ForumPostReceivedEvent pe = (ForumPostReceivedEvent) e; - if (pe.getGroupId().equals(groupId)) { + if (pe.getGroupId().equals(getGroupId())) { LOG.info("Forum post received, adding..."); final ForumPostHeader fph = pe.getForumPostHeader(); listener.runOnUiThreadUnlessDestroyed(new Runnable() { @@ -72,35 +73,38 @@ public class ForumControllerImpl @Override protected Forum loadGroupItem() throws DbException { - return forumManager.getForum(groupId); + return forumManager.getForum(getGroupId()); } @Override protected Collection loadHeaders() throws DbException { - return forumManager.getPostHeaders(groupId); + return forumManager.getPostHeaders(getGroupId()); } @Override - protected void loadBodies(Collection headers) + protected Map loadBodies( + Collection headers) throws DbException { + Map bodies = new HashMap<>(); for (ForumPostHeader header : headers) { if (!bodyCache.containsKey(header.getId())) { String body = StringUtils .fromUtf8(forumManager.getPostBody(header.getId())); - bodyCache.put(header.getId(), body); + bodies.put(header.getId(), body); } } + return bodies; } @Override protected void markRead(MessageId id) throws DbException { - forumManager.setReadFlag(groupId, id, true); + forumManager.setReadFlag(getGroupId(), id, true); } @Override - protected ForumPost createLocalMessage(GroupId g, String body, + protected ForumPost createLocalMessage(String body, @Nullable MessageId parentId) throws DbException { - return forumManager.createLocalPost(groupId, body, parentId); + return forumManager.createLocalPost(getGroupId(), body, parentId); } @Override @@ -115,8 +119,8 @@ public class ForumControllerImpl } @Override - protected ForumEntry buildItem(ForumPostHeader header) { - return new ForumEntry(header, bodyCache.get(header.getId())); + protected ForumEntry buildItem(ForumPostHeader header, String body) { + return new ForumEntry(header, body); } } diff --git a/briar-android/src/org/briarproject/android/privategroup/conversation/GroupActivity.java b/briar-android/src/org/briarproject/android/privategroup/conversation/GroupActivity.java index 0ad582734..71a4865b5 100644 --- a/briar-android/src/org/briarproject/android/privategroup/conversation/GroupActivity.java +++ b/briar-android/src/org/briarproject/android/privategroup/conversation/GroupActivity.java @@ -1,8 +1,9 @@ package org.briarproject.android.privategroup.conversation; +import android.content.Intent; import android.os.Bundle; import android.support.annotation.LayoutRes; -import android.support.annotation.Nullable; +import android.support.annotation.StringRes; import android.support.v7.app.ActionBar; import android.support.v7.widget.LinearLayoutManager; import android.view.Menu; @@ -18,11 +19,13 @@ import org.briarproject.api.privategroup.PrivateGroup; import javax.inject.Inject; +import static org.briarproject.api.privategroup.PrivateGroupConstants.MAX_GROUP_POST_BODY_LENGTH; + public class GroupActivity extends ThreadListActivity { @Inject - protected GroupController controller; + GroupController controller; @Override public void injectActivity(ActivityComponent component) { @@ -37,23 +40,18 @@ public class GroupActivity extends @Override public void onCreate(Bundle state) { super.onCreate(state); + + Intent i = getIntent(); + String groupName = i.getStringExtra(GROUP_NAME); + if (groupName != null) setTitle(groupName); + loadNamedGroup(); + list.setEmptyText(R.string.groups_no_messages); } @Override - protected @LayoutRes int getLayout() { - return R.layout.activity_forum; - } - - @Override - protected void setActionBarTitle(@Nullable String title) { - if (title != null) setTitle(title); - loadGroupItem(); - } - - @Override - protected void onGroupItemLoaded(PrivateGroup group) { - super.onGroupItemLoaded(group); + protected void onNamedGroupLoaded(PrivateGroup group) { + setTitle(group.getName()); // Created by ActionBar actionBar = getSupportActionBar(); if (actionBar != null) { @@ -62,6 +60,12 @@ public class GroupActivity extends } } + @Override + @LayoutRes + protected int getLayout() { + return R.layout.activity_forum; + } + @Override protected GroupMessageAdapter createAdapter( LinearLayoutManager layoutManager) { @@ -89,11 +93,18 @@ public class GroupActivity extends } @Override + protected int getMaxBodyLength() { + return MAX_GROUP_POST_BODY_LENGTH; + } + + @Override + @StringRes protected int getItemPostedString() { return R.string.groups_message_sent; } @Override + @StringRes protected int getItemReceivedString() { return R.string.groups_message_received; } diff --git a/briar-android/src/org/briarproject/android/privategroup/conversation/GroupControllerImpl.java b/briar-android/src/org/briarproject/android/privategroup/conversation/GroupControllerImpl.java index 18a9bb57e..dfadc9d41 100644 --- a/briar-android/src/org/briarproject/android/privategroup/conversation/GroupControllerImpl.java +++ b/briar-android/src/org/briarproject/android/privategroup/conversation/GroupControllerImpl.java @@ -15,10 +15,11 @@ import org.briarproject.api.privategroup.GroupMessage; import org.briarproject.api.privategroup.GroupMessageHeader; import org.briarproject.api.privategroup.PrivateGroup; import org.briarproject.api.privategroup.PrivateGroupManager; -import org.briarproject.api.sync.GroupId; import org.briarproject.api.sync.MessageId; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.Executor; import java.util.logging.Logger; @@ -47,7 +48,7 @@ public class GroupControllerImpl @Override public void onActivityResume() { super.onActivityResume(); - notificationManager.clearForumPostNotification(groupId); + notificationManager.clearForumPostNotification(getGroupId()); } @Override @@ -55,14 +56,14 @@ public class GroupControllerImpl super.eventOccurred(e); if (e instanceof GroupMessageAddedEvent) { - final GroupMessageAddedEvent pe = (GroupMessageAddedEvent) e; - if (!pe.isLocal() && pe.getGroupId().equals(groupId)) { + final GroupMessageAddedEvent gmae = (GroupMessageAddedEvent) e; + if (!gmae.isLocal() && gmae.getGroupId().equals(getGroupId())) { LOG.info("Group message received, adding..."); - final GroupMessageHeader fph = pe.getHeader(); + final GroupMessageHeader h = gmae.getHeader(); listener.runOnUiThreadUnlessDestroyed(new Runnable() { @Override public void run() { - listener.onHeaderReceived(fph); + listener.onHeaderReceived(h); } }); } @@ -71,35 +72,39 @@ public class GroupControllerImpl @Override protected PrivateGroup loadGroupItem() throws DbException { - return privateGroupManager.getPrivateGroup(groupId); + return privateGroupManager.getPrivateGroup(getGroupId()); } @Override protected Collection loadHeaders() throws DbException { - return privateGroupManager.getHeaders(groupId); + return privateGroupManager.getHeaders(getGroupId()); } @Override - protected void loadBodies(Collection headers) + protected Map loadBodies( + Collection headers) throws DbException { + Map bodies = new HashMap<>(); for (GroupMessageHeader header : headers) { if (!bodyCache.containsKey(header.getId())) { String body = privateGroupManager.getMessageBody(header.getId()); - bodyCache.put(header.getId(), body); + bodies.put(header.getId(), body); } } + return bodies; } @Override protected void markRead(MessageId id) throws DbException { - privateGroupManager.setReadFlag(groupId, id, true); + privateGroupManager.setReadFlag(getGroupId(), id, true); } @Override - protected GroupMessage createLocalMessage(GroupId g, String body, + protected GroupMessage createLocalMessage(String body, @Nullable MessageId parentId) throws DbException { - return privateGroupManager.createLocalMessage(groupId, body, parentId); + return privateGroupManager + .createLocalMessage(getGroupId(), body, parentId); } @Override @@ -114,8 +119,9 @@ public class GroupControllerImpl } @Override - protected GroupMessageItem buildItem(GroupMessageHeader header) { - return new GroupMessageItem(header, bodyCache.get(header.getId())); + protected GroupMessageItem buildItem(GroupMessageHeader header, + String body) { + return new GroupMessageItem(header, body); } } diff --git a/briar-android/src/org/briarproject/android/threaded/ThreadItemAdapter.java b/briar-android/src/org/briarproject/android/threaded/ThreadItemAdapter.java index 82e1fca18..01aae5c68 100644 --- a/briar-android/src/org/briarproject/android/threaded/ThreadItemAdapter.java +++ b/briar-android/src/org/briarproject/android/threaded/ThreadItemAdapter.java @@ -41,7 +41,7 @@ public abstract class ThreadItemAdapter @Override public void onBindViewHolder(ThreadItemViewHolder ui, int position) { - final I item = getVisibleItem(position); + I item = getVisibleItem(position); if (item == null) return; listener.onItemVisible(item); ui.bind(this, listener, item, position); @@ -151,10 +151,9 @@ public abstract class ThreadItemAdapter } } - public void setReplyItemById(byte[] id) { - MessageId messageId = new MessageId(id); + public void setReplyItemById(MessageId id) { for (I item : items) { - if (item.getId().equals(messageId)) { + if (item.getId().equals(id)) { setReplyItem(item); break; } diff --git a/briar-android/src/org/briarproject/android/threaded/ThreadListActivity.java b/briar-android/src/org/briarproject/android/threaded/ThreadListActivity.java index 529d588de..ab480a4bd 100644 --- a/briar-android/src/org/briarproject/android/threaded/ThreadListActivity.java +++ b/briar-android/src/org/briarproject/android/threaded/ThreadListActivity.java @@ -21,10 +21,11 @@ import org.briarproject.android.threaded.ThreadListController.ThreadListListener import org.briarproject.android.view.BriarRecyclerView; import org.briarproject.android.view.TextInputView; import org.briarproject.android.view.TextInputView.TextInputListener; -import org.briarproject.api.clients.BaseGroup; +import org.briarproject.api.clients.NamedGroup; import org.briarproject.api.clients.PostHeader; import org.briarproject.api.db.DbException; import org.briarproject.api.sync.GroupId; +import org.briarproject.api.sync.MessageId; import java.util.ArrayList; import java.util.Collection; @@ -34,7 +35,7 @@ import static android.support.design.widget.Snackbar.make; import static android.view.View.GONE; import static android.view.View.VISIBLE; -public abstract class ThreadListActivity> +public abstract class ThreadListActivity> extends BriarActivity implements ThreadListListener, TextInputListener, ThreadItemListener { @@ -46,7 +47,7 @@ public abstract class ThreadListActivity getController(); @@ -63,8 +64,6 @@ public abstract class ThreadListActivity(this) { @Override public void onResultUi(G groupItem) { - onGroupItemLoaded(groupItem); + onNamedGroupLoaded(groupItem); } @Override @@ -107,11 +102,8 @@ public abstract class ThreadListActivity getMaxBodyLength()) { + displaySnackbarShort(R.string.text_too_long); + return; + } I replyItem = adapter.getReplyItem(); UiResultExceptionHandler handler = new UiResultExceptionHandler(this) { @@ -258,6 +254,8 @@ public abstract class ThreadListActivity +public interface ThreadListController extends ActivityLifecycleController { void setGroupId(GroupId groupId); - void loadGroupItem(ResultExceptionHandler handler); + void loadNamedGroup(ResultExceptionHandler handler); void loadItem(H header, ResultExceptionHandler handler); @@ -35,7 +34,7 @@ public interface ThreadListController handler); - void deleteGroupItem(ResultExceptionHandler handler); + void deleteNamedGroup(ResultExceptionHandler handler); interface ThreadListListener extends DestroyableContext { @UiThread diff --git a/briar-android/src/org/briarproject/android/threaded/ThreadListControllerImpl.java b/briar-android/src/org/briarproject/android/threaded/ThreadListControllerImpl.java index 3fbf2bccc..c412d2eaa 100644 --- a/briar-android/src/org/briarproject/android/threaded/ThreadListControllerImpl.java +++ b/briar-android/src/org/briarproject/android/threaded/ThreadListControllerImpl.java @@ -7,8 +7,8 @@ import android.support.annotation.Nullable; import org.briarproject.android.api.AndroidNotificationManager; import org.briarproject.android.controller.DbControllerImpl; import org.briarproject.android.controller.handler.ResultExceptionHandler; -import org.briarproject.api.clients.BaseGroup; import org.briarproject.api.clients.BaseMessage; +import org.briarproject.api.clients.NamedGroup; import org.briarproject.api.clients.PostHeader; import org.briarproject.api.crypto.CryptoExecutor; import org.briarproject.api.db.DatabaseExecutor; @@ -33,7 +33,7 @@ import java.util.logging.Logger; import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; -public abstract class ThreadListControllerImpl +public abstract class ThreadListControllerImpl extends DbControllerImpl implements ThreadListController, EventListener { @@ -47,7 +47,7 @@ public abstract class ThreadListControllerImpl bodyCache = new ConcurrentHashMap<>(); - protected volatile GroupId groupId; + private volatile GroupId groupId; protected ThreadListListener listener; @@ -110,7 +110,7 @@ public abstract class ThreadListControllerImpl handler) { checkGroupId(); runOnDbThread(new Runnable() { @@ -121,7 +121,8 @@ public abstract class ThreadListControllerImpl bodies = loadBodies(headers); + bodyCache.putAll(bodies); duration = System.currentTimeMillis() - now; if (LOG.isLoggable(INFO)) LOG.info("Loading bodies took " + duration + " ms"); @@ -173,19 +171,11 @@ public abstract class ThreadListControllerImpl loadHeaders() throws DbException; - /** - * This should only be run from the DbThread. - * - * @throws DbException - */ - protected abstract void loadBodies(Collection headers) + @DatabaseExecutor + protected abstract Map loadBodies(Collection headers) throws DbException; @Override @@ -196,8 +186,10 @@ public abstract class ThreadListControllerImpl resultHandler) { runOnDbThread(new Runnable() { @Override @@ -283,11 +266,12 @@ public abstract class ThreadListControllerImpl handler) { runOnDbThread(new Runnable() { @Override @@ -328,17 +308,13 @@ public abstract class ThreadListControllerImpl buildItems(Collection headers) { List entries = new ArrayList<>(); for (H h : headers) { - entries.add(buildItem(h)); + entries.add(buildItem(h, bodyCache.get(h.getId()))); } return entries; } @@ -346,7 +322,12 @@ public abstract class ThreadListControllerImpl