search

FAForever / downlords-faf-client

Official client for Forged Alliance Forever
https://faforever.com
MIT License
194 stars 117 forks source link

#3118 add persistent properties to replay search page #3166

Closed obydog002 closed 1 month ago

obydog002 commented 1 month ago

https://github.com/FAForever/downlords-faf-client/issues/3119

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.

obydog002 commented 1 month ago

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 commented 1 month ago

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.

Sheikah45 commented 1 month ago

The code looks good but there are some failing tests

Sheikah45 commented 1 month ago

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 commented 1 month ago

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[bot] commented 1 month ago

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 12 commits behind head on develop.

:exclamation: Current head 5699227 differs from pull request most recent head 52bbfed

Please upload reports for the commit 52bbfed to get more accurate results.

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) | Coverage Δ | | |---|---|---| | [...a/com/faforever/client/map/MapVaultController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fmap%2FMapVaultController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9tYXAvTWFwVmF1bHRDb250cm9sbGVyLmphdmE=) | `71.95% <ø> (+4.13%)` | :arrow_up: | | [...a/com/faforever/client/mod/ModVaultController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fmod%2FModVaultController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9tb2QvTW9kVmF1bHRDb250cm9sbGVyLmphdmE=) | `59.72% <ø> (ø)` | | | [...a/com/faforever/client/preferences/VaultPrefs.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fpreferences%2FVaultPrefs.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9wcmVmZXJlbmNlcy9WYXVsdFByZWZzLmphdmE=) | `52.77% <100.00%> (+2.77%)` | :arrow_up: | | [...forever/client/query/CategoryFilterController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fquery%2FCategoryFilterController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9xdWVyeS9DYXRlZ29yeUZpbHRlckNvbnRyb2xsZXIuamF2YQ==) | `82.85% <100.00%> (+1.60%)` | :arrow_up: | | [...m/faforever/client/query/TextFilterController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fquery%2FTextFilterController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9xdWVyeS9UZXh0RmlsdGVyQ29udHJvbGxlci5qYXZh) | `69.69% <33.33%> (-3.64%)` | :arrow_down: | | [...faforever/client/query/ToggleFilterController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fquery%2FToggleFilterController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9xdWVyeS9Ub2dnbGVGaWx0ZXJDb250cm9sbGVyLmphdmE=) | `68.18% <33.33%> (-5.51%)` | :arrow_down: | | [...aforever/client/vault/search/SearchController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fvault%2Fsearch%2FSearchController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC92YXVsdC9zZWFyY2gvU2VhcmNoQ29udHJvbGxlci5qYXZh) | `54.82% <0.00%> (ø)` | | | [.../faforever/client/query/RangeFilterController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fquery%2FRangeFilterController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9xdWVyeS9SYW5nZUZpbHRlckNvbnRyb2xsZXIuamF2YQ==) | `76.92% <33.33%> (-3.64%)` | :arrow_down: | | [...ver/client/replay/OnlineReplayVaultController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Freplay%2FOnlineReplayVaultController.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9yZXBsYXkvT25saW5lUmVwbGF5VmF1bHRDb250cm9sbGVyLmphdmE=) | `74.64% <90.00%> (+6.22%)` | :arrow_up: | | [...aforever/client/preferences/ReplaySearchPrefs.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fcom%2Ffaforever%2Fclient%2Fpreferences%2FReplaySearchPrefs.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9wcmVmZXJlbmNlcy9SZXBsYXlTZWFyY2hQcmVmcy5qYXZh) | `61.64% <61.64%> (ø)` | | ... and [14 files with indirect coverage changes](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Last update [4030778...52bbfed](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3166?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever).
Sheikah45 commented 1 month ago

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

obydog002 commented 1 month ago

Yes I believe so