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

Launcher: Update message that displays when installing a custom apworld for a game in main #3607

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ScipioWright
Copy link
Collaborator

What is this fixing or adding?

image
When double-clicking a beta Tunic apworld, I got the above message. I did not have the regular Launcher open, so I could see this being confusing for some users.
So, this PR updates the message that displays to hopefully be a bit clearer.

How was this tested?

Reading

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 2, 2024
@ScipioWright ScipioWright added is: documentation Improvements or additions to documentation. labels Jul 2, 2024
@@ -131,8 +131,9 @@ def _install_apworld(apworld_src: str = "") -> Optional[Tuple[pathlib.Path, path
found_already_loaded = True
break
if found_already_loaded:
raise Exception(f"Installed APWorld successfully, but '{module_name}' is already loaded,\n"
"so a Launcher restart is required to use the new installation.")
raise Exception(f"Installed APWorld successfully, but '{module_name}' is already installed,\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like this change from "loaded" to "installed".

After #3226, it could be installed and not require a restart because it's not loaded.
So what matters is that it's loaded, not that it's installed.

Copy link
Collaborator Author

@ScipioWright ScipioWright Jul 2, 2024

Choose a reason for hiding this comment

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

After #3226, the message will need to be changed to omit the restart bit anyway.

In my eyes, if I saw just that message without having the launcher open, and it said loaded, I'd be a little confused since I don't have anything "loaded" atm.
If I saw that message with the launcher open, and it said installed, I would understand what it meant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After #3226, the message will need to be changed to omit the restart bit anyway.

Why? That doesn't provide a way to unload worlds, so it would still need to be restarted if the world is loaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, my mistake, I didn't read through that pr thoroughly enough.

The rest of the response still stands, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Supporting the user's incorrect ideas is not a good way to avoid confusion.

If the user is thinking they don't have anything loaded when they see this, that idea is just wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to change it to your suggestion. I don't agree with it still, but it's way too much time spent on a single word.

Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

This is an ok temporary workaround.

A better solution would be to have a way to tell this apworld installer whether it's being called with the GUI open, so it doesn't need to display any message at all with no Launcher GUI. But that would require a more significant and dangerous change.

@ScipioWright ScipioWright 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 Jul 2, 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. is: documentation Improvements or additions to documentation. 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.

None yet

3 participants