Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature #7735: Implement protocol handling #7981

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

@JMcKiern
Copy link
Contributor

@JMcKiern JMcKiern commented Feb 6, 2020

Basically does what #7735 asks for.

In openttd.cpp L475-483, I added cases for joining as a new company ("new") and joining as a spectator (""). These changes will affect the way a command line connection string (-n host:post#company) is parse but I think that would be ideal.

Let me know what you think.

@LordAro
Copy link
Member

@LordAro LordAro commented Feb 6, 2020

I'd recommend "Feature" as the commit prefix

@JMcKiern JMcKiern force-pushed the protocol-handler branch from 1cb904c to 278477d Feb 6, 2020
@JMcKiern
Copy link
Contributor Author

@JMcKiern JMcKiern commented Feb 6, 2020

I haven't actually changed any installation code. So far, this is only another command line option. It still needs to be added to the installation so that clicking a link would open OpenTTD.

Also, I forgot about "ask the player for confirmation before beginning to connect"...

src/network/network.cpp Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
} else if (strcmp(name, "companypassword") == 0) {
scanner->join_company_password = value;
} else if (strcmp(name, "version") == 0) {
/* Nothing for now */
Copy link
Contributor

@nielsmh nielsmh Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having multiple versions handling in from the beginning would be ideal. Have a per-platform function for launching a different installed version (by version string) that as a fallback just does nothing.
By having fully functioning multiple versions handling in from the beginning, any installed version can be registered as the primary URL handler, and will always be able to re-launch any other installed version.

Copy link
Contributor Author

@JMcKiern JMcKiern Feb 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @nielsmh, I'm wondering how the program would look for other versions that are installed? I tried installing 2 versions of OpenTTD using the Windows installers but only the latest version is kept. This leads me to think that each version would have to be installed in different user-chosen locations...

Copy link
Contributor

@nielsmh nielsmh Feb 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would have to be a new thing done by something. I don't have a bulletproof idea yet, but something with versions of the game (that support URL handler) registering itself when it starts. So if you have a bunch of different portable versions with this feature, the first time you run one it registers itself with version and path, and you end up with all of them registered and available for URL handlers.

Perhaps also a mode where earlier versions can be registered so an URL-handler-supporting version can convert the URL into the older commandline parameters format and start those.

Copy link
Contributor Author

@JMcKiern JMcKiern Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah, i see what you mean. What information would we need to store? Is it just version number and filepath for each executable? Presumably this could be stored in a simple config file, ignoring weird characters (eg a newline) in the file path. I guess binary file could be used instead to allow for this.

Maybe for registering old versions, a -r (--register) option could be added that would take the path of an openttd executable and the new version would find its version number and register it?

Also, there would need to be some way to tell if the older cli parameters would need to be used. Could that just be a hardcode if version < some_version: use old cli or is that too rigid/fragile?

Copy link
Contributor

@nielsmh nielsmh Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows I would do it in registry.
Key name HKCU\Software\OpenTTD\Versions\<version>
Default value = (string) path to exe file
Value NetworkJoinMode= (dword) 0 for classic, 1 for -U parameter support

The <version> in the key name ("folder") should be the network version of the build, which should usually be the same as the product version string in the exe file's version resource. This means the main URL handler will be able to verify that the exe file pointed to is the actual version intended before launching it, and abort if it's not a match. That would work for even very old versions.

As for registering old versions/versions not supporting the URL handler, it could either be done in a manual way, or an automatic-ish scanner. Manual would be prompting the user to find the exe file for a specific version when they try to join a server where they don't have the version for, or as you suggest having a call to register a specific version. Automatic scanner would probably be best as a separate downloadable tool.

On Unix'y systems, I'd put the database in a similar format (maybe an INI format) in the user's profile ~/.openttd/.
On macOS, the right way might involve a PList of some kind.
I'm not sure how you could verify the version before launching in either of those, apart from maybe a checksum of the binary file.

Copy link
Contributor Author

@JMcKiern JMcKiern Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cool.

I don't actually have access to a macOS machine (nor have I even heard of PLists!) but I can give it a go anyway.

A prompt does make sense. And i suppose the automatic scanner idea can be made at a later date.

I'll have a look into all this whenever i get some time

Should i just continue in this pr or should i make a pr on my fork to (temporarily) separate the changes? I've never added this much in a pr before...

@nielsmh nielsmh changed the title Close #7735: Implement protocol handling Feature #7735: Implement protocol handling Feb 8, 2020
@JMcKiern JMcKiern force-pushed the protocol-handler branch from 84f2c2d to 1053bf9 Feb 8, 2020
src/openttd.cpp Outdated Show resolved Hide resolved
@nielsmh nielsmh dismissed their stale review Feb 13, 2020

Critical issues resolved.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 24, 2020

Haven't looked into this in much detail yet, but as we are adding public/secret authentication to the client, I think it is good to note that we should not do this with "passwords" in the URLs. That is just asking for trouble :D

If the public/secret authentication lands in master, there will be a possibility to generate invite codes (temporary passwords basically). Those will be more than fine to add in such URLs. So I think we should either wait with this PR till that lands, or remove the password support for now. I personally prefer the first, but I have no timeline for it either.

This comment is mostly so I don't forget, and others are up-to-date too :)

@JMcKiern
Copy link
Contributor Author

@JMcKiern JMcKiern commented Dec 24, 2020

Considering how long I've let this PR sit for (sorry!), waiting until the public/secret authentication lands in master is fine by me.

I need to look in to the idea of OpenTTD acting as a launcher for other installed OpenTTD versions before this PR will be ready anyway.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 24, 2020

I need to look in to the idea of OpenTTD acting as a launcher for other installed OpenTTD versions before this PR will be ready anyway.

Every time I read this, the only sane thing I can think of to implement that, is an OpenTTD Launcher. The cool part about that idea is, that it could also contain an auto-updater. That again would mean we can release a lot more often, as there is a lot less effort from people to update. And would make multiplayer a lot more sane. But .. not sure how we would approach a standalone launcher :)

Just some random thoughts in my head :D

@JMcKiern
Copy link
Contributor Author

@JMcKiern JMcKiern commented Dec 24, 2020

Yep, I agree that a launcher makes the most sense. Not quite the scope of this PR though!

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 24, 2020

Fully agree; but one can wonder if we would do a launcher, if this code shouldn't be part of that launcher (and not OpenTTD itself). I really do not know btw, as I cannot oversee the complexity it would need to write such launcher. I just like the idea enough to make me want to try out :D

@JMcKiern
Copy link
Contributor Author

@JMcKiern JMcKiern commented Dec 24, 2020

Good point. If that was the case then the only requirement for OpenTTD would be to parse the relevant arguments as received from the launcher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants