Added the abillity for vnc to use 9 mouse buttons instead of 7. #1711

manny33 · 2023-12-29T14:34:00Z

This adds the Xinput device buttons back and forward for the mouse.

Both Client and server message reader and writters as well as connections signal their support for the extra mouse buttons. The client signals the server through support of a psuedo encoding "pseudoEncodingExtendedMouseButtons". The server responds with a server message "msgTypeExtendedMouseSupport". Not sure if this is the best way to do this. After confirming both both client and server support the extended mouse button msg, the client starts sending new message with a u16 write instead of a u8. The button layout doesn't change to maintain support for clients that don't support more mouse buttons.

… adds the Xinput device buttons back and forward for the mouse. Both Client and server message reader and writters as well as connections signal their support for the extra mouse buttons. The client signals the server through support of a psuedo encoding "pseudoEncodingExtendedMouseButtons". The server responds with a server message "msgTypeExtendedMouseSupport". Not sure if this is the best way to do this. After confirming both both client and server support the extended mouse button msg, the client starts sending new message with a u16 write instead of a u8. The button layout doesn't change to maintain support for clients that don't support more mouse buttons.

CendioOssman

This looks promising. Nice work!

There are some things we should have another look at, though. See my comments.

CendioOssman · 2023-12-29T14:42:13Z

common/rfb/CMsgReader.cxx

@@ -118,6 +118,9 @@ bool CMsgReader::readMsg()
    case msgTypeEndOfContinuousUpdates:
      ret = readEndOfContinuousUpdates();
      break;
+    case msgTypeExtendedMouseSupport:
+      ret = readSupportExtendedMouseButton();
+      break;


We can avoid having to allocate both a pseudo-encoding and a message type by sending an empty rect instead. See how QEMU extended key events are handled.

CendioOssman · 2023-12-29T14:43:51Z

common/rfb/CMsgWriter.cxx

+  os->writeU16(p.x);
+  os->writeU16(p.y);
+  endMsg();
+}


This looks small enough that we can hopefully keep things in the same method? Like we do for key events.

for sure. I had it in different methods to lessen merge conflicts since i was just running this locally. it makes sense to make the main method handle both cases if it's going to be in master.

CendioOssman · 2023-12-29T15:27:13Z

common/rfb/SMsgReader.cxx

@@ -281,6 +284,16 @@ bool SMsgReader::readPointerEvent()
  return true;
 }

+bool SMsgReader::readPointerEvenExt()


A missing t here.

CendioOssman · 2023-12-29T15:30:08Z

common/rfb/encodings.h

@@ -36,6 +36,7 @@ namespace rfb {

  const int pseudoEncodingXCursor = -240;
  const int pseudoEncodingCursor = -239;
+  const int pseudoEncodingExtendedMouseButtons = -241;


These need to be allocated with IANA before we can merge things:

https://www.iana.org/form/protocol-assignment
https://www.iana.org/assignments/rfb/rfb.xml

I'd also like to see a PR for documenting this new extension:

https://github.com/rfbproto/rfbproto

Thanks, i had no idea what these numbers were haha.

CendioOssman · 2023-12-29T15:43:18Z

unix/xserver/hw/vnc/vncInput.c

@@ -50,7 +50,7 @@ extern const unsigned int code_map_qnum_to_xorgevdev_len;
 extern const unsigned short code_map_qnum_to_xorgkbd[];
 extern const unsigned int code_map_qnum_to_xorgkbd_len;

-#define BUTTONS 7
+#define BUTTONS 9


If we're extending things, we might as well try to support everything.

evdev (which is the reference) has 8 buttons defined currently. Adding the 4 scroll buttons puts it at 12 buttons.

If we look at the Xorg evdev driver, it allocates room for 16 buttons, plus the 4 scroll buttons. Which means that we need more than 16 bits. I'm not sure if there is support for any such mice, though?

Perhaps we should at least use the full 16 (12+4) buttons we've made room for?

yeah, if we're extending functionality it would be nice to fully extend it.

CendioOssman · 2023-12-29T15:46:00Z

unix/xserver/hw/vnc/vncInput.c

@@ -206,6 +206,9 @@ static int vncPointerProc(DeviceIntPtr pDevice, int onoff)
 		btn_labels[4] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_DOWN);
 		btn_labels[5] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_HWHEEL_LEFT);
 		btn_labels[6] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_HWHEEL_RIGHT);
+		btn_labels[7] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_BACK);
+		btn_labels[8] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_FORWARD);


Annoyingly enough, this is not what the evdev driver does. It marks these as "side" and "extra".

