Core: Add launch function to call launch_subprocess only if multiprocessing is actually necessary

[qwint]

What is this fixing or adding?

alternative to #3633 and making all clients that want similar handle it themselves

skips opening a new process if kivy (and thus the launcher gui) hasn't been loaded so stdin can function as expected on --nogui and similar

if there's a cleaner way to do this (even with a launcher rewrite) we should probably do that instead, but components seem pretty intentionally agnostic to if they're launched from the component arg handling or gui so I didn't really want to change that Found a Util function to do the kivy check lol

How was this tested?

opened text client and bizhawk client with --nogui and could type commands just fine
opened launcher GUI and could open text client and close launcher without issue
launched messenger from url and could open text client from the popup and close the popup without issues

If this makes graphical changes, please attach screenshots.

[silasary]

Looks good to me

[Jouramie]

Not tested, code LGTM.

[black-sliver]

I'm a bit worried about the naming. Does anyone have problems with keeping a technically-wrong name for this new behaviour?

[qwint]

to repeat what I mentioned in discord,
I have no qualms about making this a new function and keeping the existing one's functionality intact,
I just doubt that the many if not all uses of the current function intended to always spawn a subprocess and believe they instead just use it because it's the wrapper you use when launching a client from the launcher (in which case the change will be welcome)

[Berserker66]

I also dislike functions lieing to me.

[black-sliver]

Was about to merge this, but I just realized that this now adds a function with wrong typing.

For one, this is missing typing for Callable, which is not a simple problem to solve - if args was an unpacked TypeVarTuple, i.e.

Ts = TypeVarTuple("Ts")

def launch(f: Callable[[*Ts], Any], args: Tuple[*Ts] = ()) -> None:
    f(*args)

, the default = () would be invalid.

Also there may be components that pass in a list[str] or a runtime-defined tuple rather than a fixed tuple?
So maybe we merge as-is and solve this in the future?

The other thing, which we should fix, is the type of name. name: str = None is invalid and should be str | None.

[silasary]

For now, I'd lean towards merging as-is or with Callable[..., Any] (Which is functionally identical, but looks nicer).

The Type Hint isn't wrong, it's just less useful than it could be.

[black-sliver]

The thing about bare Callable is that is is underspecified, which is similar to being untyped, but has its own error in mypy --strict.

So changing it to [..., Any] would be better, but this still can not check the call that is being made in the body.
I.e. it will assume that it can pass in any arguments even if that's not the case. (We know that we pass strings or nothing, but it happly accepts a function that takes only ints and doesnt check in the line that does the call, making it effectively untyped)

[qwint]

Bare callable is what launch_subprocess already used, if we want to fix both while we're here I have no issue applying whatever typing you land on, but blocking this over using typing we already use somewhere else seems silly
fixed the str=None typing anyways because that's easy

[black-sliver]

blocking this over using typing we already use somewhere else seems silly

This is how typing goes to sh!* 😁

The name was "critical" since it's a real error even with lax settings. The other one is just missing typing, basically. Partially-typed functions are not great, but we don't require full typing, so this can be solved later.

Gonna review when i'm at PC.

[black-sliver]

Change should be safe since it's opt-in now. Tested CommonClient from button, command line, with and without --nogui and it behaves correctly.

Thanks!