Add Ability to Change the User Status Freely #90

Merged
delvh merged 7 commits from f/change-user-status into develop 2020-10-10 14:16:29 +02:00

Additionally added display of own user status and added logout option to StatusTrayIcon (on top of the usual keyboard shortcuts and system commands of course).

It would be good, if someone whose StatusTrayIcon is supported, would test whether my TrayIcon related changes are doing what they are supposed to be.

Additionally added display of own user status and added logout option to StatusTrayIcon (on top of the usual keyboard shortcuts and system commands of course). It would be good, if someone whose StatusTrayIcon is supported, would test whether my TrayIcon related changes are doing what they are supposed to be.
delvh added this to the v0.3-beta milestone 2020-10-08 17:12:21 +02:00
delvh added the
client
L
labels 2020-10-08 17:12:21 +02:00
delvh self-assigned this 2020-10-08 17:12:21 +02:00
delvh requested review from mpk 2020-10-08 17:12:31 +02:00
delvh requested review from kske 2020-10-08 17:12:31 +02:00
kske requested changes 2020-10-08 18:27:49 +02:00
kske left a comment
Owner

This is a rather essential feature. Can't believe it has taken us so long to implement.

Apart from a little bug and some code cleanup stuff, it looks quite good.

Also, I think it would be much easier to change my status if the combo box wasn't part of the settings, but rather directly available from the chat scene. However, I am not sure how too make this look good as combo boxes are rather bulky and don't fit nicely into the scene.
Maybe take a look at Skype for inspiration.