It's code is a bit annoying to decipher, but it matches what this code does:

https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/hw/xfree86/drivers/inputtest/xf86-input-inputtest.c?ref_type=heads#L272

We should do the same for compatibility.

CendioOssman · 2023-12-29T15:53:11Z

vncviewer/Viewport.cxx

+    if(Fl::event_buttons()&FL_BUTTON(4))
+        buttonMask |= 128;
+    if(Fl::event_buttons()&FL_BUTTON(5))
+        buttonMask |= 256;


Are you sure about these? FLTK follows X11's button numbering. So I would expect 7 and 8 here.

(this also means we should probably pass through every high button from FLTK, to support all extra buttons)

Yeah, i tested and use these values. I did find it odd that java and c had different button combos (the clients). I'll look into this again after extending all buttons.

I had a chance to look into Java's behavior in the context of implementing horizontal scroll wheel events. Java Buttons 4 through 7 are assigned differently depending on the O/S, because horizontal scroll wheel events are implemented differently in different O/S's. On macOS and Windows, horizontal scroll wheel events are implemented as vertical scroll wheel events with the Shift key held down. Since Java has a separate event subclass for scroll wheel events, it doesn't need to use Java Buttons 4 and 5 for scroll wheel events. Thus, on macOS and Windows, Java Buttons 4 and 5 are used for the front and back mouse buttons. On X11, however, horizontal scroll wheel events are implemented using X11 Buttons 6 and 7. Java/X11 still uses a separate event subclass for vertical scroll wheel events, but it uses Java Buttons 4 and 5 for horizontal scroll wheel events, which means that it uses Java Buttons 6 and 7 for the front and back mouse buttons.

CendioOssman · 2023-12-29T15:54:01Z

vncviewer/Viewport.cxx

+    if(Fl::event_buttons()&FL_BUTTON(4))
+        buttonMask |= 128;
+    if(Fl::event_buttons()&FL_BUTTON(5))
+        buttonMask |= 256;


Have you looked at Windows and macOS support? It seems FLTK doesn't natively support those, but we might be able to fetch what we need indirectly.

I've been testing from WSL and an Arch laptop i have. I need to set up my build chain for windows. I don't have a mac though.

manny33 · 2024-01-03T15:56:19Z

Update on this.

I've done all the changes requested in the PR. The handshake happens with an empty rect to reduce the need to for a specific Msg identifier. Xinput buttons are mapped to reflect xserver's implementation. Typos have been corrected.

The main issue now is that FLTK doesn't properly support more than 7 mice buttons. FL::event_buttons masks the event state with 0x7f000000. This drops bit 31, which coincidentally is the back mouse button in the internal state. You can get that bit with the unmasked state "Fl::event_state()&FL_BUTTON(8)". But i haven't found a way to find if the forward button is pressed. Either way, bit 31 being high for mouse back seems more of a happy accident than something we can depend on. I'd have to track down the internal state in fltk source.

I possibly could use the native message pump to handle only the back and forward button. The downside is that it would have to be implemented for every platform.

CendioOssman · 2024-01-04T13:42:56Z

It doesn't look like Fl::e_state is particularly useful here, unfortunately. It basically maps to the X11 state, which only tracks the first five buttons.

So I think we'll need to convert things to track mouse button state ourselves and look at Fl::e_keysym instead.

manny33 · 2024-01-04T15:30:30Z

That worked. Thanks!

Fl::event_button() maps back to 8, forward to 9. It seems to have the button order flipped versus xservers "init_button_labels"

//Fl::event_button is only good for FL_PUSH and FL_RELEASE
    if(event == FL_PUSH) {
      if (Fl::event_button() == 8) //Mouse Back
        buttonMask |= 1024;
      if (Fl::event_button() == 9) //Mouse Forward
        buttonMask |= 512;
    }

vs the extended vncInput.c;

btn_labels[0] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_LEFT);
btn_labels[1] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_MIDDLE);
btn_labels[2] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_RIGHT);
btn_labels[3] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_UP);
btn_labels[4] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_DOWN);
btn_labels[5] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_HWHEEL_LEFT);
btn_labels[6] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_HWHEEL_RIGHT);
btn_labels[7] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_SIDE);
btn_labels[8] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_EXTRA);
btn_labels[9] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_FORWARD);
btn_labels[10] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_BACK);

I'll start getting that rfb assignment and working on the PR documentations.

CendioOssman · 2024-01-05T08:38:00Z

If you push your updated code, we can have a look and test here as well.

…ons 8 and 9. Removed the use of the Client message for the psuedo encoding.

manny33 · 2024-01-05T14:30:06Z

unix/xserver/hw/vnc/vncInput.c

+   		btn_labels[8] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_EXTRA);
+    	btn_labels[9] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_FORWARD);
+    	btn_labels[10] = XIGetKnownProperty(BTN_LABEL_PROP_BTN_BACK);
+


I'll have to fix the indenting here. Not sure what happened.

manny33 · 2024-01-05T16:00:14Z

New code pushed.

CendioOssman · 2024-01-08T15:42:50Z

vncviewer/Viewport.cxx

+        buttonMask |= 1024;
+      if (Fl::event_button() == 9) //Mouse Forward
+        buttonMask |= 512;
+    }


This does not work for me here with Fedora 38. I get button 10 and 11 in the session, instead of the expected 8 and 9.

I feared this would be the case. What would be the solution here, read a config file for the button mapping?

That should not be necessary. Applications such as Chrome and Firefox do not have a config file for this, so it shouldn't be necessary for us.

Where did you get the values 1024 and 512?

the way the buttons are mapped from the input is a bit shift in vncPointerButtonAction. Back is the 10th button in the map so 1<<10.

Do you mean it's the one labeled BTN_LABEL_PROP_BTN_BACK? Unfortunately, that label is wrong. You'll need to compare with the events you get locally, since that is what the applications expect. Things such as xev should look identical locally and in the VNC session.

And no, we don't want to fix the labels. We need to be bug-for-bug compatible with a local system to avoid breaking applications.

In other words, the back and forward buttons will have the labels "side" and "extra". This is what we want, since this is what happens for a normal, local login.

CendioOssman · 2024-01-08T15:45:20Z

vncviewer/Viewport.cxx

+        buttonMask |= 1024;
+      if (Fl::event_button() == 9) //Mouse Forward
+        buttonMask |= 512;
+    }


I cannot hold down any of these buttons and move the mouse. It then immediately thinks I released the button.

You'll need to keep track of the button mask, unfortunately. FLTK will not provide a correct one for you.

CendioOssman · 2024-01-08T15:46:21Z

unix/xserver/hw/vnc/vncInput.c

@@ -50,7 +50,7 @@ extern const unsigned int code_map_qnum_to_xorgevdev_len;
 extern const unsigned short code_map_qnum_to_xorgkbd[];
 extern const unsigned int code_map_qnum_to_xorgkbd_len;

-#define BUTTONS 7
+#define BUTTONS 11


Let's use all the 16 buttons we have, so we can support devices with many buttons.

what would the button label property be for the extra buttons? just go down the defines in xserver-properties?

#define BTN_LABEL_PROP_BTN_TASK "Button Task" #define BTN_LABEL_PROP_BTN_TRIGGER "Button Trigger" #define BTN_LABEL_PROP_BTN_THUMB "Button Thumb" #define BTN_LABEL_PROP_BTN_THUMB2 "Button Thumb2" #define BTN_LABEL_PROP_BTN_TOP "Button Top" #define BTN_LABEL_PROP_BTN_TOP2 "Button Top2" #define BTN_LABEL_PROP_BTN_PINKIE "Button Pinkie" #define BTN_LABEL_PROP_BTN_BASE "Button Base" #define BTN_LABEL_PROP_BTN_BASE2 "Button Base2"

The standard driver just uses BTN_LABEL_PROP_BTN_UNKNOWN for the extra ones, so that should be good enough for us as well.

CendioOssman · 2024-01-09T14:38:48Z

I got my hands on a Logitech G604, which has 6 buttons on the side. Unfortunately, this thing is a bit magical and generates fake keyboard keys instead of mouse buttons for the extra ones. So it ended up not being particularly useful here. :/

CendioOssman · 2024-03-26T09:05:40Z

How are things progressing here, @manny33? It would be great to get this merged.

manny33 · 2024-03-26T14:31:40Z

@CendioOssman , pardon i got a bit busy with some holiday travel and work deliverables. I was planning on looking into this again come next week.

CendioOssman · 2024-04-09T06:19:03Z

Have you had a chance to look further at this, @manny33?

CendioHalim · 2024-05-28T15:10:44Z

Hey @manny33, I really like the work you've done and would really like this feature to be integrated into TigerVNC. I've continued where you left off and managed to get a prototype that works on Linux, macOS and Windows.

You are welcome to contribute and follow my work on my fork: https://github.com/CendioHalim/tigervnc/tree/mouse-button-support

manny33 · 2024-05-28T15:36:52Z

