Show division in replays when available

[BlackYps]

WIP

[codecov]

Codecov Report

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

Project coverage is 58.84%. Comparing base (618ba92) to head (fb2576a).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #3042      +/-   ##
=============================================
+ Coverage      58.72%   58.84%   +0.12%     
- Complexity      3956     3982      +26     
=============================================
  Files            573      576       +3     
  Lines          19164    19284     +120     
  Branches        1014     1018       +4     
=============================================
+ Hits           11254    11348      +94     
- Misses          7421     7442      +21     
- Partials         489      494       +5     

Files	Coverage Δ
.../java/com/faforever/client/api/FafApiAccessor.java	90.18% <100.00%> (+0.06%)	⬆️
...a/com/faforever/client/domain/api/GameOutcome.java	100.00% <100.00%> (ø)
...m/faforever/client/domain/api/GamePlayerStats.java	66.66% <ø> (ø)
...aforever/client/domain/api/LeagueScoreJournal.java	100.00% <100.00%> (ø)
.../java/com/faforever/client/replay/DisplayType.java	100.00% <100.00%> (ø)
...a/com/faforever/client/mapstruct/ReplayMapper.java	52.17% <0.00%> (ø)
...ava/com/faforever/client/replay/ReplayService.java	77.92% <0.00%> (-1.80%)	⬇️
...om/faforever/client/game/PlayerCardController.java	63.12% <76.00%> (+7.28%)	⬆️
...aforever/client/replay/ReplayDetailController.java	77.51% <82.50%> (+3.27%)	⬆️
.../com/faforever/client/game/TeamCardController.java	64.65% <52.00%> (-5.94%)	⬇️

... and 2 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 618ba92...fb2576a. Read the comment docs.

[BlackYps]

The core functionality is there, but it is still a bit rough around the edges.
There is a slight delay because we have to fetch the division info from the api, so at the moment it behaves a bit ugly visually.
I appreciate input how this could be solved better.
Similarly the team title still displays (0) because I didn't manage to properly set the teamrating to null to trigger the removal of the rating display alltogether.
Edit: Fixed, but I don't know if the way I did it is good style

[Sheikah45]

@BlackYps were you ever going to get back to this?

There have also been a few changes to the underlying objects as an FYI.

[BlackYps]

I don't plan to abandon this, on the other hand I am really not motivated to do the refactoring that is required here.
So I guess the honest answer is, I really don't know...

[Sheikah45]

Then I might just close this in a week and it can be reopened when you revisit it.

[BlackYps]

I'm picking this up again

[BlackYps]

The population of the values of the controllers and the displaying of these values are now nicely decoupled. There is one issue though. In a matchmaker game the people that don't have a division still display the rating and the team rating is displayed as well. The problem is that the player cards without a division don't know about the presence of divisions in different player cards. We need some way to check if we found any league stats for the replay (the api call) and then disable rating display for the whole thing somehow

[Sheikah45]

For displaying the whole thing you can just have a display type enum that has a property in the player card. Then you can set that property appropriately to display either rating or league. Could even just bind it to some property in the parent.

[BlackYps]

I guess a boolean would even be enough, something like gameHasLeagueScore. I don't have to differentiate multiple states, just this binary decision

[Sheikah45]

Yeah I was just thinking of an enum to make it clearer what it signifies. Since hasLeagueScore doesn't imply as much that ratings won't be shown.

Where as something like display type would make that clearer.

[BlackYps]

yeah, I can see that.

[Sheikah45]

Was testing this out and one thing that stuck out as a little bit odd to me. Why do we require pressing the show game result to show the game outcome on the team card. Maybe we should just display it always on load?

[BlackYps]

I think this kind as introduced to make it possible to watch a game without being spoiled on which side wins

[Sheikah45]

Sure but now you can also play directly from the search result page

[BlackYps]

Hmm, that's true. Maybe we should discuss this with more users to better understand how they use this button at the moment. It's definitely an interesting suggestion, but imo it is unrelated to this PR

[Sheikah45]

What is the status of this?

[BlackYps]

From my pov it is ready

[Sheikah45]

The only consequential thing I see is a race condition between populateTeamContainers and loading the league score results.

[BlackYps]

Can I do something about that race condition?

[Sheikah45]

You could make sure that you don't try to load the league data until after the team containers are populated

[BlackYps]

I think I first need to understand the race condition a little better. As I see it we run the risk that the league info is applied to the old teamcardcontrollers, so they get cleared right after and new controllers for the new replay get added in populateTeamsContainer if this happens after the application of the league info.
Is this the correct one, or did you have a different race condition in mind?
And why is createTeamCardControllers called asyncronously? I do not immediately see any operation in that method that would make this necessary.

[Sheikah45]

They are created asynchronously so they don't block the application thread and cause the ui to freeze. And to cure the race condition you would just load the league info after create team controllers is called or make it apart of create team controllers

[Sheikah45]

There was an issue with the team card controllers being added twice but I fixed it.

[RoraRaven]

Showing division is nice, but was it necessary to remove the rating change? It means you need to check your profile after every match and keep a spreadsheet if you want to track changes rather than it being plainly displayed in the replays.

[BlackYps]

Yes, the divisions are supposed to replace the rating. The rating system is considered to be part of the backend, it's not necessary to show it to the user when the division system exists

[RoraRaven]

Are we going to get transparency in how divisions are calculated? Currently divisions are more or less meaningless, every other system (ingame player lists, lobby entry requirements, matchmaking, game quality rating) works on rating.

Divisions are really large and imprecise, you could win 15 games in a row and see no change. Even systems that obfuscate Elo / MMR (eg LoL) show points to measure how far up or down you are inside a division.

[BlackYps]

What exactly is the problem you are having? I'm trying to und erstand it so I can hopefully address it in my answer.
And did you have a look at the leaderboard tab? Your comment gives me the impression that you didn't, because you can see points there and it's not possible to win 15 games in a row without changing divisions, because each division is only 10 points big.

[RoraRaven]

I wasn't aware about the Leaderboards or Score system at all. I was under the impression that division was based on rating (e.g. Gold V being 800 - 900).

This does answer my questions.

I still don't understand why you would hide information from the player, even if you consider rating to be back-end, but that's your prerogative.