Abstract information from RFB to the UI #950

samhed · 2017-11-12T00:34:09Z

We had a problem with strings being passed from RFB to UI using the notification and disconnect events. These were strings that were intended to be shown directly to the end user. Since these strings were defined in RFB the UI did not have control over translations.

This PR removes the notification event, and the updatestate event. It adds a connect event, a securityfailure event, and modifies the disconnect event:

The notification event was unnecessary and was easily replaced by logging
The updatestate event was being sent at alongside the disconnect event, causing unnecessary complexity for the applications. Applications do normally not have any use for the more detailed internal state information anyway.
There is now a connect event that works nicely with the disconnect event.
The disconnect event no longer includes a reason. Instead it includes a boolean that marks if the disconnection was clean (normal) or not clean (exited with errors).
A securityfailure event is now available for applications that want to display the security result failure string that can be sent directly from the server. One example of this is authentication failures.

I still have to update the API documentation, but feel free to review.

This interface was a band aid for poor design. The two cases where it was used was replaced by logging.

And make the log message a bit more detailed

CendioOssman

👍 Looks good, just some minor questions.

CendioOssman · 2017-11-13T08:30:56Z

app/ui.js

        } else if (UI.getSetting('reconnect', false) === true && !UI.inhibit_reconnect) {
-            document.getElementById("noVNC_transition_text").textContent = _("Reconnecting...");
-            document.documentElement.classList.add("noVNC_reconnecting");
+


Extra blank lines in this general area

I'll remove them

CendioOssman · 2017-11-13T08:33:25Z

vnc_lite.html

@@ -200,8 +180,7 @@
        };

        function updatePowerButtons() {
-            var powerbuttons;
-            powerbuttons = document.getElementById('noVNC_power_buttons');
+            let powerbuttons = document.getElementById('noVNC_power_buttons');


Bonus change? ;)

I'll remove this change

CendioOssman · 2017-11-13T08:35:43Z

core/rfb.js

@@ -11,7 +11,6 @@
 */

 import * as Log from './util/logging.js';
-import _ from './util/localization.js';


We should probably move localization.js to the app side of things now.

Yeah, one small thing to note is that tests/test.util.js is generally written for core/util and will now test app/localization.js as well, but not a big problem

Or just split that test?

yeah, sounds good

CendioOssman · 2017-11-13T08:41:46Z

app/ui.js

+                visible_status_type = 'normal';
+            }
+        }
+        if (visible_status_type === 'error') { return; }


How about if warn is visible and a normal wants to be shown?

samhed · 2017-11-13T11:11:31Z

I discovered an issue with the focus handling that was introduced with the 118d929 commit. It seems like running .focus() on an element that isn't rendered have no effect. I'll just move the focus call to after showing the canvas (like it worked before).

samhed · 2017-11-13T11:14:47Z

All fixed now. API documentation still remaining though

samhed · 2017-11-13T14:01:04Z

API documentation updated as well now, I'm satisfied.

samhed · 2017-11-14T08:23:32Z

Any thoughts @DirectXMan12?

CendioOssman · 2017-11-14T08:19:15Z

docs/API.md

-new connections.
+The `connect` event is fired after all the handshaking with the server
+is completed and the connection is fully established. After this event
+the `RFB` object is ready to recieve grahpics updates and to send input.


CendioOssman · 2017-11-14T08:23:17Z

core/rfb.js

+
+            let event = new CustomEvent(
+                "securityfailure",
+                { detail: { result: security_result, reason: reason } });


The entire message is called SecurityResult, but the code field is called status, so perhaps we should use the same terminology here?

I missed that, I'll change it

CendioOssman · 2017-11-14T08:24:49Z

docs/API.md

+property is an `Object` optionally containing the property `reason`
+which is a `DOMString` specifiying the reason for the failure. The
+server can choose to not send a reason and in these cases the property
+`reason` will be omitted.


Can you try to make it clearer that this is a server provided message, rather than something internal? Perhaps also mention that the language is not fully known (although most likely english)?

Yes, will do

CendioOssman · 2017-11-14T08:25:24Z

docs/API.md

+property is an `Object` optionally containing the property `reason`
+which is a `DOMString` specifiying the reason for the failure. The
+server can choose to not send a reason and in these cases the property
+`reason` will be omitted.


You haven't documented reason/status.

