Add Ability to Delete Messages Locally #70

Merged
delvh merged 6 commits from f/delete-messages into develop 2 years ago
delvh commented 2 years ago
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 2 years ago
delvh added the
M
client
labels 2 years ago
delvh self-assigned this 2 years ago
delvh requested review from DieGurke 2 years ago
delvh requested review from kske 2 years ago
kske approved these changes 2 years ago
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.
}
/**
* Removes the message with the given ID
Owner

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
delvh marked this conversation as resolved
* Removes the message with the given ID
*
* @param messageID the ID of the message to remove
* @return whether any message has been removed
Owner

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

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
}
/**
* Deletes the message with the given ID, if any is present.
Owner

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
* @param message the event that was
* @since Envoy Client v0.3-beta
*/
@Event()
Owner

The parentheses after @Event are unnecessary here.

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

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

This does not make sense as this is a client-sided event.
Poster
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
// 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();
Owner

I thought we don't declare primitives as var?

I thought we don't declare primitives as `var`?
Poster
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?
Owner

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

So, I don't have to change it?

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

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
}
/**
* Clears the current message selection
Owner

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
delvh marked this conversation as resolved
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();
Owner

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
* @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); }
Owner

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
* @since Envoy Client v0.3-beta
*/
public static void saveAttachment(Message message) {
if (!message.hasAttachment()) throw new IllegalStateException("Cannot save a non-existing attachment");
Owner

This would rather be an IllegalArgumentException.

This would rather be an `IllegalArgumentException`.
delvh marked this conversation as resolved
import envoy.server.net.ConnectionManager;
/**
* Contains operations used for data retrieval.
Owner

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 2 years ago
DieGurke approved these changes 2 years ago
DieGurke left a comment
Owner

Seems fine to me 👍

Seems fine to me 👍
delvh merged commit 80795a3fc2 into develop 2 years ago
kske deleted branch f/delete-messages 2 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.