zdm/envoy
zdm/envoy
Archived
3
Fork 0
Code Issues 22 Pull Requests Releases 1 Wiki Activity

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
Conversation 0 Commits 6 Files Changed 8 +13 -13
6 changed files with 13 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 640ecbf41b - Show all commits

8
client/src/main/java/envoy/client/data/Chat.java
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);
@ -122,10 +122,10 @@ public class Chat implements Serializable {
}
/**
* Removes the message with the given ID
* Removes the message with the given ID.
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:48:40 +02:00
Outdated
Review
Copy Link

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 any message has been removed
* @return whether the message has been found and removed
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:49:35 +02:00
Outdated
Review
Copy Link

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.
delvh commented 2020-09-30 10:42:33 +02:00
Outdated
Review
Copy Link

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.
kske commented 2020-09-30 10:55:27 +02:00
Outdated
Review
Copy Link

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); }

4
client/src/main/java/envoy/client/data/LocalDB.java
View File

@ -275,12 +275,12 @@ public final class LocalDB implements EventListener {
}
/**
* Deletes the message with the given ID, if any is present.
* Deletes the message with the given ID, if present.
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:50:23 +02:00
Outdated
Review
Copy Link

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()
@Event
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:51:23 +02:00
Outdated
Review
Copy Link

The parentheses after @Event are unnecessary here.

The parentheses after `@Event` are unnecessary here.
private void onMessageDeletion(MessageDeletion message) {
Platform.runLater(() -> {

2
client/src/main/java/envoy/client/event/MessageDeletion.java
View File

@ -3,7 +3,7 @@ package envoy.client.event;
import envoy.event.Event;
/**
* Conveys the deletion of a message between clients and server.
* Conveys the deletion of a message.
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:51:59 +02:00
Outdated
Review
Copy Link

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

This does not make sense as this is a client-sided event.
delvh commented 2020-09-30 10:43:48 +02:00
Outdated
Review
Copy Link

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

2
client/src/main/java/envoy/client/ui/controller/ChatScene.java
View File

@ -772,7 +772,7 @@ public final class ChatScene implements EventListener, Restorable {
}
/**
* Clears the current message selection
* Clears the current message selection.
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:57:40 +02:00
Outdated
Review
Copy Link

Append a dot to the end of the sentence.

Append a dot to the end of the sentence.
*
* @since Envoy Client v0.3-beta
*/

8
client/src/main/java/envoy/client/util/MessageUtil.java
View File

@ -47,7 +47,7 @@ public class MessageUtil {
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();
if (controller instanceof ChatScene) ((ChatScene) controller).clearMessageSelection();
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 07:59:26 +02:00
Outdated
Review
Copy Link

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);
@ -62,7 +62,7 @@ public class MessageUtil {
* @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); }
public static void forwardMessage(Message message) { logger.log(Level.FINEST, "Message forwarding was requested for " + message); }
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 08:02:49 +02:00
Outdated
Review
Copy Link

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.
@ -71,7 +71,7 @@ public class MessageUtil {
* @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); }
public static void quoteMessage(Message message) { logger.log(Level.FINEST, "Message quotation was requested for " + message); }
/**
* Saves the attachment of a message, if present.
@ -81,7 +81,7 @@ public class MessageUtil {
* @since Envoy Client v0.3-beta
*/
public static void saveAttachment(Message message) {
if (!message.hasAttachment()) throw new IllegalStateException("Cannot save a non-existing attachment");
if (!message.hasAttachment()) throw new IllegalArgumentException("Cannot save a non-existing attachment");
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 08:00:12 +02:00
Outdated
Review
Copy Link

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();

2
server/src/main/java/envoy/server/data/PersistenceManager.java
View File

@ -9,7 +9,7 @@ import envoy.data.User.UserStatus;
import envoy.server.net.ConnectionManager;
/**
* Contains operations used for data retrieval.
* Contains operations used for persistence.
delvh marked this conversation as resolved Outdated
kske commented 2020-09-30 08:01:22 +02:00
Outdated
Review
Copy Link

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äfer