New filter system

[Marc-Spector]

Closes #2767
Closes #1946

[Marc-Spector]

I removed LiveReplayControllerTest class because there is nothing to test there

[codecov]

Codecov Report

Merging #2769 (27b2220) into develop (0af2e15) will increase coverage by 0.23%.
The diff coverage is 81.04%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #2769      +/-   ##
=============================================
+ Coverage      65.40%   65.64%   +0.23%     
- Complexity      4936     5064     +128     
=============================================
  Files            531      550      +19     
  Lines          20308    20618     +310     
  Branches        1163     1157       -6     
=============================================
+ Hits           13283    13534     +251     
- Misses          6353     6411      +58     
- Partials         672      673       +1     

Impacted Files	Coverage Δ
.../main/java/com/faforever/client/chat/ListItem.java	100.00% <ø> (ø)
.../java/com/faforever/client/fx/BrowserCallback.java	14.28% <0.00%> (+0.72%)	⬆️
...om/faforever/client/player/CountryFlagService.java	39.39% <0.00%> (-5.44%)	⬇️
...ver/client/replay/OnlineReplayVaultController.java	80.43% <0.00%> (ø)
...aforever/client/vault/search/SearchController.java	56.06% <0.00%> (+0.23%)	⬆️
...java/com/faforever/client/main/MainController.java	52.00% <25.00%> (-0.40%)	⬇️
...orever/client/filter/AbstractFilterController.java	29.16% <29.16%> (ø)
.../client/filter/converter/FeaturedModConverter.java	33.33% <33.33%> (ø)
...faforever/client/ui/list/NoFocusModelListView.java	33.33% <33.33%> (ø)
...om/faforever/client/game/GamesTableController.java	70.49% <57.89%> (-1.55%)	⬇️
... and 37 more

---

Continue to review full report at Codecov.

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

[Sheikah45]

Looking good with the structure. One thing I don't quite understand though is what are the FilterNames used for? And what is the idea behind the primary and secondary filters?

[Marc-Spector]

what are the FilterNames used for?

Filter names are necessary because different places use a specific set of filters where the same data type will be filtered. They will also be displayed in the order you specify.
For example:
In custom games (used type: game bean) we will use following set of filters:
PRIVATE_GAME, WITH_MODS, FEATURED_MOD, PLAYER_NAME, ...
In live replays (same type: game bean) we will use another set of filter:
GAME_TYPE, FEATURED_MOD, MAP_NAME, PLAYER_NAME, ...

The feature is that you don't need to duplicate an extra class to create a new set of filters of the same type.

what is the idea behind the primary and secondary filters?

Primary filters are always visible, we filter some properties that are often used.
Secondary filters are initially hidden, they can be displayed or hidden using the button.
I just implemented it, because if there are a lot of filters in one place, then you can hide some filters by making them secondary

[Sheikah45]

Got it so the names are mostly there to differentiate between the primary and secondary filters.

Also we should make sure to differentiate these filters enough from the filters in the query package that are used for the api. As things are named right now they might be easy to confuse, or question why both exist.

[Marc-Spector]

I did not complicate everything and removed the concepts of "primary" and "secondary" filters. All the specified filters are always visible

[Marc-Spector]

I think it would be nice to add a counter of how many elements are hidden for each filter. I can make it optional.

This complicates the code. I won't do it

[Marc-Spector]

I will also replace Show word withHide in the Show private games and Show modded games filters to maintain consistency in other places

[Marc-Spector]

I did not expect the PR to be big. :)

[Sheikah45]

I will review this more in depth over the weekend but had some high level thoughts.

For the country filters is there anyway you think to limit the countries shown only to those where players are actually in the list?

I noticed that the filtering is all done in the application thread I wonder if there is a way to move some of the heavy lifting the a background thread.

I am still not convinced of the need for the FilterNames especially since it looks like each class that uses them creates a custom implementation for them anyway so it isn't like there is any unity between them especially with the filterBuilder which makes it easier. As it is now I would assume that any filter can take any of the filter names but that isn't the case. Because of that I think it might be better to just construct the filters in place and add them without the filter names but I could be convinced otherwise if there is a good reason I am not seeing. And at most it looks like you are just using the names to bind the preferences which can still be done at construction. Also I think it is just better for these filter controller to be a bit more static since the pieces they are made of are very modular to anyway.

And yeah in depth PRs like this one have a way of growing ha XD

Also for records I would just make the closing bracket on the same line

[Sheikah45]

Overall looking good just a few minor things.

Also I had changed the name of some methods but then realized they were overriding abstract ones. Thought I removed all those comments but you can ignore them if I forgot to remove them.

[Sheikah45]

Maybe check if the current predicate is the old default before overwriting

[Marc-Spector]

There is no need. I do not see that it is possible to call the method twice.

[Marc-Spector]

I am still not convinced of the need for the FilterNames especially since it looks like each class that uses them creates a custom implementation for them anyway so it isn't like there is any unity between them especially with the filterBuilder which makes it easier. As it is now I would assume that any filter can take any of the filter names but that isn't the case. Because of that I think it might be better to just construct the filters in place and add them without the filter names but I could be convinced otherwise if there is a good reason I am not seeing. And at most it looks like you are just using the names to bind the preferences which can still be done at construction. Also I think it is just better for these filter controller to be a bit more static since the pieces they are made of are very modular to anyway.

Done.

For the country filters is there anyway you think to limit the countries shown only to those where players are actually in the list?

I created a local class. He works. I will add it.

I noticed that the filtering is all done in the application thread I wonder if there is a way to move some of the heavy lifting the a background thread.

Could you point out where processing the heavy code?

To be continued...

[Sheikah45]

I noticed that the filtering is all done in the application thread I wonder if there is a way to move some of the heavy lifting the a background thread.

Could you point out where processing the heavy code?

To be continued...

I can't find it anymore so it is all good.

Do you have anything more you wanted to do with this?

[Marc-Spector]

For the country filters is there anyway you think to limit the countries shown only to those where players are actually in the list?

The only thing I did not have time to do. I can do this in a separate PR in the future.

[Sheikah45]

For the country filters is there anyway you think to limit the countries shown only to those where players are actually in the list?

The only thing I did not have time to do. I can do this in a separate PR in the future.

Works for me

[BlackYps]

Great work on this @Marc-Spector, I love it!