Re-Implementation of Term and VNC Features.

[ricardoalcantara]

Hi, recently I opened two issues:

#147: This issue was caused by a breakline that prevented me from using xtermjs.org. After removing the breakline, it worked fine.
#148: I was unable to use VNC at all.
After some research, I decided to re-implement the solution since I use this library frequently, and these features are important to me.

Additionally, I created a proof of concept (POC), hosted in this repository: https://github.com/ricardoalcantara/go-proxmox-term-and-vnc, which I will soon incorporate into my own project.

I would like to contribute my work and I'm open to further discussion.

[luthermonson]

astounding work, this vnc/term stuff has always needed help and testing and I really appreciate you getting into this code. a couple thoughts...

• changing the channel to []byte was required? we could stay in v0.1.x if that change wasn't in there but no issues bumping to v0.2.x if this is the better solution.
• there are actually integration tests for some of this code you might want to confirm still work and maybe add to them? e.g. https://github.com/luthermonson/go-proxmox/blob/main/tests/integration/nodes_test.go#L121
• could you add some docs on how this stuff works and can be setup and work? maybe you'd want to start an examples dir and bring in your poc to this repo with a more detailed README.md?

[ricardoalcantara]

Hi, thanks for the reply.

changing the channel to []byte was required? we could stay in v0.1.x if that change wasn't in there but no issues bumping to v0.2.x if this is the better solution.

For Term, no, it wasn’t strictly necessary. For VNC, it’s actually not useful since it handles a lot of unreadable bytes. It was initially converting []byte to string, sending it, and then converting it back to []byte. But this conversion is unnecessary for Term, and it's better to only convert when absolutely needed. The app is basically acting as a proxy in between. I suggest leaving it as is. Could the conversion break if someone is using an uncommon encoding?

there are actually integration tests for some of this code you might want to confirm still work and maybe add to them? e.g. https://github.com/luthermonson/go-proxmox/blob/main/tests/integration/nodes_test.go#L121

I had to fix all the broken tests by using context.TODO(). I’m not entirely sure if that’s the correct approach. I also fixed TermProxy and wrote a basic test for VNCProxy, although it might break if the display is set to Serial 0.

could you add some docs on how this stuff works and can be setup and work? maybe you'd want to start an examples dir and bring in your poc to this repo with a more detailed README.md?

I don’t mind moving my PoC to the project. However, I’m not really a fan of writing documentation. I prefer reading code instead. I can add it in a later PR, right after this one, so I can point to the correct library.

Later Edit:
Actually, the Term implementation will have breaking changes anyway because it was using VNCWebSocket, which doesn’t work for both Term and VNC. That’s why I created a new method and moved it.

[luthermonson]

No worries then, we are going to v0.2.0! ok on everything else, let's get this merged and prep for the release

[luthermonson]

When you're ready make an examples dir and bring in your project. This is ok to merge as is