#3118 add persistent properties to replay search page #3166

obydog002 · 2024-05-11T06:05:59Z

Every field except for the game date filter when searching are now remembered after closing/opening the client and navigating away/to the replay search page.

Sheikah45

Instead of passing in a property to the add*Filter methods and adding setters to the Filter controllers it would be better to instead expose the relevant value properties of the Filter controllers with appropriate getters and setter so that the creators can then do with as they choose.

Additionally in the vaultPrefs object it is probably better to create sub objects like ReplaySearchPrefs to represent the search state.

Sheikah45

One thing we should probably additionally do is set a max date range for the replays. We currently initialize the range to be one year from today due to server load and probably don't want the persistence to undo that.

Sheikah45 · 2024-05-11T13:54:25Z

src/main/java/com/faforever/client/preferences/ReplaySearchPrefs.java

+  private final ObjectProperty<ObservableList<String>> featuredModFilter = new SimpleObjectProperty<ObservableList<String>>(
+    FXCollections.emptyObservableList());
+  private final ObjectProperty<ObservableList<String>> leaderboardFilter = new SimpleObjectProperty<ObservableList<String>>(
+    FXCollections.emptyObservableList());


These should be ListProperties

Sheikah45 · 2024-05-11T13:55:07Z

src/main/java/com/faforever/client/query/RangeFilterController.java

+  public javafx.beans.property.DoubleProperty lowValueProperty() {
+    return rangeSlider.lowValueProperty();
+  }
+
+  public javafx.beans.property.DoubleProperty highValueProperty() {
+    return rangeSlider.highValueProperty();
+  }


import DoubleProperty instead of using the full name

This one I had to write the full name as DoubleProperty clashes with the existing com.github.rutledgepaulv.qbuilders.properties.concrete.DoubleProperty import

got it I missed that

Sheikah45 · 2024-05-11T13:55:18Z

src/main/java/com/faforever/client/query/TextFilterController.java

+  public javafx.beans.property.StringProperty textFieldProperty() {
+    return textField.textProperty();
+  }


import instead of full name

same here as well

Sheikah45 · 2024-05-11T13:58:01Z

src/main/java/com/faforever/client/replay/OnlineReplayVaultController.java

+    ReplaySearchPrefs replaySearchPrefs = vaultPrefs.getReplaySearch();
+    TextFilterController textFilterController = searchController.addTextFilter("playerStats.player.login", i18n.get("game.player.username"), true);
+    textFilterController.textFieldProperty().bindBidirectional(replaySearchPrefs.playerNameFieldProperty());
+    textFilterController = searchController.addTextFilter("mapVersion.map.displayName", i18n.get("game.map.displayName"), false);
+    textFilterController.textFieldProperty().bindBidirectional(replaySearchPrefs.mapNameFieldProperty());
+    textFilterController = searchController.addTextFilter("mapVersion.map.author.login", i18n.get("game.map.author"), false);
+    textFilterController.textFieldProperty().bindBidirectional(replaySearchPrefs.mapAuthorFieldProperty());
+    textFilterController = searchController.addTextFilter("name", i18n.get("game.title"), false);
+    textFilterController.textFieldProperty().bindBidirectional(replaySearchPrefs.titleFieldProperty());
+    textFilterController = searchController.addTextFilter("id", i18n.get("game.id"), true);
+    textFilterController.textFieldProperty().bindBidirectional(replaySearchPrefs.replayIDFieldProperty());


Rather than binding bidirectionally we should just set the initial value and then bind the searchPrefs property to the filter property. Additionally since the replaySearch properties are long lived we need to limit the binding to the lifetime of the VaultController by adding .when(showing). This will cause the binding to automatically stop whenever the vault is not showing. This will prevent memory leaks

Sheikah45 · 2024-05-11T14:02:07Z

src/main/java/com/faforever/client/replay/OnlineReplayVaultController.java

+                        replaySearchPrefs.featuredModFilterProperty().get().stream().forEach((item) -> featuredModFilterController.checkItem(item));
+                        replaySearchPrefs.featuredModFilterProperty().bind(Bindings.createObjectBinding(() -> featuredModFilterController.getCheckedItems()));


There is a utility method Bindings.bindContent that can be used here. Although we have to be more careful about unbinding when the vault controller is closed. This could be easily done by adding the unsubscription with the addSubscription method of the controller

I dont understand whats needed about unbinding

ahh, to prevent memory leaks as above

I don't understand how to add an unsubscription