What do you mean?

Ah, you mean result. Yeah, I missed to document that since we don't currently use it in the UI

To clarify; result which will be renamed to status wasn't documented

Instead of exposing all the internal connection states, the RFB module will now only send events on connect and on disconnect. This makes it simpler for the application and gets rid of the double events that were being sent on disconnect (previously updatestate and disconnect).

samhed · 2017-11-14T13:12:40Z

Fixed all your comments now @CendioOssman, I also found that I had forgotten to add tests for the new securityfailure event, so I added that.

CendioOssman · 2017-11-14T13:33:03Z

docs/API.md

+| `status` | `long`      | The failure status code
+| `reason` | `DOMString` | The **optional** reason for the failure
+
+The property `status` corresponds to the *SecurityResult* status code in


Users of the API might not be that familiar with the protocol. A link to the relevant section of rfbproto would be nice here.

The API allowed strings to be passed from the RFB module to the application using the disconnect reason. This caused problems since the application didn't have control over translations for these strings. Most of the information being passed using this string was very technical and not helpful to the end user. One exception to this was the security result information regarding for example authentication failures. The protocol allows the VNC server to pass a string directly to the user in the security result. So the disconnect reason is replaced by a boolean saying if the disconnection was clean or not. And for the security result information from the server, a new event has been added.

And only show the first error. This means that if UI.showStatus() is called for a new error while one error is already showing, the new error will not be shown. However, if a warning was showing and a new error comes up, the warning will be overwritten.

Since it is no longer used in core. Also splits localization tests into a separate file.

samhed · 2017-11-14T14:37:56Z

Added a link to SecurityResult in rfbproto for the securityfailure event status as per @CendioOssman comment

samhed · 2017-11-16T12:46:39Z

I'll give people until tomorrow to review this, then I will go ahead and merge.

DirectXMan12 · 2017-11-17T19:36:24Z

@samhed @CendioOssman hey, sorry, was out sick. FYI, next week is a holiday in the US, so I'll probably be mostly unresponsive ;-).

samhed added 4 commits November 11, 2017 02:10

Add missing semicolons

4e1c543

Remove RFB.notification()

5b20d33

This interface was a band aid for poor design. The two cases where it was used was replaced by logging.

Translate user visible text

2592c24

And make the log message a bit more detailed

Don't translate log messages

8317524

samhed added feature WIP labels Nov 12, 2017

samhed added this to the v0.7.0 milestone Nov 12, 2017

samhed mentioned this pull request Nov 12, 2017

Finalize public API #924

Closed

CendioOssman reviewed Nov 13, 2017

View reviewed changes

samhed force-pushed the disconnectapi branch from 68c78f7 to 5309a8c Compare November 13, 2017 11:13

samhed force-pushed the disconnectapi branch 2 times, most recently from f4a4ffc to c4a7b77 Compare November 13, 2017 13:57

samhed removed the WIP label Nov 13, 2017

samhed mentioned this pull request Nov 13, 2017

[RFC] New upstream noVNC release #889

Closed

samhed requested a review from DirectXMan12 November 14, 2017 08:23

CendioOssman requested changes Nov 14, 2017

View reviewed changes

samhed force-pushed the disconnectapi branch from c4a7b77 to e48dd94 Compare November 14, 2017 13:11

CendioOssman reviewed Nov 14, 2017

View reviewed changes

samhed force-pushed the disconnectapi branch from e48dd94 to 65d6a6d Compare November 14, 2017 14:32

samhed added 4 commits November 14, 2017 15:36

Move UI.cancelReconnect() to related functions

6895803

Move localization.js to app

7279364

Since it is no longer used in core. Also splits localization tests into a separate file.

samhed force-pushed the disconnectapi branch from 65d6a6d to 7279364 Compare November 14, 2017 14:36

CendioOssman approved these changes Nov 14, 2017

View reviewed changes

samhed merged commit db46e36 into novnc:master Nov 17, 2017

samhed deleted the disconnectapi branch November 17, 2017 10:01

samhed mentioned this pull request Apr 16, 2018

add get-only connectionState property and dispatch CustomEvent when state is connecting #1050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract information from RFB to the UI #950

Abstract information from RFB to the UI #950

Abstract information from RFB to the UI #950

Abstract information from RFB to the UI #950

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment