[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Continuous integration builds have changed state: Travis build 1104. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/129436968. Appveyor build 941. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1302593_result_screen-941. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Thanks again! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
I have also done some limited testing using the 'report_result' Lua function and I can confirm that the end statues are saved and loaded correctly. Win conditions should also work properly since they make use of this function. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Review: Approve code LGTM. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Duh! Thanks for having my back. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Review: Needs Fixing code
Review.
Diff comments:
> === modified file 'src/game_io/game_player_info_packet.cc'
> --- src/game_io/game_player_info_packet.cc2016-02-16 10:27:23 +
> +++ src/game_io/game_player_info_packet.cc2016-05-11 06:51:52 +
> @@ -72,6 +73,21 @@
> player.civil_blds_defeated_ =
> fr.unsigned_32();
> }
> }
> +
> + // Result screen
> + if (packet_version >= 19) {
Version 19 needs to be excluded here.
> + PlayersManager* manager = game.player_manager();
> + const uint8_t no_endstatus = fr.unsigned_8();
> + for (uint8_t i = 0; i < no_endstatus; ++i) {
> + PlayerEndStatus status;
> + status.player = fr.unsigned_8();
> + status.result =
> static_cast(fr.unsigned_8());
> + status.time = fr.unsigned_32();
> + status.info = fr.c_string();
> + manager->set_player_end_status(status);
> + }
> + }
> +
> game.read_statistics(fr);
> } else {
> throw UnhandledVersionError("GamePlayerInfoPacket",
> packet_version, kCurrentPacketVersion);
>
> === modified file 'src/logic/playersmanager.cc'
> --- src/logic/playersmanager.cc 2016-02-16 10:27:23 +
> +++ src/logic/playersmanager.cc 2016-05-11 06:51:52 +
> @@ -120,5 +118,16 @@
> }
> }
>
> +void PlayersManager::set_player_end_status(const PlayerEndStatus& status)
> +{
> + for (auto& pes : players_end_status_) {
> + if (pes.player == status.player) {
> + pes = status;
> + return;
Additional space before 'return;'.
> + }
> + }
> + players_end_status_.push_back(status);
> +}
> +
>
> } // namespace Widelands
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1302593-result-screen.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Continuous integration builds have changed state: Travis build 1081. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/127194426. Appveyor build 912. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1302593_result_screen-912. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Bunnybot encountered an error while working on this merge proposal: HTTP Error 500: Internal Server Error -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Bunnybot encountered an error while working on this merge proposal: HTTP Error 500: Internal Server Error -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Review: Resubmit Now I get it - I forgot to use a reference instead of a copy. Should be all fixed now :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Bunnybot encountered an error while working on this merge proposal: HTTP Error 503: Service Unavailable -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Well, it does nothing useful in the original code, so it is redundant. It modifies a copy of the element (not the actual element!) that is destroyed immediately after. That said, when I wrote the comment, it did not occur to me you were trying to modify the vector's element - I thought it was just some leftover code. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Ah, so the line is not redundant then. I'll implement the other changes that you suggested :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Extended reply.
Diff comments:
>
> === modified file 'src/logic/playersmanager.cc'
> --- src/logic/playersmanager.cc 2016-02-16 10:27:23 +
> +++ src/logic/playersmanager.cc 2016-05-02 11:22:37 +
> @@ -120,5 +120,21 @@
> }
> }
>
> +void PlayersManager::set_player_end_status(const PlayerEndStatus& status)
> +{
> + bool found = false;
> + for (auto it = players_end_status_.begin(); it !=
> players_end_status_.end(); ++it) {
> + PlayerEndStatus pes = *it;
> + if (pes.player == status.player) {
> + pes = status;
You could also do "PlayerEndStatus & pes = *it; ... pes = status;" with
iterators.
> + found = true;
> + break;
> + }
> + }
> + if (!found) {
> + players_end_status_.push_back(status);
> + }
> +}
> +
>
> } // namespace Widelands
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1302593-result-screen.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Replied to the question in the diff comments.
Diff comments:
>
> === modified file 'src/logic/playersmanager.cc'
> --- src/logic/playersmanager.cc 2016-02-16 10:27:23 +
> +++ src/logic/playersmanager.cc 2016-05-02 11:22:37 +
> @@ -120,5 +120,21 @@
> }
> }
>
> +void PlayersManager::set_player_end_status(const PlayerEndStatus& status)
> +{
> + bool found = false;
> + for (auto it = players_end_status_.begin(); it !=
> players_end_status_.end(); ++it) {
> + PlayerEndStatus pes = *it;
> + if (pes.player == status.player) {
> + pes = status;
With iterators:
*it = status;
Using more modern syntax:
for (auto & pes : players_end_status_) {
if (pes.player == status.player) {
pes = status;
return;
}
}
> + found = true;
> + break;
> + }
> + }
> + if (!found) {
> + players_end_status_.push_back(status);
> + }
> +}
> +
>
> } // namespace Widelands
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1302593-result-screen.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
I have added a question in the diff comments.
Diff comments:
>
> === modified file 'src/logic/playersmanager.cc'
> --- src/logic/playersmanager.cc 2016-02-16 10:27:23 +
> +++ src/logic/playersmanager.cc 2016-05-02 11:22:37 +
> @@ -120,5 +120,21 @@
> }
> }
>
> +void PlayersManager::set_player_end_status(const PlayerEndStatus& status)
> +{
> + bool found = false;
> + for (auto it = players_end_status_.begin(); it !=
> players_end_status_.end(); ++it) {
> + PlayerEndStatus pes = *it;
> + if (pes.player == status.player) {
> + pes = status;
If this line is redundant, how do I change the status for an existing player
status while loading a savegame?
> + found = true;
> + break;
> + }
> + }
> + if (!found) {
> + players_end_status_.push_back(status);
> + }
> +}
> +
>
> } // namespace Widelands
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1302593-result-screen.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Review: Approve compile, test, code review Testprotocol -- Testing this was not that easy ... * bzr7981[bug-1302593-result-screen] * Playing "Impact" with "Autocrat" as Barbarian versus (no AI) * Imperial and Atlanters * Saved as "Test1" before defeating Imperial * Saved as "Test2" before defeating Atlanter * defeated Atlanter -> game won, was in Pause * Saved as "Test3" * Quit * Loaded "Test3", Congratulation was still visible * Loaded "Test2" * defeated Atlanter -> game won, was in Pause ? Did I miss somthing? So this test is complete. Still the code improvements proposed by Miroslav are valid. Im not that sure about some details, as I have no complete idea how saveloading works. * I will test this again once someone has changed the code. * Shall I upload those testgames to the original Bug, so someone lese may test? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1302593-result-screen. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Review: Needs Fixing code, testing
See diff comments.
Diff comments:
> === modified file 'src/game_io/game_player_info_packet.cc'
> --- src/game_io/game_player_info_packet.cc2016-02-16 10:27:23 +
> +++ src/game_io/game_player_info_packet.cc2016-05-02 11:22:37 +
> @@ -72,6 +73,19 @@
> player.civil_blds_defeated_ =
> fr.unsigned_32();
> }
> }
> +
> + // Result screen
> + PlayersManager* manager = game.player_manager();
> + const uint8_t no_endstatus = fr.unsigned_8();
This code should not be executed for packet version 19.
> + for (uint8_t i = 0; i < no_endstatus; ++i) {
> + PlayerEndStatus status;
> + status.player = fr.unsigned_8();
> + status.result =
> static_cast(fr.unsigned_8());
> + status.time = fr.unsigned_32();
> + status.info = fr.c_string();
> + manager->set_player_end_status(status);
> + }
> +
> game.read_statistics(fr);
> } else {
> throw UnhandledVersionError("GamePlayerInfoPacket",
> packet_version, kCurrentPacketVersion);
>
> === modified file 'src/logic/playersmanager.cc'
> --- src/logic/playersmanager.cc 2016-02-16 10:27:23 +
> +++ src/logic/playersmanager.cc 2016-05-02 11:22:37 +
> @@ -120,5 +120,21 @@
> }
> }
>
> +void PlayersManager::set_player_end_status(const PlayerEndStatus& status)
> +{
> + bool found = false;
This variable is not necessary. You can 'return' when a matching status is
found.
> + for (auto it = players_end_status_.begin(); it !=
> players_end_status_.end(); ++it) {
It would be neater to use the range-based for loop syntax here.
> + PlayerEndStatus pes = *it;
> + if (pes.player == status.player) {
> + pes = status;
This line is redundant.
> + found = true;
> + break;
> + }
> + }
> + if (!found) {
> + players_end_status_.push_back(status);
> + }
> +}
> +
>
> } // namespace Widelands
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1302593-result-screen.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Continuous integration builds have changed state: Travis build 1081. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/127194426. Appveyor build 912. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1302593_result_screen-912. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Bunnybot encountered an error while working on this merge proposal: The read operation timed out -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands
Continuous integration builds have changed state: Travis build 1081. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/127194426. Appveyor build 912. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1302593_result_screen-912. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1302593-result-screen/+merge/293521 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1302593-result-screen into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
