Contact Deletion #97

Merged
delvh merged 6 commits from f/contact-deletion into develop 2020-10-19 18:09:21 +02:00

Additionally added the option to disable chatting with a user / group before entirely deleting it.
Fixes #10.
Fixes #82.

Additionally added the option to disable chatting with a user / group before entirely deleting it. Fixes #10. Fixes #82.
delvh added this to the v0.3-beta milestone 2020-10-16 20:24:05 +02:00
delvh added the
L
server
client
labels 2020-10-16 20:24:05 +02:00
delvh self-assigned this 2020-10-16 20:24:05 +02:00
kske changed title from Contact Deletion Both for Client2Client- as well as for Client2Group-chats to Contact Deletion 2020-10-16 20:26:32 +02:00
kske requested review from mpk 2020-10-17 10:11:42 +02:00
delvh requested review from kske 2020-10-17 10:11:56 +02:00
kske added the due date 2020-10-18 2020-10-17 10:12:01 +02:00
kske requested changes 2020-10-17 11:42:05 +02:00
kske left a comment
Owner

The server looks very clean, we have discussed some architectural improvements for the client.

Overall, very good work 👍

The 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?

Why is this `private` even though we permit subclassing here?
Poster
Owner

Because we don't need to access this in subclasses. Subclasses themselves should never disable themselves.

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.

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

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.

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**.
kske marked this conversation as resolved
@ -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.

There is an unnecessary space in the template.
delvh marked this conversation as resolved
@ -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.

Use `{@code true}` here.
delvh marked this conversation as resolved
@ -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.

Use 'chat' here instead.
delvh marked this conversation as resolved
@ -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.

Doesn't sound very conclusive to me.
delvh marked this conversation as resolved
@ -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.

Integrate this into `loadUserData` as discussed.
delvh marked this conversation as resolved
@ -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'.

I think you mean 'overwritten'.
Poster
Owner

See here.

See [here](https://www.dict.cc/?s=%C3%BCberschrieben).
Poster
Owner

Past tense of override.

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.

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](https://wikidiff.com/overwrite/override) for a comparison of both terms.
Poster
Owner

Still, both are applicable here. My preference is overridden, yours is overwritten.
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...)

Still, both are applicable here. My preference is `overridden`, yours is `overwritten`. 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...)
delvh marked this conversation as resolved
@ -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?

Can this even happen? If it can, should it be logged?
Poster
Owner

While it could theoretically occur, even if the user deleted a group while being offline, it should not happen.
Removing it now.

While it could theoretically occur, even if the user deleted a group while being offline, it should not happen. Removing it now.
kske marked this conversation as resolved
@ -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 👍

Appreciate that 👍
delvh marked this conversation as resolved
@ -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.

As `users` only contains users that have locally logged in, this is unnecessary.
delvh marked this conversation as resolved
@ -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.

Please document why this is necessary, or at least give some conclusive message to the user.
delvh marked this conversation as resolved
@ -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'.

Please change this to '0.3-beta'.
delvh marked this conversation as resolved
@ -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.

Maybe log 'Disabled chat ...' here instead.
delvh marked this conversation as resolved
@ -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 be volatile and thus moved down into the line with rejected.

This should be called `handshake`, also it should be `volatile` and thus moved down into the line with `rejected`.
delvh marked this conversation as resolved
@ -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?

Maybe set `rejected` here as well?
delvh marked this conversation as resolved
@ -178,0 +188,4 @@
}
@Event(eventType = ContactsChangedSinceLastLogin.class, priority = 1000)
private void onContactsChangedSinceLastLogin() {

As discussed, remove this and the boolean.

As discussed, remove this and the boolean.
delvh marked this conversation as resolved
@ -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.

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.

We could just use parentheses, but I guess this is less ambiguous.
delvh marked this conversation as resolved
@ -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'.

You have a typo here, it's called 'dependent'.
delvh marked this conversation as resolved
@ -286,0 +274,4 @@
@Event
private void onGroupResize(GroupResize resize) {
final var chatFound = localDB.getChat(resize.getGroupID());

Why not use Optional#ifPresent here?

Why not use `Optional#ifPresent` here?
delvh marked this conversation as resolved
@ -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.

See #98.
delvh marked this conversation as resolved
@ -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.

`currentChat != null` is unnecessary here.
Poster
Owner

Let's hope I' ll stay unable to reproduce what I once have produced...

Let's hope I' ll stay unable to reproduce what I once have produced...
delvh marked this conversation as resolved
@ -375,2 +381,4 @@
topBarStatusLabel.setText(status);
topBarStatusLabel.getStyleClass().clear();
topBarStatusLabel.getStyleClass().add(status.toLowerCase());
topBarStatusLabel.setVisible(true);

This is unnecessary (your words).

This is unnecessary (your words).
delvh marked this conversation as resolved
@ -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.

Please remove this, the user has already enough visual cues.
delvh marked this conversation as resolved
@ -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.

You can use `Recorder#isRecording` here.
delvh marked this conversation as resolved
@ -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.

Use an event here instead. This would also simplify the interaction with the local database.
delvh marked this conversation as resolved
@ -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.

Please reset this class to `develop`.
delvh marked this conversation as resolved
kske added a new dependency 2020-10-17 16:04:10 +02:00
kske approved these changes 2020-10-18 14:37:04 +02:00
delvh merged commit 77a75fc37c into develop 2020-10-19 18:09:20 +02:00
This repo is archived. You cannot comment on pull requests.
There is no content yet.