FAForever / downlords-faf-client

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

Display more player information for replay cards #3084

Closed obydog002 closed 5 months ago

obydog002 commented 7 months ago

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

This PR only addresses the first part and not the replay id. Adds player factions to replays and allows right click on player names

obydog002 commented 7 months ago

faf-replay-teams-demo

Sheikah45 commented 7 months ago

The cutoff of the player names is a bigger loss than the gain from the extra information from my point of view

obydog002 commented 6 months ago

Hmm, I agree. Would it then make sense to change the style such that more of the names are visible (since its left aligned) and give more space for the teams, or remove the rating for players?

Or should I not bother?

Sheikah45 commented 6 months ago

I think the names shouldn't be cutoff really in any situation for a less than 2 team game.

As far as the styling outside of that I don't have great guidelines unfortunately

obydog002 commented 6 months ago

3teams

demo2

codecov[bot] commented 6 months ago

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (563b9e1) 59.20% compared to head (f9bbd8a) 58.91%.

:exclamation: Current head f9bbd8a differs from pull request most recent head 2e77a5d. Consider uploading reports for the commit 2e77a5d to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #3084 +/- ## ============================================= - Coverage 59.20% 58.91% -0.29% + Complexity 4479 4397 -82 ============================================= Files 574 568 -6 Lines 20443 20087 -356 Branches 1012 982 -30 ============================================= - Hits 12103 11835 -268 + Misses 7806 7737 -69 + Partials 534 515 -19 ``` | [Files](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3084?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) | Coverage Δ | | |---|---|---| | [...om/faforever/client/game/PlayerCardController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3084?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9nYW1lL1BsYXllckNhcmRDb250cm9sbGVyLmphdmE=) | `55.73% <25.00%> (-1.41%)` | :arrow_down: | | [.../faforever/client/replay/ReplayCardController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3084?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9yZXBsYXkvUmVwbGF5Q2FyZENvbnRyb2xsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | | ... and [35 files with indirect coverage changes](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3084/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/3084?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/3084?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Last update [563b9e1...2e77a5d](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/3084?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).
obydog002 commented 6 months ago

Theres a bit of a highlighting issue when mousing over the names to click

Brutus5000 commented 6 months ago

Your example screens will probably break if people have avatars. Or where these removed?

obydog002 commented 6 months ago

@Brutus5000 thats a good point, didnt consider that

Sheikah45 commented 6 months ago

@obydog002 What is the status of this?

obydog002 commented 6 months ago

Had a holiday, but still progressing the PR. Will have some updates in the week

obydog002 commented 6 months ago

Theres a few problems:

obydog002 commented 6 months ago

The lag is natural because we have to download the images and load them into the UI

Is there any way to preemptively download the images so they appear instantly?

Sheikah45 commented 6 months ago

It is the imageview object actually getting initialized with the pixels.

We already do cache all the images but every image and imageview is set to load in the background so as to not block the UI

obydog002 commented 6 months ago

What else needs to be done for this? @Sheikah45

Sheikah45 commented 6 months ago

I will run it and check out the visuals. Can you also remove some of the unused imports.

obydog002 commented 5 months ago

Yes, Ill add ReplayCardController tests in a seperate PR.