-
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
Add "touchpad mode" with pinch-zoom for mobile view. #1835
Conversation
- Add touchpad.svg to images. - Add #noVNC_touchpad_button button to #noVNC_mobile_buttons div. - Add/modify css rules so margin applies to mobile buttons, not the surrounding div.
This looks promising, thanks! I've not looked at things in detail yet, but there are two things I noticed so far:
|
Happy to contribute! This is an awesome project. Thanks for all your work on it.
Should I create a separate option for this in settings? If so, any preference on what to call it? "Free Scale" is the first term that comes to my mind.
That's totally up to you (and other maintainers). Since drag mode, default mode, and touchpad mode are mutually exclusive, my first thought was to put it here to easily see the relationship. |
Nice! I've found myself using this feature in both RealVNC and TeamViewer a lot, I love it. Personally, I switch a lot between the modes. I would find it annoying to have it hidden in the settings. |
If we don't want to populate the side bar too much, I see a few alternatives:
|
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.
Thank you for your patience.
We have now tested your changes and I must say the overall impression and feeling is very nice!
We have used a phone for testing, and the mapping between finger and mouse movements feels well calibrated. 🥇
We think this is a very impressive feature that will give our users a better
experience, especially if using small screens. 📱
Synaptics is, in our minds, the most well-known developer of trackpads. For reference, here's a list of “gestures” for the Synaptics driver on Linux:
https://man.archlinux.org/man/synaptics.4
However, we don't think we must support all the features they support. The most important thing is that your trackpad implementation doesn't conflict with theirs, since that's what most users expect from a trackpad.
Aside from testing your implementation, we also compared it with Microsoft's RDP Android app, and RealVNC's Android app:
-
In general, we think your implementation matches great with the “mouse mode” in Microsoft's RDP app. They both use the same gestures and actions. The feeling is very similar, that is an excellent thing.
There are two small differences when compared to Microsoft's RDP; Firstly, they have some inertia that makes the cursor stop softly after lifting the finger (it continues moving a little bit, slower and slower until coming to a complete stop). We're not sure whether we like this effect. Secondly, their center area is larger, the area where the cursor can move freely without moving the viewport.
Similar to noVNC, Microsoft's RDP also has a “touch mode”, which behaves like our standard mode. The gestures they use in the equivalent mode are listed here:
-
RealVNC only has this trackpad mode, no mode where touch positions are directly mapped to mouse movements. Your implementation matches their trackpad mode well in terms of available gestures and how they function. One major difference is that the cursor is always centered in RealVNC's variant. We like your implementation better.
RealVNC has the option of showing a “mouse button overlay”, see below screenshot.
While this might be useful in some cases, we don't think it is something we need
to include in the first version.
After testing the other apps, I noticed that Microsoft refers to their “standard mode” as “touch mode”, and their trackpad mode as “mouse mode”. I'm a bit worried that your “touchpad mode” could easily be confused with “touch mode”, which would be incorrect. What are your thoughts on renaming your new mode to “trackpad mode”?
Lastly, have you considered unit tests for your code?
@@ -553,6 +556,12 @@ html { | |||
:root:not(.noVNC_connected) #noVNC_mobile_buttons { | |||
display: none; | |||
} | |||
|
|||
#noVNC_mobile_buttons > .noVNC_button { |
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.
If you make this selector more specific, you can avoid the *:not(#noVNC_mobile_buttons)
above.
I think #noVNC_control_bar > .noVNC_scroll > #noVNC_mobile_buttons > .noVNC_button {
should work.
* @param { "normal" | "info" | "warn" | "warning" | "error" } statusType | ||
* @param {number} time | ||
* @returns | ||
*/ |
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.
This seems a bit out of place, we don't have these types of docstrings anywhere else.
@@ -1061,8 +1070,10 @@ const UI = { | |||
UI.rfb.qualityLevel = parseInt(UI.getSetting('quality')); | |||
UI.rfb.compressionLevel = parseInt(UI.getSetting('compression')); | |||
UI.rfb.showDotCursor = UI.getSetting('show_dot'); | |||
UI.rfb.touchpadMode = WebUtil.readSetting('touchpad_mode', 'false') === '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.
Doesn't UI.getSetting()
work for you?
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.
Should we default to touchpad_mode on very small screens?
}, | ||
|
||
updateViewDrag() { | ||
if (!UI.connected) return; | ||
|
||
const viewDragButton = document.getElementById('noVNC_view_drag_button'); | ||
|
||
if (UI.rfb.dragViewport) { | ||
UI.rfb.touchpadMode = 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.
Good that you remembered adding this!
if (!UI.rfb) return; | ||
|
||
UI.rfb.touchpadMode = !UI.rfb.touchpadMode; | ||
WebUtil.writeSetting('touchpad_mode', UI.rfb.touchpadMode); |
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.
UI.saveSetting()
?
this._sendMouse(cursorPos.x, cursorPos.y, 0x1); | ||
} | ||
break; | ||
} |
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 that I understand this piece of code? Could you try to explain it?
@@ -1301,13 +1376,21 @@ export default class RFB extends EventTargetMixin { | |||
break; | |||
case 'drag': | |||
case 'longpress': | |||
this._fakeMouseMove(ev, pos.x, pos.y); | |||
// In TouchpadMode, we want to move the cursor from its | |||
// current position, not to where the touch currently is. |
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.
These comments are helpful!
// required for MS Surface | ||
(navigator.maxTouchPoints > 0) || | ||
(navigator.msMaxTouchPoints > 0); | ||
} |
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.
So you've seen issues where this didn't update properly? Anyway, this should be a separate commit.
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.
@@ -79,6 +79,9 @@ <h1 class="noVNC_logo" translate="no"><span>no</span><br>VNC</h1> | |||
<div id="noVNC_mobile_buttons"> | |||
<input type="image" alt="Keyboard" src="app/images/keyboard.svg" | |||
id="noVNC_keyboard_button" class="noVNC_button" title="Show Keyboard"> | |||
|
|||
<input type="image" alt="Touchpad Mode" src="app/images/touchpad.svg" | |||
id="noVNC_touchpad_button" class="noVNC_button" title="Touchpad Mode"> |
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.
We also have some future ideas for changes to the view drag mode (the “hand”). With trackpad mode in place, it would not be as critical anymore. |
@bitbound are you interested in continuing working on this? |
I'm sorry. I forgot to update this PR. I took my project in another direction (WebRTC) and won't be using noVNC anymore. I won't have time to finish this. Of course, you or anyone else is free to take over and finish this if they want. Cheers! |
This PR adds a "touchpad mode" option for input when in mobile view. It includes pinch-zoom.
In touchpad mode, it will function similar to other remote control apps (RealVNC, TeamViewer, etc.). Touch movements will "push" the cursor in the same direction, and tap events will occur at the cursor's current location.
Long-press or two-tap will perform a right-click. Double-tap-and-drag will perform a click-and-drag with the left mouse button.
TouchpadMode.mp4