@CendioOssman Sorry for the delay. I work in the game industry and have been swamped.

What's missing here is to figure out how the clients can identify which button is mouse forward and mouse backward in their hardware. Once that's identified we can translate that to mouse buttons on the server.

Once that's working we can request a message id from IANA with the accompanying documentation.

@CendioHalim I'll check out your branch and get back to you.

CendioHalim · 2024-05-30T15:00:26Z

@manny33 I was wondering if we really need msgTypePointerEventExt?

I figured since there is only support for 7 buttons right now, and we send the button mask using 8 bits, could we not use that last bit to indicate that the message contains 8 more bits? I haven't got around to check if this would work properly, do you think it could work?

If you'd like, I could send a proposal to IANA for the pseudo-encoding, since we need it in any case. The msgType can always be added at a later time.

manny33 · 2024-05-30T23:29:01Z

@CendioHalim ahh i see, UTF-8 like tricks there. It should work for tiger VNC, but i don't know what every server would do if they found the high bit set, they may have some assert on it and cause a crash.

CendioHalim · 2024-05-31T07:27:35Z

@manny33 I figured that we only check the high bit (+ the additional 8 bits) if the pseudo-encoding is supported by both the client and server. That should hopefully preserve compatibility with other implementations without the need for a new msgType.

manny33 · 2024-05-31T14:21:08Z

@CendioHalim I see. That would be a nice trick. Once we handshake on the pseudo encodings, the client and server can both agree what the high bit means. We can then decode the message properly on the server. I'm all for it. It would simplify the code.

CendioHalim · 2024-05-31T14:44:14Z

@manny33

My idea is the following:

With the buttonMask we get on the client, we keep the lower 7 bits as they are, and take the upper 9 bits and shift them 1 step to the left. This frees the 8th bit to be used only to indicate the message type, and the upper 7 bits can be used for different buttons.

On the server side, we can decode the message in two ways:

We start by reading 8 bits and look at the MSB to detect message type. If the MSB is 1, we read 8 more bits, shift them 8 steps to the left, and combine it with the first 8 bits.
We call is->setRestorePoint() before reading 16 bits directly and look at the 8th bit. If it is 1, we can go ahead and decode the message. If the 8th bit is 0, we call is->gotoRestorePoint() and read 8 bits as usual.

I haven't had the time to get this working fully yet, but will probably continue after the weekend. Do you think this is a sane approach? I think I'm more in favour of the 2nd alternative.

Here's a small snippet I'm playing with (client side only):

void CMsgWriter::writePointerEvent(const Point& pos, int buttonMask)
{
  Point p(pos);
  if (p.x < 0) p.x = 0;
  if (p.y < 0) p.y = 0;
  if (p.x >= server->width()) p.x = server->width() - 1;
  if (p.y >= server->height()) p.y = server->height() - 1;

  startMsg(msgTypePointerEvent);
  if (server->supportsExtendedMouseButtons && buttonMask >= 1 << 7) {
    int higherBits;
    int lowerBits;
    int extendedButtonMask;

    // Clear the lower 7 bits, and set the 8th bit to indicate extended
    // button event. The upper 8 bits are used to store the extended
    // button mask and are shifted 1 bit to the left.
    higherBits = buttonMask & ~127;
    higherBits <<= 1;
    higherBits |= 1 << 7;

    // Keep the lower 7 bits
    lowerBits = buttonMask & 127;

    extendedButtonMask = higherBits | lowerBits;

    os->writeU16(extendedButtonMask);
  } else {
    os->writeU8(buttonMask);
  }
    os->writeU16(p.x);
    os->writeU16(p.y);
    endMsg();
}

It relies heavily on other VNC implementations not using the 8th bit at all, so we would have to have a look and test with different clients/servers. I don't think this is a good idea if it doesn't play well with other implementations.

manny33 · 2024-05-31T15:07:36Z

@CendioHalim
I didn't know about the Set / goto restore pointer. Those are neat for the reader. I think the approach would work. We also don't need to shift the high bits back, as we can just declare bits 9 and 10 as the back / forward button on the server.

I am worried about the 8th bit compatibility like you said. That bit lives in limbo, and relying on it may raise compatibility issues. As long as we don't set it when the client isn't expecting it, it should be fine. Compatibility issues may arise, but at that point I suppose we'd need to talk about standardizing that bit or the extended mouse button support.

The new message id approach does make it a clean break though. I don't know enough about the VNC community to make a clear judgement on it. There could be considerations with other client and servers I'm not aware of.

CendioHalim · 2024-06-03T07:09:50Z

@manny33
My concern with not shifting the bits back is what to do with the 8th index in btn_labels. I think it's safer to add some complexity on the protocol side rather than doing something in vncInput.c if the 8th bit happened to be set.

I'm also not sure about the new message type and how this approach plays with other implementations, but I feel like if we can design the protocol to avoid the need for a new message type it's worth it.

My hope is to have a version of the protocol to test with this week.

CendioHalim · 2024-06-03T12:24:27Z

manny33 · 2024-06-03T12:48:16Z

@CendioHalim that would be great. Thanks again for taking point with this. I'm not very experienced with writing these proposals as I'm just a hobbyist.

Neustradamus · 2024-06-26T02:36:05Z

To follow this PR :)

CendioHalim · 2024-07-01T08:36:45Z

The pseudo-encoding has been registered with IANA and the assignment is -316 (the previous value was already allocated).

While writing the draft for rfbproto, I realized that the implementation might be a bit overcomplicated? If we decide that the server and client completely switch to 16 bit buttonmasks, after both are sure that the other supports the encoding, we can completely skip the 8th bit marker?

I tested this (CendioHalim@8d0afe9) and it worked, combining new/old client/servers. The implementation is much simpler, but I wonder if there are some unforeseen consequences I've missed?

Another thing I'm thinking about is what to do with the remaining 7 bits? I had a look at how QT and GTK handle mouse buttons, and there doesn't seem to be a clear consensus.

Here is my draft CendioHalim/rfbproto@b00f2d8, I'll make a PR to rfbproto soon.

CendioHalim · 2024-07-01T08:46:58Z

I also had a look at some game engines: Godot, Unity and Unreal, and they all looked like they only supported the normal 7 buttons, plus the forward and back buttons.

CendioHalim · 2024-07-02T09:20:28Z

While writing the draft for rfbproto, I realized that the implementation might be a bit overcomplicated? If we decide that the server and client completely switch to 16 bit buttonmasks, after both are sure that the other supports the encoding, we can completely skip the 8th bit marker?

I realized that this creates a race condition, where the server will start accepting PointerEvents as having 2b buttonMasks as soon as SMsgHandler::setEncodings() is called, before responding with SMsgWriter::writeExtendedMouseButtonRect() The client will still send 1b buttonMasks until the response rect is sent back, causing a crash.

I've reverted CendioHalim@8d0afe9, going back to the previous implementation. The rfbproto draft will be updated soon.

CendioHalim · 2024-07-03T11:30:38Z

rfbproto PR: Document ExtendedMouseButtons pseudo-encoding rfbproto/rfbproto#57

dcommander · 2024-07-05T20:28:41Z

@CendioHalim IMHO a better approach would be to use a different message type, much like the QEMU Extended Key Event message does. Other VNC server implementations that still use C rather than C++ (such as LibVNCServer or TurboVNC) use fixed structures for different message types, e.g.:

typedef struct _rfbPointerEventMsg {
    CARD8 type;                 /* always rfbPointerEvent */
    CARD8 buttonMask;           /* bits 0-7 are buttons 1-8, 0=up, 1=down */
    CARD16 x;
    CARD16 y;
} rfbPointerEventMsg;

#define sz_rfbPointerEventMsg 6

(That is what TightVNC and RealVNC v3 and prior did as well.) Thus, the message can be read in one throw, rather than with the separate reads that TigerVNC and RealVNC v4 and later do. It's not a deal-breaker. It's more about being consistent with how RFB extensions were traditionally implemented, which would make the extension friendlier to other implementations that still conform that that tradition.

dcommander · 2024-07-05T21:03:48Z

@CendioHalim Another possibility, if you didn't want to get a new message type assigned to the extension, would be to send the extended button info after the base RFB pointer message. That would allow more traditional VNC implementations to continue using the rfbPointerEventMsg struct in rfbproto.h. They could simply analyze the buttonMask field after the struct is read and determine whether they should read an additional byte.

dcommander · 2024-07-05T21:09:22Z

@CendioHalim Also, if you send buttons 8-15 as a separate byte after the base message, then the meaning of Bits 0-6 in the base message does not change based on whether the extension is in use. Implementations would always take the pressed state of Buttons 1-7 from those bits. If the pseudo-encoding is enabled, then implementations would use Bit 7 to determine whether to read the additional byte, then they would take the pressed state of Buttons 8-15 from that additional byte. That seems like it would be easier to document as well.

I suppose that, if you wanted to future-proof it, you could have the extension use an additional unsigned short rather than an additional byte, which would allow it to support up to 23 total buttons.

CendioHalim · 2024-07-08T08:53:09Z

@CendioHalim IMHO a better approach would be to use a different message type, much like the QEMU Extended Key Event message does.

I think it's better to avoid a new message type if it is possible, which it seems to be right now. They are a limited resource.

@CendioHalim Another possibility, if you didn't want to get a new message type assigned to the extension, would be to send the extended button info after the base RFB pointer message. That would allow more traditional VNC implementations to continue using the rfbPointerEventMsg struct in rfbproto.h. They could simply analyze the buttonMask field after the struct is read and determine whether they should read an additional byte.

Sending the extra byte after the base message seems like a good thing to do, especially if, as you say, it would play better with other implementations.

@CendioHalim Also, if you send buttons 8-15 as a separate byte after the base message, then the meaning of Bits 0-6 in the base message does not change based on whether the extension is in use. Implementations would always take the pressed state of Buttons 1-7 from those bits. If the pseudo-encoding is enabled, then implementations would use Bit 7 to determine whether to read the additional byte, then they would take the pressed state of Buttons 8-15 from that additional byte. That seems like it would be easier to document as well.

This is pretty much how the protocol already is designed as of now.

I suppose that, if you wanted to future-proof it, you could have the extension use an additional unsigned short rather than an additional byte, which would allow it to support up to 23 total buttons.

I was thinking this as well, but I'm not sure what to do with the additional bytes. Future-proofing is not a bad idea, you could even let the server respond with how many additional bytes are supported in the response rect if you want to be super flexible. I'm just not sure what this means for the protocol, as we'd have bit 0-9 defined, and 10-31 reserved for future use?

dcommander · 2024-07-08T15:10:15Z

I think it's better to avoid a new message type if it is possible, which it seems to be right now. They are a limited resource.

Fair enough.

I suppose that, if you wanted to future-proof it, you could have the extension use an additional unsigned short rather than an additional byte, which would allow it to support up to 23 total buttons.

I was thinking this as well, but I'm not sure what to do with the additional bytes. Future-proofing is not a bad idea, you could even let the server respond with how many additional bytes are supported in the response rect if you want to be super flexible. I'm just not sure what this means for the protocol, as we'd have bit 0-9 defined, and 10-31 reserved for future use?

Referring to #1711 (comment), there are potentially 16 mouse buttons that need to be handled, but the current implementation only handles 15. It would be straightforward to use an unsigned short rather than a byte for Buttons 8-N, which would give us room for 16 additional buttons on top of the 7-8 that can already be handled by the base pointer event message. However, the counterarguments are:

Using an unsigned short would increase the overall pointer event message size by 33% rather than 17%.
The message size increase would apply to any implementation that needs more than 7 buttons (but fortunately only for messages that use the 8th or higher-numbered buttons.)
Few if any implementations would ever need more than 15 buttons.
Implementations that need more than 15 buttons could always use the GII extension to support an arbitrary number of buttons. (The counter-counter argument is that GII is such a complex and fine-grained extension that, effectively, every implementation of it defines a new protocol that won't be compatible with other GII implementations unless you intentionally make it so, whereas this mouse button extension is dead simple and has no compatibility wiggle room.)

Specifying the number of additional bytes would require either sacrificing more of the mask bits in the base pointer event message or sending yet another byte after the base message to indicate the number of bytes that follow. I may be paranoid to care so much about the message size, but since pointer events are sent so frequently and they are normally only 6 bytes in length, it seems like a good idea to avoid sending even one additional byte unless absolutely necessary. I like CendioHalim@275670c as it is currently implemented (minus the one comment I made on it), because it allows up to 7 buttons (enough to accommodate both vertical and horizontal scroll wheel events) without increasing the message size. The argument in favor of extending it to handle 16-23 buttons is weak, and the argument in favor of extending it to handle 24+ buttons is even weaker.

I would be OK with simply using an unsigned short mask for Buttons 8-23, as opposed to a byte mask for Buttons 8-15, if you feel that supporting more than 15 buttons may ever be necessary. In most cases, the additional unsigned short would only be sent if the forward and back buttons are activated, which is rare, so the incremental message size increase wouldn't apply to the vast majority of pointer events. However, I would also be OK with using a byte mask for Buttons 8-15, as currently implemented in CendioHalim@275670c, and foregoing support for more than 15 buttons.

CendioHalim · 2024-07-09T06:42:06Z

Regarding #1711 (comment):

From what I've tested, some apps that support forward and back mouse buttons listen on both
[BTN_LABEL_PROP_BTN_SIDE, BTN_LABEL_PROP_BTN_FORWARD] for forward, and
[BTN_LABEL_PROP_BTN_EXTRA, BTN_LABEL_PROP_BTN_BACK] for back.

The label usage is inconsistent across X applications. For example, I tested a Logitech G400s, which has 7 buttons (left, middle, right, forward, back, and three extra). Two of the extra buttons are to change the DPI and don't generate any button events. The third button, which is "button 10" / BTN_LABEL_PROP_BTN_FORWARD with xinput --test <id>, navigates forward in Firefox and Chrome, but does nothing in Nautilus file explorer.

I've tested three different mice (2 logitech, 1 microsoft) with forward/back buttons, and they all map SIDE/EXTRA to forward/back. I'm assuming most X11 applications use these labels for forward/back.

Since the protocol will only specify which bits correspond to a forward/back button, does it not mean that we have to pick which label we use for forward/back in Xvnc? Otherwise, we'd need additional bits for full support, and that would only work with X11 VNC clients that also support these labels? I would be OK with only using SIDE/EXTRA in Xvnc and only allocating 2 bits for these.

Regarding 1 vs 2 extra bytes, I don't really know what's best. In my mind, forward/back buttons will satisfy 99 % of users. At the same time, clients can choose to only send an extended mask when necessary, and the total data increase for a session would be negligible.

CendioOssman

I guess this is not updated to the latest specification?

CendioOssman · 2024-08-06T11:42:08Z

common/rfb/CConnection.cxx

@@ -860,5 +860,7 @@ void CConnection::updateEncodings()
  if (qualityLevel >= 0 && qualityLevel <= 9)
      encodings.push_back(pseudoEncodingQualityLevel0 + qualityLevel);

+  encodings.push_back(pseudoEncodingExtendedMouseButtons);


Why not with all the other stuff further up?

CendioOssman · 2024-08-06T11:42:50Z

common/rfb/CMsgHandler.h

@@ -58,6 +58,7 @@ namespace rfb {
    virtual void fence(uint32_t flags, unsigned len, const uint8_t data[]);
    virtual void endOfContinuousUpdates();
    virtual void supportsQEMUKeyEvent();
+    virtual void supportExtendedMouseButtons();


rudironsonijr · 2024-08-15T15:31:35Z

@mdevaev is it possible to implement this functionality in PiKVM's vnc server?

mdevaev · 2024-08-17T19:44:38Z

@rudironsonijr PiKVM emulates only 8 buttons now. I can add them when the patch will bi applied to TigerVNC upstream.

CendioHalim · 2024-08-28T13:45:14Z

I've created a new PR with continuation of the work done in this PR:

Add support for forward and back mouse buttons #1826

CendioOssman · 2024-08-30T14:44:08Z

Okay. Let's continue the work there then.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit adds support for the pseudo-encoding ExtendedMouseButtons in Xvnc, which makes it possible to use to use the back/forward mouse buttons. The back/forward buttons are mapped to the X11 button labels BTN_LABEL_PROP_BTN_SIDE and BTN_LABEL_PROP_BTN_EXTRA respectively. These seems to be used more widely both by mice with extra side buttons, as well as X applications to incidate a back/forward navigation. There are two other labels: BTN_LABEL_PROP_BTN_BACK, BTN_LABEL_PROP_BTN_EXTRA, which some applications listen to as well. But from what I've seen, the applications that listen to these two labels also listen to SIDE/EXTRA as well.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit implements the pseudo-encoding ExtendedMouseButtons which makes it possible to use the back/forward mouse buttons. With this change, we have to keep track of the mouse button state ourselves, as FLTK does not keep track of button states other than the three standard buttons (LMB/MMB/RMB). Unfortunately, there is a bug in FLTK where don't get enough information in certain edge cases to keep a consistent state. There is a workaround in this commit for the three standard buttons, but not for the back/forward buttons.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit adds support for the pseudo-encoding ExtendedMouseButtons in Xvnc, which makes it possible to use to use the back/forward mouse buttons. The back/forward buttons are mapped to the X11 button labels BTN_LABEL_PROP_BTN_SIDE and BTN_LABEL_PROP_BTN_EXTRA respectively. These seems to be used more widely both by mice with extra side buttons, as well as X applications to incidate a back/forward navigation. There are two other labels: BTN_LABEL_PROP_BTN_BACK, BTN_LABEL_PROP_BTN_EXTRA, which some applications listen to as well. But from what I've seen, the applications that listen to these two labels also listen to SIDE/EXTRA as well.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit implements the pseudo-encoding ExtendedMouseButtons which makes it possible to use the back/forward mouse buttons. With this change, we have to keep track of the mouse button state ourselves, as FLTK does not keep track of button states other than the three standard buttons (LMB/MMB/RMB). Unfortunately, there is a bug in FLTK where don't get enough information in certain edge cases to keep a consistent state. There is a workaround in this commit for the three standard buttons, but not for the back/forward buttons.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit adds support for the pseudo-encoding ExtendedMouseButtons in Xvnc, which makes it possible to use to use the back/forward mouse buttons. The back/forward buttons are mapped to the X11 button labels BTN_LABEL_PROP_BTN_SIDE and BTN_LABEL_PROP_BTN_EXTRA respectively. These seems to be used more widely both by mice with extra side buttons, as well as X applications to incidate a back/forward navigation. There are two other labels: BTN_LABEL_PROP_BTN_BACK, BTN_LABEL_PROP_BTN_EXTRA, which some applications listen to as well. But from what I've seen, the applications that listen to these two labels also listen to SIDE/EXTRA as well.

This commit contains work originally done by manny33: * TigerVNC#1711 This commit implements the pseudo-encoding ExtendedMouseButtons which makes it possible to use the back/forward mouse buttons. With this change, we have to keep track of the mouse button state ourselves, as FLTK does not keep track of button states other than the three standard buttons (LMB/MMB/RMB). Unfortunately, there is a bug in FLTK where don't get enough information in certain edge cases to keep a consistent state. There is a workaround in this commit for the three standard buttons, but not for the back/forward buttons.

CendioOssman requested changes Dec 29, 2023

View reviewed changes

Fixed recommended issues. Added support on native FLTK for mouse butt…

1105449

…ons 8 and 9. Removed the use of the Client message for the psuedo encoding.

manny33 commented Jan 5, 2024

View reviewed changes

Fixed Some indentation that was off in vncinput.c

14a6d25

CendioOssman requested changes Jan 8, 2024

View reviewed changes

CendioHalim mentioned this pull request Jul 3, 2024

Document ExtendedMouseButtons pseudo-encoding rfbproto/rfbproto#57

Merged

CendioOssman requested changes Aug 6, 2024

View reviewed changes

CendioHalim mentioned this pull request Aug 28, 2024

Add support for forward and back mouse buttons #1826

Merged

CendioOssman closed this Aug 30, 2024

Added the abillity for vnc to use 9 mouse buttons instead of 7. #1711

Added the abillity for vnc to use 9 mouse buttons instead of 7. #1711

Conversation

manny33 commented Dec 29, 2023

CendioOssman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manny33 commented Jan 3, 2024

CendioOssman commented Jan 4, 2024

manny33 commented Jan 4, 2024

CendioOssman commented Jan 5, 2024

Choose a reason for hiding this comment

manny33 commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CendioOssman commented Jan 9, 2024

CendioOssman commented Mar 26, 2024

manny33 commented Mar 26, 2024

CendioOssman commented Apr 9, 2024

CendioHalim commented May 28, 2024

manny33 commented May 28, 2024

CendioHalim commented May 30, 2024

manny33 commented May 30, 2024

CendioHalim commented May 31, 2024

manny33 commented May 31, 2024

CendioHalim commented May 31, 2024

manny33 commented May 31, 2024

CendioHalim commented Jun 3, 2024 • edited Loading

CendioHalim commented Jun 3, 2024 • edited Loading

manny33 commented Jun 3, 2024

Neustradamus commented Jun 26, 2024

CendioHalim commented Jul 1, 2024

CendioHalim commented Jul 1, 2024

CendioHalim commented Jul 2, 2024 • edited Loading

CendioHalim commented Jul 3, 2024

dcommander commented Jul 5, 2024

dcommander commented Jul 5, 2024

dcommander commented Jul 5, 2024 • edited Loading

CendioHalim commented Jul 8, 2024

dcommander commented Jul 8, 2024 • edited Loading

CendioHalim commented Jul 9, 2024

CendioOssman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudironsonijr commented Aug 15, 2024

mdevaev commented Aug 17, 2024

CendioHalim commented Aug 28, 2024

CendioOssman commented Aug 30, 2024

CendioHalim commented Jun 3, 2024 •

edited

Loading

CendioHalim commented Jun 3, 2024 •

edited

Loading

CendioHalim commented Jul 2, 2024 •

edited

Loading

dcommander commented Jul 5, 2024 •

edited

Loading

dcommander commented Jul 8, 2024 •

edited

Loading