Handle rptools-maptool: URI for quick-join links #5076
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is shaping up nicely! Loving the cleanup you've done along the way! |
|
I added an abstraction over In the process of investigating what authority might be valid for service discovery I discovered it is possible to bind it to a given interface, which would theoretically be a suitable authority, but it is useless for providing a connection link. The other option for what might be a valid authority would be to make the multicast address configurable, which we probably don't want to make configurable but probably should change it from From my reading of the Teredo RFC spec MapTool won't cause Teredo to break because it expects to get a valid response after seeing a multicast datagram, but if a service using Teredo happens to use the same port I'm not so sure |
fishface60
force-pushed
the
develop
branch
4 times, most recently
from
1775a42 to
0a71105
Compare
|
This has grown to be a bit of a monster. I've split the features I discovered I wanted along the way into separate PRs and there's a common refactoring PR, but it's still huge. I could split it down further if preferred, but having a bunch of very granular PRs may not be an improvement so I didn't do so in advance. |
fishface60
changed the title
Connecting via URI will require connecting separately from the dialog that is opened by clicking the button associated with CONNECT_TO_SERVER.
Your role is determined by whether you enter the correct password or what your role is in the password database it is not even settable in the dialog.
This should make it more obvious that the role is not set by the caller and is instead set after handshake completes.
This structure was mostly unused in the remote connection code path and filling it with dummy values obscures what's needed.
With useWebRTC as a boolean flag the hostName and port are unused or the server name is unused. By using sealed interface and record types it's harder to accidentally use inappropriate fields.
The caller has all the information needed to make it so can do so instead of passing extra parameters to each function.
It was more or less the same type as the stripped-down ServerConfig so we can save some effort shuffling bytes between objects. This also restructures the ConnectToServerDialog result handling so that a single result is returned instead of having an accepted flag and multiple result properties which enables simply annotating the result as nullable instead of documenting that if accepted() returns true then the result parameters won't be null, but if it's false then they may be.
This abstracts over net.tsc.servicediscovery.ServiceFinder to hard-code the service group and adapt the message to our types so that this logic isn't duplicated when used elsewhere.
This represents different ways you can connect to a server, though this is an alternative to the connection dialog since that receives service discovery notifications directly.
InetAddress is a commmon base class of both Inet4Address and Inet6Address so if you happen to have IPv6 addresses first then it will be returned for both.
The parsed value is more directly useful for most callers since we can assume it is a valid address. Callers that want it as a string have to convert it to a string now and handle that it might be null but that's easier than turning it into the InetAddress from the String since InetAddress.getByName can raise an exception, and CompletableFutire.thenApply can't take functions that throw exceptions.
It is a lot more useful to callers to be able to use thenApply.
The MapToolServer class calling into the ConnectionInfoDialog class to get a cached copy of the external address wasn't a clean abstraction. This introduces a new NetUtil class because this already needs to be done in two places and I intend to add a third. Since a completed future holds the result of its computation it is handy to keep a reference to it around rather than jump through hoops to have it assign its result to the cache.
Manually shutting down the executor on the first success leaves the possibility that if none succeed then the future will hang.
PR RPTools#4208 had a request to add a reachability check on the grounds that if an address hasn't got a route to rptools.net then it's probably not configured to be reachable from your peers since your routing table is used to send packets back the other way. This was not implemented correctly, it just added a check that rptools.net resolved, which happens through the preferred interface which is able to reach the configured DNS server rather than the address being iterated over. https://docs.oracle.com/en/java/javase/21/docs/api//java.base/java/net/InetAddress.html#isReachable(java.net.NetworkInterface,int,int) could be used for this reachability check but that's implemented as using IGMP (ping) where available or falling back to a TCP connection to port 7. Restrictive firewalling could cause this to fail but we know rptools.net serves SSL, so we can connect to that and immediately close the connection.
Rather than picking the first non-loopback, non-link-local address for each protocol, get all the non-loopback addresses, sort them by reachability, including whether they are routable to www.rptools.net, and pick the first. This means if you have no internet-routable addresses you can still use site-local ones, and if you have no site-local ones you can use link-local.
Abeille reads the forms in src/main/resources/net/rptools/maptool/client/ui/forms/ but the UI is managed by `*View.form` files in src/main/java/net/rptools/maptool/client/ui/ and Abeille is no longer used in favour of the IntelliJ forms designer. To reduce confusion about which tools to use, this commit replaces the documentation about Abeille and removes its config files.
The button that looks like file copy copies the rptools-maptool URI to the clipboard, and the icon is meant to suggest copying. The button that looks like a cursor is meant to imply clicking, which the copyable HTTP URI is because chat applications will automatically turn that text into a link that can be clicked but does require the web browser opening the page and forwarding to the payload first.
cwisniew
approved these changes
Jan 20, 2025
|
Merged and will be included in 1.17 |
github-merge-queue
bot
removed this pull request from the merge queue due to invalid changes in the merge commit
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
fixes: #5075
depends on: #5143
Description of the Change
This change includes some refactoring.
It is structured in sequential commits that aim to be individually understandable, but later changes do replace earlier ones, so the commits are intended to make reading the changes more easily digestable, but review of the code should be performed on the end result.
The command-line parser is extended to handle positional arguments being URIs pointing to a running server instead of just file paths to a campaign file.
The URIs are parsed for connection information and if a valid server is provided MapTool will connect to it.
The packaging on Linux is extended to add URI handlers.
The Connection Info dialog is extended to add buttons for copying the URIs to the clipboard.
Possible Drawbacks
A slight slowdown for parsing file paths as URI first.
If the feature doesn't prove useful then it's additional code to maintain.
Requires an additional service running on rptools.net to bounce from an https:// URL that can be pasted into chat and automatically turned into a link into the rptools-maptool: scheme URI.
Documentation Notes
rptools-maptool://URIsWhen MapTool is installed it installs URI scheme handlers for:
rptools-maptool+registry:///serverNamerptools-maptool+lan:///lanIdrptools-maptool+tcp://address:port/When MapTool is started by clicking on a link with this URI it will connect to that server.
The URIs may be copied to your clipboard by clicking on the "Copy" buttons in the Connection Info dialog.
Chat apps such as Discord do not provide the ability to create your own links, and instead will only turn text that it recognises as a link into a clickable link, so the button that looks like a mouse cursor may be clicked to copy a URL that will open a page in your web browser and then redirect to the MapTool link.
Release Notes
This change is