Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #675 factions Random & Nomads in Replay Info #676

Merged
merged 1 commit into from
Apr 1, 2017
Merged

fix #675 factions Random & Nomads in Replay Info #676

merged 1 commit into from
Apr 1, 2017

Conversation

GrotheFAF
Copy link
Contributor

@GrotheFAF GrotheFAF commented Mar 22, 2017 •
edited
Loading

add Random.png to Replay icons
add mod-nomads conditions to retrieveIconFaction, so random-faction selected (in game lobby) is now shown correct.

  • PR branch should be named issuenum-fix/feature/cleanup-description
  • Code is split into logical commits
  • Code has tests
  • Final commit includes "Fixes #issue" in commit message

When all builds pass and a maintainer is happy with your PR, the "ready" label will be applied. Please complete these tasks then:

  • Rebase onto develop
  • Add changelog entry
  • Remove this entire template section

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 22.184% when pulling 871a9fe on GrotheFAF:fix-replayinfo-nomads-random-faction into bb31c93 on FAForever:develop.

@Wesmania
Copy link
Contributor

#reviewed
Also seems like this is a good place for a refactor so we don't handle all the mods in one megaclass. I'll make an extra issue for that.

@duk3luk3
Copy link
Member

Thank you for that fix. I agree we need to refactor this - especiall random faction having different ID based on mod is bad. But this may not be client only problem.

@duk3luk3 duk3luk3 added this to the 0.12.x milestone Mar 23, 2017
@duk3luk3 duk3luk3 added the ready label Mar 23, 2017
@duk3luk3
Copy link
Member

duk3luk3 commented Mar 23, 2017 •
edited
Loading

I need

  • Someone to test this
  • @GrotheFAF to add changelog entry

Then I will merge it immediately.

GrotheFAF added a commit to GrotheFAF/client that referenced this pull request Mar 23, 2017
@GrotheFAF
Add changelog entry for FAForever#676
2522142
@GrotheFAF
fix FAForever#675 factions Random & Nomads in Replay Info
722c8f2 
In FA random faction = 5, but with Nomads mod nomads = 5 and random = 6.
This fixes this with a nomads mod case just for replayinfo.
Add Random.png icon
Add changelog entry for FAForever#676
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 22.184% when pulling 2522142 on GrotheFAF:fix-replayinfo-nomads-random-faction into bb31c93 on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-replayinfo-nomads-random-faction branch from 2522142 to 722c8f2 Compare March 23, 2017 13:35
@GrotheFAF
Copy link
Contributor Author

Added changelog and some commit comment. (and merged ...I like that ;-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 22.184% when pulling 722c8f2 on GrotheFAF:fix-replayinfo-nomads-random-faction into bb31c93 on FAForever:develop.

@GrotheFAF
Copy link
Contributor Author

'refactor' random faction (to say 0) just leads to an new extra case, cause we still want to show old replays right, nes pas?

@Wesmania
Copy link
Contributor

I did some work trying to refactor the class over at Wesmania/client@0e521ee. This could probably be improved, maybe replace HTML generation with some Qt .ui files. Might be a good starting point for a future larger refactor.

@duk3luk3
Copy link
Member

So now I need someone to test this :-)

@GrotheFAF
Copy link
Contributor Author

I tested it, works fine :-)

@duk3luk3
Copy link
Member

duk3luk3 commented Apr 1, 2017

Ran it in my VM, looks fine! 👍

@duk3luk3 duk3luk3 merged commit 722c8f2 into FAForever:develop Apr 1, 2017
@duk3luk3 duk3luk3 removed the ready label Apr 1, 2017
@GrotheFAF GrotheFAF deleted the fix-replayinfo-nomads-random-faction branch April 1, 2017 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.x
Development

Successfully merging this pull request may close these issues.
4 participants
@GrotheFAF @coveralls @Wesmania @duk3luk3