Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/robust-file-saving into lp:widelands
Sorry, I am very busy lately and was not able to do anything Widelands-related or even look here at all. This will likely continue for quite some time, but I'll have a look at it this weekend. -- https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/robust-file-saving. ___ 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/robust-file-saving into lp:widelands
Merry Christmas, and thanks for testing. As for "ways to break it", there *should* at least be no new ways to do so. After all, the main purpose of this branch is to prevent any such breaking when we save maps/games. (And there aren't a lot of new things that could break on their own.) The actual saving routine itself could potentially break somewhere (I didn't change anything there), but as long as that involves throwing an exception, it will be caught. Under Windows I tested this quite a lot myself. Usually, errors should "normally" not happen without someone trying to force them. Error scenarios that could be forced for testing would be: 1) after choosing a dir and filename, replace the dir by a non-dir file of that name, then try saving -> dir creation fails 2) lock a file (e.g. open it with a zip packer or so that locks the file), then try saving to that file -> renaming (for backup) fails 3) make a file read-only, then try saving to that file -> deleting backup (renamed file) fails (no ingame message though) 4) try saving on a read-only medium -> saving data fails (There might be slight differences between platforms, especially with the file locking.) Other errors would actually be hard to set off on purpose. That would likely require different processes to access the same file at the same time. Hard to even force, and extremely unlikely to happen by accident, but such errors are caught just in case. (In my testing for those scenarios I just temporarily added exceptions throws at some places.) -- https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/robust-file-saving. ___ 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/bugfix-1805042-S2map-preloader-checks into lp:widelands
Arty has proposed merging
lp:~widelands-dev/widelands/bugfix-1805042-S2map-preloader-checks into
lp:widelands.
Commit message:
validity checks for S2 map headers to avoid crashes when preloading invalid S2
map files
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1805042 in widelands: "segfault when trying to preload invalid file"
https://bugs.launchpad.net/widelands/+bug/1805042
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bugfix-1805042-S2map-preloader-checks/+merge/360762
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bugfix-1805042-S2map-preloader-checks into
lp:widelands.
=== modified file 'src/map_io/s2map.cc'
--- src/map_io/s2map.cc 2018-04-11 06:55:01 +
+++ src/map_io/s2map.cc 2018-12-12 00:02:20 +
@@ -61,6 +61,29 @@
char bulk[2290]; // unknown
} /* size 2352 */;
+// Some basic checks to identify obviously invalid headers
+bool is_valid_header(const S2MapDescrHeader& header) {
+ if (strncmp(header.magic, "WORLD_V1.0", 10)) {
+ return false;
+ }
+ if (header.name[19]) {
+ return false;
+ }
+ if (header.w <= 0 || header.h <= 0) {
+ return false;
+ }
+ if (header.uses_world < 0 || header.uses_world > 2) {
+ return false;
+ }
+ if (header.nplayers < 0 || header.nplayers > 7) {
+ return false;
+ }
+ if (header.author[19]) {
+ return false;
+ }
+ return true;
+}
+
// TODO(unknown): the following bob types appear in S2 maps but are unknown
// Somebody who can run Settlers II please check them out
// 11 (0x0B)
@@ -400,9 +423,11 @@
}
/**
- * Load informational data of an S2 map
+ * Loads informational data of an S2 map.
+ * Throws exception if data is invalid.
*/
void S2MapLoader::load_s2mf_header(FileRead& fr) {
+ // no need to check file size: fr.data(..) already throws if the file is too small
S2MapDescrHeader header;
memcpy(&header, fr.data(sizeof(header)), sizeof(header));
@@ -414,6 +439,11 @@
header.h = swap_16(header.h);
#endif
+ // Check header validity to prevent unexpected crashes later
+ if (!is_valid_header(header)) {
+ throw wexception("invalid S2 file");
+ }
+
// don't really set size, but make the structures valid
map_.width_ = header.w;
map_.height_ = header.h;
___
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/robust-file-saving into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/robust-file-saving into lp:widelands has been updated. Commit message changed to: Introduces a new class to robustly handle saving of files (incl. dealing with backups, handling errors, etc.) and use it whenever maps/games are being saved. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-file-saving 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/io-dependency-fix into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/io-dependency-fix into lp:widelands has been updated. Commit message changed to: Remove a few superfluous includes which also breaks a small dependency cycle For more details, see: https://code.launchpad.net/~widelands-dev/widelands/io-dependency-fix/+merge/359658 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/io-dependency-fix 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/io-dependency-fix into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/io-dependency-fix into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/io-dependency-fix/+merge/359658 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/io-dependency-fix into lp:widelands. === modified file 'src/io/filesystem/CMakeLists.txt' --- src/io/filesystem/CMakeLists.txt 2018-05-16 13:51:48 + +++ src/io/filesystem/CMakeLists.txt 2018-11-27 23:25:38 + @@ -18,7 +18,6 @@ base_log base_macros graphic_text_layout -io_fileread io_stream third_party_minizip ) === modified file 'src/io/filesystem/disk_filesystem.cc' --- src/io/filesystem/disk_filesystem.cc 2018-11-14 10:14:50 + +++ src/io/filesystem/disk_filesystem.cc 2018-11-27 23:25:38 + @@ -41,7 +41,6 @@ #include #endif -#include "base/log.h" #include "base/macros.h" #include "base/wexception.h" #include "io/filesystem/filesystem_exceptions.h" === modified file 'src/io/filesystem/layered_filesystem.cc' --- src/io/filesystem/layered_filesystem.cc 2018-05-17 05:01:45 + +++ src/io/filesystem/layered_filesystem.cc 2018-11-27 23:25:38 + @@ -22,9 +22,7 @@ #include #include -#include "base/log.h" #include "base/wexception.h" -#include "io/fileread.h" #include "io/streamread.h" LayeredFileSystem* g_fs; ___ 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-1796364-blinking-buildings into lp:widelands
I finished looking over it but admittedly not too deeply because it's just old code re-enabled. Code looks fine (aside from the ~ operator mentioned in the diff comment). -- https://code.launchpad.net/~widelands-dev/widelands/bug-1796364-blinking-buildings/+merge/359348 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1796364-blinking-buildings 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1796364-blinking-buildings into lp:widelands
Started reviewing the code. Have one diff comment so far. Will continue later
tonight.
Diff comments:
>
> === added file 'src/logic/map_objects/draw_text.h'
> --- src/logic/map_objects/draw_text.h 1970-01-01 00:00:00 +
> +++ src/logic/map_objects/draw_text.h 2018-11-24 07:43:42 +
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2006-2018 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +#ifndef WL_LOGIC_MAP_OBJECTS_DRAW_TEXT_H
> +#define WL_LOGIC_MAP_OBJECTS_DRAW_TEXT_H
> +
> +enum class TextToDraw {
> + kNone = 0,
> + kCensus = 1,
> + kStatistics = 2,
> +};
> +
> +inline TextToDraw operator|(TextToDraw a, TextToDraw b) {
> + return static_cast(static_cast(a) |
> static_cast(b));
> +}
> +inline TextToDraw operator&(TextToDraw a, TextToDraw b) {
> + return static_cast(static_cast(a) &
> static_cast(b));
> +}
> +inline TextToDraw operator~(TextToDraw a) {
> + return static_cast(~static_cast(a));
That might be dangerous. The result of the ~ is out of the range of the enum,
and - as far as I could find out - that means that the actual result is
unspecified, i.e., compilers might do unexpected things.
I coudn't actually find a lot where this is explicitly mentioned, but it's
mentioned for example in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2347.pdf (page 12,
first paragraph) which I think was the latest proposal for enum class which
eventually got accepted.
A safe version would be
static_cast(3 & ~static_cast(a))
althought this "3" would have to be adjusted if at some point the enum gets
extended.
Or maybe we could just not define this operator. Simple checks can also easily
be done without it. (And generally there shouldn't be any need for elobrate
expressions where the operator would come in handy.)
> +}
> +
> +#endif // end of include guard: WL_LOGIC_MAP_OBJECTS_DRAW_TEXT_H
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1796364-blinking-buildings/+merge/359348
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1796364-blinking-buildings 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/terrain_affinity_as_int into lp:widelands
Review: Approve Looks good now. The comment with the assciativity is kinda obsolete now though. And using int64_t wasn't really necessary now that you use the weights linearly. -- https://code.launchpad.net/~widelands-dev/widelands/terrain_affinity_as_int/+merge/358299 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/terrain_affinity_as_int. ___ 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/terrain_affinity_as_int into lp:widelands
Yes, the exponential function is the problem. Approximating it with highish
precision using only integer calculations would require a lot of computation,
which isn't really feasible. There are reasonable options though if we don't
mind straying a little. The function we use here is basically a
three-dimensional Gauß-curve, and I think we could find an alternative that
roughly preserves the shape and can be done with a small number of integer
operations.
It's probably not necessary though, but if we keep getting desyncs because of
such platform/compiler-dependent computation differences, this would provide an
option.
I also replied to your replies.
Diff comments:
>
> === modified file 'src/logic/map_objects/terrain_affinity.cc'
> --- src/logic/map_objects/terrain_affinity.cc 2018-04-07 16:59:00 +
> +++ src/logic/map_objects/terrain_affinity.cc 2018-11-16 06:27:07 +
> @@ -33,85 +33,92 @@
>
> namespace {
>
> +// Literature on cross-platform floating point precision-problems:
> +// https://arxiv.org/abs/cs/0701192
> +// Monniaux, David (2008): "The pitfalls of verifying floating-point
> computations",
> +// in: ACM Transactions on Programming Languages and Systems 30, 3 (2008) 12.
> +//
> +// Recommends using heximal float constants, but we'd need to switch to
> C++17 for that.
> +//
> +//
> https://randomascii.wordpress.com/2012/03/21/intermediate-floating-point-precision/
> +
> constexpr double pow2(const double& a) {
> return a * a;
> }
>
> // Helper function for probability_to_grow
> // Calculates the probability to grow for the given affinity and terrain
> values
> -double calculate_probability_to_grow(const TerrainAffinity& affinity,
> - double terrain_humidity,
> - double terrain_fertility,
> - double terrain_temperature) {
> -
> - constexpr double kHumidityWeight = 0.500086642549548;
> - constexpr double kFertilityWeight = 0.5292268046607387;
> - constexpr double kTemperatureWeight = 61.31300863608306;
> -
> - const double sigma_humidity = (1. - affinity.pickiness());
> - const double sigma_temperature = (1. - affinity.pickiness());
> - const double sigma_fertility = (1. - affinity.pickiness());
> -
> - return exp((-pow2((affinity.preferred_fertility() - terrain_fertility) /
> - (kFertilityWeight * sigma_fertility)) -
> - pow2((affinity.preferred_humidity() - terrain_humidity) /
> - (kHumidityWeight * sigma_humidity)) -
> - pow2((affinity.preferred_temperature() -
> terrain_temperature) /
> - (kTemperatureWeight * sigma_temperature))) /
> -2);
> +inline unsigned int calculate_probability_to_grow(const TerrainAffinity&
> affinity,
> + int terrain_humidity,
> + int terrain_fertility,
> + int terrain_temperature) {
> + constexpr double kHumidityWeight = 5.00086642549548;
> + constexpr double kFertilityWeight = 5.292268046607387;
> + constexpr double kTemperatureWeight = 0.6131300863608306;
> +
> +// Avoid division by 0
> +assert(affinity.pickiness() < 100);
> + const double sigma = std::floor(100.0 - affinity.pickiness());
> +
> + const double result = exp(-
> + (pow2(((affinity.preferred_fertility() -
> terrain_fertility) / kFertilityWeight) / sigma) +
> + (pow2(((affinity.preferred_humidity() -
> terrain_humidity) / kHumidityWeight) / sigma) +
The operator "+" is left-to-right associative in C++
(https://en.cppreference.com/w/cpp/language/operator_precedence) so without
those extra brackets the evaluation (inside the "exp(-(...))") would always be
"(fertility_part + humidity_part) + temperature_part".
for every compiler. You are simply forcing it to always be
"fertility_part + (humidity_part + temperature_part)", which is different but
not inherently better.
Of course those two different evaluation orders could have slightly different
results, but the order is specified by the expression, so every compiler would
use the same order anyway.
Operator associativity is defined by the C++ standard, so compiler
optimizations should not be able to change the order, but if they were then the
extra brackets wouldn't stop them either.
Anyway, it doesn't really matter which way, I just think that with such a big
expression we don't need to make it even harder to read by more brackets,
especially not to separate those three semantically similar terms.
> +pow2(((affinity.preferred_temperature() -
> terrain_temperature) / kTemperatureWeight) / sigma))) / 2.0);
> +
> +return static_cast(std::max(0.0, std::floor(result *
> static_cast(TerrainAffinity::kPrecisionFactor;
> }
>
> } // n
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
Review: Needs Fixing
Reviewed the code and tested quite a lot. Looks pretty good, but still doesn't
work quite right. Not hard to fix though.
1) Now that you changed the design to requiring the buildcost table to be empty
instead of listing wares with amount 0, the dismantle button does not appear
for such empty tables. This is easily fixed in ‘wui/buildingwindow.cc’ by
removing a ware check, i.e., removing lines 287 and 297 (and unindenting the
block inbetween). I already tested this, it works fine.
2) Having no items shown in the box when hovering over the dismantle button
looks good enough for me, but I can imagine that some might find this not nice.
Maybe adding a little text “nothing” or so might be good. And (at a later
point) maybe even a little icon.
3) See diff comments.
Diff comments:
>
> === modified file 'data/campaigns/emp04.wmf/scripting/tribes/farm1.lua'
> --- data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-09-09
> 17:58:55 +
> +++ data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-11-21
> 09:48:10 +
> @@ -10,10 +10,6 @@
> enhancement = "empire_farm2",
>
> return_on_dismantle = {
> - planks = 0,
> - granite = 0,
> - marble = 0,
> - marble_column = 0
> },
For farm1, return_on_dismantle should now be gone completely for the campaign
to work properly. (The farm click check relies on having no dismantle button,
and the text also says that the farms can't be dismantled.)
>
> animations = {
>
> === modified file 'src/logic/map_objects/buildcost.cc'
> --- src/logic/map_objects/buildcost.cc2018-04-07 16:59:00 +
> +++ src/logic/map_objects/buildcost.cc2018-11-21 09:48:10 +
> @@ -37,32 +35,37 @@
> Buildcost::Buildcost(std::unique_ptr table, const Tribes& tribes)
> : std::map() {
> for (const std::string& warename : table->keys()) {
> - int32_t value = INVALID_INDEX;
> - try {
> - DescriptionIndex const idx =
> tribes.safe_ware_index(warename);
> - if (count(idx)) {
> - throw GameDataError(
> -"A buildcost item of this ware type has
> already been defined: %s", warename.c_str());
> - }
> - value = table->get_int(warename);
> - const uint8_t ware_count = value;
> - if (ware_count != value) {
> - throw GameDataError("Ware count is out of range
> 1 .. 255");
> - }
> - insert(std::pair(idx,
> ware_count));
> - } catch (const WException& e) {
> - throw GameDataError("[buildcost] \"%s=%d\": %s",
> warename.c_str(), value, e.what());
> - }
> + // Read ware index
> + if (!tribes.ware_exists(warename)) {
> + throw GameDataError("Buildcost: Unknown ware: %s",
> warename.c_str());
> + }
> + DescriptionIndex const idx = tribes.safe_ware_index(warename);
> + if (count(idx) != 0) {
This condition is actually never true, because table->keys() returns a
std::set, so even if a key appears multiple times in a lua table, it only
occurs once in the set, so this loop also touches each key only once.
This is unrelated to this branch though, but we should probably fix this at
some point. Maybe this should be fixed more generally anyway. (Not even sure if
multiple occurrences of the same key make sense somewhere.) I guess I’ll make a
proper bug report after thinking about it a bit more. But if you know a simple
way to check this properly here, you can just add a fix here.
> + throw GameDataError(
> +"Buildcost: Ware '%s' is listed twice",
> warename.c_str());
> + }
> +
> + // Read value
> + int32_t value = table->get_int(warename);
> + if (value < 1) {
> + throw GameDataError("Buildcost: Ware count needs to be
> > 0 in \"%s=%d\".\nEmpty buildcost tables are allowed if you wish to have an
> amount of 0.", warename.c_str(), value);
> + } else if (value > 255) {
> + throw GameDataError("Buildcost: Ware count needs to be
> <= 255 0 in \"%s=%d\".", warename.c_str(), value);
1) The 0 after 255 should go away.
2) This throw actually crashes the whole application. The problem is the “<” in
the text which - I assume - causes a crash in the rich text handling (attempt
to interpret it as opening a tag) when trying to show the message box with the
error. (Didn’t check this thoroughly yet, but simply deleting the “<” from the
message avoids the crash. I’ll check this properly and make a bug report.)
For now, just replace the “<=” by “at most” or so.
> + }
> +
> + // Add
> + insert(std::pair(idx, value));
> }
> }
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/terrain_affinity_as_int into lp:widelands
Review: Needs Fixing
I reviewed the code and also tested somewhat. Still has a few issues, most of
them minor though. See diff comments.
The file data\world\immovables\bush1\init.lua still has a few comments (lines
86-94) referring to the floating point values, this should also be changed in
this branch.
Overall it should be much less likely now to get different results based on
platform/compiler dependent precision choices, but they might still happen.
There are still things based on floating point calculations (like the main
probability calculation in calculate_probability_to_grow) but they might be
difficult to replace with pure integer based stuff.
That said, considering that the ratios of the final probabilities of the six
best immovables already change significantly with this branch, we could
possibly also change the basic probability calculation to somewhat that uses
only integer operations but is not too far off from the current approach (which
seems to have been based roughly on multivariate normal distributions).
Btw, where did those three weight constants in calculate_probability_to_grow
originally come from? Their precision is too high to be from a simple "try and
error until it feels right" approach. They still feel quite arbitrary though.
Diff comments:
>
> === modified file 'src/logic/map_objects/terrain_affinity.cc'
> --- src/logic/map_objects/terrain_affinity.cc 2018-04-07 16:59:00 +
> +++ src/logic/map_objects/terrain_affinity.cc 2018-11-16 06:27:07 +
> @@ -33,85 +33,92 @@
>
> namespace {
>
> +// Literature on cross-platform floating point precision-problems:
> +// https://arxiv.org/abs/cs/0701192
> +// Monniaux, David (2008): "The pitfalls of verifying floating-point
> computations",
> +// in: ACM Transactions on Programming Languages and Systems 30, 3 (2008) 12.
> +//
> +// Recommends using heximal float constants, but we'd need to switch to
> C++17 for that.
> +//
> +//
> https://randomascii.wordpress.com/2012/03/21/intermediate-floating-point-precision/
> +
> constexpr double pow2(const double& a) {
Can you please rename that to "sqr" or some such? ("pow2" usually stands for
computing powers of two. It's really irritating here.) The function is only
used three times in the function below, so a renaming is also quick to apply.
> return a * a;
> }
>
> // Helper function for probability_to_grow
> // Calculates the probability to grow for the given affinity and terrain
> values
> -double calculate_probability_to_grow(const TerrainAffinity& affinity,
> - double terrain_humidity,
> - double terrain_fertility,
> - double terrain_temperature) {
> -
> - constexpr double kHumidityWeight = 0.500086642549548;
> - constexpr double kFertilityWeight = 0.5292268046607387;
> - constexpr double kTemperatureWeight = 61.31300863608306;
> -
> - const double sigma_humidity = (1. - affinity.pickiness());
> - const double sigma_temperature = (1. - affinity.pickiness());
> - const double sigma_fertility = (1. - affinity.pickiness());
> -
> - return exp((-pow2((affinity.preferred_fertility() - terrain_fertility) /
> - (kFertilityWeight * sigma_fertility)) -
> - pow2((affinity.preferred_humidity() - terrain_humidity) /
> - (kHumidityWeight * sigma_humidity)) -
> - pow2((affinity.preferred_temperature() -
> terrain_temperature) /
> - (kTemperatureWeight * sigma_temperature))) /
> -2);
> +inline unsigned int calculate_probability_to_grow(const TerrainAffinity&
> affinity,
> + int terrain_humidity,
> + int terrain_fertility,
> + int terrain_temperature) {
> + constexpr double kHumidityWeight = 5.00086642549548;
> + constexpr double kFertilityWeight = 5.292268046607387;
> + constexpr double kTemperatureWeight = 0.6131300863608306;
> +
> +// Avoid division by 0
> +assert(affinity.pickiness() < 100);
> + const double sigma = std::floor(100.0 - affinity.pickiness());
> +
> + const double result = exp(-
> + (pow2(((affinity.preferred_fertility() -
> terrain_fertility) / kFertilityWeight) / sigma) +
> + (pow2(((affinity.preferred_humidity() -
> terrain_humidity) / kHumidityWeight) / sigma) +
One pair of brackets is obsolete: The opening bracket before "pow2" in the line
above, and the corresponding closing bracket in the line below. (The just group
the humidity and temperature part together in the sum, but the swapped
summation order won't give any benefit here.)
> +pow2(((affinity.preferred_temperature() -
> terrain_temperature) / kTemperatureWeight) / sigma))) / 2.0);
> +
> +return stat
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
I got ninja'd yesterday when I reviewed the code. :-) As for the "needs positive return on dismantle" condition, I don't have a strong opinion, but I'd prefer to not have it, even though in most circumstances dismantling without return would be useless. My reasons: 1) The condition - even though it makes sense - doesn't really add anything. The game would work fine without it. And regular buildings (and most special builidings in campaigns) have proper return on dismantle amounts anyway, so I wouldn't expect "false" bug reports about this. 2) Not having the condition allows for a little more flavour in missions. Like in emp04. Or maybe there'll be a mission where destroying would come with a fire hazard to neighboring buildings, possibly destroying them, too. (Not sure this is possibly atm, i.e., whether we can detect the difference between dismantling and destroying via mission scripts. Just a general idea.) 3) If the building is not far from a warehouse with a builder available, dismantling without return is even a little faster than destroying. Difference is minor though. -- https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle. ___ 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/empire04_unused_key_return_on_dismantle_no_ui into lp:widelands
Code looks good in principle, but haven't tested yet whether it really works as
intended.
Two minor nits:
1) see diff comment
2) All the new lines (except when they were copied over) are indented with
spaces instead of tabs.
Diff comments:
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc 2018-09-23 11:10:56 +
> +++ src/logic/map_objects/tribes/building.cc 2018-11-16 06:07:58 +
> @@ -151,9 +156,9 @@
> return_enhanced_ =
>
> Buildcost(table.get_table("return_on_dismantle_on_enhanced"),
> egbase_.tribes());
> } catch (const WException& e) {
> - throw wexception("An enhanced building must define
> \"enhancement_cost\""
> + throw wexception("The enhanced building '%s' must
> define \"enhancement_cost\""
This message is (and was before) not really related to this catch. If this
catch block is reached then either because the other key
("return_on_dismantle_on_enhanced") wasn't defined or because the Buildcost
constructor threw when trying to read one of the table.
I think you should just get rid of the whole try-catch here and do it the same
way you already did in the "buildcost" part above, i.e., check if the second
key ("return_on_dismantle_on_enhanced") is defined and throw if not, but don't
actually catch any exceptions. The Buildcost constructor will throw with a
proper message anyway if it fails, no need to catch that and throw again with a
message that is possibly even unrelated.
>"and
> \"return_on_dismantle_on_enhanced\": %s",
> - e.what());
> + name().c_str(), e.what());
> }
> }
>
--
https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui/+merge/358305
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui.
___
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-1798297-locale-C into lp:widelands
Code LGTM, just some minor indentation irregularities (see comments below).
Not tested because I don't have a working Linux system atm.
Diff comments:
> === modified file 'src/base/i18n.cc'
> --- src/base/i18n.cc 2018-08-12 06:47:11 +
> +++ src/base/i18n.cc 2018-11-16 06:30:53 +
> @@ -290,15 +290,26 @@
> found = alt_str.find(',', 0);
> }
> if (leave_while) {
> - setenv("LANG", locale.c_str(), 1);
> + setenv("LC_ALL", locale.c_str(), 1);
> +setenv("LANG", locale.c_str(), 1);
indent should be tabs instead of spaces
> setenv("LANGUAGE", locale.c_str(), 1);
> } else {
> - log("No corresponding locale found - trying to set it via
> LANGUAGE=%s, LANG=%s\n",
> - lang.c_str(), lang.c_str());
> + log("No corresponding locale found\n");
> +log(" - Set LANGUAGE, LANG and LC_ALL to '%s'\n",
indent again
> + lang.c_str());
> +
> setenv("LANGUAGE", lang.c_str(), 1);
> setenv("LANG", lang.c_str(), 1);
> - SETLOCALE(LC_MESSAGES, ""); // set locale according to the
> env. variables
> - // --> see $ man 3 setlocale
> +setenv("LC_ALL", lang.c_str(), 1);
> +
> +try {
> +SETLOCALE(LC_MESSAGES, "en_US.utf8"); // set locale according
> to the env. variables
> + // --> see $ man 3 setlocale
> +log(" - Set system locale to 'en_US.utf8' to make '%s'
> accessible to libintl\n", lang.c_str());
> +} catch (std::exception&) {
> +SETLOCALE(LC_MESSAGES, ""); // set locale according to the env.
> variables
> + // --> see $ man 3 setlocale
> +}
> // assume that it worked
indent again (in all 10 lines above)
> // maybe, do another check with the return value (?)
> locale = lang;
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1798297-locale-C/+merge/358364
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1798297-locale-C 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/robust-file-saving into lp:widelands
Oops, forgot to enable codecheck again. Will be fixed in the next round. -- https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-file-saving 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/robust-file-saving into lp:widelands
Switched to a type safe error type. -- https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-file-saving 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
Fixed those things.
Sure, I can do code reviews. I have looked at other active reviews before but
mostly ignored them because they were either trivial one-line fixes or part of
the code I wasn't familiar with at all yet.
Anything in particular I should look out for in code reviews?
Diff comments:
>
> === modified file 'src/wui/load_or_save_game.cc'
> --- src/wui/load_or_save_game.cc 2018-10-26 07:09:29 +
> +++ src/wui/load_or_save_game.cc 2018-11-14 12:37:16 +
> @@ -269,12 +276,69 @@
> table_.focus();
> }
> if (do_delete) {
> + // Failed deletions aren't a serious problem, we just catch the
> errors
> + // and keep track to notify the player.
> + std::set failed_selections;
> + bool failed;
> for (const uint32_t index : selections) {
> + failed = false;
> const std::string& deleteme = get_filename(index);
> - g_fs->fs_unlink(deleteme);
> - if (filetype_ == FileType::kReplay) {
> - g_fs->fs_unlink(deleteme + kSavegameExtension);
> - }
> + try {
> + g_fs->fs_unlink(deleteme);
> + } catch (const FileError& e) {
> + log("player-requested file deletion failed:
> %s", e.what());
> + failed = true;
> + }
> + if (filetype_ == FileType::kReplay) {
> + try {
> + g_fs->fs_unlink(deleteme +
> kSavegameExtension);
> + // If at least one of the two relevant
> files of a replay are
> + // successfully deleted then count it
> as success.
> + // (From the player perspective the
> replay is gone.)
Technically, in this branch a remaining savegame (when the deletion of the
replay file succeeded) would not be cleaned up (but this issue was there
before).
This is already fixed in the "robust file saving" branch though. When I added
the new cleanup routine there, I also fixed the other cleanup routines with it
(mainly catching file errors but also fixing this particular issue).
> + failed = false;
> + // If it was a multiplayer replay, also
> delete the synchstream. Do
> + // it here, so it's only attempted if
> replay deletion was successful.
> + if (g_fs->file_exists(deleteme +
> kSyncstreamExtension)) {
> + g_fs->fs_unlink(deleteme +
> kSyncstreamExtension);
> + }
> + } catch (const FileError& e) {
> + log("player-requested file deletion
> failed: %s", e.what());
> + }
> + }
> + if (failed) {
> + failed_selections.insert(index);
> + }
> + }
> + if (!failed_selections.empty()) {
> + // Notify the player.
> + std::string caption = ngettext("Error Deleting File!",
I actually thought about this, but saw that in a Russian po-file all the
plurals are defined for strings without placeholder anyway. Should also have
checked whether it's actually displayed properly.
There was another older wrong use of this in this cc-file, I fixed this one,
too. (Or shouldn't I have? I figured there'd be new translations to do anyway.)
> +"Error Deleting Files!", failed_selections.size());
> + if (filetype_ == FileType::kReplay) {
> + if (selections.size() == 1) {
> + header = _("The replay could not be
> deleted.");
> + } else {
> + header =
> +(boost::format(ngettext("%s replay
> could not be deleted.",
> +"%s replays could not be deleted.",
> failed_selections.size()))
> +% failed_selections.size()).str();
> + }
> + } else {
> + if (selections.size() == 1) {
> + header = _("The game could not be
> deleted.");
> + } else {
> + header =
> +(boost::format(ngettext("%s game
> could not be deleted.",
> +"%s games could not be deleted.",
> failed_selections.size(
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/robust-file-saving into lp:widelands
I admit the bitmask stuff is still my default way from before C++11, without having to re-define operators or typecast all the time. But you are right, I should do this properly with enum class now. I'll do that. As for the code duplication between the game saving and map saving, I feel that this doesn't really belong into the GenericSaveHandler. I wanted to keep it on a separate abstraction level, in particular without relying on any UI stuff or other user interaction. (I am not even sure I should have put the generation of localized messages in there.) The duplication part you pointed out isn't even that big (and half of it just comments), but there is overall a lot of code duplication between the map saving UI and the game saving UI. I think we should rather try to merge those into a single class. There are of course a few key differences between handling games and maps there, but those could possibly be dealt with via overloading or templates or even just via some parameters (including using lambda functions). Same goes for the loading UI and possibly a few others. I wouldn't mind looking into that, but that would be a somewhat bigger change (and require some more consideration how feasible this even is), so that would rather be something for the build21. -- https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-file-saving 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/handling-various-fs-errors into lp:widelands
Arty has proposed merging
lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1798816 in widelands: "Replay menu does not respect Show filenames after
deleting a replay"
https://bugs.launchpad.net/widelands/+bug/1798816
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands.
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-11-12 06:52:50 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-14 12:37:16 +
@@ -156,14 +156,35 @@
/** TRANSLATORS: A folder that hasn't been given a name yet */
MainMenuSaveMapMakeDirectory md(this, _("unnamed"));
if (md.run() == UI::Panel::Returncodes::kOk) {
- g_fs->ensure_directory_exists(curdir_);
- // Create directory.
std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname();
// Trim it for preceding/trailing whitespaces in user input
boost::trim(fullname);
- g_fs->make_directory(fullname);
- fill_table();
+ if (g_fs->file_exists(fullname)) {
+ const std::string s = "A file or directory with that name already exists.";
+ UI::WLMessageBox mbox(this, _("Error Creating Directory!"), s,
+ UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+ } else {
+ try {
+g_fs->ensure_directory_exists(curdir_);
+// Create directory.
+g_fs->make_directory(fullname);
+ } catch (const FileError& e) {
+log("directory creation failed in MainMenuSaveMap::"
+"clicked_make_directory: %s\n", e.what());
+const std::string s =
+ (boost::format(_("Creating directory ‘%s’ failed."))
+ % fullname).str();
+UI::WLMessageBox mbox(this, _("Error Creating Directory!"), s,
+ UI::WLMessageBox::MBoxType::kOk);
+mbox.run();
+ }
+ fill_table();
+ }
}
+ table_.focus();
+ // TODO(Arty): In case of successful dir creation we should select the
+ // new dir in the table.
}
void MainMenuSaveMap::clicked_edit_options() {
=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc'
--- src/editor/ui_menus/main_menu_save_map_make_directory.cc 2018-07-08 13:53:45 +
+++ src/editor/ui_menus/main_menu_save_map_make_directory.cc 2018-11-14 12:37:16 +
@@ -19,9 +19,12 @@
#include "editor/ui_menus/main_menu_save_map_make_directory.h"
+#include
+
#include "base/i18n.h"
#include "graphic/font_handler.h"
#include "io/filesystem/layered_filesystem.h"
+#include "logic/filesystem_constants.h"
MainMenuSaveMapMakeDirectory::MainMenuSaveMapMakeDirectory(UI::Panel* const parent,
char const* dirname)
@@ -63,10 +66,12 @@
vbox_.set_size(get_inner_w() - 2 * padding_, get_inner_h() - 3 * padding_ - buth_);
edit_.set_text(dirname_);
- edit_.changed.connect(boost::bind(&MainMenuSaveMapMakeDirectory::edit_changed, this));
+ edit_.changed.connect(
+ boost::bind(&MainMenuSaveMapMakeDirectory::edit_changed, this));
+ edit_.ok.connect(
+ boost::bind(&MainMenuSaveMapMakeDirectory::clicked_ok, this));
ok_button_.sigclicked.connect(
- boost::bind(&MainMenuSaveMapMakeDirectory::end_modal,
- boost::ref(*this), UI::Panel::Returncodes::kOk));
+ boost::bind(&MainMenuSaveMapMakeDirectory::clicked_ok, this));
ok_button_.set_enabled(!dirname_.empty());
cancel_button_.sigclicked.connect(
boost::bind(&MainMenuSaveMapMakeDirectory::end_modal,
@@ -82,7 +87,24 @@
const std::string& text = edit_.text();
// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
const bool is_legal_filename = FileSystem::is_legal_filename(text);
- ok_button_.set_enabled(is_legal_filename);
- edit_.set_tooltip(is_legal_filename ? "" : illegal_filename_tooltip_);
+ // Prevent the user from creating directory names that the game would
+ // try to interpret as maps
+ const bool has_map_extension =
+ boost::iends_with(text, kWidelandsMapExtension) ||
+ boost::iends_with(text, kS2MapExtension1) ||
+ boost::iends_with(text, kS2MapExtension2);
+ ok_button_.set_enabled(is_legal_filename && !has_map_extension);
+ edit_.set_tooltip(is_legal_filename ?
+ (has_map_extension ? _("This extension is reserved!") : "" ) :
+ illegal_filename_tooltip_);
dirname_ = text;
}
+
+/**
+ * Clicked OK button oder pressed Enter in edit box
+ */
+void MainMenuSaveMapMakeDirectory::clicked_ok() {
+ if (!o
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
Some of the "handling" is just catching and logging. In GameClient::handle_packet, when renaming a file to a backup fails, we could possibly handle this by finding another filename to save to, but I think this would have to be agreed upon by the participating clients. I didn't attempt this, because I am not yet familiar with the network code. -- https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/handling-various-fs-errors 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/robust-file-saving into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/robust-file-saving into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/robust-file-saving into lp:widelands.
=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt 2018-07-19 16:43:14 +
+++ src/editor/CMakeLists.txt 2018-11-13 16:47:56 +
@@ -99,6 +99,7 @@
logic
logic_constants
logic_filesystem_constants
+logic_generic_save_handler
logic_tribe_basic_info
logic_widelands_geometry
map_io
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-11-11 20:42:54 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-13 16:47:56 +
@@ -37,6 +37,7 @@
#include "io/filesystem/layered_filesystem.h"
#include "io/filesystem/zip_filesystem.h"
#include "logic/filesystem_constants.h"
+#include "logic/generic_save_handler.h"
#include "map_io/map_saver.h"
#include "map_io/widelands_map_loader.h"
#include "ui_basic/messagebox.h"
@@ -119,7 +120,9 @@
* Called when the ok button was pressed or a file in list was double clicked.
*/
void MainMenuSaveMap::clicked_ok() {
- assert(ok_.enabled());
+ if (!ok_.enabled()) {
+ return;
+ }
std::string filename = editbox_->text();
std::string complete_filename;
@@ -243,15 +246,12 @@
/**
* Save the map in the current directory with
- * the current filename
+ * the given filename
*
* returns true if dialog should close, false if it
* should stay open
*/
bool MainMenuSaveMap::save_map(std::string filename, bool binary) {
- // Make sure that the current directory exists and is writeable.
- g_fs->ensure_directory_exists(curdir_);
-
// Trim it for preceding/trailing whitespaces in user input
boost::trim(filename);
@@ -272,6 +272,7 @@
if (mbox.run() == UI::Panel::Returncodes::kBack)
return false;
}
+<<<<<<< TREE
// Try deleting file (if it exists). If it fails, give a message and let the player choose a new
// name.
@@ -288,6 +289,8 @@
mbox.run();
return false;
}
+===
+>>>>>>> MERGE-SOURCE
Widelands::EditorGameBase& egbase = eia().egbase();
Widelands::Map* map = egbase.mutable_map();
@@ -305,6 +308,7 @@
} else {
map->delete_tag("artifacts");
}
+<<<<<<< TREE
// Try saving.
try {
@@ -322,5 +326,38 @@
}
die();
+===
+
+ // Try saving the map.
+ GenericSaveHandler gsh(
+ [&egbase](FileSystem& fs) {
+ Widelands::MapSaver wms(fs, egbase);
+ wms.save();
+ },
+ complete_filename,
+ binary ? FileSystem::ZIP : FileSystem::DIR
+ );
+ uint32_t error = gsh.save();
+ if (error == GenericSaveHandler::kSuccess ||
+ error == GenericSaveHandler::kDeletingBackupFailed) {
+ // No need to bother the player if only the temporary backup couldn't
+ // be deleted. Automatic cleanup will try to deal with it later.
+ egbase.get_ibase()->log_message(_("Map saved"));
+ return true;
+ }
+
+ std::string msg = gsh.localized_formatted_result_message();
+ UI::WLMessageBox mbox(
+ this, _("Error Saving Map!"), msg, UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+
+ // If only the backup failed (likely just because of a file lock),
+ // then leave the dialog open for the player to try with a new filename.
+ if (error == GenericSaveHandler::kBackupFailed) {
+ return false;
+ }
+
+ // In the other error cases close the dialog.
+>>>>>>> MERGE-SOURCE
return true;
}
=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc 2018-04-07 16:59:00 +
+++ src/io/filesystem/disk_filesystem.cc 2018-11-13 16:47:56 +
@@ -167,15 +167,13 @@
}
/**
- * Create a sub filesystem out of this filesystem
+ * Make a sub filesystem out of this filesystem
*/
FileSystem* RealFSImpl::make_sub_file_system(const std::string& path) {
FileSystemPath fspath(canonicalize_name(path));
if (!fspath.exists_) {
- throw wexception("RealFSImpl: unable to create sub filesystem, path does not exist for '%s'"
- " in directory '%s'",
- fspath.c_str(), directory_.c_str());
+ throw FileNotFoundError("RealFSImpl::make_sub_file_system", fspath);
}
if (fspath.is_directory_)
@@ -190,10 +188,7 @@
FileSystem* RealFSImpl::create_sub_file_system(const std::string& path, Type const fs) {
FileSystemPath fspath(canonicalize_name(path));
if (fspath.exists_)
- throw wexception(
- "path '%s'' already exists in directory &#
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/filesystem-errors into lp:widelands
Good to know, thanks. So I guess I shouldn't even make any child branches at all before the parent branch is fully revised and scheduled for merging into trunk. Or maybe just not push them online or at least not bother anyone else with them via merge requests; and then later resolve merge conflicts on my own (possibly just copying it over into a new branch) before making everything available. I feel that this kinda destroys what I thought to be a big advantage of this whole branching concept. -- https://code.launchpad.net/~widelands-dev/widelands/filesystem-errors/+merge/358219 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/filesystem-errors. ___ 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/filesystem-errors into lp:widelands
Oh okay, then I'll leave it. I am still somewhat fuzzy about some specifics with bazaar, specifically merge conflicts. Let's say for examply I'd fix all those little things in disk_filesystem.cc, then merge the result into my two child branches (which itself should be fine because those don't make any changes to the file themselve). Then - if I got this right - the only conflicts would arise if someone else is doing other changes to a version of the branches from before the merge (which of course is easily possible during the review) or if someone is changing the file in some other branch. Right? -- https://code.launchpad.net/~widelands-dev/widelands/filesystem-errors/+merge/358219 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/filesystem-errors. ___ 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/filesystem-errors into lp:widelands
regarding code review: There're plenty more long lines and if-blocks without curly braces in there. I'll fix this tonight. Do we generally follow a strict rule of curly braces even for short single line blocks? -- https://code.launchpad.net/~widelands-dev/widelands/filesystem-errors/+merge/358219 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/filesystem-errors. ___ 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/RTL-alignment-fix into lp:widelands
I am not actually sure there is a place where this currently makes any difference visually. Most text areas are used in automove mode and automatically change their position and size based on the text, so text alignment doesn't matter unless they are resized from the outside. In the cases where such resizing is done (e.g. window titles) the alignment is kCenter. So visially nothing has changed yet. I made the fix mainly so that RTL would be displayed correctly when we use single-line textareas in non-center mode and with properly set layout. And we already do the same adjustment for multi-line textareas. Where it theoretically could make a difference is the campaign select menu. That one uses some single-line textareas, but we don't have a layout defined there that would resize the text areas, so they stay collapsed to the text width and don't react to my fix yet. (And we might even put everything there in a multi-line textarea when we do a proper box layout.) If you want to just quickly test it, set one of the menu title alignments to Left or Right. Without the fix, language doesn't make a diffence, with the fix alignment is swapped properly for Arabic. Side note: Even the menus where we have defined a proper layout with multi-line textareas, some of them are not resized properly (like game/map name in the description area), so RTL is ignored. I guess all this RTL stuff has very low priority anyway. Aside from a small amount of Arabic, we don't even seem to have translations. -- https://code.launchpad.net/~widelands-dev/widelands/RTL-alignment-fix/+merge/358596 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/RTL-alignment-fix 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/fix-language-entry-ms into lp:widelands
Disclosure: I actually don't speak the language nor do I know anyone who does. I only stumbled upon this by accident when I checked RTL stuff and found weird behaviour: The language entry in the options menu was arabic looking and RTL, but all the translations were in Latin script and - even though aligned right (probably because if the font setting) - seemed to be meant for reading left-to-right. Acording to Google+Wikipedia there still is an arabic script for the language but also Latin one, which is much more widespread and also the official script. And it's not RTL. Given that (as far as I could see) all our existing ms translations use the Latin script, I thought we should change the entry in the lua file, so the options menu shows the Latin name and the alignment is not automatically switched. -- https://code.launchpad.net/~widelands-dev/widelands/fix-language-entry-ms/+merge/358597 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/fix-language-entry-ms 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/fix-language-entry-ms into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/fix-language-entry-ms
into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/fix-language-entry-ms/+merge/358597
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/fix-language-entry-ms into lp:widelands.
=== modified file 'data/i18n/locales.lua'
--- data/i18n/locales.lua 2018-11-10 09:20:09 +
+++ data/i18n/locales.lua 2018-11-10 18:49:34 +
@@ -249,9 +249,9 @@
},
ms = {
- name = "بهاس ملايو",
+ name = "Bahasa Melayu",
sort_name = "Melayu",
- font = "arabic"
+ font = "default"
},
my = {
___
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/RTL-alignment-fix into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/RTL-alignment-fix into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/RTL-alignment-fix/+merge/358596
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/RTL-alignment-fix into lp:widelands.
=== modified file 'src/ui_basic/textarea.cc'
--- src/ui_basic/textarea.cc 2018-07-08 13:53:45 +
+++ src/ui_basic/textarea.cc 2018-11-10 18:48:51 +
@@ -125,9 +125,10 @@
*/
void Textarea::draw(RenderTarget& dst) {
if (!text_.empty()) {
+ Align alignment = mirror_alignment(align_, text_);
Vector2i anchor(
- (align_ == Align::kCenter) ? get_w() / 2 : (align_ == UI::Align::kRight) ? get_w() : 0, 0);
- rendered_text_->draw(dst, anchor, align_);
+ (alignment == Align::kCenter) ? get_w() / 2 : (alignment == UI::Align::kRight) ? get_w() : 0, 0);
+ rendered_text_->draw(dst, anchor, alignment);
}
}
___
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/bugfix-1801208 into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/bugfix-1801208 into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1801208 in widelands: "Message boxes with long unbreakable strings show
empty "
https://bugs.launchpad.net/widelands/+bug/1801208
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bugfix-1801208/+merge/358594
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bugfix-1801208 into lp:widelands.
=== modified file 'src/ui_basic/multilinetextarea.cc'
--- src/ui_basic/multilinetextarea.cc 2018-08-12 16:35:30 +
+++ src/ui_basic/multilinetextarea.cc 2018-11-10 17:41:58 +
@@ -30,7 +30,8 @@
namespace UI {
-static const uint32_t RICHTEXT_MARGIN = 2;
+// int instead of uint because of overflow situations
+static const int32_t RICHTEXT_MARGIN = 2;
MultilineTextarea::MultilineTextarea(Panel* const parent,
const int32_t x,
@@ -141,11 +142,19 @@
int anchor = 0;
Align alignment = mirror_alignment(align_, text_);
switch (alignment) {
+ // TODO(Arty): We might want to revisit this after the font renderer can handle long strings
+ // without whitespaces differently.
+ // Currently, such long unbreakable strings are silently assumed to fit the line exactly,
+ // which means that rendered_text_->width() might actually be larger than the effective width
+ // of the textarea. If we'd allow the anchor here to become negative in this case, it would
+ // properly position the longest line (just truncated), BUT the positioning of shorter lines
+ // would be off (possibly even outside the textarea, thus invisible) because their positioning
+ // is calculated without regard for overlong lines.
case UI::Align::kCenter:
- anchor = (get_eff_w() - rendered_text_->width()) / 2;
+ anchor = std::max(0, (get_eff_w() - rendered_text_->width()) / 2);
break;
case UI::Align::kRight:
- anchor = get_eff_w() - rendered_text_->width() - RICHTEXT_MARGIN;
+ anchor = std::max(0, get_eff_w() - rendered_text_->width() - RICHTEXT_MARGIN);
break;
case UI::Align::kLeft:
anchor = RICHTEXT_MARGIN;
=== modified file 'src/ui_basic/multilinetextarea.h'
--- src/ui_basic/multilinetextarea.h 2018-07-08 09:18:33 +
+++ src/ui_basic/multilinetextarea.h 2018-11-10 17:41:58 +
@@ -60,7 +60,8 @@
}
void set_text(const std::string&);
- uint32_t get_eff_w() const {
+ // int instead of uint because of overflow situations
+ int32_t get_eff_w() const {
return scrollbar_.is_enabled() ? get_w() - Scrollbar::kSize : get_w();
}
___
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-1753230-working-with-tempfiles into lp:widelands
Oh, now I see where your original comments are. Don't know why I didn't notice them before. Sorry about that. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ 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-1753230-working-with-tempfiles into lp:widelands
Oh, I see you had some other nits that you fixed. Fair enough. I should have checked more thoroughly. Or did you mention some specifics before somewhere and I had missed them? As for the naming of the temp dir, I also had it named "temp" first but felt that might encourage players to just delete it (possibly while the game is up, thus messing up their next save). I guess I was overreacting because a friend of mine notoriously does this kind of thing, always complaining that stupid programmers never clean up their temp stuff. But honestly, "temp" is perfectly fine. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ 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-1753230-working-with-tempfiles into lp:widelands
Huh? I had already fixed them. Did I miss something? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ 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/robust-file-saving into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/robust-file-saving into
lp:widelands with
lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles as a
prerequisite.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358473
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/robust-file-saving into lp:widelands.
=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt 2018-07-19 16:43:14 +
+++ src/editor/CMakeLists.txt 2018-11-08 02:11:06 +
@@ -99,6 +99,7 @@
logic
logic_constants
logic_filesystem_constants
+logic_generic_save_handler
logic_tribe_basic_info
logic_widelands_geometry
map_io
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-11-08 02:11:06 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-08 02:11:06 +
@@ -29,6 +29,7 @@
#include "base/i18n.h"
#include "base/macros.h"
+#include "base/time_string.h"
#include "base/wexception.h"
#include "editor/editorinteractive.h"
#include "editor/ui_menus/main_menu_map_options.h"
@@ -37,6 +38,7 @@
#include "io/filesystem/layered_filesystem.h"
#include "io/filesystem/zip_filesystem.h"
#include "logic/filesystem_constants.h"
+#include "logic/generic_save_handler.h"
#include "map_io/map_saver.h"
#include "map_io/widelands_map_loader.h"
#include "ui_basic/messagebox.h"
@@ -119,7 +121,8 @@
* Called when the ok button was pressed or a file in list was double clicked.
*/
void MainMenuSaveMap::clicked_ok() {
- assert(ok_.enabled());
+ if (!ok_.enabled())
+ return;
std::string filename = editbox_->text();
std::string complete_filename;
@@ -243,15 +246,12 @@
/**
* Save the map in the current directory with
- * the current filename
+ * the given filename
*
* returns true if dialog should close, false if it
* should stay open
*/
bool MainMenuSaveMap::save_map(std::string filename, bool binary) {
- // Make sure that the current directory exists and is writeable.
- g_fs->ensure_directory_exists(curdir_);
-
// Trim it for preceding/trailing whitespaces in user input
boost::trim(filename);
@@ -272,18 +272,6 @@
if (mbox.run() == UI::Panel::Returncodes::kBack)
return false;
}
-
- // Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
- try {
- g_fs->fs_unlink(complete_filename);
- } catch (const std::exception& e) {
- const std::string s =
- (boost::format(_("File ‘%s.tmp’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
- + " " + _("Try saving under a different name!");
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
- return false;
- }
Widelands::EditorGameBase& egbase = eia().egbase();
Widelands::Map* map = egbase.mutable_map();
@@ -301,23 +289,34 @@
} else {
map->delete_tag("artifacts");
}
-
- // Try saving.
- try {
- std::unique_ptr fs(
- g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
- Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
- wms->save();
- delete wms;
- fs.reset();
- } catch (const std::exception& e) {
- std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
- "given:\n");
- s += e.what();
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
+
+ // Try saving the map.
+ GenericSaveHandler gsh(
+ [&egbase](FileSystem& fs) {
+ Widelands::MapSaver wms(fs, egbase);
+ wms.save();
+ },
+ complete_filename,
+ binary ? FileSystem::ZIP : FileSystem::DIR
+ );
+ uint32_t error = gsh.save();
+ if (error == GenericSaveHandler::kSuccess ||
+ error == GenericSaveHandler::kDeletingBackupFailed) {
+ // No need to bother the player if only the temporary backup couldn't be deleted.
+ // Automatic cleanup will try to deal with it later.
+ egbase.get_ibase()->log_message(_("Map saved"));
+ return true;
}
- die();
+ std::string msg = gsh.localized_formatted_result_message();
+ UI::WLMessageBox mbox(this, _("Error Saving Map!"), msg, UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+
+ // If only the backup failed (likely just because of a file lock),
+ // then leave the dialog open for the player to try with a new filename.
+ if (error == GenericSaveHandler::kBackupFailed)
+ return false;
+
+ // In the other error cases close the dialog.
return true;
}
==
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1800182-focus-save-menu into lp:widelands
Bug report is up. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1800182-focus-save-menu/+merge/358284 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1800182-focus-save-menu. ___ 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-1800182-focus-save-menu into lp:widelands
Or rather than letting the save menu (and possibly some others) deal with "let's not accidentally refer key handling to the game window" issues, maybe the game window needs a "state" variable to distinguish between "game is running normally", "game is paused in background behind some other window" (and possibly other states), and then any key handling is suspended in certain states. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1800182-focus-save-menu/+merge/358284 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1800182-focus-save-menu. ___ 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-1800182-focus-save-menu into lp:widelands
That keypresses affect the game window from the save menu is a more general issue that should be resolved. Sure, the name edit box grabs the keys when it has the focus, but as soon as the filename table is focued (which happends naturally when someone selects a name there), key handling (aside from a few keys like Up/Down) seems to be referred to the game window in the background. We should generally prevent that. (Not necessarily in this branch.) Maybe it's enough to have the LoadOrSave table simply grab the keys instead of referring the key handling to something else. Hard to say right now, I haven't even checked which route the key handling takes to end up at the game window. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1800182-focus-save-menu/+merge/358284 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1800182-focus-save-menu. ___ 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-1753230-working-with-tempfiles into lp:widelands
No, I didn't. :-) The string is already in the last trunk. I just noticed it and removed it together with the other small fix I commited. (It just didn't feel important enough to mention it in the log.) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/robust-saving into lp:widelands
Oops, I just realized that I my commit log isn't quite correct. GameMainMenuSaveGame actually uses GenericSaveHandler directly, not via SaveHandler. I did it this way, so the formatted message (to show the player if something went wrong) could be accessed without having to change SaveHandler to allow routing it through there. Basically SaveHandler is now only used for autosaves (timed and requested via lua scripts) and to store the current filename for use in the Save Menu. -- https://code.launchpad.net/~widelands-dev/widelands/robust-saving/+merge/358220 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-saving 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Btw, this autosave-thing you observed was a (I think even relatively recently introduced and somewhat dirty) error handling attempt by someone. When the wl_autosave_00.wgf couldn't be renamed (for backup purposes) then there was some "let's just try other filenames" approach, and the game would just find the next autosave filename (up to 09) where there didn't exist any file. At 09 it would just stop and not check any further, but use this as the target name, so from this point on wl_autosave_09.wgf would just have been overwritten with every autosave. (Which worked in your case, because this one wasn't file-locked.) My latest branch is stricter in this regard. If some relevant renaming or deletion fails, then the autosaving is simply aborted, and there is a little message. I don't think the game needs to handle this in some special way. The player gets a message and can try to fix a problem (if there's a file lock for example) or has to save manually. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
1. Regarding the autosave stuff: This wasn't anything this branch tried to solve. It only solved the the conflicts that arose from trying to modify files where the map fs pointed to, resulting in either some crash or error (if the file was currently locked) or resulting in corrupt savefiles. Those other file conflicts (like trying to modify a savefile that is locked because you have it open with 7zip, or having a file write-protected or some such) are solved with the latest branch: https://code.launchpad.net/~widelands-dev/widelands/robust-saving As for trying to specifically test "is_writable" instead of "file_exists" (or other potentially problematic file states) beforehand, I don't think it's worth it. For one thing, we might have to deal with the specifics of the various access control models on the different platforms. (Just looking at this stuff makes me nauseaous.) There is also the more general problem with any filesystem stuff that you can never be sure that the file state you just checked is still the same when you try your file operation next. Sure, it's very likely, but there can always be race conditions, and some other process might just have snatched the file away between your state check and your attempt to operate on a file. So aside from a few minor exist checks I favour a simple, practical approach: just try the file operation, but catch and handle any errors. The error codes give a hint about what the actual problem was, but usually that doesn't matter much anyway: either it worked and we continue or it didn't and we abort. Anyway, the latest branch https://code.launchpad.net/~widelands-dev/widelands/robust-saving does exactly that and handles file errors in all the map/game saving routines and cleanup routines. I still need to check in what other places we don't catch file errors but should. (Definitely when deleting files in the load/save menu, maybe synchstream creation, probably some other places.) That's going to be part of future branches though. 2. Those travis build errors I hadn't actually seen this before. (I'm still a noob on Launchpad and don't know my way around very well, but I am trying to catch up. If there are any important features I might not but should know about, feel free to point something out.) Anyway, those errors seem like minor nitpicks. I'll fix them. Btw, are those builds made automatically here for any new branches? Or do I have to initiate them manually? (I haven't checked out the online building possibilities yet. Still on my todo list.) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles 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/robust-saving into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/robust-saving into
lp:widelands with
lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles as a
prerequisite.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/robust-saving/+merge/358220
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/robust-saving into lp:widelands.
=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt 2018-07-19 16:43:14 +
+++ src/editor/CMakeLists.txt 2018-11-02 11:15:15 +
@@ -99,6 +99,7 @@
logic
logic_constants
logic_filesystem_constants
+logic_generic_save_handler
logic_tribe_basic_info
logic_widelands_geometry
map_io
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-11-02 11:15:14 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-02 11:15:15 +
@@ -29,6 +29,7 @@
#include "base/i18n.h"
#include "base/macros.h"
+#include "base/time_string.h"
#include "base/wexception.h"
#include "editor/editorinteractive.h"
#include "editor/ui_menus/main_menu_map_options.h"
@@ -37,12 +38,13 @@
#include "io/filesystem/layered_filesystem.h"
#include "io/filesystem/zip_filesystem.h"
#include "logic/filesystem_constants.h"
+#include "logic/generic_save_handler.h"
#include "map_io/map_saver.h"
#include "map_io/widelands_map_loader.h"
#include "ui_basic/messagebox.h"
#include "wui/mapdetails.h"
#include "wui/maptable.h"
-
+#include
inline EditorInteractive& MainMenuSaveMap::eia() {
return dynamic_cast(*get_parent());
}
@@ -119,7 +121,8 @@
* Called when the ok button was pressed or a file in list was double clicked.
*/
void MainMenuSaveMap::clicked_ok() {
- assert(ok_.enabled());
+ if (!ok_.enabled())
+ return;
std::string filename = editbox_->text();
std::string complete_filename;
@@ -243,15 +246,12 @@
/**
* Save the map in the current directory with
- * the current filename
+ * the given filename
*
* returns true if dialog should close, false if it
* should stay open
*/
bool MainMenuSaveMap::save_map(std::string filename, bool binary) {
- // Make sure that the current directory exists and is writeable.
- g_fs->ensure_directory_exists(curdir_);
-
// Trim it for preceding/trailing whitespaces in user input
boost::trim(filename);
@@ -272,18 +272,6 @@
if (mbox.run() == UI::Panel::Returncodes::kBack)
return false;
}
-
- // Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
- try {
- g_fs->fs_unlink(complete_filename);
- } catch (const std::exception& e) {
- const std::string s =
- (boost::format(_("File ‘%s.tmp’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
- + " " + _("Try saving under a different name!");
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
- return false;
- }
Widelands::EditorGameBase& egbase = eia().egbase();
Widelands::Map* map = egbase.mutable_map();
@@ -301,23 +289,34 @@
} else {
map->delete_tag("artifacts");
}
-
- // Try saving.
- try {
- std::unique_ptr fs(
- g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
- Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
- wms->save();
- delete wms;
- fs.reset();
- } catch (const std::exception& e) {
- std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
- "given:\n");
- s += e.what();
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
+
+ // Try saving the map.
+ GenericSaveHandler gsh(
+ [&egbase](FileSystem& fs) {
+ Widelands::MapSaver wms(fs, egbase);
+ wms.save();
+ },
+ complete_filename,
+ binary ? FileSystem::ZIP : FileSystem::DIR
+ );
+ uint32_t error = gsh.save();
+ if (error == GenericSaveHandler::kSuccess ||
+ error == GenericSaveHandler::kDeletingBackupFailed) {
+ // No need to bother the player if only the temporary backup couldn't be deleted.
+ // Automatic cleanup will try to deal with it later.
+ egbase.get_ibase()->log_message(_("Map saved"));
+ return true;
}
- die();
+ std::string msg = gsh.localized_formatted_result_message();
+ UI::WLMessageBox mbox(this, _("Error Saving Map!"), msg, UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+
+ // If only the backup failed (likely just because of a file lock),
+ // then leave the dialog op
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/filesystem-errors into lp:widelands
Arty has proposed merging lp:~widelands-dev/widelands/filesystem-errors into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/filesystem-errors/+merge/358219
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/filesystem-errors into lp:widelands.
=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc 2018-09-25 06:32:35 +
+++ src/editor/editorinteractive.cc 2018-11-02 11:11:32 +
@@ -187,6 +187,7 @@
}
ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
+ egbase().postload();
egbase().load_graphics(loader_ui);
map_changed(MapWas::kReplaced);
}
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-06-01 08:50:29 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-02 11:11:32 +
@@ -272,16 +272,14 @@
if (mbox.run() == UI::Panel::Returncodes::kBack)
return false;
}
-
- // save to a tmp file/dir first, rename later
- // (important to keep script files in the script directory)
- const std::string tmp_name = complete_filename + ".tmp";
- if (g_fs->file_exists(tmp_name)) {
+
+ // Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
+ try {
+ g_fs->fs_unlink(complete_filename);
+ } catch (const std::exception& e) {
const std::string s =
- (boost::format(
- _("A file with the name ‘%s.tmp’ already exists. You have to remove it manually.")) %
- FileSystem::fs_filename(filename.c_str()))
- .str();
+ (boost::format(_("File ‘%s.tmp’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
+ + " " + _("Try saving under a different name!");
UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
mbox.run();
return false;
@@ -290,51 +288,35 @@
Widelands::EditorGameBase& egbase = eia().egbase();
Widelands::Map* map = egbase.mutable_map();
- { // fs scope
+ // Recompute seafaring tag
+ map->cleanup_port_spaces(egbase.world());
+ if (map->allows_seafaring()) {
+ map->add_tag("seafaring");
+ } else {
+ map->delete_tag("seafaring");
+ }
+
+ if (map->has_artifacts()) {
+ map->add_tag("artifacts");
+ } else {
+ map->delete_tag("artifacts");
+ }
+
+ // Try saving.
+ try {
std::unique_ptr fs(
- g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
-
- // Recompute seafaring tag
- map->cleanup_port_spaces(egbase.world());
- if (map->allows_seafaring()) {
- map->add_tag("seafaring");
- } else {
- map->delete_tag("seafaring");
- }
-
- if (map->has_artifacts()) {
- map->add_tag("artifacts");
- } else {
- map->delete_tag("artifacts");
- }
-
- try {
- Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
- wms->save();
- delete wms;
- // Reset filesystem to avoid file locks on saves
- fs.reset();
- map->reset_filesystem();
- eia().set_need_save(false);
- g_fs->fs_unlink(complete_filename);
- g_fs->fs_rename(tmp_name, complete_filename);
- // Also change fs, as we assign it to the map below
- fs.reset(g_fs->make_sub_file_system(complete_filename));
- // Set the filesystem of the map to the current save file / directory
- map->swap_filesystem(fs);
- // DONT use fs as of here, its garbage now!
-
- } catch (const std::exception& e) {
- std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
- "given:\n");
- s += e.what();
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
-
- // cleanup tmp file if it was created
- g_fs->fs_unlink(tmp_name);
- }
- } // end fs scope, dont use it
+ g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
+ Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
+ wms->save();
+ delete wms;
+ fs.reset();
+ } catch (const std::exception& e) {
+ std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
+ "given:\n");
+ s += e.what();
+ UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+ }
die();
return true;
=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc 2018-04-07 16:59:00 +
+++ src/io/filesystem/disk_filesystem.cc 2018-11-02 11:11:32 +
@@ -167,15 +167,13 @@
}
/**
- * Creat
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Arty has proposed merging
lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1753230 in widelands: "Scenarios: Some files get removed on autosave"
https://bugs.launchpad.net/widelands/+bug/1753230
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into
lp:widelands.
=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc 2018-09-25 06:32:35 +
+++ src/editor/editorinteractive.cc 2018-10-22 18:50:51 +
@@ -187,6 +187,7 @@
}
ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
+ egbase().postload();
egbase().load_graphics(loader_ui);
map_changed(MapWas::kReplaced);
}
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2018-06-01 08:50:29 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2018-10-22 18:50:51 +
@@ -272,16 +272,14 @@
if (mbox.run() == UI::Panel::Returncodes::kBack)
return false;
}
-
- // save to a tmp file/dir first, rename later
- // (important to keep script files in the script directory)
- const std::string tmp_name = complete_filename + ".tmp";
- if (g_fs->file_exists(tmp_name)) {
+
+ // Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
+ try {
+ g_fs->fs_unlink(complete_filename);
+ } catch (const std::exception& e) {
const std::string s =
- (boost::format(
- _("A file with the name ‘%s.tmp’ already exists. You have to remove it manually.")) %
- FileSystem::fs_filename(filename.c_str()))
- .str();
+ (boost::format(_("File ‘%s.tmp’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
+ + " " + _("Try saving under a different name!");
UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
mbox.run();
return false;
@@ -290,51 +288,35 @@
Widelands::EditorGameBase& egbase = eia().egbase();
Widelands::Map* map = egbase.mutable_map();
- { // fs scope
+ // Recompute seafaring tag
+ map->cleanup_port_spaces(egbase.world());
+ if (map->allows_seafaring()) {
+ map->add_tag("seafaring");
+ } else {
+ map->delete_tag("seafaring");
+ }
+
+ if (map->has_artifacts()) {
+ map->add_tag("artifacts");
+ } else {
+ map->delete_tag("artifacts");
+ }
+
+ // Try saving.
+ try {
std::unique_ptr fs(
- g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
-
- // Recompute seafaring tag
- map->cleanup_port_spaces(egbase.world());
- if (map->allows_seafaring()) {
- map->add_tag("seafaring");
- } else {
- map->delete_tag("seafaring");
- }
-
- if (map->has_artifacts()) {
- map->add_tag("artifacts");
- } else {
- map->delete_tag("artifacts");
- }
-
- try {
- Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
- wms->save();
- delete wms;
- // Reset filesystem to avoid file locks on saves
- fs.reset();
- map->reset_filesystem();
- eia().set_need_save(false);
- g_fs->fs_unlink(complete_filename);
- g_fs->fs_rename(tmp_name, complete_filename);
- // Also change fs, as we assign it to the map below
- fs.reset(g_fs->make_sub_file_system(complete_filename));
- // Set the filesystem of the map to the current save file / directory
- map->swap_filesystem(fs);
- // DONT use fs as of here, its garbage now!
-
- } catch (const std::exception& e) {
- std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
- "given:\n");
- s += e.what();
- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
- mbox.run();
-
- // cleanup tmp file if it was created
- g_fs->fs_unlink(tmp_name);
- }
- } // end fs scope, dont use it
+ g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
+ Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
+ wms->save();
+ delete wms;
+ fs.reset();
+ } catch (const std::exception& e) {
+ std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
+ "given:\n");
+ s += e.what();
+ UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
+ mbox.run();
+ }
die();
return true;
=== modified file 'src/io/filesystem/zip_fil
