Skip to content

Conversation

@silasary
Copy link
Contributor

@silasary silasary commented Oct 5, 2025

What is this fixing or adding?

Utils.messagebox is called directly from various places within core code. This means that if an individual client wants to use those functions while in nogui mode, it will end up popping a GUI messagebox (or worse, crashing with "ModuleNotFoundError: No module named 'tkinter'").

This PR is based on alwaysintreble's work in #4925 and adds a gui_enabled check to Utils.messagebox, falling back to a Logging call if the user specified --nogui.

How was this tested?

  • Using my APWorld manager, python .\Launcher.py worldman -- --update-all --nogui. This loops through LauncherComponents.install_apworld which attempts to show a messagebox per install.
  • Using python .\Launcher.py "BizHawk Client" -- corrupt_patch.apemerald --nogui (A hand-mangled Emerald patch), which calls Utils.messagebox if it fails to patch the game.

If this makes graphical changes, please attach screenshots.

@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 Oct 5, 2025
from NetUtils import (Endpoint, decode, NetworkItem, encode, JSONtoTextParser, ClientStatus, Permission, NetworkSlot,
RawJSONtoTextParser, add_json_text, add_json_location, add_json_item, JSONTypes, HintStatus, SlotType)
from Utils import Version, stream_input, async_start
from Utils import gui_enabled, Version, stream_input, async_start
Copy link
Member

@NewSoupVi NewSoupVi Oct 5, 2025

Choose a reason for hiding this comment

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

I think your changes are sensible, but I'm a bit confused about this original Treble stuff.

Imo, there are two possibilities:

  1. This should be from Utils import gui_enabled as gui_enabled
  2. Clients are "meant" to switch over to from Utils import gui_enabled, in which case this should get an API Updates ping

Reason: Relying on implicit reexporting is bad™️ (It fails mypy, for example, i.e. this change would implicitly make mypy-compliant worlds fail mypy without their knowledge)

I would like us to be clear about what the intent is here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, 100% agree with that.

My gut prefers the second option?

Copy link
Member

Choose a reason for hiding this comment

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

I agree I think

Copy link
Contributor

@benny-dreamly benny-dreamly left a comment

Choose a reason for hiding this comment

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

code LGTM, did not have the time to test, as today happens to be a holiday when I'm reviewing this

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. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants