search

LibVNC / libvncserver

LibVNCServer/LibVNCClient are cross-platform C libraries that allow you to easily implement VNC server or client functionality in your program.
GNU General Public License v2.0
1.08k stars 483 forks source link

add libvncclient extended clipboard pseudo-encoding support #545

Closed wuhanck closed 1 year ago

wuhanck commented 1 year ago

add libvncclient support for notify and text only. although it is not full support, it is working for utf-8 clipboard.

bk138 commented 1 year ago

Thanks for the contribution! Please document in the code

wuhanck commented 1 year ago

extended clipboard support 0 | text 1 | rtf 2 | html 3 | dib 4 | files and with combination of 24 | cap 25 | request 26 | peek 27 | notify 28 | provide It is a big feature. But in reality, at most cases(if it is not all), client just need to support text with notify/provide to get utf-8 clipboard working.

wuhanck commented 1 year ago

extended clipboard support need zlib. It is possible to use libvncclient without zlib? Or please help me to protect the code by some Macro.

bk138 commented 1 year ago

extended clipboard support need zlib. It is possible to use libvncclient without zlib? Or please help me to protect the code by some Macro.

Yes, I added some hints for you in the review comments. Thanks for the contribution, I think with some more work we can get this in!

bk138 commented 1 year ago

@wuhanck I think some more work is required:

wuhanck commented 1 year ago

@bk138. clipboardCap is not a bool. It is a cap bitset....Maybe current name is descriptive. useclipboardcap is confusing. I test badc53a, it works for old way(before indicate extend clipboard) I add "cl->appData.clipboardCap = rfbExtendedClipboard_Text;" before calling rfbInitClient. here is my log. 22/01/2023 09:44:58 Least significant byte first in each pixel. 22/01/2023 09:44:58 TRUE colour: max red 255 green 255 blue 255, shift red 0 green 8 blue 16 22/01/2023 09:44:58 sending clipboard text 'cl->appData.clipboardCap = rfbExtendedClipboard_Text;' 22/01/2023 09:44:58 ignore SDL event: 0x302 22/01/2023 09:44:58 rfbClientProcessExtServerCutText. default caps. 22/01/2023 09:44:58 rfbClientProcessExtServerCutText. default caps. 22/01/2023 09:44:58 client2server supported messages (bit flags) 22/01/2023 09:44:58 00: 00ff 0081 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 08: 0000 0000 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 10: 0000 0000 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 18: 0000 0000 0000 0000 - 0000 0000 0000 0008 22/01/2023 09:44:58 server2client supported messages (bit flags) 22/01/2023 09:44:58 00: 001f 0080 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 08: 0000 0000 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 10: 0000 0000 0000 0000 - 0000 0000 0000 0000 22/01/2023 09:44:58 18: 0000 0000 0000 0000 - 0000 0000 0000 0000 After that, server handle extend clipboard meet some error. maybe some digging need for that. It is not client crash, it is that server cannot handle message and close the connection.

if we want to ignore and fallback to old sytle latin-1, we need the logic after 80aacdc. we need tell the difference of server support or not extend clipboard cap and let the client to choose how to handle it. so we need add clipboardenabledcap. (it is also a bitset of cap, not a boolean) (if we just block the old path, we dont need this logic)

wuhanck commented 1 year ago

@bk138 it is simple and solid to just block old path and let the upper layer to setup corret conf. if we want to be flexible, current logic is good to handle almost all cases of old/new server. if a server had indicate that it support extended clipboardcap(with text/provide), but it use the old way to send text after that, we just fallback and not handle it nicely. (it is a rare and strange behavior) We need to change the runtime api, for example add new callback to handle this. But it changes too much. let us just stay here with limitations.

wuhanck commented 1 year ago

@bk138 I test sdlvncviewer with tigervnc standalone server. Although the ui is not correct working, the clipboard part is working use the extended clipboard. examples/server/client all not ready for extend clipboard, maybe it is not suitable to check with each other. it is better to use x11vnc/tigervnc server to check

bk138 commented 1 year ago

@wuhanck I now understood your point about clipboardCap. If you want to get this merged, please

wuhanck commented 1 year ago

OK. I refine the patch. For we want be smart at clipboard, we add clipboardEnabledCap for upper to check whether vnc-server support our cap. For upper who rely on correct configuration, it still can set clipboardCap to rfbExtendedClipboard_Text, and everything will be OK. For upper who want smart, it set clipboardCap to rfbExtendedClipboard_Text and then in callback to check whether text is supproted by server, if supported, it can use utf-8 to/from os system, if not supported, it can use latin-1 to/from os system. There is one left issue, if server annouced that it support text, but later, it still send latin-1 using old way, for now we log and ignore it, for we dont know how to handle it and this is not a rational behavior(if server send text clipboard cap, it may better use utf8 text). And we let it be.

Refine the patch for zlib check. Add comment for clipboardCap/clipboardEnabledCap. Add extra byte for some impl to flush data (maybe they dont check Z_STREAM_END), now libvncserver works with libvncclient ( we dont touch libvncserver)

wuhanck commented 1 year ago

@bk138 some commits after f135856fc in libvncserver:master break things. commit bba8f57df in wuhanck:master works. Please check/review based on bba8f57df.

pdlan commented 1 year ago

Please note that according to rfbproto, it is still allowed to use a positive value of the length for the clipboard message (i.e., still use Latin-1) even if the Extended Clipboard Pseudo Encoding is enabled. So I would suggest handling this case.

wuhanck commented 1 year ago

Please note that according to rfbproto, it is still allowed to use a positive value of the length for the clipboard message (i.e., still use Latin-1) even if the Extended Clipboard Pseudo Encoding is enabled. So I would suggest handling this case.

I know server can do this. It is designed to let it improve in future. Our choice is to let current client to support the utf8 easily. For client that already support utf8 to/from os, if it choose to rely on conf, it only add one or two lines to support. For client that want smart, it can check clipboardenabledcap to use latin-1/utf-8 automatically.

The limitation is not global. It is when a server announce clipboardcap on a session, then it use old way to send on this session. As coder, a server can support utf8 and on a session it annouce the cap, it will change the state to use new way, that is nature. And it send some in old way and send some in new way, it is not easy to code... Still, let log that behavior and ignore it for now. In future, we can add new callback, for example add xxx_fallback for client to handle this issue. But it will break more things, this patch already break some. let us first make this patch working.

as you known, we still lack lot of features which extended clipboard pseudo encoding support.... we currently only want to talk in text using utf8 and cost as little as we can. we need let things go.

bk138 commented 1 year ago

@wuhanck I saw you closed this PR and emptied the branch. I deem this to be a bit sad, as the contribution was almost ready to go in. I can understand that code review and rework can be frustrating at times, but id needs to be done for the sake of quality of the product. As this is all about open-source and voluntary contributions, your decision is OK with me.

I just want to state what I deem mergable (and the PR was close to this TBH):

So 3 of 4 ticked in my opinion.

wuhanck commented 1 year ago

Sorry, I am clean repo from github.com… I will restore and reopen a pull request. Sorry about that. Please wait. I am working on that now.

从 Windows 版邮件https://go.microsoft.com/fwlink/?LinkId=550986发送

发件人: Christian @.> 发送时间: 2023年1月24日 22:31 收件人: @.> 抄送: @.>; @.> 主题: Re: [LibVNC/libvncserver] add libvncclient extended clipboard pseudo-encoding support (PR #545)

@wuhanckhttps://github.com/wuhanck I saw you closed this PR and emptied the branch. I deem this to be a bit sad, as the contribution was almost ready to go in. I can understand that code review and rework can be frustrating at times, but id needs to be done for the sake of quality of the product. As this is all about open-source and voluntary contributions, your decision is OK with me.

I just want to state what I deem mergable (and the PR was close to this TBH):

So 3 of 4 ticked in my opinion.

— Reply to this email directly, view it on GitHubhttps://github.com/LibVNC/libvncserver/pull/545#issuecomment-1402043963, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAQSUC77HWKTK6W5OI2VQNTWT7RSVANCNFSM6AAAAAATT4TVXI. You are receiving this because you were mentioned.Message ID: @.***>

wuhanck commented 1 year ago

I reopen the pull request in a new branch. I add support for fallback for some guy seems need it. Libvncserver don’t check Z_STREAM_END，so I add extra byte to make it happy. It would harm others as far as I can see.

And I found In the master, after commit f135856fc3, some commit break things. Maybe bug or api broken, I don’t know. Please recheck them.

从 Windows 版邮件https://go.microsoft.com/fwlink/?LinkId=550986发送

发件人: Christian @.> 发送时间: 2023年1月24日 22:31 收件人: @.> 抄送: @.>; @.> 主题: Re: [LibVNC/libvncserver] add libvncclient extended clipboard pseudo-encoding support (PR #545)

@wuhanckhttps://github.com/wuhanck I saw you closed this PR and emptied the branch. I deem this to be a bit sad, as the contribution was almost ready to go in. I can understand that code review and rework can be frustrating at times, but id needs to be done for the sake of quality of the product. As this is all about open-source and voluntary contributions, your decision is OK with me.

I just want to state what I deem mergable (and the PR was close to this TBH):

So 3 of 4 ticked in my opinion.

— Reply to this email directly, view it on GitHubhttps://github.com/LibVNC/libvncserver/pull/545#issuecomment-1402043963, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAQSUC77HWKTK6W5OI2VQNTWT7RSVANCNFSM6AAAAAATT4TVXI. You are receiving this because you were mentioned.Message ID: @.***>

wuhanck commented 1 year ago

从 Windows 版邮件https://go.microsoft.com/fwlink/?LinkId=550986发送

发件人: Christian @.> 发送时间: 2023年1月24日 22:31 收件人: @.> 抄送: @.>; @.> 主题: Re: [LibVNC/libvncserver] add libvncclient extended clipboard pseudo-encoding support (PR #545)

@wuhanckhttps://github.com/wuhanck I saw you closed this PR and emptied the branch. I deem this to be a bit sad, as the contribution was almost ready to go in. I can understand that code review and rework can be frustrating at times, but id needs to be done for the sake of quality of the product. As this is all about open-source and voluntary contributions, your decision is OK with me.

I just want to state what I deem mergable (and the PR was close to this TBH):

So 3 of 4 ticked in my opinion.

— Reply to this email directly, view it on GitHubhttps://github.com/LibVNC/libvncserver/pull/545#issuecomment-1402043963, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAQSUC77HWKTK6W5OI2VQNTWT7RSVANCNFSM6AAAAAATT4TVXI. You are receiving this because you were mentioned.Message ID: @.***>