Cleanup of settings (amongst other things) #715

CendioOssman · 2016-11-14T16:20:56Z

This does various cleanups for things that have annoyed me for a while. :)
The primary focus is the settings and connect dialogs, which I felt were needlessly complex for most users.

Give it a try and see what you think.

samhed · 2016-11-14T21:48:01Z

Really nice!

I also like the ideas of removing the extra stylesheets and the token setting.

samhed · 2016-11-15T07:26:45Z

For some reason Chrome decides to do line-breaks in the settings with this new code:

This is how it looks with master:

Should we add this to the css for the panels?

white-space: nowrap;

samhed · 2016-11-15T08:12:15Z

98400d7 seems to fix an issue where the 'connecting'-transition screen appeared in front of the password dialog.

CendioOssman · 2016-11-15T08:56:58Z

For some reason Chrome decides to do line-breaks in the settings with this new code:

It always did. It's just that the input field pushed out the width of the dialog so that we didn't see any line breaks with the current elements.

A general nowrap is most likely a bad idea.But it could make sense for labels. I'll have a look.

DirectXMan12

I would like a chance to check this visually, but I do have a concern about removing the token setting.

DirectXMan12 · 2016-11-15T16:44:51Z

app/ui.js

@@ -180,7 +180,6 @@ var UI;
            UI.initSetting('view_only', false);
            UI.initSetting('path', 'websockify');
            UI.initSetting('repeaterID', '');
-            UI.initSetting('token', '');


Hmm... does this prevent token from working as a URL param? If so, it should still be in there. If we somehow broke passing a token as part of the URL at some point, then we need to fix that.

Ah, right. I see now where it was used. However seeing that I have to object to its inclusion. Users who want a special websocket path should merely specify a different path. What makes this token so special?

Edit: To clarify, people currently using https://example.com/vnc.html&token=foo should use https://example.com/vnc.html&path=websockify%26token%3Dfoo

We currently have deployed noVNC on a bastion host, with a local change that makes host and port hidden inputs, and renames the label of the token. Tokens map 1:1 to available VNC ports of internal hosts (via websockify configuration file that lists them all), so that we can run one websockify for all of them.

While I agree that this setting is confusing to users, and should not be exposed by default, keeping the token as a hidden input (so that we can turn it into a regular input and change the label locally) would help us.

Can't you just change path in that case? That's what we do for ThinLinc and it has worked out fine.

We should have a way to add in URL-only parameters. We should discuss more in a follow-up PR. For now, this is fine.

samhed · 2016-11-21T13:26:27Z

I see that you have made the "noVNC" text larger above the connect button in the middle. I think this looks nicer now!

When the browser is very narrow (for example, phones in portrait mode) it looks like this:

Should perhaps be tweaked a bit? No strong feelings about this one though

samhed · 2016-11-21T13:56:02Z

As far as the discussion regarding token goes, I don't understand why it was added in the first place (back in 2012: 4c75210)? Isn't this a Nova specific thing?

CendioOssman · 2016-11-23T10:54:01Z

I've fixed the text and button so that it scales for narrow screens now.

samhed · 2016-11-23T14:08:48Z

Looks good!

Only the token discussion remaining then imo.

DirectXMan12 · 2016-11-23T20:09:01Z

As far as the discussion regarding token goes, I don't understand why it was added in the first place (back in 2012: 4c75210)? Isn't this a Nova specific thing?

kind-of-not-really? It's the only method of doing target selection when using websockify (which many people use noVNC with), so it's a convenience factor for users of websockify. #537 and #536 are the relevant recent discussion (people seem to expect to be able to pass token in the path).

CendioOssman · 2016-11-24T09:52:31Z

It's the only method of doing target selection when using websockify (which many people use noVNC with), so it's a convenience factor for users of websockify.

As I mentioned, you can just modify the path instead.

#537 and #536 are the relevant recent discussion (people seem to expect to be able to pass token in the path).

None of these mention what's so special about token unfortunately. This is a very slippery road. token is in no way standardised. Some other gateways might want auth, or username, or password, or cookie, etc. Having an ever increasing list of these things isn't really sensible.

CendioOssman

I changed my mind. I think rgb(40, 40, 40) actually works better than a pitch black background. It is still dark an out of the way, but still has enough contrast to see when a completely black session isn't covering the entire window.

CendioOssman · 2017-01-03T15:12:04Z

Can we get this moving forward? What needs to be done to resolve the situation?

samhed · 2017-01-04T07:34:00Z

Good to merge in my opinion

Anyone with basic knowledge of CSS will easily figure out how to customise the appearance of the UI, so remove the burden of having to maintain these extra style sheets.

It was easy to confuse them as being VNC settings, so keep them all under one group.

It's been standardised for quite some time, so remove the extra noise in the CSS.

It only contained a password field, which might not be needed, and is handled by a separate dialog if it is.

It's generally the only thing the user needs to click on, so make sure it clearly stands out.

