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: hide pre-releases by default #658

Merged
merged 5 commits into from
Aug 29, 2021
Merged

Conversation

skaldarnar
Copy link
Member

@skaldarnar skaldarnar commented Aug 28, 2021

This PR adds a new checkbox to the launcher settings view for the user to enable pre-releases.
This checkbox is deselected by default. The launcher will only show pre-releases (including
release candidates) when the user explicitly opts in to show them via the settings menu.

  • when the user selects a pre-release and then unchecks the property the game selection box is empty
  • all the pre-releases are still fetched on start-up (as before)
  • I'm not sure the property exposed by LauncherSettings is always correctly initialized

Somewhat addresses #479

@skaldarnar skaldarnar added Topic: UI Related to user interface (UI) or design. Type: Enhancement New features or noticable improvements. labels Aug 28, 2021
@skaldarnar
Copy link
Member Author

It seems to pick up the setting as saved on last run.

  • start the launcher with option disabled ⇒ no pre-releases
  • enable the option via settings menu ⇒ pre-releases are shown
  • close the launcher
  • re-start the launcher ⇒ option is still enabled, pre-releases are shown

Also, switching the option on and off seems to work fine. If a pre-release version was selected when the option is disabled the box will stay empty. Even though the download button is enabled in that case, clicking it does not do anything...

@skaldarnar
Copy link
Member Author

Properly fixing the situation that leaves the game release checkbox would require a bit (if not "a lot") more work.

We currently update the selection whenever we change the selected game profile, but this part is not properly hooked up with the rest of the properties.

selectedProfile.addListener((obs, oldVal, newVal) -> {
ObservableList<GameRelease> availableReleases = gameReleaseComboBox.getItems();
GameIdentifier lastPlayedGame = launcherSettings.getLastPlayedGameVersion().orElse(null);
Optional<GameRelease> lastPlayed = availableReleases.stream()
.filter(release -> release.getId().equals(lastPlayedGame))
.findFirst();
Optional<GameRelease> lastInstalled = availableReleases.stream()
.filter(release -> installedGames.contains(release.getId()))
.findFirst();
gameReleaseComboBox.getSelectionModel().select(lastPlayed
.or(() -> lastInstalled)
.or(() -> availableReleases.stream().findFirst())
.orElse(null));
});

I'm not really sure how to best resolve this, and would prefer to clean up other parts of the code beforehand. For instance, we could improve a lot on LauncherSettings and BaseLauncherSettings by providing more Propertys there.

@@ -121,6 +121,7 @@ settings_launcher_closeLauncherAfterGameStart=Launcher beim Spielstart schlie\u0
settings_launcher_launcherDirectory=Launcher Nutzerdatenverzeichnis
settings_launcher_launcherDirectory_open=\u00D6ffnen
settings_launcher_saveDownloadedFiles=Heruntergeladene Pakete behalten
settings_launcher_showPreReleases=Prereleases (Vorabversionen) und Testversionen anzeigen
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
settings_launcher_showPreReleases=Prereleases (Vorabversionen) und Testversionen anzeigen
settings_launcher_showPreReleases=Pre-Releases (Vorabversionen) und Testversionen anzeigen

Copy link
Member Author

Choose a reason for hiding this comment

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

According to https://de.wikipedia.org/wiki/Entwicklungsstadium_(Software) it's "Prerelease" 🤷 What does the Duden say about it?

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.

Tests out fine. I don't think the blank drop-down after disabling the setting with a pre-/dev-release selected is a big problem and as long as hitting the download button doesn't result in a crash or weird intermediate state that's acceptable for now.
Consider this an approval from my side, but please wait for @keturn's view on things.

Comment on lines +380 to +383
@Override
public synchronized void setShowPreReleases(boolean selected) {
showPreReleases.setValue(selected);
properties.setProperty(PROPERTY_SHOW_PRE_RELEASES, Boolean.toString(selected));
Copy link
Member

Choose a reason for hiding this comment

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

This PR introduces the use of JavaFX Properties to the LauncherSettings interface, which seems like an appropriate usage.

The API question I have is: Why use a read-only property if you also expose a setter for it?

Is two-way data binding a concept that died with Angular.js?

(I'm bringing this up in the interest of interface design because you're doing a new type of thing on this class, but I won't make it a blocking concern for this release.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure about this either ... I'd like to have a "single source of truth" for the settings in code somewhere, and I want to avoid that arbitrary places just set/change the property (although they shouldn't).

I'm not sure whether this is a good abstraction level (probably not), and I'd like to explore the design of LauncherSettings more. Can we use package visibility rules to expose a full property to some part of the code, but only read-only properties to others? Is that necessary at all (we can change the settings anyways via the setter).

Looking forward to more discussions on this 🤓

@jdrueckert jdrueckert merged commit 55ffb4b into master Aug 29, 2021
@jdrueckert jdrueckert deleted the feat/show-pre-releases branch August 29, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: UI Related to user interface (UI) or design. Type: Enhancement New features or noticable improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants