Skip to content

Conversation

MatthewMarinets
Copy link
Contributor

@MatthewMarinets MatthewMarinets commented Sep 3, 2025

What is this fixing or adding?

The big sc2 content merge PR #5312 was merged with a last-minute change that removed Starcraft2Client.py. Unfortunately, the replacement command-line functionality -- calling Launcher.py "Starcraft 2 Client" -- <args> doesn't work properly as the launch() function was set to read args straight from sys.argv but the plumbing was passing those arguments in as function arguments.

I also took the time to poke at the webhost as I wasn't sure if the URI handling was up to snuff and I remember not being able to test it locally. As it turns out, the precise component I wanted to test is still no testable purely locally, but I uncovered a few unupdated option names in the options page that I updated here as well.

How was this tested?

  • Generated+hosted locally, connected with Launcher.py "Starcraft 2 Client" -- --connect localhost --name phaneros
  • Also checked that launching without args still worked fine -- Launcher.py "Starcraft 2 Client"
  • Updated my old automated quickstart scripts with this new syntax; they all now work again

If this makes graphical changes, please attach screenshots.

None

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 3, 2025
Copy link
Collaborator

@qwint qwint left a comment

Choose a reason for hiding this comment

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

FYI cc has a helper for parsing the url argument
https://github.com/ArchipelagoMW/Archipelago/blob/main/CommonClient.py#L1163-L1186
since you aren't actually using the namespace.url field it may require you restructure or just copy out the handling pieces you need, but I would suggest using urllib as cc does
(additionally, not actually reporting the url arg in the parser means the help text will not advertise your support for it properly

@MatthewMarinets
Copy link
Contributor Author

MatthewMarinets commented Sep 3, 2025

additionally, not actually reporting the url arg in the parser means the help text will not advertise your support for it properly

This was an explicit goal; as far I'm concerned, the URI interface is a strictly machine-to-machine interface and shouldn't be documented in human-facing help text. (Edit: A better way to put it is that the requirement I'm supporting here is not "URI support", it's "when you click the button in the webhost, the client opens it properly". Full URI parsing is actually something I want to avoid as that's just extra behaviour I don't want to run, support, or think about.)

I'll try out the CommonClient function probably tomorrow. I'm reticent to rely on third-party libraries for what amounts to a simple string split, but I have never used passwords on rooms myself so will likely have to rely on it to be sure passwords don't leak if something goes wrong.

@Berserker66
Copy link
Member

Please just follow established patterns instead of making your own special again...

@ScipioWright ScipioWright added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Sep 3, 2025
@Ziktofel Ziktofel added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 4, 2025
@Snarkie
Copy link

Snarkie commented Sep 5, 2025

Tested on my end after the update, confirmed the issue with the currently merged implementation and this fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants