Add Ability to Delete Messages Locally #70
Labels
No Label
client
server
user made
L
M
S
XL
bug
bugfix
discussion
documentation
feature
maintenance
postponed
refactoring
wontfix
No Milestone
No Assignees
3 Participants
Due Date
Dependencies
No dependencies set.
Reference: zdm/envoy#70
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "f/delete-messages"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Additionally added system commands to replace the functionality of the message control context menu.
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.
@ -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.
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.@ -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.
@ -276,0 +280,4 @@
* @param message the event that was
* @since Envoy Client v0.3-beta
*/
@Event()
The parentheses after
@Event
are unnecessary here.@ -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.
That's an artifact from when it was an Event in Common.
@ -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
?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.
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.
@ -754,2 +772,4 @@
}
/**
* Clears the current message selection
Append a dot to the end of the sentence.
@ -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.@ -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.
@ -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
.@ -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."
Seems fine to me 👍