Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/clang-tidy-round1 into lp:widelands

[SirVer]

Please leave a short comment when you merge force to explain why it was 
necessary and why you are confident that the branch is green. 

> Am 12.11.2017 um 08:23 schrieb GunChleoc :
> 
> @bunnybot merge force
> -- 
> https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
> You are reviewing the proposed merge of 
> lp:~widelands-dev/widelands/clang-tidy-round1 into lp:widelands.


-- 
https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/clang-tidy-round1.

___
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/clang-tidy-round1 into lp:widelands

[GunChleoc]

@bunnybot merge force
-- 
https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/clang-tidy-round1.

___
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/clang-tidy-round1 into lp:widelands

[bunnybot]

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

Travis build 2794. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/300705679.
-- 
https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/clang-tidy-round1.

___
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/clang-tidy-round1 into lp:widelands

[bunnybot]

Continuous integration builds have changed state:

Travis build 2794. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/300705679.
Appveyor build 2605. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_clang_tidy_round1-2605.
-- 
https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/clang-tidy-round1.

___
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/codecheck_translations into lp:widelands

[bunnybot]

Continuous integration builds have changed state:

Travis build 2792. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/300649551.
Appveyor build 2603. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_codecheck_translations-2603.
-- 
https://code.launchpad.net/~widelands-dev/widelands/codecheck_translations/+merge/333579
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/codecheck_translations 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/check_representative_image into lp:widelands

[GunChleoc]

The next thing we need to do is to triage the crashes from our game. I have 
compiled a branch for it to start testing, but then got sidetracked with 
another branch. It's important to search existing bugs for each of them too, so 
we won't create duplicates.
-- 
https://code.launchpad.net/~widelands-dev/widelands/check_representative_image/+merge/333563
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/check_representative_image 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/clang-tidy-round1 into lp:widelands

[GunChleoc]

It's a unique_ptr, so I've killed the line. Thanks for the review!

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/clang-tidy-round1/+merge/333560
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/clang-tidy-round1.

___
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:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands

[Jukka Pakarinen]

> Thanks for the fix!
> 
> Looks good to me - can you please replace UI::RenderedText::Shared with
> std::shared_ptr and get rid of the new using statement? I
> think that would make the code easier to read.

Sure, I can do that.
-- 
https://code.launchpad.net/~flegu/widelands/r8481-renderedtext-memory-leaks/+merge/333578
Your team Widelands Developers is requested to review the proposed merge of 
lp:~flegu/widelands/r8481-renderedtext-memory-leaks 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:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands

[GunChleoc]

Thanks for the fix!

Looks good to me - can you please replace UI::RenderedText::Shared with 
std::shared_ptr and get rid of the new using statement? I 
think that would make the code easier to read.
-- 
https://code.launchpad.net/~flegu/widelands/r8481-renderedtext-memory-leaks/+merge/333578
Your team Widelands Developers is requested to review the proposed merge of 
lp:~flegu/widelands/r8481-renderedtext-memory-leaks 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/codecheck_translations into lp:widelands

[GunChleoc]

GunChleoc has proposed merging 
lp:~widelands-dev/widelands/codecheck_translations into lp:widelands.

Commit message:
Added 2 codecheck rules for translation markup.

- Ensure that translators' comments are immediately above their translations.
- Detect inconsistent tag format for translators' comments
- Check that all translatable strings that have multiple printf placeholders
  have defined those as reversible.
- Check that unordered, ordered and boost placeholders aren't mixed up in the
  same string.
- Check that ngettext singular and plural strings have the same placeholders.
- Check that placeholders are numbered in ascending order.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/codecheck_translations/+merge/333579

These checks will fix the following i18n issues for the C++ side of things:

1. clang-format can break up code so that TRANSLATORS comments are no longer 
immediately above their gettext calls. This means that xgettext will no longer 
pick them up to show them to our translators.

