Address issues found in code review

This commit is contained in:
Torsten Grote
2016-10-18 15:50:05 -02:00
parent 7bf4aebdaf
commit 0523c4e718
15 changed files with 235 additions and 163 deletions

View File

@@ -41,7 +41,7 @@ public abstract class ThreadItemAdapter<I extends ThreadItem>
@Override
public void onBindViewHolder(ThreadItemViewHolder<I> 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<I extends ThreadItem>
}
}
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;
}

View File

@@ -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<G extends BaseGroup, I extends ThreadItem, H extends PostHeader, A extends ThreadItemAdapter<I>>
public abstract class ThreadListActivity<G extends NamedGroup, I extends ThreadItem, H extends PostHeader, A extends ThreadItemAdapter<I>>
extends BriarActivity
implements ThreadListListener<H>, TextInputListener,
ThreadItemListener<I> {
@@ -46,7 +47,7 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
protected BriarRecyclerView list;
protected TextInputView textInput;
protected GroupId groupId;
private byte[] replyId;
private MessageId replyId;
protected abstract ThreadListController<G, I, H> getController();
@@ -63,8 +64,6 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
if (b == null) throw new IllegalStateException("No GroupId in intent.");
groupId = new GroupId(b);
getController().setGroupId(groupId);
String groupName = i.getStringExtra(GROUP_NAME);
setActionBarTitle(groupName);
textInput = (TextInputView) findViewById(R.id.text_input_container);
textInput.setVisibility(GONE);
@@ -76,27 +75,23 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
list.setAdapter(adapter);
if (state != null) {
replyId = state.getByteArray(KEY_REPLY_ID);
replyId = new MessageId(state.getByteArray(KEY_REPLY_ID));
}
loadItems();
}
protected abstract @LayoutRes int getLayout();
@LayoutRes
protected abstract int getLayout();
protected abstract A createAdapter(LinearLayoutManager layoutManager);
protected void setActionBarTitle(@Nullable String title) {
if (title != null) setTitle(title);
else loadGroupItem();
}
protected void loadGroupItem() {
getController().loadGroupItem(
protected void loadNamedGroup() {
getController().loadNamedGroup(
new UiResultExceptionHandler<G, DbException>(this) {
@Override
public void onResultUi(G groupItem) {
onGroupItemLoaded(groupItem);
onNamedGroupLoaded(groupItem);
}
@Override
@@ -107,11 +102,8 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
});
}
@CallSuper
@UiThread
protected void onGroupItemLoaded(G groupItem) {
setTitle(groupItem.getName());
}
protected abstract void onNamedGroupLoaded(G groupItem);
private void loadItems() {
getController().loadItems(
@@ -208,7 +200,7 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
showTextInput(item);
}
protected void displaySnackbarShort(int stringId) {
protected void displaySnackbarShort(@StringRes int stringId) {
Snackbar snackbar = make(list, stringId, Snackbar.LENGTH_SHORT);
snackbar.getView().setBackgroundResource(R.color.briar_primary);
snackbar.show();
@@ -233,6 +225,10 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
public void onSendClick(String text) {
if (text.trim().length() == 0)
return;
if (text.length() > getMaxBodyLength()) {
displaySnackbarShort(R.string.text_too_long);
return;
}
I replyItem = adapter.getReplyItem();
UiResultExceptionHandler<I, DbException> handler =
new UiResultExceptionHandler<I, DbException>(this) {
@@ -258,6 +254,8 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
adapter.setReplyItem(null);
}
protected abstract int getMaxBodyLength();
@Override
public void onHeaderReceived(H header) {
getController().loadItem(header,
@@ -302,8 +300,10 @@ public abstract class ThreadListActivity<G extends BaseGroup, I extends ThreadIt
}
}
protected abstract @StringRes int getItemPostedString();
@StringRes
protected abstract int getItemPostedString();
protected abstract @StringRes int getItemReceivedString();
@StringRes
protected abstract int getItemReceivedString();
}

View File

@@ -6,21 +6,20 @@ import android.support.annotation.UiThread;
import org.briarproject.android.DestroyableContext;
import org.briarproject.android.controller.ActivityLifecycleController;
import org.briarproject.android.controller.handler.ResultExceptionHandler;
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.forum.ForumPostHeader;
import org.briarproject.api.sync.GroupId;
import org.briarproject.api.sync.MessageId;
import java.util.Collection;
public interface ThreadListController<G extends BaseGroup, I extends ThreadItem, H extends PostHeader>
public interface ThreadListController<G extends NamedGroup, I extends ThreadItem, H extends PostHeader>
extends ActivityLifecycleController {
void setGroupId(GroupId groupId);
void loadGroupItem(ResultExceptionHandler<G, DbException> handler);
void loadNamedGroup(ResultExceptionHandler<G, DbException> handler);
void loadItem(H header, ResultExceptionHandler<I, DbException> handler);
@@ -35,7 +34,7 @@ public interface ThreadListController<G extends BaseGroup, I extends ThreadItem,
void send(String body, @Nullable MessageId parentId,
ResultExceptionHandler<I, DbException> handler);
void deleteGroupItem(ResultExceptionHandler<Void, DbException> handler);
void deleteNamedGroup(ResultExceptionHandler<Void, DbException> handler);
interface ThreadListListener<H> extends DestroyableContext {
@UiThread

View File

@@ -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<G extends BaseGroup, I extends ThreadItem, H extends PostHeader, M extends BaseMessage>
public abstract class ThreadListControllerImpl<G extends NamedGroup, I extends ThreadItem, H extends PostHeader, M extends BaseMessage>
extends DbControllerImpl
implements ThreadListController<G, I, H>, EventListener {
@@ -47,7 +47,7 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
protected final Map<MessageId, String> bodyCache =
new ConcurrentHashMap<>();
protected volatile GroupId groupId;
private volatile GroupId groupId;
protected ThreadListListener<H> listener;
@@ -110,7 +110,7 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
}
@Override
public void loadGroupItem(
public void loadNamedGroup(
final ResultExceptionHandler<G, DbException> handler) {
checkGroupId();
runOnDbThread(new Runnable() {
@@ -121,7 +121,8 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
G groupItem = loadGroupItem();
long duration = System.currentTimeMillis() - now;
if (LOG.isLoggable(INFO))
LOG.info("Loading forum took " + duration + " ms");
LOG.info(
"Loading named group took " + duration + " ms");
handler.onResult(groupItem);
} catch (DbException e) {
if (LOG.isLoggable(WARNING))
@@ -132,11 +133,7 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
@DatabaseExecutor
protected abstract G loadGroupItem() throws DbException;
@Override
@@ -157,7 +154,8 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
// Load bodies
now = System.currentTimeMillis();
loadBodies(headers);
Map<MessageId, String> 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<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
@DatabaseExecutor
protected abstract Collection<H> loadHeaders() throws DbException;
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
protected abstract void loadBodies(Collection<H> headers)
@DatabaseExecutor
protected abstract Map<MessageId, String> loadBodies(Collection<H> headers)
throws DbException;
@Override
@@ -196,8 +186,10 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
public void run() {
LOG.info("Loading item...");
try {
loadBodies(Collections.singletonList(header));
I item = buildItem(header);
String body = loadBodies(Collections.singletonList(header))
.get(header.getId());
bodyCache.put(header.getId(), body);
I item = buildItem(header, body);
handler.onResult(item);
} catch (DbException e) {
if (LOG.isLoggable(WARNING))
@@ -234,11 +226,7 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
@DatabaseExecutor
protected abstract void markRead(MessageId id) throws DbException;
@Override
@@ -255,9 +243,8 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
public void run() {
LOG.info("Creating message...");
try {
M msg = createLocalMessage(groupId, body, parentId);
bodyCache.put(msg.getMessage().getId(), body);
storePost(msg, handler);
M msg = createLocalMessage(body, parentId);
storePost(msg, body, handler);
} catch (DbException e) {
if (LOG.isLoggable(WARNING))
LOG.log(WARNING, e.toString(), e);
@@ -267,15 +254,11 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
protected abstract M createLocalMessage(GroupId g, String body,
@DatabaseExecutor
protected abstract M createLocalMessage(String body,
@Nullable MessageId parentId) throws DbException;
private void storePost(final M p,
private void storePost(final M msg, final String body,
final ResultExceptionHandler<I, DbException> resultHandler) {
runOnDbThread(new Runnable() {
@Override
@@ -283,11 +266,12 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
try {
LOG.info("Store message...");
long now = System.currentTimeMillis();
H h = addLocalMessage(p);
H header = addLocalMessage(msg);
bodyCache.put(msg.getMessage().getId(), body);
long duration = System.currentTimeMillis() - now;
if (LOG.isLoggable(INFO))
LOG.info("Storing message took " + duration + " ms");
resultHandler.onResult(buildItem(h));
resultHandler.onResult(buildItem(header, body));
} catch (DbException e) {
if (LOG.isLoggable(WARNING))
LOG.log(WARNING, e.toString(), e);
@@ -297,15 +281,11 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
@DatabaseExecutor
protected abstract H addLocalMessage(M message) throws DbException;
@Override
public void deleteGroupItem(
public void deleteNamedGroup(
final ResultExceptionHandler<Void, DbException> handler) {
runOnDbThread(new Runnable() {
@Override
@@ -328,17 +308,13 @@ public abstract class ThreadListControllerImpl<G extends BaseGroup, I extends Th
});
}
/**
* This should only be run from the DbThread.
*
* @throws DbException
*/
@DatabaseExecutor
protected abstract void deleteGroupItem(G groupItem) throws DbException;
private List<I> buildItems(Collection<H> headers) {
List<I> 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<G extends BaseGroup, I extends Th
/**
* When building the item, the body can be assumed to be cached
*/
protected abstract I buildItem(H header);
protected abstract I buildItem(H header, String body);
protected GroupId getGroupId() {
checkGroupId();
return groupId;
}
private void checkGroupId() {
if (groupId == null) {