There is a method addShownSubscription that you add a subscripton to where a subscription is just a function interface. So just pass in a function that performs the unbinding

Sheikah45 · 2024-05-11T14:02:26Z

src/main/java/com/faforever/client/replay/OnlineReplayVaultController.java

+    RangeFilterController rangeFilterController = searchController.addRangeFilter("playerStats.ratingChanges.meanBefore", i18n.get("game.rating"),
        MIN_RATING, MAX_RATING, 10, 4, 0, value -> value + 300);
+    rangeFilterController.lowValueProperty().bindBidirectional(replaySearchPrefs.ratingMinProperty());
+    rangeFilterController.highValueProperty().bindBidirectional(replaySearchPrefs.ratingMaxProperty());

-    searchController.addRangeFilter("reviewsSummary.averageScore", i18n.get("reviews.averageScore"),0, 5, 10, 4, 1);
+    rangeFilterController = searchController.addRangeFilter("reviewsSummary.averageScore", i18n.get("reviews.averageScore"),0, 5, 10, 4, 1);
+    rangeFilterController.lowValueProperty().bindBidirectional(replaySearchPrefs.averageReviewScoresMinProperty());
+    rangeFilterController.highValueProperty().bindBidirectional(replaySearchPrefs.averageReviewScoresMaxProperty());

-    searchController.addDateRangeFilter("endTime", i18n.get("game.date"), 1);
-    searchController.addRangeFilter("replayTicks", i18n.get("game.duration"), 0, 60, 12, 4, 0, value -> (int) (value * 60 * 10));
-    searchController.addToggleFilter("validity", i18n.get("game.onlyRanked"), "VALID");
+    DateRangeFilterController dateRangerFilterController = searchController.addDateRangeFilter("endTime", i18n.get("game.date"), 1);
+    dateRangerFilterController.beforeDateProperty().bindBidirectional(replaySearchPrefs.gameBeforeDateProperty());
+    dateRangerFilterController.afterDateProperty().bindBidirectional(replaySearchPrefs.gameAfterDateProperty());


Same binding comments as above

obydog002 · 2024-05-12T02:40:47Z

One thing we should probably additionally do is set a max date range for the replays. We currently initialize the range to be one year from today due to server load and probably don't want the persistence to undo that.

Any ideas on how you want this to look?

Sheikah45 · 2024-05-12T12:55:49Z

One thing we should probably additionally do is set a max date range for the replays. We currently initialize the range to be one year from today due to server load and probably don't want the persistence to undo that.

Any ideas on how you want this to look?

Yeah I was thinking it would either be defaulting the preferences everytime or setting a limit on the range controller. Setting a limit might be easier.

…fix state issue with category filter and add test

Sheikah45 · 2024-05-14T02:15:56Z

src/main/java/com/faforever/client/replay/OnlineReplayVaultController.java

+                      .subscribe((mods) -> {
+                        featuredModFilterController.setItems(mods);
+                        replaySearchPrefs.featuredModFilterProperty().forEach((item) -> featuredModFilterController.checkItem(item));
+                        Bindings.bindContent(replaySearchPrefs.featuredModFilterProperty(), featuredModFilterController.getCheckedItems());


I would do the actual binding of the content in the onShow method as well to ensure if it is made visible and invisible the binding will still be there

If we do this then there is a race condition where the binding will sometimes get set to the empty controller items before checked items getting set here, therefore overwriting the preferences.

We could create a copy of the list during initialization and use that in the subscribe.

I dont love this

Well as it is right now there are cases where the selection would not be saved to the prefs

Sheikah45 · 2024-05-14T02:16:08Z

src/main/java/com/faforever/client/replay/OnlineReplayVaultController.java

+                      .subscribe((leaderboards) -> {
+                        leaderboardFilterController.setItems(leaderboards);
+                        replaySearchPrefs.leaderboardFilterProperty().forEach((item) -> leaderboardFilterController.checkItem(item));
+                        Bindings.bindContent(replaySearchPrefs.leaderboardFilterProperty(), leaderboardFilterController.getCheckedItems());


Same binding comment above

Sheikah45 · 2024-05-16T21:52:05Z

The code looks good but there are some failing tests

Sheikah45 · 2024-05-18T09:00:00Z

Adding a null check is not the way to fix the tests.

Rather you need to properly mock the filter builder in the test code so it returns mock versions of the controllers

obydog002 · 2024-05-19T09:46:08Z

There is still a minor issue, if no preferences have been set (i.e. empty client.prefs file) the range slider low and high values default to 0

codecov · 2024-05-19T09:46:59Z

Codecov Report

Attention: Patch coverage is 70.58824% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 58.34%. Comparing base (dd9ab2b) to head (5699227).
Report is 7 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #3166      +/-   ##
=============================================
- Coverage      58.83%   58.34%   -0.50%     
- Complexity      3984     4021      +37     
=============================================
  Files            576      579       +3     
  Lines          19296    19585     +289     
  Branches        1022     1030       +8     
=============================================
+ Hits           11353    11426      +73     
- Misses          7447     7658     +211     
- Partials         496      501       +5

Files	Coverage Δ
...a/com/faforever/client/map/MapVaultController.java	`71.95% <ø> (+4.13%)`	⬆️
...a/com/faforever/client/mod/ModVaultController.java	`59.72% <ø> (ø)`
...a/com/faforever/client/preferences/VaultPrefs.java	`52.77% <100.00%> (+2.77%)`	⬆️
...forever/client/query/CategoryFilterController.java	`82.85% <100.00%> (+1.60%)`	⬆️
...m/faforever/client/query/TextFilterController.java	`69.69% <33.33%> (-3.64%)`	⬇️
...faforever/client/query/ToggleFilterController.java	`68.18% <33.33%> (-5.51%)`	⬇️
...aforever/client/vault/search/SearchController.java	`54.82% <0.00%> (ø)`
.../faforever/client/query/RangeFilterController.java	`76.92% <33.33%> (-3.64%)`	⬇️
...ver/client/replay/OnlineReplayVaultController.java	`74.64% <90.00%> (+6.22%)`	⬆️
...aforever/client/preferences/ReplaySearchPrefs.java	`61.64% <61.64%> (ø)`

... and 14 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac45f8d...5699227. Read the comment docs.

Sheikah45 · 2024-05-19T09:50:00Z

In that case if null would be better we can make them Object property and that will be nullable

FAForever#3118 add persistent properties to replay search page

7fc8fc4

Sheikah45 requested changes May 11, 2024

View reviewed changes

review actions

70e5de3

Sheikah45 requested changes May 11, 2024

View reviewed changes

Review actions, fix subscription bug, remove date range persistency, …

74b6dda

…fix state issue with category filter and add test

Sheikah45 requested changes May 14, 2024

View reviewed changes

add initial list to check items, change binding to happen in onShow

3c90171

fix tests

7f0f964

fix tests, move subscriptions after onInitialize, clean up imports

5699227

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3118 add persistent properties to replay search page #3166

#3118 add persistent properties to replay search page #3166

obydog002 commented May 11, 2024 •

edited

Sheikah45 left a comment

Sheikah45 left a comment

Sheikah45 May 11, 2024

Sheikah45 May 11, 2024

obydog002 May 11, 2024

Sheikah45 May 11, 2024

Sheikah45 May 11, 2024

obydog002 May 11, 2024

Sheikah45 May 11, 2024

Sheikah45 May 11, 2024

obydog002 May 11, 2024

obydog002 May 11, 2024

obydog002 May 12, 2024

Sheikah45 May 12, 2024

Sheikah45 May 11, 2024

obydog002 commented May 12, 2024

Sheikah45 commented May 12, 2024

Sheikah45 May 14, 2024

obydog002 May 14, 2024

Sheikah45 May 14, 2024

obydog002 May 14, 2024

Sheikah45 May 14, 2024

Sheikah45 May 14, 2024

Sheikah45 commented May 16, 2024

Sheikah45 commented May 18, 2024

obydog002 commented May 19, 2024

codecov bot commented May 19, 2024

Sheikah45 commented May 19, 2024

		replaySearchPrefs.featuredModFilterProperty().get().stream().forEach((item) -> featuredModFilterController.checkItem(item));
		replaySearchPrefs.featuredModFilterProperty().bind(Bindings.createObjectBinding(() -> featuredModFilterController.getCheckedItems()));

#3118 add persistent properties to replay search page #3166

Are you sure you want to change the base?

#3118 add persistent properties to replay search page #3166

Conversation

obydog002 commented May 11, 2024 • edited

Sheikah45 left a comment

Choose a reason for hiding this comment

Sheikah45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obydog002 commented May 12, 2024

Sheikah45 commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sheikah45 commented May 16, 2024

Sheikah45 commented May 18, 2024

obydog002 commented May 19, 2024

codecov bot commented May 19, 2024

Codecov Report

Sheikah45 commented May 19, 2024

obydog002 commented May 11, 2024 •

edited