Contact Deletion #97
2 Participants
Due Date
2020-10-18
Blocks
#99 Icon for Disabled Chat
zdm/envoy
Reference: zdm/envoy#97
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "f/contact-deletion"
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 the option to disable chatting with a user / group before entirely deleting it.
Fixes #10.
Fixes #82.
Contact Deletion Both for Client2Client- as well as for Client2Group-chatsto Contact DeletionThe server looks very clean, we have discussed some architectural improvements for the client.
Overall, very good work 👍
@ -27,6 +27,8 @@ public class Chat implements Serializable {
protected int unreadAmount;
private boolean disabled;
Why is this
private
even though we permit subclassing here?Because we don't need to access this in subclasses. Subclasses themselves should never disable themselves.
There is a public setter though. The chat can literally be disabled from anywhere.
According to Meyer's open-closed principle (the O in SOLID), software entities should open for extension, while closed for modification. To me, this implies that subclasses should be able to access this value without the use of a getter.
The problem I have with this is that in this case it is literally intended that they are modified from outside and that they do not modify themselves.
@ -57,2 +59,3 @@
@Override
public String toString() { return String.format("%s[recipient=%s,messages=%d]", getClass().getSimpleName(), recipient, messages.size()); }
public String toString() {
return String.format("%s[recipient=%s,messages=%d, disabled=%b]", getClass().getSimpleName(), recipient, messages.size(), disabled);
There is an unnecessary space in the template.
@ -170,1 +174,4 @@
public void lastWritingEventWasNow() { lastWritingEvent = System.currentTimeMillis(); }
/**
* Determines whether messages can be sent in this chat. Should be true i.e. for
Use
{@code true}
here.@ -171,0 +177,4 @@
* Determines whether messages can be sent in this chat. Should be true i.e. for
* chats whose recipient deleted this client as a contact.
*
* @return whether this {@code Chat} has been disabled
Use 'chat' here instead.
@ -171,0 +186,4 @@
* Determines whether messages can be sent in this chat. Should be true i.e. for
* chats whose recipient deleted this client as a contact.
*
* @param disabled the disabled to set
Doesn't sound very conclusive to me.
@ -40,2 +44,4 @@
private CacheMap cacheMap = new CacheMap();
private String authToken;
private boolean contactsChanged;
private Stream<Chat> changedChats;
Integrate this into
loadUserData
as discussed.@ -139,1 +145,3 @@
chats = FXCollections.observableList((List<Chat>) in.readObject());
chats = FXCollections.observableList((List<Chat>) in.readObject());
// Some chats have changed and should not be overridden by the saved values
I think you mean 'overwritten'.
See here.
Past tense of override.
I know what the past tense of override is.
What you want to prevent here is the destruction of old data by the recording new data over it, which is also known as overwriting.
Look here for a comparison of both terms.
Still, both are applicable here. My preference is
overridden
, yours isoverwritten
.Let's give @DieGurke the vote on what stays in. (How great that we have an odd amount of members, I don't want to imagine disagreements with an even amount of members...)
@ -140,0 +148,4 @@
if (changedChats != null) {
changedChats.forEach(chat -> {
final var chatIndex = chats.indexOf(chat);
if (chatIndex == -1) return;
Can this even happen? If it can, should it be logged?
While it could theoretically occur, even if the user deleted a group while being offline, it should not happen.
Removing it now.
@ -198,3 +216,3 @@
@Event(eventType = EnvoyCloseEvent.class, priority = 1000)
private synchronized void save() {
EnvoyLog.getLogger(LocalDB.class).log(Level.INFO, "Saving local database...");
EnvoyLog.getLogger(LocalDB.class).log(Level.FINER, "Saving local database...");
Appreciate that 👍
@ -243,0 +263,4 @@
final var eventUser = operation.get();
switch (operation.getOperationType()) {
case ADD:
getUsers().put(eventUser.getName(), eventUser);
As
users
only contains users that have locally logged in, this is unnecessary.@ -297,2 +340,4 @@
private void onOwnStatusChange(OwnStatusChange statusChange) { user.setStatus(statusChange.get()); }
@Event(eventType = ContactsChangedSinceLastLogin.class, priority = 500)
private void onContactsChangedSinceLastLogin() { if (user != null) contactsChanged = true; }
Please document why this is necessary, or at least give some conclusive message to the user.
@ -326,0 +375,4 @@
* case they might have been deleted.
*
* @param user the user to set
* @since Envoy Client v0.2-alpha
Please change this to '0.3-beta'.
@ -326,0 +385,4 @@
// Mark chats as disabled if a contact is no longer in this users contact list
final var changedUserChats = chats.stream()
.filter(not(chat -> contacts.contains(chat.getRecipient())))
.peek(chat -> { chat.setDisabled(true); logger.log(Level.INFO, String.format("Marked %s as blocked.", chat.getRecipient())); });
Maybe log 'Disabled chat ...' here instead.
@ -29,6 +30,7 @@ public final class Client implements EventListener, Closeable {
private Socket socket;
private Receiver receiver;
private boolean online;
private boolean isHandShake;
This should be called
handshake
, also it should bevolatile
and thus moved down into the line withrejected
.@ -97,2 +101,3 @@
if (System.currentTimeMillis() - start > 5000) throw new TimeoutException("Did not log in after 5 seconds");
if (System.currentTimeMillis() - start > 5000) {
isHandShake = false;
Maybe set
rejected
here as well?@ -178,0 +188,4 @@
}
@Event(eventType = ContactsChangedSinceLastLogin.class, priority = 1000)
private void onContactsChangedSinceLastLogin() {
As discussed, remove this and the boolean.
@ -17,3 +18,3 @@
* @since Envoy Client v0.1-beta
*/
public final class ChatControl extends HBox {
public final class ChatControl extends Label {
Please move all of what you have done here to a new
AbstractListCell
, enabling you to properly display the context menu and the background color.@ -18,2 +18,3 @@
*/
public GroupSizeLabel(Group recipient) { super(recipient.getContacts().size() + " members"); }
public GroupSizeLabel(Group recipient) {
super(recipient.getContacts().size() + " member" + (recipient.getContacts().size() != 1 ? "s" : ""));
We could just use parentheses, but I guess this is less ambiguous.
@ -284,2 +270,2 @@
break;
}
private void onUserOperation(UserOperation operation) {
// All ADD dependant logic resides in LocalDB
You have a typo here, it's called 'dependent'.
@ -286,0 +274,4 @@
@Event
private void onGroupResize(GroupResize resize) {
final var chatFound = localDB.getChat(resize.getGroupID());
Why not use
Optional#ifPresent
here?@ -301,2 +302,2 @@
@Event
private void onGroupCreationResult(GroupCreationResult result) { Platform.runLater(() -> newGroupButton.setDisable(!result.get())); }
@Event(priority = 150)
private void onGroupCreationResult(GroupCreationResult result) { Platform.runLater(() -> newGroupButton.setDisable(result.get() == null)); }
See #98.
@ -332,3 +332,3 @@
@FXML
private void chatListClicked() {
if (chatList.getSelectionModel().isEmpty()) return;
if (currentChat != null && chatList.getSelectionModel().isEmpty()) return;
currentChat != null
is unnecessary here.Let's hope I' ll stay unable to reproduce what I once have produced...
@ -375,2 +381,4 @@
topBarStatusLabel.setText(status);
topBarStatusLabel.getStyleClass().clear();
topBarStatusLabel.getStyleClass().add(status.toLowerCase());
topBarStatusLabel.setVisible(true);
This is unnecessary (your words).
@ -390,1 +399,4 @@
messageSearchButton.setVisible(true);
// Change the background of the message list if the chat is disabled
if (currentChat.isDisabled()) messageList.getStyleClass().add("disabled-chat");
Please remove this, the user has already enough visual cues.
@ -738,0 +794,4 @@
recipientProfilePic.setImage(null);
// It is a valid case that cancel can cause a NullPointerException
try {
You can use
Recorder#isRecording
here.@ -62,0 +94,4 @@
logger.log(Level.INFO, "The user left a group.");
}
final var controller = context.getSceneContext().getController();
if (controller instanceof ChatScene) ((ChatScene) controller).disableChat(block, true);
Use an event here instead. This would also simplify the interaction with the local database.
@ -106,3 +106,3 @@
}
try (ObjectOutputStream out = new ObjectOutputStream(new FileOutputStream(file))) {
for (var obj : objs)
for (final var obj : objs)
Please reset this class to
develop
.