GUACAMOLE-1293: Add client-side support for receiving join and leave notifications.

[necouchman]

This is the client-side change for apache/guacamole-server#358, which adds support for notifying the connection owner when a user joins the connection.

[jmuehlner]

Hey Nick! So I've had a more thorough look through this and the backend changes and I have a couple of thoughts.

First off, I really like this idea. Having join notifications will be an awesome feature, something that I would enjoy quite a bit.

The new msg instruction would also be really handy for implementing some features that I was thinking about as well, but I think a few tweaks would make it more extensible.

One thing is that we don't want to be generating messages for display directly on the backend - this bypasses the translation system on the frontend, and is something we've avoided in the past. We can still send a human-readable message for debugging purproses (like we do with ack, etc), but it shouldn't be used for display to the user.

If the msg instruction contained "msg code" or something like that, the frontend can render something based on the code, as well as any arbitrary arguments that might accomopany any given code.

These arguments could contain, for example, a user identifier that could be used to generate (using the translation system) the same message that you're currently generating on the backend.

Or, using a different event code, the frontend might render something completely different (a session recording notification, or a notification that a user is already in a connection, etc). The frontend could just ignore any event code it doesn't understand.

On the subject of username, I think it would be better to send a machine-readable user identifier (like the user ID perhaps) than the actual text username. The frontend would know what kind of identifier it sent, and would therefore know how to render any notification based on it. This could maybe be part of a generic connection context provided to the backend, which may include other things that just the user ID.

On a side note, having a user ID on the backend would be really cool since it would allow the the user log functions to actually differentiate based on user in the log.

So to summarize, this is what I was thinking:

The frontend can provide some context when a connection is established, including some user identifier and possibly other things.

The msg message type could contain:

• A "msg code" (not sure about the name) that contains a machine-readable message type that the frontend will know how to render based on.
• An arbitrary list of arguments that is specific to the "msg code" - in the case of the user join / leave messages this would be the user identifier previously supplied in the handshake.
• A human readable message for debugging purposes.

Does that seem reasonable? @mike-jumper any thoughts?

[necouchman]

@jmuehlner Thanks for the feedback - appreciate it. I like your idea/concept for a "code" with some set of arguments so that translation can continue to happen on the front-end - that makes perfect sense to me. I'll wait to see if @mike-jumper has any additional feedback, but I'll start retooling things a bit based on this feedback.

I'm guessing we'll want a new header file where we can define well-known message codes and their human-readable debugging counter-parts, so I'll add that, and then work on tweaking the instruction to take the arguments.

[necouchman]

@jmuehlner Okay, I think I've got things mostly reworked, both guacd and guacamole-client side, to where they should be, though the code does need some clean-up (still a bunch of debugging stuff in there), and I'm struggling with one other thing.

I'm trying to make the code as generic as possible, where a message code can be passed through that will then translate to a key that is looked up in the translation system, and then an arbitrary number of arguments can be passed along, as well, that will be substituted into the translation. The general concept of the translation/translation-values is used throughout the code, but I'm struggling with the "arbitrary number" part of it.

My current method is to try to pass through an array of one or more values to the translation-values, and then be able to reference those values as you would any other array. In the template this looks like this:

    <!-- Message text -->
    <p class="client-message-text"
       translate="{{ getClientMessage() }}"
       translate-values="{ ARGS: message.args }"></p>

and then in the translation file it looks like this:

        "CLIENT_MESSAGE_JOINED"  : "User {ARGS[0]} has joined the connection.",
        "CLIENT_MESSAGE_LEFT"    : "User {ARGS[0]} has left the connection.",

But this ends up translating to User undefined has joined the connection. I see that the TranslatableMessage type has an array called variables, but this seems to be key/value pairs (objects), and it also seems to originate from the REST API, and not something generated by guacd. I'm not sure if the right way to go is to retool guacd to send any arguments as key/value pairs (and what that would look like coming out of guacd - just write key=value?), or if what I've done above should work and just isn't?

Appreciate any guidance....

[jmuehlner]

Hey Nick! I really like the direction this is going. It looks a lot better already. On the topic of the ARGS and User undefined has joined the connection - this seems to be a limitation on the way that angular-translate parses the translation strings - see https://github.com/angular-translate/angular-translate/blob/d727abb0cae045c1f645bb7a92f06f8240875987/src/directive/translate.js#L162 - the translation service would be literally looking for a key called ARGS[0] in the translate-values array.

I played around with your code and got it working by doing exactly that - the following changes get it working:

diff --git a/guacamole/src/main/frontend/src/app/client/directives/guacClientMessage.js b/guacamole/src/main/frontend/src/app/client/directives/guacClientMessage.js
index 019d434e6..ba5ff3339 100644
--- a/guacamole/src/main/frontend/src/app/client/directives/guacClientMessage.js
+++ b/guacamole/src/main/frontend/src/app/client/directives/guacClientMessage.js
@@ -43,6 +43,17 @@ angular.module('client').directive('guacClientMessage', [function guacClientMess
             
             const $log                 = $injector.get('$log');
             const ManagedClientMessage = $injector.get('ManagedClientMessage');
+
+            $scope.giveMeTheArgs = function giveMeTheArgs() {
+                return $scope.message.args.reduce(
+                    function(acc, value, index) {
+                        acc[`ARGS[${index}]`] = value;
+                        return acc;
+                    },
+                    {}
+                );
+            };
+
             
             /**
              * Uses the msgcode to retrieve the correct translation key for
diff --git a/guacamole/src/main/frontend/src/app/client/templates/guacClientMessage.html b/guacamole/src/main/frontend/src/app/client/templates/guacClientMessage.html
index a6fc9f25c..310f9aa3b 100644
--- a/guacamole/src/main/frontend/src/app/client/templates/guacClientMessage.html
+++ b/guacamole/src/main/frontend/src/app/client/templates/guacClientMessage.html
@@ -3,6 +3,6 @@
     <!-- Message text -->
     <p class="client-message-text"
        translate="{{ getClientMessage() }}"
-       translate-values="{ ARGS: message.args }"></p>
+       translate-values="{{ giveMeTheArgs() }}"></p>
 
 </div>

[necouchman]

Thanks @jmuehlner - appreciate the hint and I'll try something along those lines...

[necouchman]

@jmuehlner It's been a while, so I need to review and make sure, but I think it's ready for review, again...

[jmuehlner]

I have a few nitpicks, but the overall approach looks pretty good to me. @mike-jumper thoughts?