-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Resize the browser to match VNC session #1849
base: master
Are you sure you want to change the base?
Conversation
Thank you for your contribution! I'm thinking this might be a bit too niche, though, since it only works under very specific circumstances. Could you describe your setup a bit more, and where this feature fits in? |
Used to open the QEMU/KVM console in a new browser window, yes, QEMU/KVM will constantly change the resolution when it is launched |
I'm connected to the VNC port of KVM/QEMU via websockify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. That sounds reasonble. I'm cautious about adding niche stuff to the general UI, though.
In TigerVNC, we have a similar behaviour but without a setting. Perhaps that could be used here as well?
The principle there is that it will resize the local window if the sizes match before the resize, but not if they are different. I.e.
- VNC session is 1024x768, local window is 1024x768. VNC session resizes to 1920x1080, and the local window tries to resize to the same 1920x1080.
- VNC session is 1024x768, but local window is 800x600. VNC session resizes to 1920x1080, but the local window stays at 800x600.
@XiaoXianNv-boot, are you interested in looking at the suggested changes? |
YES |
I have added adjustment rules now. Meet automatic adjustment in the following circumstances
If the above requirements are not met, the size of the browser will be disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good step in the right direction. But I think some more adjustments are needed to get a good fit.
Please have a look and see the comments.
vnc.html
Outdated
@@ -171,6 +171,7 @@ <h1 class="noVNC_logo" translate="no"><span>no</span><br>VNC</h1> | |||
<option value="off">None</option> | |||
<option value="scale">Local Scaling</option> | |||
<option value="remote">Remote Resizing</option> | |||
<option value="browser" disabled="" id="noVNC_setting_browser_resize">Resizing browser</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a new setting for this. This should be possible to have active as long as things aren't configured to "scale"
.
core/rfb.js
Outdated
@@ -2863,10 +2879,36 @@ export default class RFB extends EventTargetMixin { | |||
this._fbWidth, this._fbHeight); | |||
} | |||
|
|||
_updateBrowserWindows(width, height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFB object isn't really aware of things outside of its container. So I think these things are better placed in the UI object.
That means it needs some more information, though. We require width
and height
read-only properties on the RFB object. And we also need a resize
event to be fired.
core/rfb.js
Outdated
if (bodyWidthBrowserResize === document.body.clientWidth && | ||
bodyHeightBrowserResize === document.body.clientHeight) { | ||
OldResolutionEqual = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this gives the desired behaviour. What we want to compare with is _fbWidth
and _fbHeight
just before the resize, and see if they match the browser window.
This code might still be doing that, but it's not obvious that it does.
4e06e27
to
d061b8b
Compare
77ff1d9
to
96c76f7
Compare
It looks like this was closed accidentally, @XiaoXianNv-boot? |
YES |
What is the situation |
set resizeBrowser(void_) { | ||
this._resizeBrowser = void_; | ||
if (this._resizeBrowser && (this._rfbConnectionState === 'connected')) { | ||
this._resizeBrowser(this._fbWidth, this._fbHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callbacks are not that flexible. Please use a CustomEvent
like we do for other things in the RFB
object.
@@ -2875,6 +2884,10 @@ export default class RFB extends EventTargetMixin { | |||
this._fbWidth = width; | |||
this._fbHeight = height; | |||
|
|||
if (this._resizeBrowser) { | |||
this._resizeBrowser(width, height); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably best if this happens last, so that all state is properly updated.
<html lang="en"> | ||
<head> | ||
<title>noVNC</title> | ||
</head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although an example is useful, I don't think we want another HTML file to maintain. I think it is sufficient with some information in the documentation somewhere.
@@ -1058,6 +1084,12 @@ const UI = { | |||
UI.rfb.clipViewport = UI.getSetting('view_clip'); | |||
UI.rfb.scaleViewport = UI.getSetting('resize') === 'scale'; | |||
UI.rfb.resizeSession = UI.getSetting('resize') === 'remote'; | |||
if (UI.getSetting('resize') === 'off') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be relevant for 'remote'
as well, I think?
UI.rfb.resizeBrowser = UI._updateBrowserWindows; | ||
} else { | ||
UI.rfb.resizeBrowser = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be simpler to always have the callback registered, and check the setting once the callback fires?
|
||
let OldResolutionEqual = false; | ||
if (UI.bodyWidthBrowserResize === document.body.clientWidth && | ||
UI.bodyHeightBrowserResize === document.body.clientHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is a very roundabout way of checking things. Why not compare with the previous VNC session size?
if (UI.bodyHeightBrowserResize === 0 || | ||
OldResolutionEqual) { | ||
if ((width != 0) && (height != 0)) { | ||
window.resizeBy(width - bodyWidth, height - bodyHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not resizeTo()
and avoid having to calculate things?
I've added a new zoom mode that allows the browser to automatically adapt to the window .
Just need to use window.open('vnc. html/?token=token&autoconnect=1&resize=browser','_blank','toolbar=no,location=no,status=no,menubar=no,resizable=yes,width=800,height=420'); Open a window
Works fine on EDGE