linuxmint / warpinator

Share files across the LAN
GNU General Public License v3.0
1.17k stars 80 forks source link

Manual connection #182

Closed slowscript closed 7 months ago

slowscript commented 11 months ago

This PR addresses the issue that some networks (e.g. my dorm WiFi, eduroam, ...) block multicast packets or put users into a bunch of different subnets and thus prevent mDNS/zeroconf device discovery from working. I've worked around this by adding a new RPC to the WarpRegistration service which is used to exchange the same information contained in mDNS records (IP address, ports, hostname...) in a single call. A connection is initiated by the user by entering an IP address and auth port into a dialog window, or scanning the displayed QR code if using a mobile device (corresponding Android implementation here). Once both devices know each other the connection proceeds as usual (exchange of certificates etc.) manual-connection I am not exactly a UI person so feedback on how this should look is welcome. The instructions could also probably be improved to make it more clear what the user needs to do.

It works in the current form although there are a few things I am not sure about. 1) The registration RPC currently runs on the main thread (or so I think). What would be a good way to parallelize it so the UI does not hang for up to 5 or so seconds (in case a timeout occurs)? I see existing code uses one-off threads in some places and an executor in others. 2) What would be a good way to let the user know that the connection failed? A message dialog? On Android I use a toast message.

There might be better ways to go about this so let me know if some tweaks or architectural changes are needed.

mtwebster commented 11 months ago

Cool idea - possibly this addresses connections while a VPN is active also.

The registration RPC currently runs on the main thread (or so I think). What would be a good way to parallelize it so the UI does not hang for up to 5 or so seconds (in case a timeout occurs)? I see existing code uses one-off threads in some places and an executor in others.

Yeah we'd definitely want to run it in its own thread, though that probably shouldn't take too much effort..

What would be a good way to let the user know that the connection failed? A message dialog? On Android I use a toast message.

I'm not sure - I'll have to think about how the whole connection process will be presented - we've tried to avoid popups in general for errors (we just show a different view with the information needed) but an error here wouldn't mean the entire app failed, just one connection.

As it is now I assume this process would have to be done each time warpinator is started? I wonder if some sort of persistence would be useful, a list of 'known' connections. Maybe not initially.

Frivolous question - can the QR code be 'customized' to incorporate the Warpinator icon design? For instance, Mint's user guides have codes to scan to open them in a browser: image Definitely not any sort of requirement here, but would be a nice touch.

I'll have a closer look at this when I get some time, right now we're focusing on the new Mint release.

Thanks!

slowscript commented 11 months ago

Thanks for the feedback.

Yeah we'd definitely want to run it in its own thread, though that probably shouldn't take too much effort..

Sure, I'll try to figure something out with the threading.

Regarding the display of error messages, maybe the connection dialog doesn't have to close after the Connect button is clicked as it does now, and instead display a status label, possibly with a spinner next to it to indicate activity (and failure when needed). This would probably require a fully custom Window for the dialog, rather than a simple Gtk.Dialog subclass.

There is currently no persistence for the manually connected remotes. Dumping the data into a JSON somewhere in .config and automatically loading them when the program starts should not be difficult, but the user would probably then want to have an option to manage them (remove unused ones, reinitiate the connection, ...) which would need some UI work and I'm not sure how that should look like.

Customizing the QR code should also be possible. The used library has a bunch of styling options and allows embedding images.

I'm leaving for holiday tomorrow and will be unavailable for about 2-3 weeks, so please excuse that it might take some time before these things are addressed in code.

slowscript commented 10 months ago

"Connecting..." status and error messages are now displayed in a label above the Connect button like this: connecting

I'm still not sure what the persistence of manually connected remotes should look like (from UI standpoint). For me it is quick enough to simply scan the QR code with my phone every time. I am however ready to add this feature if I get some hints on how it is supposed to work.

Regarding the QR code customization, I've done some experimenting and the library basically allows two things: embedding an image in the middle: image or using an image as a mask, which could look something like this: image The mask image was made by taking the symbolic svg icon and putting it onto some gradient background. If anyone has another idea how it could look, please let me know.

slowscript commented 7 months ago

Rebased to resolve merge conflicts. However, the generated protobuf files now produce a much larger diff. What version of protoc was used when generating them in 3866d77e791a30387b668d09dac9f40b6d60cdff?

mtwebster commented 7 months ago

Hi sorry for getting back to this late. The protobuf files need to be regenerated against the same version grpcio-tools as the grpc we're now bundling (1.59.x), not the one from the package manager. It's probably not critical it's the exact version (because te protobuf format changes more infrequently). I've update the comment in generate-protobuf.

Before I try to build your matching android branch for this to test, you don't happen to have an apk already made do you?

The desktop version seems to be fine (other than some text layout things that I'll get to)

Thanks

clefebvre commented 7 months ago

No need to customize the QR code imo.

slowscript commented 7 months ago

Ok, I'll use grpcio-tools 1.59 from pip. I think I already tried that, but not sure what was the problem...

you don't happen to have an apk already made do you?

Here: warpinator-manual-connect.zip

mtwebster commented 7 months ago

Hi, are you ok with this as it stands?

It seemed to work well for me - my concerns about 'remembering' connections seem a little less founded, at least when used with your android app, as yours is able to go dormant but still remember the connection info. It's less important that the PC remembers it. I think we'd still want to eventually, but for now I think it's fine.

I may tweak the layout or text of the QR dialog a bit, but it'll be easier once I merge this.

Thanks!

slowscript commented 7 months ago

Yes, I consider this done for now. Feel free to tweak anything, as I said, I'm not too good with UI things.

As for remembering previous connections, I was thinking that turning the text box for entering IP and port into a dropdown with previously entered values would be sufficient. I probably won't get to doing that very soon though.

I'm not sure what you mean by my app being able to remember connection info. I don't remember implementing anything of that sort alongside this feature. The only thing that differs between my apps and Linux Warpinator is that they have a button for manually reconnecting to known machines (which automatically connect back), so it is relatively easy to reestablish lost connection between them.

mtwebster commented 7 months ago

Ah sorry I just meant that (at least for my test phone) I can leave the app and come back later and it still has the connection info and will reconnect.