zdm/envoy
zdm
/
envoy
Archived
3
Fork 0
Code Issues 22 Pull Requests Releases 1 Wiki Activity

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
Conversation 0 Commits 7 Files Changed 25 +64 -25
6 changed files with 64 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3c8c544cbd - Show all commits

2
client/src/main/java/envoy/client/helper/AlertHelper.java
View File

@ -27,8 +27,6 @@ public final class AlertHelper {
* @since Envoy Client v0.2-beta
*/
public static void confirmAction(Alert alert, Runnable action) {
alert.setHeight(225);
alert.setWidth(400);
alert.setHeaderText("");
if (Settings.getInstance().isAskForConfirmation()) alert.showAndWait().filter(ButtonType.OK::equals).ifPresent(bu -> action.run());
else action.run();

18
client/src/main/java/envoy/client/ui/SceneContext.java
View File

@ -14,6 +14,7 @@ import envoy.client.data.Settings;
import envoy.client.event.*;
import envoy.client.helper.ShutdownHelper;
import envoy.client.util.UserUtil;
import envoy.data.User.UserStatus;
import envoy.util.EnvoyLog;
import dev.kske.eventbus.*;
@ -127,10 +128,23 @@ public final class SceneContext implements EventListener {
// Add the option to exit Linux-like with "Control" + "Q" or "Alt" + "F4"
accelerators.put(new KeyCodeCombination(KeyCode.Q, KeyCombination.CONTROL_DOWN), ShutdownHelper::exit);
// Add the option to logout using "Control"+"Shift"+"L" if not in login scene
if (sceneInfo != SceneInfo.LOGIN_SCENE)
if (sceneInfo != SceneInfo.LOGIN_SCENE) {
kske marked this conversation as resolved
kske commented 2020-10-08 17:36:00 +02:00
Review
Copy Link
  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)
delvh commented 2020-10-08 21:56:42 +02:00
Review
Copy Link

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).
kske commented 2020-10-09 08:20:51 +02:00
Review
Copy Link

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.
delvh commented 2020-10-09 11:17:13 +02:00
Review
Copy Link

See #91

See #91
// Add the option to logout using "Control"+"Shift"+"L"
accelerators.put(new KeyCodeCombination(KeyCode.L, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN), UserUtil::logout);
// Add the option to change status using "Control" + "Shift" +
// (o)F(fline)/ A(way)/ B(usy)/(o)N(line)
accelerators.put(new KeyCodeCombination(KeyCode.F, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN),
() -> UserUtil.changeStatus(UserStatus.OFFLINE));
accelerators.put(new KeyCodeCombination(KeyCode.A, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN),
() -> UserUtil.changeStatus(UserStatus.AWAY));
accelerators.put(new KeyCodeCombination(KeyCode.B, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN),
() -> UserUtil.changeStatus(UserStatus.BUSY));
accelerators.put(new KeyCodeCombination(KeyCode.N, KeyCombination.CONTROL_DOWN, KeyCombination.SHIFT_DOWN),
() -> UserUtil.changeStatus(UserStatus.ONLINE));
}
// Add the option to open the settings scene with "Control"+"S", if being in
// chat scene
if (sceneInfo == SceneInfo.CHAT_SCENE)

29
client/src/main/java/envoy/client/ui/StatusTrayIcon.java
View File

@ -7,8 +7,9 @@ import javafx.application.Platform;
import javafx.stage.Stage;
import envoy.client.helper.ShutdownHelper;
import envoy.client.util.IconUtil;
import envoy.client.util.*;
import envoy.data.Message;
import envoy.data.User.UserStatus;
import dev.kske.eventbus.*;
import dev.kske.eventbus.Event;
@ -51,12 +52,27 @@ public final class StatusTrayIcon implements EventListener {
trayIcon.setImageAutoSize(true);
trayIcon.setToolTip("You are notified if you have unread messages.");
final PopupMenu popup = new PopupMenu();
final var popup = new PopupMenu();
final MenuItem exitMenuItem = new MenuItem("Exit");
// Adding the exit menu item
final var exitMenuItem = new MenuItem("Exit");
exitMenuItem.addActionListener(evt -> ShutdownHelper.exit());
popup.add(exitMenuItem);
// Adding the logout menu item
final var logoutMenuItem = new MenuItem("Logout");
logoutMenuItem.addActionListener(evt -> { hide(); UserUtil.logout(); });
popup.add(exitMenuItem);
delvh marked this conversation as resolved Outdated
kske commented 2020-10-08 17:37:19 +02:00
Outdated
Review
Copy Link

This appears to be an error.

This appears to be an error.
delvh commented 2020-10-08 21:52:39 +02:00
Outdated
Review
Copy Link

Ctrl C, Ctrl V.

Ctrl C, Ctrl V.
// Adding the status change items
final var statusSubMenu = new Menu("Change status");
for (final var status : UserStatus.values()) {
final var statusMenuItem = new MenuItem(status.toString().toLowerCase());
statusMenuItem.addActionListener(evt -> UserUtil.changeStatus(status));
statusSubMenu.add(statusMenuItem);
}
popup.add(statusSubMenu);
trayIcon.setPopupMenu(popup);
// Only display messages if the stage is not focused
@ -89,9 +105,8 @@ public final class StatusTrayIcon implements EventListener {
@Event
private void onMessage(Message message) {
if (displayMessages) trayIcon.displayMessage(
message.hasAttachment() ? "New " + message.getAttachment().getType().toString().toLowerCase() + " message received" : "New message received",
message.getText(),
MessageType.INFO);
if (displayMessages) trayIcon
delvh marked this conversation as resolved Outdated
kske commented 2020-10-08 17:39:15 +02:00
Outdated
Review
Copy Link

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.
.displayMessage(message.hasAttachment() ? "New " + message.getAttachment().getType().toString().toLowerCase() + " message received"
: "New message received", message.getText(), MessageType.INFO);
}
}

15
client/src/main/java/envoy/client/ui/chatscene/ChatSceneCommands.java
View File

@ -4,7 +4,8 @@ import java.util.Random;
import java.util.function.*;
import java.util.logging.Level;
import javafx.scene.control.ListView;
import javafx.scene.control.*;
import javafx.scene.control.Alert.AlertType;
import javafx.scene.control.skin.VirtualFlow;
import envoy.client.data.Context;
@ -14,6 +15,7 @@ import envoy.client.ui.SceneContext.SceneInfo;
import envoy.client.ui.controller.ChatScene;
import envoy.client.util.*;
import envoy.data.Message;
import envoy.data.User.UserStatus;
import envoy.util.EnvoyLog;
/**
@ -63,6 +65,17 @@ public final class ChatSceneCommands {
.setDescription("Opens the settings screen")
.buildNoArg("settings");
// Status change initialization
builder.setAction(text -> {
try {
UserUtil.changeStatus(Enum.valueOf(UserStatus.class, text.get(0).toUpperCase()));
} catch (final IllegalArgumentException e) {
final var alert = new Alert(AlertType.ERROR);
kske marked this conversation as resolved
kske commented 2020-10-08 17:43:34 +02:00
Review
Copy Link

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.
delvh commented 2020-10-08 21:27:04 +02:00
Review
Copy Link

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.
kske commented 2020-10-09 08:23:01 +02:00
Review
Copy Link

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.
alert.setContentText("Please provide an existing status");
alert.showAndWait();
}
}).setDescription("Changes your status to the given status.").setNumberOfArguments(1).setDefaults("OFFLINE").build("status");
kske commented 2020-10-08 17:44:05 +02:00
Outdated
Review
Copy Link

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

Any reason why the default is "offline"? This should be documented.
delvh commented 2020-10-08 22:01:58 +02:00
Outdated
Review
Copy Link

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?
kske commented 2020-10-09 08:26:06 +02:00
Outdated
Review
Copy Link
  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?
delvh commented 2020-10-09 08:57:51 +02:00
Outdated
Review
Copy Link

  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)
kske commented 2020-10-09 11:14:05 +02:00
Outdated
Review
Copy Link

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.
delvh commented 2020-10-09 11:18:48 +02:00
Outdated
Review
Copy Link

Have a look at the declaration again. Already fixed.

Have a look at the declaration again. Already fixed.
// Selection of a new message initialization
messageDependantAction("s",
m -> { messageList.getSelectionModel().clearSelection(); messageList.getSelectionModel().select(m); },

22
client/src/main/java/envoy/client/ui/settings/GeneralSettingsPane.java
View File

@ -55,18 +55,16 @@ public final class GeneralSettingsPane extends SettingsPane {
combobox.setOnAction(e -> { settings.setCurrentTheme(combobox.getValue()); EventBus.getInstance().dispatch(new ThemeChangeEvent()); });
getChildren().add(combobox);
if (context.getClient().isOnline()) {
final var statusComboBox = new ComboBox<UserStatus>();
statusComboBox.getItems().setAll(UserStatus.values());
statusComboBox.setValue(context.getLocalDB().getUser().getStatus());
statusComboBox.setTooltip(new Tooltip("Change your current status"));
statusComboBox.setOnAction(e -> {
final var status = statusComboBox.getValue();
if (status == null) return;
else UserUtil.changeStatus(status);
});
getChildren().add(statusComboBox);
}
final var statusComboBox = new ComboBox<UserStatus>();
statusComboBox.getItems().setAll(UserStatus.values());
statusComboBox.setValue(context.getLocalDB().getUser().getStatus());
statusComboBox.setTooltip(new Tooltip("Change your current status"));
statusComboBox.setOnAction(e -> {
final var status = statusComboBox.getValue();
if (status == null) return;
kske commented 2020-10-08 18:18:48 +02:00
Outdated
Review
Copy Link

How is this possible?

How is this possible?
delvh commented 2020-10-08 21:19:25 +02:00
Outdated
Review
Copy Link

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.
else UserUtil.changeStatus(status);
});
getChildren().add(statusComboBox);
final var logoutButton = new Button("Logout");
logoutButton.setOnAction(e -> UserUtil.logout());

3
client/src/main/java/envoy/client/util/UserUtil.java
View File

@ -53,6 +53,7 @@ public final class UserUtil {
*/
public static void changeStatus(UserStatus newStatus) {
EventBus.getInstance().dispatch(new OwnStatusChange(newStatus));
Context.getInstance().getClient().send(new UserStatusChange(Context.getInstance().getLocalDB().getUser().getID(), newStatus));
if (Context.getInstance().getClient().isOnline())
Context.getInstance().getClient().send(new UserStatusChange(Context.getInstance().getLocalDB().getUser().getID(), newStatus));
}
}