2. I tried to reverse the order of 2 placeholders during translation today and 
forgot to add numbers, resulting in an invalid translation. So, it's best if 
the numbers are already in the source string - this will also make quality 
checks on the translations easier.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/codecheck_translations into lp:widelands.
=== modified file 'cmake/codecheck/CodeCheck.py'
--- cmake/codecheck/CodeCheck.py	2015-04-04 15:36:44 +
+++ cmake/codecheck/CodeCheck.py	2017-11-11 14:46:17 +
@@ -1,5 +1,5 @@
-#!/usr/bin/env python
-# encoding: utf-8
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
 #
 
 """

=== added file 'cmake/codecheck/rules/translations_printf'
--- cmake/codecheck/rules/translations_printf	1970-01-01 00:00:00 +
+++ cmake/codecheck/rules/translations_printf	2017-11-11 14:46:17 +
@@ -0,0 +1,162 @@
+#!/usr/bin/python -tt
+
+"""Checks that all translatable strings that have multiple printf placeholders
+have defined those as reversible.
+
+Checks that unordered, ordered and boost placeholders aren't mixed up in the same string.
+
+Checks that ngettext singular and plural strings have the same placeholders.
+
+Checks that placeholders are numbered in ascending order.
+
+"""
+
+import re
+
+# Regex to find placeholders
+find_unordered = re.compile(r"[^0-9]\%([0-9#*]*\.*[0-9#]*[a-zA-Z]{1,2})")
+find_ordered = re.compile(r"\%[|]{0,1}(\d\$[0-9#*]*\.*[0-9#]*[a-zA-Z]{1,2})")
+find_boost = re.compile(r"\%(\d)\%")
+
+
+def check_placeholders(entry):
+"""Make sure that a string satisfies our placeholder policy."""
+sanitized_entry = entry.replace('%%', '')
+unordered = find_unordered.findall(sanitized_entry)
+if len(unordered) > 1:
+return 'Translatable string has multiple sprintf placeholders that are not ordered:'
+else:
+ordered = find_ordered.findall(sanitized_entry)
+boost = find_boost.findall(sanitized_entry)
+if len(unordered) > 0 and (len(ordered) > 0 or len(boost) > 0):
+return 'Translatable string is mixing unordered sprintf placeholders with ordered or boost placeholders:'
+if len(ordered) > 0 and (len(unordered) > 0 or len(boost) > 0):
+return 'Translatable string is mixing ordered sprintf placeholders with unordered or boost placeholders:'
+if len(ordered) > 0:
+for entryno, placeholder in enumerate(ordered, 1):
+if str(entryno) != placeholder[:placeholder.find('$')]:
+return 'Translatable string has an ordered sprintf placeholder "' + placeholder + '" in position ' + str(entryno) + " - the numbers don't match:"
+if len(boost) > 0:
+for entryno, placeholder in enumerate(boost, 1):
+if str(entryno) != placeholder:
+return 'Translatable string has an ordered boost placeholder "' + placeholder + '" in position ' + str(entryno) + " - the numbers don't match:"
+return ''
+
+
+def compare_placeholders(entry1, entry2):
+"""An Ngettext string must have the same splaceholders in its singular and
+plural strings."""
+sanitized_entry1 = entry1.replace('%%', '')
+sanitized_entry2 = entry2.replace('%%', '')
+placeholders1 = find_unordered.findall(sanitized_entry1)
+if len(placeholders1) > 0:
+placeholders2 = find_unordered.findall(sanitized_entry2)
+else:
+placeholders1 = find_ordered.findall(sanitized_entry1)
+if len(placeholders1) > 0:
+placeholders2 = find_ordered.findall(sanitized_entry2)
+else:
+placeholders1 = find_boost.findall(sanitized_entry1)
+placeholders2 = find_boost.findall(sanitized_entry2)
+for entryno, placeholder in enumerate(placeholders1, 0):
+if placeholder != placeholders2[entryno]:
+

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/codecheck_translations into lp:widelands

[GunChleoc]

The proposal to merge lp:~widelands-dev/widelands/codecheck_translations into 
lp:widelands has been updated.

Commit Message changed to:

Added 2 codecheck rules for translation markup.

- Ensure that translators' comments are immediately above their translations.
- Detect inconsistent tag format for translators' comments
- Check that all translatable strings that have multiple printf placeholders
  have defined those as reversible.
- Check that unordered, ordered and boost placeholders aren't mixed up in the
  same string.
- Check that ngettext singular and plural strings have the same placeholders.
- Check that placeholders are numbered in ascending order.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/codecheck_translations/+merge/333579
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/codecheck_translations 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:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands

[Jukka Pakarinen]

Jukka Pakarinen has proposed merging 
lp:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~flegu/widelands/r8481-renderedtext-memory-leaks/+merge/333578

Valgrind analysis reveals memory leaks from RenderedText objects during startup 
of the game. The leaks are already reported in bug 1668200 #4 #6. 

The leaks are detected in Debug and Release builds on Debian 9.1. Valgrind does 
not see the leaks if a build is done with the changes in the branch.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands.
=== modified file 'src/graphic/text/rendered_text.h'
--- src/graphic/text/rendered_text.h	2017-06-24 08:47:46 +
+++ src/graphic/text/rendered_text.h	2017-11-11 14:31:33 +
@@ -115,7 +115,9 @@
 };
 
 struct RenderedText {
-	/// RenderedRects that can be drawn on screen
+using Shared = std::shared_ptr;
+
+/// RenderedRects that can be drawn on screen
 	std::vector rects;
 
 	/// The width occupied  by all rects in pixels.

=== modified file 'src/graphic/text/rt_render.cc'
--- src/graphic/text/rt_render.cc	2017-08-18 14:27:26 +
+++ src/graphic/text/rt_render.cc	2017-11-11 14:31:33 +
@@ -230,7 +230,7 @@
 	virtual uint16_t width() const = 0;
 	virtual uint16_t height() const = 0;
 	virtual uint16_t hotspot_y() const = 0;
-	virtual UI::RenderedText* render(TextureCache* texture_cache) = 0;
+	virtual UI::RenderedText::Shared render(TextureCache* texture_cache) = 0;
 
 	// TODO(GunChleoc): Remove this function once conversion is finished and well tested.
 	virtual std::string debug_info() const = 0;
@@ -527,7 +527,7 @@
 		return rv;
 	}
 
-	UI::RenderedText* render(TextureCache* texture_cache) override;
+UI::RenderedText::Shared render(TextureCache* texture_cache) override;
 
 protected:
 	uint16_t w_, h_;
@@ -550,11 +550,11 @@
 	return font_.ascent(nodestyle_.font_style);
 }
 
-UI::RenderedText* TextNode::render(TextureCache* texture_cache) {
+UI::RenderedText::Shared TextNode::render(TextureCache* texture_cache) {
 	auto rendered_image =
 	   font_.render(txt_, nodestyle_.font_color, nodestyle_.font_style, texture_cache);
 	assert(rendered_image.get() != nullptr);
-	UI::RenderedText* rendered_text = new UI::RenderedText();
+	UI::RenderedText::Shared rendered_text(new UI::RenderedText());
 	rendered_text->rects.push_back(
 	   std::unique_ptr(new UI::RenderedRect(rendered_image)));
 	return rendered_text;
@@ -579,7 +579,7 @@
 		return "ft";
 	}
 
-	UI::RenderedText* render(TextureCache*) override;
+UI::RenderedText::Shared render(TextureCache*) override;
 
 	bool is_expanding() const override {
 		return is_expanding_;
@@ -591,8 +591,8 @@
 private:
 	bool is_expanding_;
 };
-UI::RenderedText* FillingTextNode::render(TextureCache* texture_cache) {
-	UI::RenderedText* rendered_text = new UI::RenderedText();
+UI::RenderedText::Shared FillingTextNode::render(TextureCache* texture_cache) {
+UI::RenderedText::Shared rendered_text(new UI::RenderedText());
 	const std::string hash =
 	   (boost::format("rt:fill:%s:%s:%i:%i:%i:%s") % txt_ % nodestyle_.font_color.hex_value() %
 	nodestyle_.font_style % width() % height() % (is_expanding_ ? "e" : "f"))
@@ -633,9 +633,9 @@
 		return "wsp";
 	}
 
-	UI::RenderedText* render(TextureCache* texture_cache) override {
+UI::RenderedText::Shared render(TextureCache* texture_cache) override {
 		if (show_spaces_) {
-			UI::RenderedText* rendered_text = new UI::RenderedText();
+		UI::RenderedText::Shared rendered_text(new UI::RenderedText());
 			const std::string hash = (boost::format("rt:wsp:%i:%i") % width() % height()).str();
 			std::shared_ptr rendered_image = texture_cache->get(hash);
 			if (rendered_image.get() == nullptr) {
@@ -681,7 +681,7 @@
 	uint16_t hotspot_y() const override {
 		return 0;
 	}
-	UI::RenderedText* render(TextureCache* /* texture_cache */) override {
+UI::RenderedText::Shared render(TextureCache* /* texture_cache */) override {
 		NEVER_HERE();
 	}
 	bool is_non_mandatory_space() const override {
@@ -712,8 +712,8 @@
 	uint16_t hotspot_y() const override {
 		return h_;
 	}
-	UI::RenderedText* render(TextureCache* texture_cache) override {
-		UI::RenderedText* rendered_text = new UI::RenderedText();
+UI::RenderedText::Shared render(TextureCache* texture_cache) override {
+	UI::RenderedText::Shared rendered_text(new UI::RenderedText());
 		const std::string hash = (boost::format("rt:sp:%s:%i:%i:%s") % filename_ % width() %
 		  height() % (is_expanding_ ? "e" : "f"))
 		.str();
@@ -802,8 +802,8 @@
 		return desired_width_;
 	}
 
-	UI::RenderedText* render(TextureCache* texture_cache) override {
-		UI::RenderedText* rendered_text = new UI::RenderedText();
+

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/check_representative_image into lp:widelands

[Klaus Halfmann]

I have some vacation the next two weeks, so I start reviewing things
like this branch. Looks straight forward. I will look around a bit
to better understand the code. 

@Gun: if you have some issue that could need some more attention
debugging please conatct me, I now cca (with some efforts) Debug
Windows (from appvoyer), Ubuntu and OSX.
-- 
https://code.launchpad.net/~widelands-dev/widelands/check_representative_image/+merge/333563
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/check_representative_image 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