Merge lp:~widelands-dev/widelands/game_end_summary into lp:widelands

[cghislai]

So I have the game end screen prototypes and I need feedbacl. I am not sure if I must go for an ingame window or a fullscreen menu screen. If you test this branch, you will first see the ingame window, then if you click on Quit you will be warried to the fullscreen window.

To help you test, there is a Test Win win condition that will make the player 1 win after 15s - so make sure someone is occupying the 1st slot.
The layout and table sorting are not finished yet, but please voice your concerns.

[cghislai (charlyghislain)]

Also please tell me about the code design. Currently player states are stored in the game class. The win scripts report results to the game controller, which builds a EndStatus struct and passes it to the game. Once states for all players have been gathered, game ask the interactive_base to show the window.

[cghislai (charlyghislain)]

I think i will go for the ingame window, code is cleaner

[SirVer (sirver)]

I am sorry that I did not yet voice an opinion. I will definitively review this week.

[SirVer (sirver)]

This is still marked as wip. Is this correct?

[cghislai (charlyghislain)]

Yes, you can still fetch the branch as I only committed locally for now

[SirVer (sirver)]

I'd rather wait till you push a somewhat ready version - otherwise I comment on stuff that you will have changed already in your local branch. I wait till you set this branch to be ready for review again.

[cghislai (charlyghislain)]

It is now ready for review. I left the test win condition just in case, it should be removed before merging. I also updated the others to not popup the win/lost message.

[SirVer (sirver)]

I added a bunch of comments on this.

[SirVer (sirver)]

review: Needs Fixing

[cghislai (charlyghislain)]

Concerning the new class, I was thinking of a playersmanager that could, eventually, handle all player related stuff. Currently, a lot is done in editor_gamebase. Should I go toward that or only handle the end status?

[SirVer (sirver)]

Well, if you can find a nice separation of concerns that would be great. Keep in mind that the design has to work for the editor too though. Game and EditorGameBase are two balls of mud right now: they have too many responsibilities - I think the design would already improve if they could be split in a bunch of balls of mud with some less responsibilities and the original class would only contain those. Maybe a clearer design becomes apparent then.

What methods would you pull into the new class?

[SirVer (sirver)]

> Should I go toward that or only handle the end status?
I didn't address this question, sorry. My trigger finger is a little too quick these days :). If you have time to improve the design around those classes a bit that would be awesome - only go for it if you feel that it clearly improves the design though. If you feel like this is too much for now just make one for your new game ending stuff.

[cghislai (charlyghislain)]

I created a playersmanager class and moved all player handling code in there. As there are many code making use of the functions of editor_gamebase, i simply delegated the work for now.

I left a few comments

[SirVer (sirver)]

Some more comments here. I also changed some style and nits around. Also you forgot to check in logic/playersmanager.[h|cc]. I also think the file should be called players_manager.h and the class PlayersManager (though we are not very consistent about this either - we definitely should do a bunch of style cleanups throughout the code base).

review: Needs Fixing

[cghislai (charlyghislain)]

I went for an enum for the result (won/lost/resigned).
I also changed the report_result function to only take 3 arguments : player, result, and the info string.
I added some comment to show allowed values for that string, and parsed some of them in the game_summary screen.

[SirVer (sirver)]

lgtm - I merged this minus the test win condition.

review: Approve

[SirVer (sirver)]

Spoke too soon: You still need to check in logic/playersmanager.[h|cc].

review: Needs Fixing

[SirVer (sirver)]

and there are a few style problems. Could you run the style checker on this too, please?

review: Needs Fixing

[cghislai (charlyghislain)]

I fixed this along with some warnings. Sorry i forgot to add the files

[Hans Joachim Desserud (hjd)]

I saw some strings from this had made their way into the translation templates when the catalogues were updated. Was that unintential or because this is right around the corner? Just curious.

[cghislai (charlyghislain)]

I have no idea how could that happen

[SirVer (sirver)]

Doesn't matter any longer :). I merged this now. Thanks for all the work Charly!

review: Approve