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

feat: merge 'games' directory into launcher installation directory #536

Merged
merged 11 commits into from
Mar 28, 2020

Conversation

skaldarnar
Copy link
Member

@skaldarnar skaldarnar commented Mar 28, 2020

Part of #469.

Take away the user choice for the game installation directory and place it directly in the launcher installation directory under games. Technically, this introduces GAMES as launcher-managed directory and does not prompt the user for input.

  • Delete LauncherInitTask::getGameDirectory and clean up labels, etc.
  • Remove option to change the directory from settings
  • Remove notion of game installation directory from the settings code
  • games are installed at <installDir>/games/games/... for some reason 🙄

@skaldarnar skaldarnar added Status: WIP Work in Progress (not ready for review) Type: Maintenance Maintenance or chores not adding new features or fixing bugs. Topic: Configuration Related to configuration design, settings and storage labels Mar 28, 2020
@skaldarnar skaldarnar added this to the v4.0.0 milestone Mar 28, 2020
@skaldarnar skaldarnar requested a review from jdrueckert March 28, 2020 13:06
@jdrueckert
Copy link
Member

TODO Remove option to change the directory from settings was already done in #503
TODO Remove notion of game installation directory from the settings code can IMO be done post v4.0.0

@skaldarnar skaldarnar added Status: Actionable An issue or task that can immediately be worked on and removed Status: WIP Work in Progress (not ready for review) labels Mar 28, 2020
Comment on lines -104 to +114
launcherInitTask.setOnSucceeded(new EventHandler<WorkerStateEvent>() {
@Override
public void handle(final WorkerStateEvent workerStateEvent) {
try {
LauncherConfiguration config = launcherInitTask.getValue();
if (config == null) {
throw new LauncherStartFailedException("Launcher configuration was `null`.");
} else if (config instanceof NullLauncherConfiguration) {
logger.info("Closing the launcher ... (empty configuration, probably due to update)");
Platform.exit();
} else {
showMainStage(config);
}
} catch (IOException | LauncherStartFailedException e) {
openCrashReporterAndExit(e);
launcherInitTask.setOnSucceeded(workerStateEvent -> {
try {
LauncherConfiguration config = launcherInitTask.getValue();
if (config == null) {
throw new LauncherStartFailedException("Launcher configuration was `null`.");
} else if (config instanceof NullLauncherConfiguration) {
logger.info("Closing the launcher ... (empty configuration, probably due to update)");
Platform.exit();
} else {
showMainStage(config);
}
} catch (IOException | LauncherStartFailedException e) {
openCrashReporterAndExit(e);
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this does the same?
Previous implementation of EventHandler seems to me as if the workerStateEvent may not yet be there when the launcherInitTask succeeds? But maybe I'm reading this wrong...

Copy link
Member Author

Choose a reason for hiding this comment

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

This just uses a different syntax (lambda expression) for creating the handler for the onSucceeded event. As handle(WorkerStateEvent event) is the only method that needs to be implemented we can use the simplified syntax here.

new EventHandler<WorkerStateEvent>() {
	@Override
    public void handle(final WorkerStateEvent workerStateEvent) {
        // do something
    }
}

Is replaced by

workerStateEvent -> {
    // do something
}

Copy link
Member

Choose a reason for hiding this comment

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

Alright, so the new one also kind of "waits" for a workerStateEvent to come in?

@skaldarnar skaldarnar requested a review from jdrueckert March 28, 2020 15:47
Copy link
Member

@jdrueckert jdrueckert left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for explaining the EventHandler stuff

@jdrueckert jdrueckert merged commit 7e918a8 into master Mar 28, 2020
@jdrueckert jdrueckert deleted the topic/merge-game-into-application-directory branch March 28, 2020 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Actionable An issue or task that can immediately be worked on Topic: Configuration Related to configuration design, settings and storage Type: Maintenance Maintenance or chores not adding new features or fixing bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants