Merge lp:~widelands-dev/widelands/bug-1489295-mine-is-exhausted into lp:widelands

Proposed by Toni Förster
Status: Merged
Merged at revision: 9090
Proposed branch: lp:~widelands-dev/widelands/bug-1489295-mine-is-exhausted
Merge into: lp:widelands
Diff against target: 40 lines (+12/-4)
1 file modified
src/logic/map_objects/tribes/production_program.cc (+12/-4)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1489295-mine-is-exhausted
Reviewer Review Type Date Requested Status
Toni Förster testing, playing, compiling Approve
hessenfarmer Pending
Review via email: mp+366727@code.launchpad.net

Commit message

when main vein is exhausted, don't show tooltip for missing food

Description of the change

I tested this with barbarians and found no wrong tooltips. Needs extensive testing though.

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4843. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/526562024.
Appveyor build 4624. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1489295_mine_is_exhausted-4624.

9091. By Toni Förster

do proper string comparison & add some logging

Revision history for this message
Toni Förster (stonerl) wrote :

It does currently not work for scouts because they do not set a proper out of resource header. So the "Did not start..." message is not shown.

Here is the log:

Production Result ()
Out of Resource ()
Production Result (Did not start scouting because Ration is missing)
Out of Resource ()
Production Result (Did not start scouting because Ration is missing)
Out of Resource ()

9092. By Toni Förster

do empty string check

Revision history for this message
Toni Förster (stonerl) wrote :

the empty string check should do the trick.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

I don't get this.

According to your log the Production result was set properly so why do we need the empty string check?

and why doing it that complicated

why not

if ( ps.production_result() != ps.descr().out_of_resource_heading() || ps.descr().out_of_resource_heading().empty()) {
   ps.set_production_result(result_string);
  }

Revision history for this message
Toni Förster (stonerl) wrote :

The empty string check is necessary for buildings with an empty out_of_resource_heading. If we don't check for an empty string, the message "Did not start scouting because Ration is missing" for scouts will never show up, maybe other buildings will have missing tooltips as well (e.g. tavern/inn), if we don't check.

I did it the "complicated" way because I found it to be easier to read. And for log output (has to be removed later on though)

Revision history for this message
Toni Förster (stonerl) wrote :

I did only check for wrong tooltips. But forgot to check for missing tooltips. *stupidme

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Ok I just implem,ented my solution locally and it works as desired. Checked the scout with the help of the input slider. the did not scout message was turned in if no food and off if food supplied. rechecked lumberjach and arena as well. did see the correct did not start messages for brewery, bakery, tavern and inn. did even check the out of fields message for reed yard and farm. found no obvious errors anymore except one little thing.

while checking the coalmine I realized that if the mine finds some coal by the 5% chance it displays the produced coal message and afterwards the did not start message is shown again although the mine is depleted. So from my perspective we should supress the "produced..." mesage in the same way if the mine is depleted.

Revision history for this message
Toni Förster (stonerl) wrote :

Okay, then just push your implementation to this branch here. I don't mind at all.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4846. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/526796763.
Appveyor build 4627. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1489295_mine_is_exhausted-4627.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

do you agree we should suppress the rare success message if mine exhausted as well?

If yes I'll implement this to this branch.

Thanks for helping solving these very old and annoying things.

Revision history for this message
Toni Förster (stonerl) wrote :

Yes I agree :-)

9093. By hessenfarmer

suppressed also the "produced ..." and the "did not start cause the economy doesn't need..." message if a valid out of ressource message is set.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

OK here are my fixes feel free to test and change again if not satisfied. I have not tested this final one though.

Revision history for this message
Toni Förster (stonerl) wrote :

Works as expected.

review: Approve (testing, playing, compiling)
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Thanks for the support about this topic.

let's have it then once our CI reports green

@bunnybot merge

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4848. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/526967000.
Appveyor build 4629. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1489295_mine_is_exhausted-4629.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4848. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/526967000.

Revision history for this message
Toni Förster (stonerl) wrote :

Transient failure on Travis

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/logic/map_objects/tribes/production_program.cc'
--- src/logic/map_objects/tribes/production_program.cc 2019-04-30 15:04:35 +0000
+++ src/logic/map_objects/tribes/production_program.cc 2019-05-01 17:12:13 +0000
@@ -565,8 +565,10 @@
565 .str();565 .str();
566 }566 }
567 }567 }
568568 if (ps.production_result() != ps.descr().out_of_resource_heading() ||
569 ps.set_production_result(result_string);569 ps.descr().out_of_resource_heading().empty()) {
570 ps.set_production_result(result_string);
571 }
570 }572 }
571 return ps.program_end(game, result_);573 return ps.program_end(game, result_);
572}574}
@@ -916,7 +918,10 @@
916 is_missing_string)918 is_missing_string)
917 .str();919 .str();
918920
919 ps.set_production_result(result_string);921 if (ps.production_result() != ps.descr().out_of_resource_heading() ||
922 ps.descr().out_of_resource_heading().empty()) {
923 ps.set_production_result(result_string);
924 }
920 return ps.program_end(game, ProgramResult::kFailed);925 return ps.program_end(game, ProgramResult::kFailed);
921 } else { // we fulfilled all consumption requirements926 } else { // we fulfilled all consumption requirements
922 for (size_t i = 0; i < inputqueues.size(); ++i) {927 for (size_t i = 0; i < inputqueues.size(); ++i) {
@@ -1006,7 +1011,10 @@
1006 /** TRANSLATORS: %s is a list of wares. String is fetched according to total amount of1011 /** TRANSLATORS: %s is a list of wares. String is fetched according to total amount of
1007 wares. */1012 wares. */
1008 (boost::format(ngettext("Produced %s", "Produced %s", count)) % ware_list).str();1013 (boost::format(ngettext("Produced %s", "Produced %s", count)) % ware_list).str();
1009 ps.set_production_result(result_string);1014 if (ps.production_result() != ps.descr().out_of_resource_heading() ||
1015 ps.descr().out_of_resource_heading().empty()) {
1016 ps.set_production_result(result_string);
1017 }
1010}1018}
10111019
1012bool ProductionProgram::ActProduce::get_building_work(Game& game,1020bool ProductionProgram::ActProduce::get_building_work(Game& game,

Subscribers

People subscribed via source and target branches

to status/vote changes: