Separate Game and GameLauncher classes #5421
Conversation
There was a problem hiding this comment.
I think you've made a lot of non-relevant type annotations here, adding noise. But I'e done it too, those things are addictive.
I have one major change I would make- no 'game_launcher' member on Game. We should create a GameLauncher only when launching, and at other times we will need to search for the launchers (plural) that are running.
Non-running launchers should be a very ephemeral thing.
There's a lot of signals and methods that say "game" instead of "game_launcher"; I could live with that, but it feels a little inconsistent right now.
However, even as it is I think this a great improvement, and I'd merge it promptly. We can get rid of Game.game_launcher afterward easily enough over time.
👍
| @@ -19,24 +19,28 @@ def watch_game_errors(game_stop_result, game=None): | |||
| If you do not provide a game object directly, it is assumed to be in the first argument to | |||
| the decorated method (which is 'self', typically). | |||
| """ | |||
| captured_game = game | |||
| captured_game = game_launcher | |||
|
|
|||
There was a problem hiding this comment.
Maybe rename captured_game to captured_launcher?
| @@ -79,7 +79,7 @@ def on_game_stop(self, *_args): | |||
| """Stops the game""" | |||
| games = self.get_running_games() | |||
| for game in games: | |||
| game.force_stop() | |||
| game.game_launcher.force_stop() | |||
There was a problem hiding this comment.
Don't like this. get_running_games() should return the launchers- that is what is running, not the Game object.
This design should support running the same game twice, even if we don't want to surface that in the UI.
| @@ -300,7 +300,7 @@ def on_game_launch(self, *_args): | |||
| game = self.game | |||
| if game.is_installed and game.is_db_stored: | |||
| if not self.application.is_game_running_by_id(game.id): | |||
| game.launch(launch_ui_delegate=self.window) | |||
| game.game_launcher.launch(launch_ui_delegate=self.window) | |||
There was a problem hiding this comment.
Maybe we should create the launcher object at this point?
| self.launch_ui_delegate = LaunchUIDelegate() | ||
| self.install_ui_delegate = InstallUIDelegate() | ||
|
|
||
| self._running_games = [] | ||
| self._running_games: List[Game] = [] |
There was a problem hiding this comment.
This should be GameLaunchers, not games.
We should not create GameLaunchers on demand, we should create them on launch, and only on launch. Games, on the other hand, we load from the DB whenever we feel like it- a key difference.
This change would make it safe for GameLauncher to track the running game; right now we have to use _running_games to have special blessed Game objects for that purpose, and that's been a source of bugs. Let's fix that.
I might argue that _running_games might be better moved out of application.py to GameLauncher's module instead. I see no reason to tie this to the UI, and if you really do want to go to QT, you'll want to separate this from GTK.
| @@ -758,10 +757,11 @@ def on_error(game, error): | |||
| return True | |||
|
|
|||
| game = Game(db_game["id"]) | |||
| game.connect("game-error", on_error) | |||
| game.launch(self.launch_ui_delegate) | |||
| game_launcher = game.game_launcher | |||
There was a problem hiding this comment.
Again, we're launching the game here, so lets create the launcher here.
| @@ -269,7 +276,10 @@ def on_install_clicked(self, button): | |||
| """Handler for installing service games""" | |||
| self.service.install(self.db_game) | |||
|
|
|||
| def on_game_state_changed(self, game): | |||
| def on_game_launcher_state_changed(self, game_launcher: GameLauncher): | |||
There was a problem hiding this comment.
Are we going to change the signal and handler names to say 'launcher'? If so, you missed a bunch.
| @@ -145,12 +145,12 @@ def get_launch_config_command(self, gameplay_info, launch_config): | |||
|
|
|||
| return command | |||
|
|
|||
| def get_command(self): | |||
| def get_command(self) -> list: | |||
There was a problem hiding this comment.
I think the element type is important, and here we know it- this should be List[str].
| @@ -18,10 +18,14 @@ | |||
| from lutris.util.linux import LINUX_SYSTEM | |||
| from lutris.util.log import logger | |||
|
|
|||
| if TYPE_CHECKING: | |||
There was a problem hiding this comment.
Oh, I wish I had known about TYPE_CHECKING! I'll definitely be able to use that.
| @@ -363,8 +284,8 @@ def on_error(_game, error): | |||
| return True | |||
|
|
|||
| game = Game(game_id) | |||
| game.connect("game-error", on_error) | |||
| game.launch(launch_ui_delegate) | |||
| game.game_launcher.connect("game-error", on_error) | |||
There was a problem hiding this comment.
Another place to create the GameLauncher.
| @@ -1,67 +1,40 @@ | |||
| """Module that actually runs the games.""" | |||
|
|
|||
| # pylint: disable=too-many-public-methods disable=too-many-lines | |||
| from __future__ import annotations | |||
|
|
|||
There was a problem hiding this comment.
Oh, can we do this?
If so, can we use it to say str | None and the like? In Python 3.7, this works in mypy but not at runtime, I read.
…y off of GObject. This commit replaces the game-start and game-started signals with NotificationSource.
In order to avoid growing the Game class beyond what's reasonable, I've separated it into 2 classes. The Game class handles what is related to the game's data and also its management on the filesystem. The GameLauncher class handles the game execution and what's related to it.
This is quite a substantial change and there are possibly still some bugs. I tried to add a maximum of type hints to avoid as many errors as possible. I did some testing, native games and umu are working but there is still an issue with wine games which I suspect is something I missed related to PID tracking.