Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VNC] Use vnc.html #170

Merged
merged 2 commits into from
Apr 6, 2020
Merged

[VNC] Use vnc.html #170

merged 2 commits into from
Apr 6, 2020

Conversation

JesterOrNot
Copy link

No description provided.

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

Recommends adding in-code documentation i.e

<!-- 
Used to append options to vnc.html 
- autoconnect .. To automatically connect on the host
- resize=scale .. To scale the resolution based on used preview
-->

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

Note that this solution has a bar on the left side of the screen
image

Relevant: novnc/noVNC#941 (Old issue)

@JesterOrNot
Copy link
Author

How do we remove it?

@JesterOrNot
Copy link
Author

We need to remove the sidebar how do we do this?

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

@JesterOrNot How do we remove it?

I am unable to find documentation for this.

Possibilities:

  1. https://github.com/novnc/noVNC/pull/725/files ? May be possible to manipulate app/styles/base.css
  2. wip

I would try something like:

<style> #noVNC_control_bar {  display: none;} </style>
code_here

In the worst case scenario

@jankeromnes
Copy link
Contributor

We need to remove the sidebar how do we do this?

I think we want to keep it. I've found it useful before, especially on mobile (e.g. for copy/pasting and such).

@JesterOrNot
Copy link
Author

Can someone test this PR?

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

LGTM

EDIT: i've would add the in-code documentation

@JesterOrNot
Copy link
Author

Merge @jankeromnes?

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

Seems that CI hugged up
image

@meysholdt
Copy link
Member

Will this PR also add
image when the VNC client runs in the Preview View in Gitpod? I'm a bit worried there it adds more clutter than it adds value. It would distract people from working with their apps and damages the feeling that things are "simple". What use cases does it solve?

@JesterOrNot
Copy link
Author

@meysholdt It would :(

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

@meysholdt referencing #170 (comment)

I guess that asking upstream again is the only option to get more info based on the fact that we don't have documentation on this

@jankeromnes
Copy link
Contributor

Can someone test this PR?

I've tested it by opening https://gitpod.io/#https://github.com/techwithtim/Hangman and running this in the Terminal:

gp preview "$(gp url 6080)/vnc.html?resize=scale&autoconnect=true"

Works like a charm, and I don't find the controls too distracting. Cool that users can now benefit from these by default. 🙂

EDIT: i've would add the in-code documentation

I agree that documentation could be nice, e.g. in cases like these where it's unclear what some things mean or do exactly.

But it's not a strong requirement on my part, so I'll let @JesterOrNot make the call of whether to add the comment, or keep this a short-and-sweet HTML one-liner. 🙂

@JesterOrNot
Copy link
Author

@Kreyren We could always file another issue

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

@JesterOrNot your turn, be nice! :p

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

I've tested it by opening https://gitpod.io/#https://github.com/techwithtim/Hangman and running this in the Terminal:

gp preview "$(gp url 6080)/vnc.html?resize=scale&autoconnect=true"

Would be also nice to have README.md in the vnc-full directory that informs about this in case anyone wants to work on vnc in the future to know how to properly test this.

@JesterOrNot
Copy link
Author

I decided to add the comments because they can only help :)

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

Also side-note:
image

In theory we can customize this to fix gitpod-io/gitpod#1193

(should be resolved as separate issue)

@meysholdt
Copy link
Member

Tied it, too:

  • the screen is unusable because of the strong zoom factor. There should be no zoom at all. The virtual screen should have the resolution of the preview widget.
  • the "fullscreen" button is broken with a JavaScript error TypeError: fullscreen error at HTMLInputElement.toggleFullscreen (https://1233234234.ws-eu01.gitpod.io/app/ui.js:1215:42)

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

the screen is unusable because of the strong zoom factor. There should be no zoom at all. The virtual screen should have the resolution of the preview widget.

True

image

in theory we can use xrandr --output OUTPUT --scale 2x2 for this while making some kind of GUI that would allow end-user to change the scaling on demand.

the "fullscreen" button is broken with a JavaScript error TypeError: fullscreen error at HTMLInputElement.toggleFullscreen (https://1233234234.ws-eu01.gitpod.io/app/ui.js:1215:42)

WFM

EDIT: better image and confirm of the issue

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 20, 2020

the screen is unusable because of the strong zoom factor. There should be no zoom at all. The virtual screen should have the resolution of the preview widget.

When I try resize=downscale or resize=remote instead of resize=scale, it doesn't seem to do anything. Maybe this should be a bug report or feature request to noVNC?

the "fullscreen" button is broken with a JavaScript error TypeError: fullscreen error at HTMLInputElement.toggleFullscreen (https://1233234234.ws-eu01.gitpod.io/app/ui.js:1215:42)

Maybe that's because iframes are not allowed to go fullscreen in some browsers? (E.g. to prevent annoying fullscreen ads taking over your screen).

For me, fullscreen button shows the error when in a preview pane, but it works as expected when the preview is opened in a new tab.

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

Maybe that's because iframes are not allowed to go fullscreen in some browsers? (E.g. to prevent annoying fullscreen ads taking over your screen).

UserAgent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

(don't know how else to sent browser info)
image

@Kreyren
Copy link
Contributor

Kreyren commented Feb 20, 2020

For me, fullscreen button shows the error when in a preview pane, but it works as expected when the preview is opened in a new tab.

Sorry did not test it in gitpod, same(?) issue here.

From console:

Source map error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Resource URL: https://g5fd4h65sfd4h65sd4f65hs4d-eu01.gitpod.io/bundle.js
Source Map URL: bundle.js.map

Request for fullscreen was denied because at least one of the document’s containing elements is not an iframe or does not have an “allowfullscreen” attribute. vnc.html
TypeError: The expression cannot be converted to return the specified type.

@Kreyren
Copy link
Contributor

Kreyren commented Feb 22, 2020

What is the proposed course of action for this? i've check the available documentation and i can't find anything relevant to resolve the scaling issue atm.

@JesterOrNot
Copy link
Author

@Kreyren Also can you talk to the noVNC people again I don't know what you meant novnc/noVNC#1371

@Kreyren
Copy link
Contributor

Kreyren commented Feb 22, 2020

@JesterOrNot ehh what

@JesterOrNot
Copy link
Author

JesterOrNot commented Feb 22, 2020

Me: @Kreyren We could always file another issue
You: @JesterOrNot your turn, be nice! :p

@Kreyren
Copy link
Contributor

Kreyren commented Feb 22, 2020

@JesterOrNot Figured that much, but what do you expect from me assuming that it seems that you've described the issue sufficiently

also why is making a button in GUI that would changed the css style not an option? Or how do you expect it to be toggleable ?

@JesterOrNot
Copy link
Author

This is a commonly discussed when integrating noVNC in to other systems, so I think this is worth having open and investigating further.

@JesterOrNot, the toolbar is needed for many user interactions (e.g. touch, clipboard). Is this something you've considered? Perhaps there are other ways than just completely removing the toolbar.

@Kreyren
Copy link
Contributor

Kreyren commented Feb 22, 2020

iiuc the clipboard and touch capabilities are the concern here?

For clipboard i guess that the proper implementation would be use the end-user's clipboard if browser allows it.

For touch i don't think it's worth processing until gitpod has better implementation for portable devices (gitpod-io/gitpod#970) assuming that none would probably want to use touch on PC version (personally i don't see the usecase for it there assuming that the GUI is not optimized to work with touch)

@JesterOrNot
Copy link
Author

What does iiuc mean?

@Kreyren
Copy link
Contributor

Kreyren commented Feb 22, 2020

iiuc == if i understand correctly

added in https://en.wikipedia.org/w/index.php?title=SMS_language&action=history

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 22, 2020

For the resolution, I think we need to fix this in the X server config for now (until we can achieve a direct link between preview size and X screen size).

E.g. we could divide the Xvfb screen size by 2, maybe this will help make things more legible?

@jankeromnes
Copy link
Contributor

Ok, I'm going to clean up and merge this PR. Thanks again!

I've also enabled reconnect=true (autoconnect=true was a good idea, but that actually just skips the initial screen with "Connect" button -- to actually reconnect when the connection breaks, you need reconnect=true)

FYI, here are all the vnc.html options: https://github.com/novnc/noVNC/blob/master/docs/EMBEDDING.md

Also, having the noVNC preview automatically reconnect on connection breaks is compelling enough to switch from vnc_lite.html to vnc.html.

For what it's worth, I find the extra controls very useful, but if someone else really doesn't like them, we could potentially contribute a hidecontrols=true option to noVNC upstream.

@jankeromnes jankeromnes merged commit 00286cf into gitpod-io:master Apr 6, 2020
@JesterOrNot JesterOrNot deleted the patch-2 branch April 6, 2020 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants