Skip to content

Add scaleFactor option to RFB options #1376

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.

Sign up for GitHub

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

Closed
jalfd wants to merge 2 commits into from
Closed

Add scaleFactor option to RFB options #1376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de47b93
Add useNativeDpi option
May 22, 2019
2d9c8a8
Update UI to expose a Native DPI option
Jun 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions app/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ const UI = {
UI.initSetting('host', window.location.hostname);
UI.initSetting('port', port);
UI.initSetting('encrypt', (window.location.protocol === "https:"));
UI.initSetting('native_dpi', false);
UI.initSetting('view_clip', false);
UI.initSetting('resize', 'off');
UI.initSetting('quality', 6);
Expand Down Expand Up @@ -355,6 +356,8 @@ const UI = {
UI.addSettingChangeHandler('compression', UI.updateCompression);
UI.addSettingChangeHandler('view_clip');
UI.addSettingChangeHandler('view_clip', UI.updateViewClip);
UI.addSettingChangeHandler('native_dpi');
UI.addSettingChangeHandler('native_dpi', UI.updateNativeDpi);
UI.addSettingChangeHandler('shared');
UI.addSettingChangeHandler('view_only');
UI.addSettingChangeHandler('view_only', UI.updateViewOnly);
Expand Down Expand Up @@ -843,6 +846,7 @@ const UI = {

// Refresh UI elements from saved cookies
UI.updateSetting('encrypt');
UI.updateSetting('native_dpi');
UI.updateSetting('view_clip');
UI.updateSetting('resize');
UI.updateSetting('quality');
Expand Down Expand Up @@ -1044,6 +1048,7 @@ const UI = {
UI.rfb.addEventListener("clipboard", UI.clipboardReceive);
UI.rfb.addEventListener("bell", UI.bell);
UI.rfb.addEventListener("desktopname", UI.updateDesktopName);
UI.rfb.useNativeDpi = UI.getSetting('native_dpi');
UI.rfb.clipViewport = UI.getSetting('view_clip');
UI.rfb.scaleViewport = UI.getSetting('resize') === 'scale';
UI.rfb.resizeSession = UI.getSetting('resize') === 'remote';
Expand Down Expand Up @@ -1305,6 +1310,31 @@ const UI = {
/* ------^-------
* /VIEW CLIPPING
* ==============
* NATIVE DPI
Copy link
Member

@samhed samhed Jun 5, 2020

Choose a reason for hiding this comment

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

Please center this like the other section names, same goes for "/NATIVE DPI" below

* ------v------*/

// Update native DPI scaling for the viewport. If enabled, a pixel on the screen will
// correspond to one pixel on the server framebuffer.
// If disabled, the client area will be scaled according to the DPI scaling applied by the OS
// When this is disabled on a high-DPI monitor, a smaller framebuffer will be used and the
// result scaled up to fit the client area
Comment on lines +1316 to +1320
Copy link
Member

@samhed samhed Jun 5, 2020

Choose a reason for hiding this comment

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

Good explanation!

Could you keep the lines a bit shorter? Most of the code in UI attempts to stay within 80 columns I believe.

updateNativeDpi() {
if (!UI.rfb) return;

const scaling = UI.getSetting('resize') === 'scale';

if (scaling) {
// Can't control DPI if viewport is scaled to fit
UI.forceSetting('native_dpi', false);
} else {
UI.enableSetting('native_dpi');
}
UI.rfb.useNativeDpi = UI.getSetting('native_dpi');
},

/* ------^-------
* /NATIVE DPI
* ==============
* VIEWDRAG
* ------v------*/

Expand Down
23 changes: 21 additions & 2 deletions core/rfb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,16 @@ export default class RFB extends EventTargetMixin {
}
}

get useNativeDpi() { return this._nativeDpi; }
set useNativeDpi(nativeDpi) {
Copy link
Member

@CendioOssman CendioOssman Jun 4, 2020

Choose a reason for hiding this comment

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

Should this be useNativeDPI? What does JavaScript APIs usually do with abbreviations?

this._nativeDpi = nativeDpi;
this._updateScale();
this._updateClip();
if (this._resizeSession) {
this._requestRemoteResize();
}
}

get showDotCursor() { return this._showDotCursor; }
set showDotCursor(show) {
this._showDotCursor = show;
Expand Down Expand Up @@ -604,7 +614,14 @@ export default class RFB extends EventTargetMixin {

_updateScale() {
if (!this._scaleViewport) {
this._display.scale = 1.0;
if (this._nativeDpi) {
const dpr = window.devicePixelRatio || 1;
const scaledWidth = Math.floor(window.innerWidth * dpr);
// Compute the exact scale factor to match the floored width/height
this._display.scale = window.innerWidth / scaledWidth;
Copy link
Member

@CendioOssman CendioOssman Jun 4, 2020

Choose a reason for hiding this comment

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

I don't understand the purpose of this calculation. Why not just 1 / window.devicePixelRatio?

} else {
this._display.scale = 1;
}
} else {
const size = this._screenSize();
this._display.autoscale(size.w, size.h);
Expand All @@ -624,8 +641,10 @@ export default class RFB extends EventTargetMixin {
}

const size = this._screenSize();
size.w = Math.floor(size.w / this._display.scale);
size.h = Math.floor(size.h / this._display.scale);
Copy link
Member

@CendioOssman CendioOssman Jun 4, 2020

Choose a reason for hiding this comment

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

I'm not sure we want to tie this in to the display scaling right now. I would prefer if you just use window.devicePixelRatio and this._nativeDpi here.

RFB.messages.setDesktopSize(this._sock,
Math.floor(size.w), Math.floor(size.h),
size.w, size.h,
this._screenID, this._screenFlags);

Log.Debug('Requested new desktop size: ' +
Expand Down
3 changes: 3 additions & 0 deletions vnc.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ <h1 class="noVNC_logo" translate="no"><span>no</span><br>VNC</h1>
<label><input id="noVNC_setting_view_only" type="checkbox"> View Only</label>
</li>
<li><hr></li>
<li>
<label><input id="noVNC_setting_native_dpi" type="checkbox">Use native DPI</label>
Copy link
Member

@samhed samhed Jun 5, 2020

Choose a reason for hiding this comment

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

The other surrounding checkboxes have a space in front of text

</li>
<li>
<label><input id="noVNC_setting_view_clip" type="checkbox"> Clip to Window</label>
</li>
Expand Down