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
Owner

Additionally added system commands to replace the functionality of the message control context menu.

Additionally added system commands to replace the functionality of the message control context menu.
delvh added this to the v0.3-beta milestone 2020-09-29 18:53:00 +02:00
delvh added the
M
client
labels 2020-09-29 18:53:00 +02:00
delvh self-assigned this 2020-09-29 18:53:00 +02:00
delvh requested review from mpk 2020-09-29 18:57:31 +02:00
delvh requested review from kske 2020-09-29 18:57:32 +02:00
kske approved these changes 2020-09-30 08:12:25 +02:00
kske left a comment
Owner

The code looks good to me apart from some Javadoc stuff.

The code looks good to me apart from some Javadoc stuff.
@ -122,2 +122,4 @@
}
/**
* Removes the message with the given ID

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
delvh marked this conversation as resolved
@ -124,0 +125,4 @@
* Removes the message with the given ID
*
* @param messageID the ID of the message to remove
* @return whether any message has been removed

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.
Author
Owner

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.

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.
delvh marked this conversation as resolved
@ -274,2 +275,4 @@
}
/**
* Deletes the message with the given ID, if any is present.

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.
delvh marked this conversation as resolved
@ -276,0 +280,4 @@
* @param message the event that was
* @since Envoy Client v0.3-beta
*/
@Event()

The parentheses after @Event are unnecessary here.

The parentheses after `@Event` are unnecessary here.
delvh marked this conversation as resolved
@ -0,0 +3,4 @@
import envoy.event.Event;
/**
* Conveys the deletion of a message between clients and server.

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

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

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

That's an artifact from when it was an Event in Common.
delvh marked this conversation as resolved
@ -225,3 +225,2 @@
// 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();

I thought we don't declare primitives as var?

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

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?

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.
Author
Owner

So, I don't have to change it?

So, I don't have to change it?

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.
delvh marked this conversation as resolved
@ -754,2 +772,4 @@
}
/**
* Clears the current message selection

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
delvh marked this conversation as resolved
@ -0,0 +47,4 @@
public static void deleteMessage(Message message) {
final var messageDeletionEvent = new MessageDeletion(message.getID());
final var controller = Context.getInstance().getSceneContext().getController();
if (controller.getClass().equals(ChatScene.class)) ((ChatScene) controller).clearMessageSelection();

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.
delvh marked this conversation as resolved
@ -0,0 +62,4 @@
* @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); }

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.
delvh marked this conversation as resolved
@ -0,0 +81,4 @@
* @since Envoy Client v0.3-beta
*/
public static void saveAttachment(Message message) {
if (!message.hasAttachment()) throw new IllegalStateException("Cannot save a non-existing attachment");

This would rather be an IllegalArgumentException.

This would rather be an `IllegalArgumentException`.
delvh marked this conversation as resolved
@ -9,6 +9,8 @@ import envoy.data.User.UserStatus;
import envoy.server.net.ConnectionManager;
/**
* Contains operations used for data retrieval.

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."
delvh marked this conversation as resolved
kske added the due date 2020-10-01 2020-09-30 12:58:45 +02:00
mpk approved these changes 2020-09-30 20:48:52 +02:00
mpk left a comment
Owner

Seems fine to me 👍

Seems fine to me 👍
delvh merged commit 80795a3fc2 into develop 2020-09-30 20:50:59 +02:00
kske deleted branch f/delete-messages 2020-09-30 20:51:10 +02:00
This repo is archived. You cannot comment on pull requests.
No description provided.