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

Core: have webhost slot name links go through the launcher #2779

Merged
merged 19 commits into from
Sep 7, 2024

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

Changes the slot name links on the website room pages to go through launcher instead of straight to common client. it then tries to find the matching valid component and will call that component if it can, falling back on common client with the uri as an arg.

How was this tested?

https://discord.com/channels/731205301247803413/731214280439103580/1201703250789929031

If this makes graphical changes, please attach screenshots.

Screenshot_341

@alwaysintreble
Copy link
Collaborator Author

worth mentioning separate, but the reasoning for both game_name and supports_uri is that we need the game_name to find the component from the uri, but we might also potentially need it for component caching. if we use game_name for caching, it will be required for all worlds, so supports_uri is to differentiate between those that don't want/can't handle the uri. if we ultimately don't end up wanting to use game_name for caching then that bool can possibly be removed, but i think that might introduce bugs from newer devs just assuming it's required anyways.

@alwaysintreble alwaysintreble added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Jan 30, 2024
@qwint
Copy link
Contributor

qwint commented Feb 7, 2024

with minor changes to my apworld (Minit):

  • to support args in the registered component func
  • and setting the new game_name and supports_uri component values

i was able to successfully open my CommonClient extension in both passing the relevant url into commandline parameters (i.e. python launcher.py [url] and archipelagolauncher.exe "[url]") and by clicking on the relevant link in the updated webhost.

And can confirm that when my .apworld was not configured correctly and/or missing that it defaulted back to the text client as expected

I also confirmed that the new links still open up the text client if the version of AP installed does not contain these updates

@PoryGone PoryGone added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 10, 2024
Copy link
Contributor

@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.

had found some weird cases where a ghost launcher window launched on frozen builds, but after rebuilding clean on just this branch and just this branch sync'd with main confirmed that both options frozen did not have that, so it was likely some bad merge I had done when pulling in other PRs

@github-actions github-actions bot added the affects: webhost Issues/PRs that touch webhost and may need additional validation. label May 4, 2024
@NewSoupVi NewSoupVi self-requested a review August 31, 2024 15:21
@NewSoupVi NewSoupVi 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: peer-review Issue/PR has not been reviewed by enough people yet. labels Aug 31, 2024
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

I've been staring at this code a lot recently and came to a lot of the same conclusions. I think the popup could probably be expanded upon a bit in the future, but it's fine as is.

Launcher.py Outdated Show resolved Hide resolved
@NewSoupVi NewSoupVi merged commit 430b71a into ArchipelagoMW:main Sep 7, 2024
20 checks passed
qwint added a commit to qwint/Archipelago that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. 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.

5 participants