Contact Deletion #97

Merged
delvh merged 6 commits from f/contact-deletion into develop 2 years ago
delvh commented 2 years ago
Owner

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 2 years ago
delvh added the
L
server
client
labels 2 years ago
delvh self-assigned this 2 years ago
kske changed title from Contact Deletion Both for Client2Client- as well as for Client2Group-chats to Contact Deletion 2 years ago
kske requested review from DieGurke 2 years ago
delvh requested review from kske 2 years ago
kske added the due date 2020-10-18 2 years ago
kske requested changes 2 years ago
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 👍
protected int unreadAmount;
private boolean disabled;
Owner

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

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

There is an unnecessary space in the template.

There is an unnecessary space in the template.
delvh marked this conversation as resolved
public void lastWritingEventWasNow() { lastWritingEvent = System.currentTimeMillis(); }
/**
* Determines whether messages can be sent in this chat. Should be true i.e. for
Owner

Use {@code true} here.

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

Use 'chat' here instead.

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

Doesn't sound very conclusive to me.

Doesn't sound very conclusive to me.
delvh marked this conversation as resolved
private CacheMap cacheMap = new CacheMap();
private String authToken;
private boolean contactsChanged;
private Stream<Chat> changedChats;
Owner

Integrate this into loadUserData as discussed.

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

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

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
if (changedChats != null) {
changedChats.forEach(chat -> {
final var chatIndex = chats.indexOf(chat);
if (chatIndex == -1) return;
Owner

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
@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...");
Owner

Appreciate that 👍

Appreciate that 👍
delvh marked this conversation as resolved
final var eventUser = operation.get();
switch (operation.getOperationType()) {
case ADD:
getUsers().put(eventUser.getName(), eventUser);
Owner

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
private void onOwnStatusChange(OwnStatusChange statusChange) { user.setStatus(statusChange.get()); }
@Event(eventType = ContactsChangedSinceLastLogin.class, priority = 500)
private void onContactsChangedSinceLastLogin() { if (user != null) contactsChanged = true; }
Owner

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
* case they might have been deleted.
*
* @param user the user to set
* @since Envoy Client v0.2-alpha
Owner

Please change this to '0.3-beta'.

Please change this to '0.3-beta'.
delvh marked this conversation as resolved
// 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())); });
Owner

Maybe log 'Disabled chat ...' here instead.

Maybe log 'Disabled chat ...' here instead.
delvh marked this conversation as resolved
private Socket socket;
private Receiver receiver;
private boolean online;
private boolean isHandShake;
Owner

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
if (System.currentTimeMillis() - start > 5000) throw new TimeoutException("Did not log in after 5 seconds");
if (System.currentTimeMillis() - start > 5000) {
isHandShake = false;
Owner

Maybe set rejected here as well?

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

As discussed, remove this and the boolean.

As discussed, remove this and the boolean.
delvh marked this conversation as resolved
* @since Envoy Client v0.1-beta
*/
public final class ChatControl extends HBox {
public final class ChatControl extends Label {
Owner

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.
*/
public GroupSizeLabel(Group recipient) { super(recipient.getContacts().size() + " members"); }
public GroupSizeLabel(Group recipient) {
super(recipient.getContacts().size() + " member" + (recipient.getContacts().size() != 1 ? "s" : ""));
Owner

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
break;
}
private void onUserOperation(UserOperation operation) {
// All ADD dependant logic resides in LocalDB
Owner

You have a typo here, it's called 'dependent'.

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

Why not use Optional#ifPresent here?

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

See #98.

See #98.
delvh marked this conversation as resolved
@FXML
private void chatListClicked() {
if (chatList.getSelectionModel().isEmpty()) return;
if (currentChat != null && chatList.getSelectionModel().isEmpty()) return;
Owner

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
topBarStatusLabel.setText(status);
topBarStatusLabel.getStyleClass().clear();
topBarStatusLabel.getStyleClass().add(status.toLowerCase());
topBarStatusLabel.setVisible(true);
Owner

This is unnecessary (your words).

This is unnecessary (your words).
delvh marked this conversation as resolved
messageSearchButton.setVisible(true);
// Change the background of the message list if the chat is disabled
if (currentChat.isDisabled()) messageList.getStyleClass().add("disabled-chat");
Owner

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
recipientProfilePic.setImage(null);
// It is a valid case that cancel can cause a NullPointerException
try {
Owner

You can use Recorder#isRecording here.

You can use `Recorder#isRecording` here.
delvh marked this conversation as resolved
logger.log(Level.INFO, "The user left a group.");
}
final var controller = context.getSceneContext().getController();
if (controller instanceof ChatScene) ((ChatScene) controller).disableChat(block, true);
Owner

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
}
try (ObjectOutputStream out = new ObjectOutputStream(new FileOutputStream(file))) {
for (var obj : objs)
for (final var obj : objs)
Owner

Please reset this class to develop.

Please reset this class to `develop`.
delvh marked this conversation as resolved
kske added a new dependency 2 years ago
kske approved these changes 2 years ago
delvh merged commit 77a75fc37c into develop 2 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.