We have enough layers now that we need to have some system for this. E.g. make sure that dialogs during connect show up in front of the blocking transition layer.

DirectXMan12

LGTM 👍

DirectXMan12 · 2017-01-06T19:07:32Z

app/ui.js

@@ -180,7 +180,6 @@ var UI;
            UI.initSetting('view_only', false);
            UI.initSetting('path', 'websockify');
            UI.initSetting('repeaterID', '');
-            UI.initSetting('token', '');


We should have a way to add in URL-only parameters. We should discuss more in a follow-up PR. For now, this is fine.

DirectXMan12 requested changes Nov 15, 2016

View reviewed changes

samhed added the feature label Nov 16, 2016

samhed added this to the v0.7.0 milestone Nov 16, 2016

CendioOssman force-pushed the cleanup branch from a0357be to 6100e81 Compare November 21, 2016 12:47

CendioOssman force-pushed the cleanup branch from 6100e81 to 832bfd3 Compare November 23, 2016 10:53

CendioOssman commented Jan 3, 2017

View reviewed changes

CendioOssman added 11 commits January 4, 2017 15:04

Black out background when connected

8f2ca5f

Center canvas vertically as well

47fbdce

Remove unused token setting

58fc267

Remove alternative style sheets

340d9d6

Anyone with basic knowledge of CSS will easily figure out how to customise the appearance of the UI, so remove the burden of having to maintain these extra style sheets.

Group WebSocket settings together

575f698

It was easy to confuse them as being VNC settings, so keep them all under one group.

Group rarely changed settings together

13304ba

Remove vendor prefix from border-radius

ef3e12b

It's been standardised for quite some time, so remove the extra noise in the CSS.

Remove connect dialog

512d360

It only contained a password field, which might not be needed, and is handled by a separate dialog if it is.

Make connect button more prominent

b3c932c

It's generally the only thing the user needs to click on, so make sure it clearly stands out.

Clean up z index ordering

cac935f

We have enough layers now that we need to have some system for this. E.g. make sure that dialogs during connect show up in front of the blocking transition layer.

Avoid wrapping labels

8134589

CendioOssman force-pushed the cleanup branch from 832bfd3 to 8134589 Compare January 4, 2017 14:08

DirectXMan12 approved these changes Jan 6, 2017

View reviewed changes

DirectXMan12 merged commit 16ed7b8 into novnc:master Jan 6, 2017

DirectXMan12 mentioned this pull request Jan 6, 2017

The future of the token setting #742

Closed

CendioOssman deleted the cleanup branch June 8, 2017 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of settings (amongst other things) #715

Cleanup of settings (amongst other things) #715

CendioOssman commented Nov 14, 2016

samhed commented Nov 14, 2016

samhed commented Nov 15, 2016

samhed commented Nov 15, 2016

CendioOssman commented Nov 15, 2016

DirectXMan12 left a comment

DirectXMan12 Nov 15, 2016

CendioOssman Nov 16, 2016 •

edited

Loading

patrakov Nov 24, 2016

CendioOssman Nov 24, 2016

DirectXMan12 Jan 6, 2017

samhed commented Nov 21, 2016

samhed commented Nov 21, 2016

CendioOssman commented Nov 23, 2016

samhed commented Nov 23, 2016

DirectXMan12 commented Nov 23, 2016 •

edited

Loading

CendioOssman commented Nov 24, 2016

CendioOssman left a comment

CendioOssman commented Jan 3, 2017

samhed commented Jan 4, 2017

DirectXMan12 left a comment

DirectXMan12 Jan 6, 2017

Cleanup of settings (amongst other things) #715

Cleanup of settings (amongst other things) #715

Conversation

CendioOssman commented Nov 14, 2016

samhed commented Nov 14, 2016

samhed commented Nov 15, 2016

samhed commented Nov 15, 2016

CendioOssman commented Nov 15, 2016

DirectXMan12 left a comment

Choose a reason for hiding this comment

DirectXMan12 Nov 15, 2016

Choose a reason for hiding this comment

CendioOssman Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

patrakov Nov 24, 2016

Choose a reason for hiding this comment

CendioOssman Nov 24, 2016

Choose a reason for hiding this comment

DirectXMan12 Jan 6, 2017

Choose a reason for hiding this comment

samhed commented Nov 21, 2016

samhed commented Nov 21, 2016

CendioOssman commented Nov 23, 2016

samhed commented Nov 23, 2016

DirectXMan12 commented Nov 23, 2016 • edited Loading

CendioOssman commented Nov 24, 2016

CendioOssman left a comment

Choose a reason for hiding this comment

CendioOssman commented Jan 3, 2017

samhed commented Jan 4, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

DirectXMan12 Jan 6, 2017

Choose a reason for hiding this comment

CendioOssman Nov 16, 2016 •

edited

Loading

DirectXMan12 commented Nov 23, 2016 •

edited

Loading