This is a rather essential feature. Can't believe it has taken us so long to implement. Apart from a little bug and some code cleanup stuff, it looks quite good. Also, I think it would be much easier to change my status if the combo box wasn't part of the settings, but rather directly available from the chat scene. However, I am not sure how too make this look good as combo boxes are rather bulky and don't fit nicely into the scene. Maybe take a look at Skype for inspiration.
@ -0,0 +5,4 @@
import envoy.event.Event;
/**
* Conveys the action that the currently logged in {@link User} has changed his

This construction is complicated even for my standards.

What about "Signifies a manual status change of the client user".

This construction is complicated even for my standards. What about "Signifies a manual status change of the client user".
delvh marked this conversation as resolved
@ -129,3 +131,1 @@
// Add the option to logout using "Control"+"Shift"+"L" if not in login scene
if (sceneInfo != SceneInfo.LOGIN_SCENE)
accelerators.put(new KeyCodeCombination(KeyCode.L, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN), ShutdownHelper::logout);
if (sceneInfo != SceneInfo.LOGIN_SCENE) {
  1. Are these key combinations documented anywhere?
  2. Can we stop putting scene specific code inside SceneContext, that could be completely generic as to what scenes it displays, except for being littered with code specific to LoginScene which nobody would expect here?

(yes, these are both rhetorical questions)

1. Are these key combinations documented anywhere? 2. Can we stop putting scene specific code inside `SceneContext`, that could be completely generic as to what scenes it displays, except for being littered with code specific to `LoginScene` which nobody would expect here? (yes, these are both rhetorical questions)
Poster
Owner

to 1. While I know what you mean, I'd still say that this goes beyond the scope of this branch.
to 2. While I know what you mean, I'd still say that this goes beyond the scope of this branch.

It seems as if one of the next branches should cleanup that in SceneContext. I could do it in that branch, but than this branch will have two completely separate tasks on it (and it would be eve larger than it already is).

to 1. While I know what you mean, I'd still say that this goes beyond the scope of this branch. to 2. While I know what you mean, I'd still say that this goes beyond the scope of this branch. It seems as if one of the next branches should cleanup that in SceneContext. I could do it in that branch, but than this branch will have two completely separate tasks on it (and it would be eve larger than it already is).

No, I think you are right. The cleanup should be performed inside a separate branch.

No, I think you are right. The cleanup should be performed inside a separate branch.
Poster
Owner

See #91

See #91
kske marked this conversation as resolved
@ -60,0 +62,4 @@
// Adding the logout menu item
final var logoutMenuItem = new MenuItem("Logout");
logoutMenuItem.addActionListener(evt -> { hide(); UserUtil.logout(); });
popup.add(exitMenuItem);

This appears to be an error.

This appears to be an error.
Poster
Owner

Ctrl C, Ctrl V.

Ctrl C, Ctrl V.
delvh marked this conversation as resolved
@ -93,3 +108,1 @@
message.hasAttachment() ? "New " + message.getAttachment().getType().toString().toLowerCase() + " message received" : "New message received",
message.getText(),
MessageType.INFO);
if (displayMessages) trayIcon

I think the point of the "busy" status is that the user does not want to be disturbed, so maybe put a check in place that prevents the notification in this status.

Also, could we keep the previous formatting of this section? The new one looks very confusing to me.

I think the point of the "busy" status is that the user does not want to be disturbed, so maybe put a check in place that prevents the notification in this status. Also, could we keep the previous formatting of this section? The new one looks very confusing to me.
delvh marked this conversation as resolved
@ -66,0 +70,4 @@
try {
UserUtil.changeStatus(Enum.valueOf(UserStatus.class, text.get(0).toUpperCase()));
} catch (final IllegalArgumentException e) {
final var alert = new Alert(AlertType.ERROR);

We implement a standardized way of handling system command failures. Wou don't be able to display an alert if Envoy is running on the command line.

We implement a standardized way of handling system command failures. Wou don't be able to display an alert if Envoy is running on the command line.
Poster
Owner

While I agree with you, there is a reason why I accepted your solution of automatically displaying an alert in case of a system command only with gritted teeth.
For now, this has to suffice. In the future I can well imagine that we can pass a consumer accepting an error to the calling method to handle errors in a command.

But, I'd say this is a branch of its own again and far beyond the scope of this branch.

While I agree with you, there is a reason why I accepted your solution of automatically displaying an alert in case of a system command only with gritted teeth. For now, this has to suffice. In the future I can well imagine that we can pass a consumer accepting an error to the calling method to handle errors in a command. But, I'd say this is a branch of its own again and far beyond the scope of this branch.

I would suggest to introduce a new exception and catch that at a convenient place. No need for consumers.

I however do agree, that this is beyond the scope of this branch.

I would suggest to introduce a new exception and catch that at a convenient place. No need for consumers. I however do agree, that this is beyond the scope of this branch.
kske marked this conversation as resolved
@ -66,0 +74,4 @@
alert.setContentText("Please provide an existing status");
alert.showAndWait();
}
}).setDescription("Changes your status to the given status.").setNumberOfArguments(1).setDefaults("OFFLINE").build("status");

Any reason why the default is "offline"? This should be documented.

Any reason why the default is "offline"? This should be documented.
Poster
Owner

to 1. Well, normally when executing this command I'd argue that you want to change it, (from "ONLINE" in the standard case) and "OFFLINE" seemed like the most reasonable value. If you want another value, I can change that, i.e. BUSY?
to 2. Where should I document it? Our wiki is a laughable joke and the only location where it could be partially useful for the consumer would be inside the description of this system command. Should I add it there?

to 1. Well, normally when executing this command I'd argue that you want to change it, (from "ONLINE" in the standard case) and "OFFLINE" seemed like the most reasonable value. If you want another value, I can change that, i.e. BUSY? to 2. Where should I document it? Our wiki is a laughable joke and the only location where it could be partially useful for the consumer would be inside the description of this system command. Should I add it there?
  1. In my opinion, there is no sensible default. Rather, the parameter should be mandatory.
  2. If you would put the documentation inside the wiki, it would become less of a joke. Maybe it's also possible to display the default values to the user in some way?
1. In my opinion, there is no sensible default. Rather, the parameter should be mandatory. 2. If you would put the documentation inside the wiki, it would become less of a joke. Maybe it's also possible to display the default values to the user in some way?
Poster
Owner
  1. Okay, I can remove the default value. Even though that completely defies the purpose of default values.

  2. I've already thought about doing that (inspired by Skype)
    but that'll require some major changes to the underlying system command system (=>another branch)

1. Okay, I can remove the default value. Even though that completely defies the purpose of default values. 2. I've already thought about doing that (inspired by Skype) but that'll require some major changes to the underlying system command system (=>another branch)

To my understanding, default values are only used if there is a default that actually makes sense. In case of status changes, I don't see, why a specific status would be the most frequently used one.

To my understanding, default values are only used if there is a default that actually makes sense. In case of status changes, I don't see, why a specific status would be the most frequently used one.
Poster
Owner

Have a look at the declaration again. Already fixed.

Have a look at the declaration again. Already fixed.
@ -38,3 +35,1 @@
clip.setArcHeight(32);
clip.setArcWidth(32);
contactProfilePic.setClip(clip);
final var contactProfilePic = new ProfilePicImageView(chat instanceof GroupChat ? groupIcon : userIcon, 32);

As we are using a dedicated component for this, why not handle the selection of the correct profile picture inside ProfilePicImageView?

As we are using a dedicated component for this, why not handle the selection of the correct profile picture inside `ProfilePicImageView`?
Poster
Owner

Well, if I do that right inside this branch, then I'll add a (transient) Image to Chat (that will be for now always null) whose value I'm going to null check inside the mentioned constructor. Should I really do that here?

Well, if I do that right inside this branch, then I'll add a (transient) Image to Chat (that will be for now always null) whose value I'm going to null check inside the mentioned constructor. Should I really do that here?

I don't see why the solution would be that complicated, but you might also resolve this in a separate branch.

I don't see why the solution would be that complicated, but you might also resolve this in a separate branch.
kske marked this conversation as resolved
@ -34,3 +29,1 @@
} else {
getChildren().add(new Label(contact.getContacts().size() + " members"));
}
getChildren().add(contact instanceof User ? new UserStatusLabel((User) contact) : new GroupSizeLabel((Group) contact));

I did not locate the source of the problem, but the group size label changes its color occasionally when a member of the group changes his status to "busy".

If this is intended, how would we go about multiple group members with different statuses?

I did not locate the source of the problem, but the group size label changes its color occasionally when a member of the group changes his status to "busy". If this is intended, how would we go about multiple group members with different statuses?
Poster
Owner

Wait what? How's that possible? That shouldn't be possible at all.

Wait what? How's that possible? That shouldn't be possible at all.
Poster
Owner

I'd love to hear how that problem is caused because I have no clue what could cause it.

I'd love to hear how that problem is caused because I have no clue what could cause it.
Poster
Owner

Are you sure you managed to change the color of the group size label?
I've tried reproducing using the instructions you just gave, but could not reproduce it.

Are you sure you managed to change the color of the group size label? I've tried reproducing using the instructions you just gave, but could not reproduce it.

I will provide some screenshots in a few hours.

I will provide some screenshots in a few hours.

How to reproduce:

Preparation:

  • 2 users inside a group

Step-by-step reproduction:

  1. Start the server and both clients
  2. Select the group chat in Client A and the chat with Client A in Client B
  3. Change the status of Client A to "busy"
  4. Select the group chat in Client B
  5. Observe the red member count over the chat list

grafik

There might be a more general reproduction, however this is what I tried at the moment.

How to reproduce: Preparation: - 2 users inside a group Step-by-step reproduction: 1. Start the server and both clients 2. Select the group chat in Client A and the chat with Client A in Client B 3. Change the status of Client A to "busy" 4. Select the group chat in Client B 5. Observe the red member count over the chat list ![grafik](/attachments/66a0e9b5-d075-4a52-a4d1-53c3a7e15ae6) There might be a more general reproduction, however this is what I tried at the moment.
Poster
Owner

The issue is that you rejected my idea to use a separate component (ContactControl) there. That would have prevented that.

The issue is that you rejected my idea to use a separate component (ContactControl) there. That would have prevented that.
Poster
Owner

Because we had a case here of not reinitializing the value correctly.

Because we had a case here of not reinitializing the value correctly.
@ -192,0 +194,4 @@
settingsButton.setAlignment(Pos.BOTTOM_RIGHT);
HBox.setHgrow(spaceBetweenUserAndSettingsButton, Priority.ALWAYS);
generateOwnStatusControl();
System.out.println(ownContactControl.getChildren());

This seems to be a leftover from development.

This seems to be a leftover from development.
delvh marked this conversation as resolved
@ -699,6 +719,13 @@ public final class ChatScene implements EventListener, Restorable {
attachmentView.setVisible(visible);
}
private void generateOwnStatusControl() {

You could annotate this as an event handler directly.

You could annotate this as an event handler directly.
delvh marked this conversation as resolved
@ -63,1 +62,3 @@
statusComboBox.setOnAction(e -> {});
statusComboBox.setOnAction(e -> {
final var status = statusComboBox.getValue();
if (status == null) return;

How is this possible?

How is this possible?
Poster
Owner

Artifact from when I called it using the selectionModel before stumbling upon this method.
Yes, agreed, their Javadoc doesn't mention anything about it,so I'll remove it.

Artifact from when I called it using the selectionModel before stumbling upon this method. Yes, agreed, their Javadoc doesn't mention anything about it,so I'll remove it.
kske approved these changes 2020-10-09 12:23:09 +02:00
kske left a comment
Owner

Yeah, this looks really nice now 👍

Yeah, this looks really nice now 👍
mpk requested changes 2020-10-09 22:39:42 +02:00
mpk left a comment
Owner

Looks good overall despite some minor complains I have besides that good job!

Looks good overall despite some minor complains I have besides that good job!
@ -50,3 +35,1 @@
AlertHelper.confirmAction(alert, () -> {
EnvoyLog.getLogger(ShutdownHelper.class).log(Level.INFO, "A logout was requested");
public static void exit(boolean force) {

Why exactly do you want the option of force closing the application if you hardcoded it to false when you are calling the method?

Why exactly do you want the option of force closing the application if you hardcoded it to false when you are calling the method?
Poster
Owner

Because otherwise we encounter a problem in StatusTrayIcon as I've noticed:
The previous method only checked whether settings.isHideOnClose() and StatusTryIcon.isSupported() are true. If a StatusTrayIcon is shown, both are automatically true. Hence, nothing would happen when you click on Exit in the TrayIcon (except that the stage would be minimized, which is not what is wanted here). That's the reason why this extra method was needed.

Because otherwise we encounter a problem in `StatusTrayIcon` as I've noticed: The previous method only checked whether `settings.isHideOnClose()` and `StatusTryIcon.isSupported()` are `true`. If a StatusTrayIcon is shown, both are automatically true. Hence, nothing would happen when you click on Exit in the TrayIcon (except that the stage would be minimized, which is not what is wanted here). That's the reason why this extra method was needed.

OK than it is clear

OK than it is clear
mpk marked this conversation as resolved
@ -702,0 +720,4 @@
private void generateOwnStatusControl() {
final var ownUserControl = new ContactControl(localDB.getUser());
ownUserControl.setAlignment(Pos.CENTER_LEFT);
if (ownContactControl.getChildren().get(0) instanceof ContactControl) ownContactControl.getChildren().set(0, ownUserControl);

I do not see the point of doing this as you can just check if it is null if I understand that correctly. If not please correct and explane the exact purpose to me.

I do not see the point of doing this as you can just check if it is null if I understand that correctly. If not please correct and explane the exact purpose to me.
Poster
Owner

As you can see a few lines above, this method is annotated with an Event: It gets called every time the user changes his own status.
Hence I always have to replace it then.
And for the first part: I cannot do that because initially this component is no part of the HBox (you remember, the thing with importing the by now outdated jar into the SceneBuilder...). To avoid that, it gets dynamically added.

But thinking about it, it may be possible, however I do not know if it'll work as bug-free as this implementation: As soon as you initialize this variable somewhere else, you might encounter a bug that duplicates it.

As you can see a few lines above, this method is annotated with an Event: It gets called every time the user changes his own status. Hence I always have to replace it then. And for the first part: I cannot do that because initially this component is no part of the HBox (you remember, the thing with importing the by now outdated jar into the SceneBuilder...). To avoid that, it gets dynamically added. But thinking about it, it may be possible, however I do not know if it'll work as bug-free as this implementation: As soon as you initialize this variable somewhere else, you might encounter a bug that duplicates it.

What if, instead of regenerating the whole control, we would just change the displayed information in the event handler?

What if, instead of regenerating the whole control, we would just change the displayed information in the event handler?
Poster
Owner

It could be possible if we make some changes to the underlying ContactControl.
The question however is: Should it be done?

It could be possible if we make some changes to the underlying `ContactControl`. The question however is: Should it be done?
Poster
Owner

Oh, and also: there is a difference between ownUserControl and ownContactControl.

Oh, and also: there is a difference between ownUserControl and ownContactControl.
Poster
Owner

Better?

Better?

Yes better

Yes better
@ -123,3 +120,3 @@
String token;
if (user.getAuthToken() != null && user.getAuthTokenExpiration().isAfter(Instant.now())) {
if (user.getAuthToken() != null && user.getAuthTokenExpiration().isAfter(Instant.now()))

Are you sure all of the code in the body of this if statement gets executed if you remove the curly brackets?

Are you sure all of the code in the body of this if statement gets executed if you remove the curly brackets?
Poster
Owner

Was my formatter, so I'd guess so. Otherwise blame Eclipse.
Same for below.

Was my formatter, so I'd guess so. Otherwise blame Eclipse. Same for below.
Poster
Owner

And yes, it's only one statement, so why shouldn't it?

And yes, it's only one statement, so why shouldn't it?

Because of the comment but I guess it does not count

Because of the comment but I guess it does not count
mpk marked this conversation as resolved
@ -145,2 +141,3 @@
for (final var msg : pendingMessages) {
final var msgCommon = msg.toCommon();
if (msg.getCreationDate().isAfter(credentials.getLastSync())) {
if (msg.getCreationDate().isAfter(credentials.getLastSync()))

same as above

same as above
Poster
Owner

Still only one statement.

Still only one statement.
mpk marked this conversation as resolved
@ -199,3 +195,3 @@
// Sending group message status changes
if (gmsg.getStatus() == SENT && gmsg.getLastStatusChangeDate().isAfter(gmsg.getCreationDate())
|| gmsg.getStatus() == RECEIVED && gmsg.getLastStatusChangeDate().isAfter(gmsg.getReceivedDate())) {
|| gmsg.getStatus() == RECEIVED && gmsg.getLastStatusChangeDate().isAfter(gmsg.getReceivedDate()))

same as above

same as above
Poster
Owner

Still only one statement

Still only one statement
mpk marked this conversation as resolved
mpk reviewed 2020-10-09 22:48:35 +02:00
mpk left a comment
Owner

Oh and I noticed that the settigs button is not sticking to the right end of the new OwnContactControl so please make sure you add a region in the middle and setting the hgrow of this component to Priority.ALWAYS to ensue proper allignment.

Oh and I noticed that the settigs button is not sticking to the right end of the new OwnContactControl so please make sure you add a region in the middle and setting the hgrow of this component to `Priority.ALWAYS` to ensue proper allignment.
mpk requested changes 2020-10-09 22:50:14 +02:00
mpk left a comment
Owner

Oh and I noticed that the settigs button is not sticking to the right end of the new OwnContactControl so please make sure you add a region in the middle and setting the hgrow of this component to Priority.ALWAYS to ensue proper allignment.
(Sorry messes up the converation a little bit with the top comment beeing a comment that can't be deleted instead of beeing another request changes.)

Oh and I noticed that the settigs button is not sticking to the right end of the new OwnContactControl so please make sure you add a region in the middle and setting the hgrow of this component to `Priority.ALWAYS` to ensue proper allignment. (Sorry messes up the converation a little bit with the top comment beeing a comment that can't be deleted instead of beeing another request changes.)
delvh reviewed 2020-10-09 23:13:45 +02:00
@ -192,0 +192,4 @@
// Set the design of the box in the upper-left corner
settingsButton.setAlignment(Pos.BOTTOM_RIGHT);
HBox.setHgrow(spaceBetweenUserAndSettingsButton, Priority.ALWAYS);
Poster
Owner

@DieGurke The region you requested is already here, but for some strange reason it doesn't work as expected.
Any idea why?
Feel free to take on that challenge, because I've tried it for about twenty minutes and could not get it to work.

@DieGurke The region you requested is already here, but for some strange reason it doesn't work as expected. Any idea why? Feel free to take on that challenge, because I've tried it for about twenty minutes and could not get it to work.

I could't fix it neither. Preferably you open an issue for that.

I could't fix it neither. Preferably you open an issue for that.
delvh marked this conversation as resolved
mpk approved these changes 2020-10-10 14:07:11 +02:00
delvh merged commit 08bd915f04 into develop 2020-10-10 14:16:28 +02:00
delvh deleted branch f/change-user-status 2020-10-10 14:16:45 +02:00
This repo is archived. You cannot comment on pull requests.
There is no content yet.