Verified Commit ae0fa351 authored by Torsten Grote's avatar Torsten Grote

Better explain why messages could not be deleted

This also fixes a bug in the IntroductionManager that would allow to
delete only part of a session's visible messages.
parent 11c43dc7
Pipeline #3908 passed with stage
in 10 minutes and 40 seconds
......@@ -67,6 +67,7 @@ import org.briarproject.briar.api.conversation.ConversationManager;
import org.briarproject.briar.api.conversation.ConversationMessageHeader;
import org.briarproject.briar.api.conversation.ConversationRequest;
import org.briarproject.briar.api.conversation.ConversationResponse;
import org.briarproject.briar.api.conversation.DeletionResult;
import org.briarproject.briar.api.conversation.event.ConversationMessageReceivedEvent;
import org.briarproject.briar.api.forum.ForumSharingManager;
import org.briarproject.briar.api.introduction.IntroductionManager;
......@@ -857,9 +858,9 @@ public class ConversationActivity extends BriarActivity
list.showProgressBar();
runOnDbThread(() -> {
try {
boolean allDeleted =
DeletionResult result =
conversationManager.deleteAllMessages(contactId);
reloadConversationAfterDeletingMessages(allDeleted);
reloadConversationAfterDeletingMessages(result);
} catch (DbException e) {
logException(LOG, WARNING, e);
runOnUiThreadUnlessDestroyed(() -> list.showData());
......@@ -874,9 +875,9 @@ public class ConversationActivity extends BriarActivity
if (actionMode != null) actionMode.finish();
runOnDbThread(() -> {
try {
boolean allDeleted =
DeletionResult result =
conversationManager.deleteMessages(contactId, selected);
reloadConversationAfterDeletingMessages(allDeleted);
reloadConversationAfterDeletingMessages(result);
} catch (DbException e) {
logException(LOG, WARNING, e);
runOnUiThreadUnlessDestroyed(() -> list.showData());
......@@ -885,22 +886,55 @@ public class ConversationActivity extends BriarActivity
}
private void reloadConversationAfterDeletingMessages(
boolean allDeleted) {
DeletionResult result) {
runOnUiThreadUnlessDestroyed(() -> {
adapter.clear();
list.showProgressBar(); // otherwise clearing shows empty state
loadMessages();
if (!allDeleted) showNotAllDeletedDialog();
if (!result.allDeleted()) showNotAllDeletedDialog(result);
});
}
private void showNotAllDeletedDialog() {
private void showNotAllDeletedDialog(DeletionResult result) {
StringBuilder msg = new StringBuilder();
// get failures the user can not immediately prevent
StringBuilder fails = new StringBuilder();
if (result.hasSessionInProgress()) {
if (result.hasIntroduction()) {
String s = getString(
R.string.dialog_message_not_deleted_ongoing_introductions);
fails.append(s).append("\n");
}
if (result.hasInvitation()) {
String s = getString(
R.string.dialog_message_not_deleted_ongoing_invitations);
fails.append(s).append("\n");
}
}
if (result.hasNotFullyDownloaded()) {
String s = getString(
R.string.dialog_message_not_deleted_partly_downloaded);
fails.append(s).append("\n");
}
// add failures to message if there are any
if (fails.length() > 0) {
String s = getString(R.string.dialog_message_not_deleted,
fails.toString());
msg.append(s);
}
// add problems the user can resolve
if (result.hasNotAllSelected()) {
if (msg.length() > 0) msg.append("\n\n");
String s = getString(
R.string.dialog_message_not_deleted_not_all_selected);
msg.append(s);
}
// show dialog
AlertDialog.Builder builder =
new AlertDialog.Builder(this, R.style.BriarDialogTheme);
builder.setTitle(
getString(R.string.dialog_title_not_all_messages_deleted));
builder.setMessage(
getString(R.string.dialog_message_not_all_messages_deleted));
builder.setMessage(msg.toString());
builder.setPositiveButton(R.string.ok, null);
builder.show();
}
......@@ -912,7 +946,8 @@ public class ConversationActivity extends BriarActivity
new AlertDialog.Builder(ConversationActivity.this,
R.style.BriarDialogTheme);
builder.setTitle(getString(R.string.dialog_title_delete_contact));
builder.setMessage(getString(R.string.dialog_message_delete_contact));
builder.setMessage(
getString(R.string.dialog_message_delete_contact));
builder.setNegativeButton(R.string.delete, okListener);
builder.setPositiveButton(R.string.cancel, null);
builder.show();
......@@ -953,7 +988,8 @@ public class ConversationActivity extends BriarActivity
private void showImageOnboarding() {
// TODO: remove cast when removing feature flag
((TextAttachmentController) sendController).showImageOnboarding(this);
((TextAttachmentController) sendController)
.showImageOnboarding(this);
}
private void showIntroductionOnboarding(@Nullable Boolean show) {
......@@ -973,7 +1009,8 @@ public class ConversationActivity extends BriarActivity
View target = null;
for (int i = 0; i < toolbar.getChildCount(); i++) {
if (toolbar.getChildAt(i) instanceof ActionMenuView) {
ActionMenuView menu = (ActionMenuView) toolbar.getChildAt(i);
ActionMenuView menu =
(ActionMenuView) toolbar.getChildAt(i);
// The overflow icon should be the last child of the menu
target = menu.getChildAt(menu.getChildCount() - 1);
// If the menu hasn't been populated yet, use the menu itself
......@@ -999,7 +1036,8 @@ public class ConversationActivity extends BriarActivity
@UiThread
@Override
public void respondToRequest(ConversationRequestItem item, boolean accept) {
public void respondToRequest(ConversationRequestItem item,
boolean accept) {
item.setAnswered();
int position = adapter.findItemPosition(item);
if (position != INVALID_POSITION) {
......
......@@ -139,7 +139,11 @@
<string name="dialog_title_delete_all_messages">Confirm Message Deletion</string>
<string name="dialog_message_delete_all_messages">Are you sure that you want to delete all messages?</string>
<string name="dialog_title_not_all_messages_deleted">Could not delete all messages</string>
<string name="dialog_message_not_all_messages_deleted">Messages related to\n\n• ongoing introductions\n• ongoing (blog/forum/group) invitations\n• partly downloaded messages\n\ncannot be deleted until they conclude.</string>
<string name="dialog_message_not_deleted">Messages related to\n\n%s\ncan not be deleted until they conclude.</string>
<string name="dialog_message_not_deleted_ongoing_introductions">• ongoing introductions</string>
<string name="dialog_message_not_deleted_ongoing_invitations">• ongoing (blog/forum/group) invitations</string>
<string name="dialog_message_not_deleted_partly_downloaded">• partly downloaded messages</string>
<string name="dialog_message_not_deleted_not_all_selected">To delete an invitation/introduction, you need to select the request and the response.</string>
<string name="delete_contact">Delete contact</string>
<string name="dialog_title_delete_contact">Confirm Contact Deletion</string>
<string name="dialog_message_delete_contact">Are you sure that you want to remove this contact and all messages exchanged with this contact?</string>
......
......@@ -17,6 +17,12 @@ import java.util.Set;
@NotNullByDefault
public interface ConversationManager {
int DELETE_SESSION_IS_INTRODUCTION = 1;
int DELETE_SESSION_IS_INVITATION = 1 << 1;
int DELETE_SESSION_INCOMPLETE = 1 << 2;
int DELETE_SESSION_IN_PROGRESS = 1 << 3;
int DELETE_NOT_DOWNLOADED = 1 << 4;
/**
* Clients that present messages in a private conversation need to
* register themselves here.
......@@ -39,17 +45,13 @@ public interface ConversationManager {
/**
* Deletes all messages exchanged with the given contact.
*
* @return true if all messages could be deleted, false otherwise
*/
boolean deleteAllMessages(ContactId c) throws DbException;
DeletionResult deleteAllMessages(ContactId c) throws DbException;
/**
* Deletes the given set of messages associated with the given contact.
*
* @return true if all given messages could be deleted, false otherwise
*/
boolean deleteMessages(ContactId c, Collection<MessageId> messageIds)
DeletionResult deleteMessages(ContactId c, Collection<MessageId> messageIds)
throws DbException;
@NotNullByDefault
......@@ -75,10 +77,8 @@ public interface ConversationManager {
/**
* Deletes all messages associated with the given contact.
*
* @return true if all messages could be deleted, false otherwise
*/
boolean deleteAllMessages(Transaction txn,
DeletionResult deleteAllMessages(Transaction txn,
ContactId c) throws DbException;
/**
......@@ -86,10 +86,8 @@ public interface ConversationManager {
* <p>
* The set of message IDs must only include message IDs returned by
* {@link #getMessageIds}.
*
* @return true if all messages could be deleted, false otherwise
*/
boolean deleteMessages(Transaction txn, ContactId c,
DeletionResult deleteMessages(Transaction txn, ContactId c,
Set<MessageId> messageIds) throws DbException;
}
......
package org.briarproject.briar.api.conversation;
import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
import javax.annotation.concurrent.NotThreadSafe;
import static org.briarproject.briar.api.conversation.ConversationManager.DELETE_NOT_DOWNLOADED;
import static org.briarproject.briar.api.conversation.ConversationManager.DELETE_SESSION_INCOMPLETE;
import static org.briarproject.briar.api.conversation.ConversationManager.DELETE_SESSION_IN_PROGRESS;
import static org.briarproject.briar.api.conversation.ConversationManager.DELETE_SESSION_IS_INTRODUCTION;
import static org.briarproject.briar.api.conversation.ConversationManager.DELETE_SESSION_IS_INVITATION;
@NotThreadSafe
@NotNullByDefault
public class DeletionResult {
private int result = 0;
public void addDeletionResult(DeletionResult deletionResult) {
result |= deletionResult.result;
}
public void addInvitationNotAllSelected() {
result |= DELETE_SESSION_INCOMPLETE | DELETE_SESSION_IS_INVITATION;
}
public void addInvitationSessionInProgress() {
result |= DELETE_SESSION_IN_PROGRESS | DELETE_SESSION_IS_INVITATION;
}
public void addIntroductionNotAllSelected() {
result |= DELETE_SESSION_INCOMPLETE | DELETE_SESSION_IS_INTRODUCTION;
}
public void addIntroductionSessionInProgress() {
result |= DELETE_SESSION_IN_PROGRESS | DELETE_SESSION_IS_INTRODUCTION;
}
public void addNotFullyDownloaded() {
result |= DELETE_NOT_DOWNLOADED;
}
public boolean allDeleted() {
return result == 0;
}
public boolean hasIntroduction() {
return (result & DELETE_SESSION_IS_INTRODUCTION) != 0;
}
public boolean hasInvitation() {
return (result & DELETE_SESSION_IS_INVITATION) != 0;
}
public boolean hasSessionInProgress() {
return (result & DELETE_SESSION_IN_PROGRESS) != 0;
}
public boolean hasNotAllSelected() {
return (result & DELETE_SESSION_INCOMPLETE) != 0;
}
public boolean hasNotFullyDownloaded() {
return (result & DELETE_NOT_DOWNLOADED) != 0;
}
}
......@@ -32,6 +32,7 @@ import org.briarproject.bramble.api.versioning.ClientVersioningManager.ClientVer
import org.briarproject.briar.api.client.MessageTracker;
import org.briarproject.briar.api.client.SessionId;
import org.briarproject.briar.api.conversation.ConversationMessageHeader;
import org.briarproject.briar.api.conversation.DeletionResult;
import org.briarproject.briar.api.introduction.IntroductionManager;
import org.briarproject.briar.api.introduction.IntroductionRequest;
import org.briarproject.briar.api.introduction.IntroductionResponse;
......@@ -561,73 +562,45 @@ class IntroductionManagerImpl extends ConversationClientImpl
@FunctionalInterface
private interface MessageRetriever {
Map<MessageId, BdfDictionary> getMessages(Transaction txn,
GroupId contactGroup) throws DbException;
}
@FunctionalInterface
private interface MessageDeletionChecker {
/**
* This is called for all messages belonging to a session.
* It returns true if the given {@link MessageId} causes a problem
* so that the session can not be deleted.
*/
boolean causesProblem(MessageId messageId);
Set<MessageId> getMessages(Set<MessageId> allMessages);
}
@Override
public boolean deleteAllMessages(Transaction txn, ContactId c)
public DeletionResult deleteAllMessages(Transaction txn, ContactId c)
throws DbException {
return deleteMessages(txn, c, (txn1, g) -> {
// get metadata for all messages in the group
Map<MessageId, BdfDictionary> messages;
try {
messages = clientHelper.getMessageMetadataAsDictionary(txn1, g);
} catch (FormatException e) {
throw new DbException(e);
}
return messages;
}, messageId -> false);
return deleteMessages(txn, c, allMessages -> allMessages);
}
@Override
public boolean deleteMessages(Transaction txn, ContactId c,
public DeletionResult deleteMessages(Transaction txn, ContactId c,
Set<MessageId> messageIds) throws DbException {
return deleteMessages(txn, c, (txn1, g) -> {
// get metadata for messages that shall be deleted
Map<MessageId, BdfDictionary> messages =
new HashMap<>(messageIds.size());
for (MessageId m : messageIds) {
BdfDictionary d;
try {
d = clientHelper.getMessageMetadataAsDictionary(txn1, m);
} catch (FormatException e) {
throw new DbException(e);
}
// If message metadata does not exist,
// getMessageMetadataAsDictionary(txn, m) returns empty Metadata
if (!d.isEmpty()) messages.put(m, d);
}
return messages;
// don't delete sessions if a message is not part of messageIds
}, messageId -> !messageIds.contains(messageId));
return deleteMessages(txn, c, allMessages -> messageIds);
}
private boolean deleteMessages(Transaction txn, ContactId c,
MessageRetriever retriever, MessageDeletionChecker checker)
throws DbException {
private DeletionResult deleteMessages(Transaction txn, ContactId c,
MessageRetriever retriever) throws DbException {
// get ID of the contact group
GroupId g = getContactGroup(db.getContact(txn, c)).getId();
// get metadata for all messages in the group
Map<MessageId, BdfDictionary> messages;
try {
messages = clientHelper.getMessageMetadataAsDictionary(txn, g);
} catch (FormatException e) {
throw new DbException(e);
}
// get messages to be deleted
Map<MessageId, BdfDictionary> messages = retriever.getMessages(txn, g);
Set<MessageId> selected = retriever.getMessages(messages.keySet());
// assign protocol messages to their sessions
// get sessions for selected messages
Map<SessionId, DeletableSession> sessions = new HashMap<>();
for (Entry<MessageId, BdfDictionary> entry : messages.entrySet()) {
for (MessageId id : selected) {
BdfDictionary d = messages.get(id);
if (d == null) continue; // throw new NoSuchMessageException()
MessageMetadata m;
try {
m = messageParser.parseMetadata(entry.getValue());
m = messageParser.parseMetadata(d);
} catch (FormatException e) {
throw new DbException(e);
}
......@@ -644,6 +617,31 @@ class IntroductionManagerImpl extends ConversationClientImpl
session = getDeletableSession(txn, g, m.getSessionId());
sessions.put(m.getSessionId(), session);
}
session.messages.add(id);
}
// assign other protocol messages to their sessions
for (Entry<MessageId, BdfDictionary> entry : messages.entrySet()) {
// we handled selected messages above
if (selected.contains(entry.getKey())) continue;
MessageMetadata m;
try {
m = messageParser.parseMetadata(entry.getValue());
} catch (FormatException e) {
throw new DbException(e);
}
if (!m.isVisibleInConversation()) continue;
if (m.getSessionId() == null) {
// This can only be an unhandled REQUEST message.
// Its session is created and stored in incomingMessage(),
// and getMessageMetadata() only returns delivered messages,
// so the session ID should have been assigned.
throw new AssertionError("missing session ID");
}
// get session from map or database
DeletableSession session = sessions.get(m.getSessionId());
if (session == null) continue; // not a session of a selected msg
session.messages.add(entry.getKey());
}
......@@ -652,10 +650,10 @@ class IntroductionManagerImpl extends ConversationClientImpl
for (MessageStatus status : db.getMessageStatus(txn, c, g)) {
if (!status.isSeen()) notAcked.add(status.getMessageId());
}
boolean allDeleted =
deleteCompletedSessions(txn, sessions, notAcked, checker);
DeletionResult result =
deleteCompletedSessions(txn, sessions, notAcked, selected);
recalculateGroupCount(txn, g);
return allDeleted;
return result;
}
private DeletableSession getDeletableSession(Transaction txn,
......@@ -677,28 +675,31 @@ class IntroductionManagerImpl extends ConversationClientImpl
}
}
private boolean deleteCompletedSessions(Transaction txn,
private DeletionResult deleteCompletedSessions(Transaction txn,
Map<SessionId, DeletableSession> sessions, Set<MessageId> notAcked,
MessageDeletionChecker checker) throws DbException {
Set<MessageId> selected) throws DbException {
// find completed sessions to delete
boolean allDeleted = true;
DeletionResult result = new DeletionResult();
for (DeletableSession session : sessions.values()) {
if (!session.state.isComplete()) {
allDeleted = false;
result.addIntroductionSessionInProgress();
continue;
}
// we can only delete sessions
// where delivery of all messages was confirmed (aka ACKed)
boolean allAcked = true;
boolean sessionDeletable = true;
for (MessageId m : session.messages) {
if (notAcked.contains(m) || checker.causesProblem(m)) {
allAcked = false;
allDeleted = false;
if (notAcked.contains(m) || !selected.contains(m)) {
sessionDeletable = false;
if (notAcked.contains(m))
result.addIntroductionSessionInProgress();
if (!selected.contains(m))
result.addIntroductionNotAllSelected();
break;
}
}
// delete messages of session, if all were ACKed
if (allAcked) {
if (sessionDeletable) {
for (MessageId m : session.messages) {
db.deleteMessage(txn, m);
db.deleteMessageMetadata(txn, m);
......@@ -707,7 +708,7 @@ class IntroductionManagerImpl extends ConversationClientImpl
// and then needs the previous MessageIds
}
}
return allDeleted;
return result;
}
@Override
......
......@@ -9,6 +9,7 @@ import org.briarproject.bramble.api.sync.MessageId;
import org.briarproject.briar.api.client.MessageTracker.GroupCount;
import org.briarproject.briar.api.conversation.ConversationManager;
import org.briarproject.briar.api.conversation.ConversationMessageHeader;
import org.briarproject.briar.api.conversation.DeletionResult;
import java.util.ArrayList;
import java.util.Collection;
......@@ -75,27 +76,27 @@ class ConversationManagerImpl implements ConversationManager {
}
@Override
public boolean deleteAllMessages(ContactId c) throws DbException {
public DeletionResult deleteAllMessages(ContactId c) throws DbException {
return db.transactionWithResult(false, txn -> {
boolean allDeleted = true;
DeletionResult result = new DeletionResult();
for (ConversationClient client : clients) {
allDeleted = client.deleteAllMessages(txn, c) && allDeleted;
result.addDeletionResult(client.deleteAllMessages(txn, c));
}
return allDeleted;
return result;
});
}
@Override
public boolean deleteMessages(ContactId c, Collection<MessageId> toDelete)
public DeletionResult deleteMessages(ContactId c, Collection<MessageId> toDelete)
throws DbException {
return db.transactionWithResult(false, txn -> {
boolean allDeleted = true;
DeletionResult result = new DeletionResult();
for (ConversationClient client : clients) {
Set<MessageId> idSet = client.getMessageIds(txn, c);
idSet.retainAll(toDelete);
allDeleted = client.deleteMessages(txn, c, idSet) && allDeleted;
result.addDeletionResult(client.deleteMessages(txn, c, idSet));
}
return allDeleted;
return result;
});
}
......
......@@ -31,6 +31,7 @@ import org.briarproject.briar.api.client.MessageTracker;
import org.briarproject.briar.api.client.MessageTracker.GroupCount;
import org.briarproject.briar.api.conversation.ConversationManager.ConversationClient;
import org.briarproject.briar.api.conversation.ConversationMessageHeader;
import org.briarproject.briar.api.conversation.DeletionResult;
import org.briarproject.briar.api.messaging.Attachment;
import org.briarproject.briar.api.messaging.AttachmentHeader;
import org.briarproject.briar.api.messaging.FileTooBigException;
......@@ -430,7 +431,7 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
}
@Override
public boolean deleteAllMessages(Transaction txn, ContactId c)
public DeletionResult deleteAllMessages(Transaction txn, ContactId c)
throws DbException {
GroupId g = getContactGroup(db.getContact(txn, c)).getId();
// this indiscriminately deletes all raw messages in this group
......@@ -440,13 +441,13 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
db.deleteMessageMetadata(txn, messageId);
}
messageTracker.initializeGroupCount(txn, g);
return true;
return new DeletionResult();
}
@Override
public boolean deleteMessages(Transaction txn, ContactId c,
public DeletionResult deleteMessages(Transaction txn, ContactId c,
Set<MessageId> messageIds) throws DbException {
boolean allDeleted = true;
DeletionResult result = new DeletionResult();
for (MessageId m : messageIds) {
// get attachment headers
List<AttachmentHeader> headers;
......@@ -480,12 +481,12 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
db.deleteMessage(txn, m);
db.deleteMessageMetadata(txn, m);
} else {
allDeleted = false;
result.addNotFullyDownloaded();
}
}
GroupId g = getContactGroup(db.getContact(txn, c)).getId();
recalculateGroupCount(txn, g);
return allDeleted;
return result;
}
private void recalculateGroupCount(Transaction txn, GroupId g)
......
......@@ -27,6 +27,7 @@ import org.briarproject.bramble.api.versioning.ClientVersioningManager.ClientVer
import org.briarproject.briar.api.client.MessageTracker;
import org.briarproject.briar.api.client.SessionId;
import org.briarproject.briar.api.conversation.ConversationMessageHeader;
import org.briarproject.briar.api.conversation.DeletionResult;