Support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings #271

samhed · 2013-06-20T13:09:08Z

Somewhat related to noVNC issue #117, but instead of scaling it is using the RFB protocol message 'SetDesktopSize' from the client to the server.

Edit:
This patch adds support in noVNC for the encodings 'ExtendedDesktopSize' (-308) and 'SetDesktopSize' (251). If the 'ExtendedDesktopSize' encoding is supported by the server, this patch allows for automatic resizes of the session shown in noVNC to fit the browser window. Note that the server still has the option of denying the resize request from noVNC.

When supported, this is a client-initiated resize. For this to work the 'ExtendedDesktopSize' encoding is required on the server. Aside from client-initiated resizes this encoding brings support from the server for multiple monitors (Note that mulitple monitors are not supported by noVNC). Documentation can be found here.

The resize is triggered by changes to the browser window's size. Note that this will have the effect that if multiple viewers are connected to one session on the VNC server the latest change will decide the size of the session as viewed on all clients.

The 'ExtendedDesktopSize' encoding is fairly new to the protocol and I know that many VNC servers have not yet implemented this, but I do know that TigerVNC's server have implemented it.

Summary:

This requires the VNC server to support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings.
Automatically resizes the session to fit the browser window
TigerVNC's server have the required encodings.

…mewhat related to noVNC issue novnc#117

…ndentation fixes

* Implemented correct use of the FBU.bytes variable. * Removed the non-incremental update from ext_desktop_size to prevent the server from sending extra ExtendedDesktopSize rectangles. * The code is now using the 'onFBUComplete' callback from rfb to resize the session on reconnect instead of a timer.

samhed · 2013-07-03T14:35:53Z

I have added a couple of fixes to this, I appreciate any comments.

kanaka · 2013-07-13T00:30:54Z

Sorry for the slow reply. Hopefully I'll be able to take a look and give some feedback this weekend.

Conflicts: include/ui.js

samhed · 2013-07-24T08:53:26Z

Merged to keep up to date with head

thardeck · 2013-08-01T08:51:55Z

How can I test your changes?
If there is more space then noVNC requires the canvas size doesn't get increased and if I shrink the browser window so I need to scroll to see the whole canvas the size also isn't changed.
Did I misunderstood your changes?

samhed · 2013-08-16T17:54:31Z

Sorry for the slow response! I don't know what you are doing wrong, could be some restrictions with your server since this sort of resize is initated by the client. Which browser do you use?

I do not think you have missunderstood my changes, both the scenarios you describe should work and they did work for us when I left off. I am still on vacation, I will be back to work in about two weeks, I will then verify that the code still works and I will also write down a way for you to test my changes properly.

In the meanwhile, would you mind trying to invoke a resize of the session from the server? To resize the session on the server you can use the xrandr -s command. Please use the setup where this patch doesn't work. This server side resize should work without problems, but I am interested in the behavior of your browser when you do this.

Edit: formatted for readability

…due to the new control-bar height.

samhed · 2013-08-30T15:04:40Z

Okey, I am back and I can verify that my code still works flawlessly for us. I found a small error due to the code being somewhat out of date, but the effect of the bug was minor.

@thardeck I would like to get some info about which vnc server and browser you use so that I can try and reproduce a scenario where my patch doens't work.

thardeck · 2013-09-05T09:58:08Z

I have tested it with Chromium 30 and Firefox 23 on Linux against QEMU.

I need to scroll in case of 1024x768 guest resolutions and if I decrease the browser window size, the noVNC canvas keeps its original size.

samhed · 2013-09-05T10:15:45Z

As far as I could find the vnc server in QEMU does not support ExtendedDesktopSize which is required for this patch to work. You could send feedback to them if you would like them to implement it.

I also updated my first post here to clarify.

thardeck · 2013-09-05T10:20:49Z

Ok, that does explain it, thanks for the clarification.

The bug would prevent the viewport from properly changing size.

samhed · 2013-09-11T08:50:57Z

This now works for vnc_auto.html as well.

Conflicts: vnc_auto.html

samhed · 2013-10-15T21:15:50Z

include/display.js


    fb_width = width;
    fb_height = height;

    rescale(conf.scale);
-    that.viewportChange();
+    that.viewportChange(0, 0, width, height);


It seems this change breaks the viewport pan function.. @kanaka do you have a quick explanation? Otherwise I will keep digging.

it's doesn't work on my project,i used libvirt in centos,javascript in the browser,i just can't let the vnc's width&height follow the browser change.could u help me?thanks!

Yeah.. first of all you need a VNC server which supports the ExtendedDesktopSize encoding. Then you need the functionality that this PR is supposed to give. As I mentioned here, this function works but sadly it breaks the viewport pan function.

DirectXMan12 · 2014-02-17T23:24:13Z

@samhed : are you still interested in working on this? It looks like a good feature to have.

samhed · 2014-02-18T08:40:12Z

@DirectXMan12 yes, I will get back to solving the bug in this patch, as soon as I get time. I do have some things that I have to prioritize at the moment, but this is still on my list.

This reverts commit e894bf7.

Conflicts: include/display.js include/rfb.js include/ui.js vnc_auto.html

…lbars when "clipping" is disabled.

…e or smaller than the remote session size.

… make the code easier to read.

samhed · 2015-01-23T18:47:43Z

Okay, I'm finally done with this! 🎉

I'll give people a chance to review the patch over the weekend before I merge it.

…bar instead

samhed · 2015-01-23T19:02:15Z

Had forgot to test vnc_auto.html, works now after I added two quick fixes.

DirectXMan12 · 2015-01-29T16:25:41Z

😮 I'll try to take a look at this tomorrow. Thanks for finishing this up -- it'll be a good feature to have.

samhed · 2015-02-03T12:18:10Z

I'd love to put this PR behind me, it's been nagging me for one and a half years now :) Did you get a chance to have a look?

DirectXMan12 · 2015-02-03T19:51:24Z

vnc_auto.html

@@ -198,9 +222,10 @@
                           'local_cursor': WebUtil.getQueryVar('cursor', true),
                           'shared':       WebUtil.getQueryVar('shared', true),
                           'view_only':    WebUtil.getQueryVar('view_only', false),
-                           'onUpdateState':  updateState,
+                           'updateState':  updateState,


This should still be onUpdateState (otherwise the status bar doesn't function correctly)

DirectXMan12 · 2015-02-03T20:29:44Z

@samhed : the actual functionality looks like a charm ✨, but I have a couple of general comments, as well as some inline comments.

It would be nice to have an option in vnc.html to disable the automatic resize, since some people with small screens may still with to view a full-sized desktop.
Can you squash the "fix error from previous commit in this pull request" commits back into their respective original PR? It seems to me like this commit should be broken up into around three commits: one with the changes to display.js, the next with the changes to rfb.js, and the last with the changes to ui.js, vnc.html, and vnc_auto.html. This way, we can keep our history relatively clean.
Can you add in a couple of basic tests for this (I can help with this part, if need-be)? We don't want to accidentally break anything in the future.

samhed · 2015-02-04T09:55:00Z

Thank you @DirectXMan12 for taking your time to go through this! I agree with your comments and will get to work.

It would be nice to have an option in vnc.html to disable the automatic resize, since some people with small screens may still with to view a full-sized desktop.

Yes, I was contemplating doing this.. the reason I didn't was that since not all VNC servers have this functionality. Do you think that an option called "Resize remote session to fit the browser window" would be confusing?

Can you squash the "fix error from previous commit in this pull request" commits back into their respective original PR?

I'm not 100% sure what you mean by "original PR" here, but yes I will clean it up. 👍

DirectXMan12 · 2015-02-04T15:57:02Z

Yes, I was contemplating doing this.. the reason I didn't was that since not all VNC servers have this functionality. Do you think that an option called "Resize remote session to fit the browser window" would be confusing?

Perhaps look at it the other way around: have an option that says "Disable remote session resize to fit browser window". That might be a bit less confusing (not checked just means we haven't explicitly disabled it, not that it's always supported). Another option (albeit a bit more complicated) would be a dropdown with three options: "off", "ask", and "always try". "Off" would mean "never resize"; "Ask" (the default) would mean that if a server sent ExtendedDesktopSize, ask the user if they would like to send SetDesktopSize in the future; and "always try" would mean always use it if the server supported it (we may or may not need this -- it may be sufficient just to have "ask"). This also paves the road for supporting other resize methods, such as automatic scaling (just add another dropdown option).

I'm not 100% sure what you mean by "original PR" here, but yes I will clean it up. 👍

whoops, I mean to say "back to their original commit" ;-)

samhed · 2015-02-06T11:11:11Z

Perhaps look at it the other way around: have an option that says "Disable remote session resize to fit browser window". That might be a bit less confusing (not checked just means we haven't explicitly disabled it, not that it's always supported). Another option (albeit a bit more complicated) would be a dropdown with three options: "off", "ask", and "always try". "Off" would mean "never resize"; "Ask" (the default) would mean that if a server sent ExtendedDesktopSize, ask the user if they would like to send SetDesktopSize in the future; and "always try" would mean always use it if the server supported it (we may or may not need this -- it may be sufficient just to have "ask"). This also paves the road for supporting other resize methods, such as automatic scaling (just add another dropdown option).

Hm, that's true, your first option might be a bit more clear for users in some cases. But I don't like the idea of having an option which you can check to "disable" something. I feel the second option seems a bit too much, I would personally prefer something very simple-looking.

noVNC is after all a VNC client without attachments to any specific VNC servers. The goal should be to be able to handle messages any server following the RFB-protocol sends our way. I think I lean towards simply labeling the option "Resize to window" in the same short style as the rest of the options.

DirectXMan12 · 2015-02-06T16:37:58Z

I think I lean towards simply labeling the option "Resize to window" in the same short style as the rest of the options.

That works for me. You may want to make it "resize desktop to window" or "resize remote to window" so it's not unclear what it does if we merge a scaling patch as well (with "resize to window" vs "scale to window", it's unclear how they're different).

samhed · 2015-02-07T00:34:52Z

That sounds good! I will create a new PR with cleaned up commits. I will start looking on the tests on monday.

samhed · 2015-02-09T15:53:44Z

Hm.. when I want to try to run some of my tests I get this:

[samuel@samuel-80 noVNC]$ ./tests/run_from_console.js -t tests/test.rfb.js 
Running tests tests/test.rfb.js using provider SpookyJS (CapserJS on PhantomJS)
ERROR: Error: Child terminated with non-zero exit code 1

And nothing more, no hints!

If i run 'PATH=$PATH:node_modules/karma/bin npm tests' I do get more output:

FAILED TESTS:
  Remote Frame Buffer Protocol Client
    Public API Basic Behavior
      #setDesktopSize
        ✖ should send the request with the given size
          PhantomJS 1.9.8 (Linux)
        AssertionError: expected { Object (_websocket, _rQ, ...) } to have sent the data [ Array(24) ], but it actually sent [ Array(24) ]
            at /home/samuel/noVNC/node_modules/chai/chai.js:925
            at /home/samuel/noVNC/tests/assertions.js:22
            at /home/samuel/noVNC/node_modules/chai/chai.js:3638
            at /home/samuel/noVNC/tests/test.rfb.js:226

I'm not sure what I can do to actually understand what's wrong! 😟

DirectXMan12 · 2015-02-09T16:00:46Z

sometimes, the console test runners leave a bit to be desired (specifically, PhantomJS's array-related errors are not always the best). Try ./tests/run_from_console.js -g -o -t tests/test.rfb.js, which will open up the generated HTML file in your browser of choice (or skip the -o option and copy and paste the file name yourself)

samhed · 2015-02-09T16:21:51Z

expected to have sent the data [ Array(24) ], but it actually sent [ Array(24) ]

When the data type is the same but the assertion fails like in this case, should we not have displayed the difference in the data?

Hehe well, I guess I can resort to debugging my tests through "console.log" and the javascript console in the browser at least!

DirectXMan12 · 2015-02-09T20:20:36Z

When the data type is the same but the assertion fails like in this case, should we not have displayed the difference in the data?

I didn't get that fancy when I wrote the assertion ;-). The actual browsers often display the full arrays (instead of Phantom's "[ Array(24) ]"), so running with the -g -o options should help.

samhed added 3 commits June 20, 2013 14:45

automatically resizes the noVNC session to fit the browser window, so…

0bb4537

…mewhat related to noVNC issue novnc#117

fixed some errors in the logic from my last commit, also some minor i…

b0871c1

…ndentation fixes

astrand mentioned this pull request Jul 15, 2013

Connect to single application #277

Closed

samhed added 2 commits July 19, 2013 09:56

Merge branch 'master' of https://github.com/kanaka/noVNC

9a6bcce

Conflicts: include/ui.js

Merge branch 'master' of https://github.com/kanaka/noVNC

4579940

samhed added 2 commits August 30, 2013 16:29

Merge branch 'master' of https://github.com/kanaka/noVNC

01f9d2e

Small fix for deducting the correct number of pixels from the height …

21256e9

…due to the new control-bar height.

samhed added 2 commits September 10, 2013 16:42

Fixed a bug which sometimes occured on touch devices.

359f88a

The bug would prevent the viewport from properly changing size.

Added the automatic resize functionality for vnc_auto.html.

0ac6c31

Merge branch 'master' of https://github.com/kanaka/noVNC

af82a2c

Conflicts: vnc_auto.html

samhed reviewed Oct 15, 2013
View reviewed changes

samhed mentioned this pull request Oct 18, 2013

Need a interface for resize the canvas by the size of the documentElement.client size #312

Closed

TESTING, TO BE REMOVED

e894bf7

samhed added 4 commits January 22, 2015 10:02

Revert "TESTING, TO BE REMOVED"

6f4310e

This reverts commit e894bf7.

Merge to keep up to date with upstream

9ae9fde

Conflicts: include/display.js include/rfb.js include/ui.js vnc_auto.html

Make sure to take the clipping setting into account and do show scrol…

8bb048c

…lbars when "clipping" is disabled.

Fixed merge-errors

dd54a45

samhed added 2 commits January 22, 2015 14:09

Hide the view_drag_button (the hand) when the display area is the sam…

1f57d1e

…e or smaller than the remote session size.

Split viewportChange into viewportChangePos and viewportChangeSize to…

87002c8

… make the code easier to read.

samhed added 2 commits January 23, 2015 19:52

removed duplicate code in vnc_auto

9b9ead6

noVNC-control-bar doesn't exist in vnc_auto.html, check noVNC_status_…

9891d8a

…bar instead

samhed force-pushed the master branch from 7dbe17c to 9891d8a Compare February 3, 2015 12:02

DirectXMan12 reviewed Feb 3, 2015
View reviewed changes

This was referenced Feb 3, 2015

Auto Scaling #117

Closed

How can I scale novnc the screen size to fit my client pc? I use vmlite vnc server. #424

Closed

samhed mentioned this pull request Feb 7, 2015

Support ExtendedDesktopSize and SetDesktopSize #444

Merged

DirectXMan12 closed this Feb 9, 2015

overcookedTOFU mentioned this pull request Dec 7, 2016

vnc: Scale display to the size of the window kimchi-project/kimchi#146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings #271

Support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings #271

samhed commented Jun 20, 2013

samhed commented Jul 3, 2013

kanaka commented Jul 13, 2013

samhed commented Jul 24, 2013

thardeck commented Aug 1, 2013

samhed commented Aug 16, 2013

samhed commented Aug 30, 2013

thardeck commented Sep 5, 2013

samhed commented Sep 5, 2013

thardeck commented Sep 5, 2013

samhed commented Sep 11, 2013

samhed Oct 15, 2013

mikeching Nov 29, 2014

samhed Nov 30, 2014

DirectXMan12 commented Feb 17, 2014

samhed commented Feb 18, 2014

samhed commented Jan 23, 2015

samhed commented Jan 23, 2015

DirectXMan12 commented Jan 29, 2015

samhed commented Feb 3, 2015

DirectXMan12 Feb 3, 2015

DirectXMan12 commented Feb 3, 2015

samhed commented Feb 4, 2015

DirectXMan12 commented Feb 4, 2015

samhed commented Feb 6, 2015

DirectXMan12 commented Feb 6, 2015

samhed commented Feb 7, 2015

samhed commented Feb 9, 2015

DirectXMan12 commented Feb 9, 2015

samhed commented Feb 9, 2015

DirectXMan12 commented Feb 9, 2015

Support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings #271

Support the 'ExtendedDesktopSize' and the 'SetDesktopSize' encodings #271

Conversation

samhed commented Jun 20, 2013

samhed commented Jul 3, 2013

kanaka commented Jul 13, 2013

samhed commented Jul 24, 2013

thardeck commented Aug 1, 2013

samhed commented Aug 16, 2013

samhed commented Aug 30, 2013

thardeck commented Sep 5, 2013

samhed commented Sep 5, 2013

thardeck commented Sep 5, 2013

samhed commented Sep 11, 2013

samhed Oct 15, 2013

Choose a reason for hiding this comment

mikeching Nov 29, 2014

Choose a reason for hiding this comment

samhed Nov 30, 2014

Choose a reason for hiding this comment

DirectXMan12 commented Feb 17, 2014

samhed commented Feb 18, 2014

samhed commented Jan 23, 2015

samhed commented Jan 23, 2015

DirectXMan12 commented Jan 29, 2015

samhed commented Feb 3, 2015

DirectXMan12 Feb 3, 2015

Choose a reason for hiding this comment

DirectXMan12 commented Feb 3, 2015

samhed commented Feb 4, 2015

DirectXMan12 commented Feb 4, 2015

samhed commented Feb 6, 2015

DirectXMan12 commented Feb 6, 2015

samhed commented Feb 7, 2015

samhed commented Feb 9, 2015

DirectXMan12 commented Feb 9, 2015

samhed commented Feb 9, 2015

DirectXMan12 commented Feb 9, 2015