Add Ability to Delete Messages Locally #70

Merged
delvh merged 6 commits from f/delete-messages into develop 2020-09-30 20:50:59 +02:00
8 changed files with 233 additions and 66 deletions

View File

@ -74,7 +74,7 @@ public class Chat implements Serializable {
public boolean equals(Object obj) {
if (this == obj) return true;
if (!(obj instanceof Chat)) return false;
final Chat other = (Chat) obj;
final var other = (Chat) obj;
return Objects.equals(recipient, other.recipient);
}
@ -89,7 +89,7 @@ public class Chat implements Serializable {
*/
public void read(WriteProxy writeProxy) {
for (int i = messages.size() - 1; i >= 0; --i) {
final Message m = messages.get(i);
final var m = messages.get(i);
if (m.getSenderID() == recipient.getID()) if (m.getStatus() == MessageStatus.READ) break;
else {
m.setStatus(MessageStatus.READ);
@ -121,6 +121,15 @@ public class Chat implements Serializable {
messages.add(0, message);
}
/**
* Removes the message with the given ID.
delvh marked this conversation as resolved Outdated
Outdated
Review

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
*
* @param messageID the ID of the message to remove
* @return whether the message has been found and removed
delvh marked this conversation as resolved Outdated
Outdated
Review

You might consider changing this to "whether the message has been removed", as "any" implies the removal of random messages.

You might consider changing this to "whether the message has been removed", as "any" implies the removal of random messages.
Outdated
Review

While I do see your point, I'd also point out that the current tag is pretty much equivalent to Collection.removeIf, which only states that it returns true if any element was removed.
But okay, I can change it.

While I do see your point, I'd also point out that the current tag is pretty much equivalent to `Collection.removeIf`, which only states that it returns true if any element was removed. But okay, I can change it.
Outdated
Review

That's true, because Collection.removeIf acts on arbitrary collections, while a list of messages has the property that the message IDs are unique.

That's true, because `Collection.removeIf` acts on arbitrary collections, while a list of messages has the property that the message IDs are unique.
* @since Envoy Client v0.3-beta
*/
public boolean remove(long messageID) { return messages.removeIf(m -> m.getID() == messageID); }
/**
* Increments the amount of unread messages.
*

View File

@ -7,6 +7,7 @@ import java.time.Instant;
import java.util.*;
import java.util.logging.*;
import javafx.application.Platform;
import javafx.collections.*;
import envoy.client.event.*;
@ -273,6 +274,24 @@ public final class LocalDB implements EventListener {
cacheMap.clear();
}
/**
* Deletes the message with the given ID, if present.
delvh marked this conversation as resolved Outdated
Outdated
Review

Again, consider rewording this to "... if present", as there is no such thing as multiple messages with the same ID.

Again, consider rewording this to "... if present", as there is no such thing as multiple messages with the same ID.
*
* @param message the event that was
* @since Envoy Client v0.3-beta
*/
@Event
delvh marked this conversation as resolved Outdated
Outdated
Review

The parentheses after @Event are unnecessary here.

The parentheses after `@Event` are unnecessary here.
private void onMessageDeletion(MessageDeletion message) {
Platform.runLater(() -> {
// We suppose that messages have unique IDs, hence the search can be stopped
// once a message was removed
final var messageID = message.get();
for (final var chat : chats)
if (chat.remove(messageID)) break;
});
}
/**
* @return a {@code Map<String, User>} of all users stored locally with their
* user names as keys

View File

@ -0,0 +1,20 @@
package envoy.client.event;
import envoy.event.Event;
/**
* Conveys the deletion of a message.
delvh marked this conversation as resolved Outdated
Outdated
Review

This does not make sense as this is a client-sided event.

This does not make sense as this is a client-sided event.
Outdated
Review

That's an artifact from when it was an Event in Common.

That's an artifact from when it was an Event in Common.
*
* @author Leon Hofmeister
* @since Envoy Common v0.3-beta
*/
public class MessageDeletion extends Event<Long> {
private static final long serialVersionUID = 1L;
/**
* @param messageID the ID of the deleted message
* @since Envoy Common v0.3-beta
*/
public MessageDeletion(long messageID) { super(messageID); }
}

View File

@ -1,8 +1,6 @@
package envoy.client.ui.control;
import java.awt.Toolkit;
import java.awt.datatransfer.StringSelection;
import java.io.*;
import java.io.ByteArrayInputStream;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;
import java.util.Map;
@ -12,11 +10,10 @@ import javafx.geometry.*;
import javafx.scene.control.*;
import javafx.scene.image.*;
import javafx.scene.layout.*;
import javafx.stage.FileChooser;
import envoy.client.data.*;
import envoy.client.ui.*;
import envoy.client.util.IconUtil;
import envoy.client.net.Client;
import envoy.client.util.*;
import envoy.data.*;
import envoy.data.Message.MessageStatus;
import envoy.util.EnvoyLog;
@ -32,13 +29,13 @@ public final class MessageControl extends Label {
private final boolean ownMessage;
private final LocalDB localDB = Context.getInstance().getLocalDB();
private final SceneContext sceneContext = Context.getInstance().getSceneContext();
private final LocalDB localDB = context.getLocalDB();
private final Client client = context.getClient();
private static final Context context = Context.getInstance();
private static final DateTimeFormatter dateFormat = DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm:ss")
.withZone(ZoneId.systemDefault());
private static final Map<MessageStatus, Image> statusImages = IconUtil.loadByEnum(MessageStatus.class, 16);
private static final Settings settings = Settings.getInstance();
private static final Logger logger = EnvoyLog.getLogger(MessageControl.class);
/**
@ -47,6 +44,8 @@ public final class MessageControl extends Label {
* @since Envoy Client v0.1-beta
*/
public MessageControl(Message message) {
ownMessage = message.getSenderID() == localDB.getUser().getID();
// Creating the underlying VBox and the dateLabel
final var hbox = new HBox();
if (message.getSenderID() != localDB.getUser().getID() && message instanceof GroupMessage) {
@ -67,18 +66,42 @@ public final class MessageControl extends Label {
final var vbox = new VBox(hbox);
// Creating the actions for the MenuItems
final var contextMenu = new ContextMenu();
final var copyMenuItem = new MenuItem("Copy");
final var deleteMenuItem = new MenuItem("Delete");
final var forwardMenuItem = new MenuItem("Forward");
final var quoteMenuItem = new MenuItem("Quote");
final var infoMenuItem = new MenuItem("Info");
copyMenuItem.setOnAction(e -> copyMessage(message));
deleteMenuItem.setOnAction(e -> deleteMessage(message));
forwardMenuItem.setOnAction(e -> forwardMessage(message));
quoteMenuItem.setOnAction(e -> quoteMessage(message));
final var contextMenu = new ContextMenu();
final var items = contextMenu.getItems();
// Copy message action
if (!message.getText().isEmpty()) {
final var copyMenuItem = new MenuItem("Copy Text");
copyMenuItem.setOnAction(e -> MessageUtil.copyMessageText(message));
items.add(copyMenuItem);
}
// Delete message - if own message - action
if (ownMessage && client.isOnline()) {
final var deleteMenuItem = new MenuItem("Delete locally");
deleteMenuItem.setOnAction(e -> MessageUtil.deleteMessage(message));
items.add(deleteMenuItem);
}
// As long as these types of messages are not implemented and no caches are
// defined for them, we only want them to appear when being online
if (client.isOnline()) {
// Forward menu item
final var forwardMenuItem = new MenuItem("Forward");
forwardMenuItem.setOnAction(e -> MessageUtil.forwardMessage(message));
items.add(forwardMenuItem);
// Quote menu item
final var quoteMenuItem = new MenuItem("Quote");
quoteMenuItem.setOnAction(e -> MessageUtil.quoteMessage(message));
items.add(quoteMenuItem);
}
// Info actions
final var infoMenuItem = new MenuItem("Info");
infoMenuItem.setOnAction(e -> loadMessageInfoScene(message));
contextMenu.getItems().addAll(copyMenuItem, deleteMenuItem, forwardMenuItem, quoteMenuItem, infoMenuItem);
items.add(infoMenuItem);
// Handling message attachment display
// TODO: Add missing attachment types
@ -97,8 +120,8 @@ public final class MessageControl extends Label {
break;
}
final var saveAttachment = new MenuItem("Save attachment");
saveAttachment.setOnAction(e -> saveAttachment(message));
contextMenu.getItems().add(saveAttachment);
saveAttachment.setOnAction(e -> MessageUtil.saveAttachment(message));
items.add(saveAttachment);
}
// Creating the textLabel
final var textLabel = new Label(message.getText());
@ -116,12 +139,8 @@ public final class MessageControl extends Label {
hBoxBottom.getChildren().add(statusIcon);
hBoxBottom.setAlignment(Pos.BOTTOM_RIGHT);
getStyleClass().add("own-message");
ownMessage = true;
hbox.setAlignment(Pos.CENTER_RIGHT);
} else {
getStyleClass().add("received-message");
ownMessage = false;
}
} else getStyleClass().add("received-message");
vbox.getChildren().add(hBoxBottom);
// Adjusting height and weight of the cell to the corresponding ListView
paddingProperty().setValue(new Insets(5, 20, 5, 20));
@ -129,41 +148,8 @@ public final class MessageControl extends Label {
setGraphic(vbox);
}
// Context Menu actions
private void copyMessage(Message message) {
Toolkit.getDefaultToolkit().getSystemClipboard().setContents(new StringSelection(message.getText()), null);
}
private void deleteMessage(Message message) { logger.log(Level.FINEST, "message deletion was requested for " + message); }
private void forwardMessage(Message message) { logger.log(Level.FINEST, "message forwarding was requested for " + message); }
private void quoteMessage(Message message) { logger.log(Level.FINEST, "message quotation was requested for " + message); }
private void loadMessageInfoScene(Message message) { logger.log(Level.FINEST, "message info scene was requested for " + message); }
private void saveAttachment(Message message) {
File file;
final var fileName = message.getAttachment().getName();
final var downloadLocation = settings.getDownloadLocation();
// Show save file dialog, if the user did not opt-out
if (!settings.isDownloadSavedWithoutAsking()) {
final var fileChooser = new FileChooser();
fileChooser.setInitialFileName(fileName);
fileChooser.setInitialDirectory(downloadLocation);
file = fileChooser.showSaveDialog(sceneContext.getStage());
} else file = new File(downloadLocation, fileName);
// A file was selected
if (file != null) try (FileOutputStream fos = new FileOutputStream(file)) {
fos.write(message.getAttachment().getData());
logger.log(Level.FINE, "Attachment of message was saved at " + file.getAbsolutePath());
} catch (final IOException e) {
logger.log(Level.WARNING, "Could not save attachment of " + message + ": ", e);
}
}
/**
* @return whether the message stored by this {@code MessageControl} has been
* sent by this user of Envoy

View File

@ -82,6 +82,7 @@ public class TextInputContextMenu extends ContextMenu {
copyMI.disableProperty().bind(control.selectedTextProperty().isEmpty());
deleteMI.disableProperty().bind(control.selectedTextProperty().isEmpty());
clearMI.disableProperty().bind(control.textProperty().isEmpty());
selectAllMI.disableProperty().bind(control.textProperty().isEmpty());
setOnShowing(e -> pasteMI.setDisable(!Clipboard.getSystemClipboard().hasString()));
selectAllMI.getProperties().put("refreshMenu", Boolean.TRUE);

View File

@ -223,8 +223,8 @@ public final class ChatScene implements EventListener, Restorable {
// The sender of the message is the recipient of the chat
// Exceptions: this user is the sender (sync) or group message (group is
// recipient)
final boolean ownMessage = message.getSenderID() == localDB.getUser().getID();
final var recipientID = message instanceof GroupMessage || ownMessage ? message.getRecipientID() : message.getSenderID();
final var ownMessage = message.getSenderID() == localDB.getUser().getID();
delvh marked this conversation as resolved
Review

I thought we don't declare primitives as var?

I thought we don't declare primitives as `var`?
Review

We don't?
Well, we have to decide quick since I reactived use var instad of field in my formatter, and this was the result of it.
Should I deactivate it?

We don't? Well, we have to decide quick since I reactived `use var instad of field` in my formatter, and this was the result of it. Should I deactivate it?
Review

You can deactivate it, however I will publish a new version of the formatter soon.

You can deactivate it, however I will publish a new version of the formatter soon.
Review

So, I don't have to change it?

So, I don't have to change it?
Review

It's up to you. With the new formatter, it will get changed anyway sooner or later.

It's up to you. With the new formatter, it will get changed anyway sooner or later.
final var recipientID = message instanceof GroupMessage || ownMessage ? message.getRecipientID() : message.getSenderID();
localDB.getChat(recipientID).ifPresent(chat -> {
chat.insert(message);
@ -336,6 +336,24 @@ public final class ChatScene implements EventListener, Restorable {
.setNumberOfArguments(0)
.setDescription("Opens the settings screen")
.build("settings");
// Copy text of selection initialization
builder.setAction(text -> {
final var selectedMessage = messageList.getSelectionModel().getSelectedItem();
if (selectedMessage != null) MessageUtil.copyMessageText(selectedMessage);
}).setNumberOfArguments(0).setDescription("Copies the text of the currently selected message").build("cp-s");
// Delete selection initialization
builder.setAction(text -> {
final var selectedMessage = messageList.getSelectionModel().getSelectedItem();
if (selectedMessage != null) MessageUtil.deleteMessage(selectedMessage);
}).setNumberOfArguments(0).setDescription("Deletes the currently selected message").build("del-s");
// Save attachment of selection initialization
builder.setAction(text -> {
final var selectedMessage = messageList.getSelectionModel().getSelectedItem();
if (selectedMessage != null && selectedMessage.hasAttachment()) MessageUtil.saveAttachment(selectedMessage);
}).setNumberOfArguments(0).setDescription("Copies the text of the currently selected message").build("save-a-s");
}
@Override
@ -387,7 +405,7 @@ public final class ChatScene implements EventListener, Restorable {
if (currentChat != null) {
topBarContactLabel.setText(currentChat.getRecipient().getName());
if (currentChat.getRecipient() instanceof User) {
final String status = ((User) currentChat.getRecipient()).getStatus().toString();
final var status = ((User) currentChat.getRecipient()).getStatus().toString();
topBarStatusLabel.setText(status);
topBarStatusLabel.getStyleClass().add(status.toLowerCase());
recipientProfilePic.setImage(IconUtil.loadIconThemeSensitive("user_icon", 43));
@ -395,7 +413,7 @@ public final class ChatScene implements EventListener, Restorable {
topBarStatusLabel.setText(currentChat.getRecipient().getContacts().size() + " members");
recipientProfilePic.setImage(IconUtil.loadIconThemeSensitive("group_icon", 43));
}
final Rectangle clip = new Rectangle();
final var clip = new Rectangle();
clip.setWidth(43);
clip.setHeight(43);
clip.setArcHeight(43);
@ -753,6 +771,13 @@ public final class ChatScene implements EventListener, Restorable {
pendingAttachment = messageAttachment;
}
/**
* Clears the current message selection.
delvh marked this conversation as resolved Outdated
Outdated
Review

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
*
* @since Envoy Client v0.3-beta
*/
public void clearMessageSelection() { messageList.getSelectionModel().clearSelection(); }
@FXML
private void searchContacts() {
chats.setPredicate(contactSearch.getText().isBlank() ? c -> true

View File

@ -0,0 +1,105 @@
package envoy.client.util;
import java.awt.Toolkit;
import java.awt.datatransfer.StringSelection;
import java.io.*;
import java.util.logging.*;
import javafx.stage.FileChooser;
import envoy.client.data.*;
import envoy.client.event.MessageDeletion;
import envoy.client.ui.controller.ChatScene;
import envoy.data.Message;
import envoy.util.EnvoyLog;
import dev.kske.eventbus.EventBus;
/**
* Contains methods that are commonly used for {@link Message}s.
*
* @author Leon Hofmeister
* @since Envoy Client v0.3-beta
*/
public class MessageUtil {
private MessageUtil() {}
private static Logger logger = EnvoyLog.getLogger(MessageUtil.class);
/**
* Copies the text of the given message to the System Clipboard.
*
* @param message the message whose text to copy
* @since Envoy Client v0.3-beta
*/
public static void copyMessageText(Message message) {
logger.log(Level.FINEST, "A copy of message text \"" + message.getText() + "\" was requested");
Toolkit.getDefaultToolkit().getSystemClipboard().setContents(new StringSelection(message.getText()), null);
}
/**
* Deletes the given message.
*
* @param message the message to delete
* @since Envoy Client v0.3-beta
*/
public static void deleteMessage(Message message) {
final var messageDeletionEvent = new MessageDeletion(message.getID());
final var controller = Context.getInstance().getSceneContext().getController();
if (controller instanceof ChatScene) ((ChatScene) controller).clearMessageSelection();
delvh marked this conversation as resolved Outdated
Outdated
Review

You can use instanceof here, which is faster and more readable. Also, consider to comment this line.

You can use `instanceof` here, which is faster and more readable. Also, consider to comment this line.
// Removing the message locally
EventBus.getInstance().dispatch(messageDeletionEvent);
logger.log(Level.FINEST, "message deletion was requested for " + message);
}
/**
* Forwards the given message.
* Currently not implemented.
*
* @param message the message to forward
* @since Envoy Client v0.3-beta
*/
public static void forwardMessage(Message message) { logger.log(Level.FINEST, "Message forwarding was requested for " + message); }
delvh marked this conversation as resolved Outdated
Outdated
Review

Why are we adding context menu actions and methods that are not implemented on a branch that has nothing to do with it?

By the way, most logger statements start with a capital letter, so if you decide to keep the method, please consider that.

Why are we adding context menu actions and methods that are not implemented on a branch that has nothing to do with it? By the way, most logger statements start with a capital letter, so if you decide to keep the method, please consider that.
/**
* Quotes the given message.
* Currently not implemented.
*
* @param message the message to quote
* @since Envoy Client v0.3-beta
*/
public static void quoteMessage(Message message) { logger.log(Level.FINEST, "Message quotation was requested for " + message); }
/**
* Saves the attachment of a message, if present.
*
* @param message the message whose attachment to save
* @throws IllegalStateException if no attachment is present in the message
* @since Envoy Client v0.3-beta
*/
public static void saveAttachment(Message message) {
if (!message.hasAttachment()) throw new IllegalArgumentException("Cannot save a non-existing attachment");
delvh marked this conversation as resolved Outdated
Outdated
Review

This would rather be an IllegalArgumentException.

This would rather be an `IllegalArgumentException`.
File file;
final var fileName = message.getAttachment().getName();
final var downloadLocation = Settings.getInstance().getDownloadLocation();
// Show save file dialog, if the user did not opt-out
if (!Settings.getInstance().isDownloadSavedWithoutAsking()) {
final var fileChooser = new FileChooser();
fileChooser.setInitialFileName(fileName);
fileChooser.setInitialDirectory(downloadLocation);
file = fileChooser.showSaveDialog(Context.getInstance().getSceneContext().getStage());
} else file = new File(downloadLocation, fileName);
// A file was selected
if (file != null) try (var fos = new FileOutputStream(file)) {
fos.write(message.getAttachment().getData());
logger.log(Level.FINE, "Attachment of message was saved at " + file.getAbsolutePath());
} catch (final IOException e) {
logger.log(Level.WARNING, "Could not save attachment of " + message + ": ", e);
}
}
}

View File

@ -9,6 +9,8 @@ import envoy.data.User.UserStatus;
import envoy.server.net.ConnectionManager;
/**
* Contains operations used for persistence.
delvh marked this conversation as resolved Outdated
Outdated
Review

As its not only retrieval, but also storage, maybe change this to "... for persistence."

As its not only retrieval, but also storage, maybe change this to "... for persistence."
*
* @author Leon Hofmeister
* @author Maximilian K&auml;fer
* @since Envoy Server Standalone v0.1-alpha