Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/furnace into lp:widelands
Are you sure that "furnace" is the right term? From what I see, it seems to describe what we call smelting works (that is at least what a quick look in http://en.wikipedia.org/wiki/Charcoal reveals). Furthermore, "charcoal furnace" (with quotation mark) has only 36,100 hit on Google, while "charcoal burner" has 181,000. Another possible word seems collier. I would like to hear the opinion of an English native speaker. Please do not understand me wrong: I think unifying charcoal burner and charcoal burner's house is a good idea (it has always bothered me that there were two different terms). What has to be updated: - Some maps talk about not containing much coal and advise to build charcoal burners in their description. I do not know if such maps are shipped with Widelands though. - The translation table in our wiki. -- https://code.launchpad.net/~widelands-dev/widelands/furnace/+merge/227810 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/furnace 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-1341674 into lp:widelands
Review: Resubmit Fixed up the Doxygen comments, so this is good to go. I'll have a separate beanch for the code check rule. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341674/+merge/227693 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341674. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341674_codecheck into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1341674_codecheck into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1341674 in widelands: "Consolidate TODO FIXME BUG to one style." https://bugs.launchpad.net/widelands/+bug/1341674 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1341674_codecheck/+merge/227936 Codecheck rule for https://code.launchpad.net/~widelands-dev/widelands/bug-1341674 The other branch needs to be committed first, or this will flag up tons of stuff that isn't fixed in Trunk yet. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341674_codecheck/+merge/227936 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1341674_codecheck into lp:widelands. === added file 'cmake/codecheck/rules/format_TODO_comments' --- cmake/codecheck/rules/format_TODO_comments 1970-01-01 00:00:00 + +++ cmake/codecheck/rules/format_TODO_comments 2014-07-23 14:52:16 + @@ -0,0 +1,21 @@ +#!/usr/bin/python + +error_msg ="Please use the format \"TODO(): ...\" for your TODO comments, and don't put them in the doygen comments" + +regexp = r"""(FIXME|(\s|/|[*])BUG|TODO(?![(])|\Wtodo(?![(])|[*]\s*TODO|///\s*TODO)""" + +forbidden = [ +"// FIXME this is a todo comment", +"// BUG this is a todo comment", +"// TODO this is a todo comment", +"// TODO: This is a todo comment", +"* TODO: This is a todo comment", +"/// TODO: This is a todo comment", +"\TODO: This is a todo comment", +"\\\todo: This is a todo comment" +] + +allowed = [ +"// TODO() this is a todo comment", +"// TODO(): This is a todo comment" +] ___ 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-1341081 into lp:widelands
OK, next try, but I need somebody to help me hunt down a segfault. I have described the problem in my commit message http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7121 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ 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-1341079 into lp:widelands
Done except for removing the name/descname wrappers in L_Map_Object. I opened a new bug for that one, because it's a bigger change. Regarding the spritesheet thing, maybe we can sole this with a new richtext tag that can take a general Image and then use filename + coordinates to get the image from the spritesheet? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341079/+merge/227441 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341079. ___ 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-1341081 into lp:widelands
GunChleoc, it failed to compile here: [ 82%] Building CXX object src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o In file included from /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc:22:0: /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc: In member function ‘void Widelands::Map_Object_Packet::LoadFinish()’: /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc:116:69: error: ‘const struct Widelands::Map_Object_Descr’ has no member named ‘c_str’ "load_pointers for %s: %s", (*i.current)->get_object()->descr().c_str(), e.what()); ^ /var/widelands/BZR/bug-1341081/bug-1341081/src/base/wexception.h:60:57: note: in definition of macro ‘wexception’ #define wexception(...) _wexception(__FILE__, __LINE__, __VA_ARGS__) ^ src/map_io/CMakeFiles/map_io.dir/build.make:399: recipe for target 'src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o' failed make[2]: *** [src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o] Error 1 CMakeFiles/Makefile2:21975: recipe for target 'src/map_io/CMakeFiles/map_io.dir/all' failed make[1]: *** [src/map_io/CMakeFiles/map_io.dir/all] Error 2 Makefile:127: recipe for target 'all' failed make: *** [all] Error 2 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ 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/furnace into lp:widelands
To unify the, we could all call them "Charcoal Burner's House". I did some digging online and there doesn't seem to be an easy English term for the German "Köhlerei" at all. You can have a pit or a stack, but what you see in-game is a house: What about "kiln"? http://www.charcoalburners.co.uk/charcoal.php As long as it's clear it's not a pottery kiln... *starts tearing out own hair* -- https://code.launchpad.net/~widelands-dev/widelands/furnace/+merge/227810 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/furnace 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/furnace into lp:widelands
> Some maps talk about not containing much coal and advise to build charcoal > burners in their description. I do not know if such maps are shipped with > Widelands though. The plateau was the only map I found which mentioned "burner" in any way. >The translation table in our wiki. While I didn't mention it, this is of course on my todo list. >As long as it's clear it's not a pottery kiln... ..and you have to make sure it's not confused for the lime kiln. *Sigh* -- https://code.launchpad.net/~widelands-dev/widelands/furnace/+merge/227810 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/furnace 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/furnace into lp:widelands
None of the proposed options really convinces me. I do not like "kiln" because we already have a "lime kiln". As it stands, I prefer renaming all three to "Charcoal Burner's House". -- https://code.launchpad.net/~widelands-dev/widelands/furnace/+merge/227810 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/furnace 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/furnace into lp:widelands
Barbarians do not build houses. I understand that there is movement to unify names, but please also keep the in mind that the three tribes should be different and should have their own individuality. aD, I added you as a reviewer since your a native speaker. Can you add anything to the discussion? -- https://code.launchpad.net/~widelands-dev/widelands/furnace/+merge/227810 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/furnace 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-1341079 into lp:widelands
Review: Approve I merged this now, but removed the TODO discussion around spritesheets. I think it can be handled transparently - .menu can return a hash of an image in the image cache. Since the renderer uses the image cache for looking up it's images, this should be opaque for the renderer. The one problem remaining is with texts that are saved into savegames that contain this richtext message. Since .menu is never called on load, the menu pic might not be in the image cache. Oh well, this is a future problem to be solved. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341079/+merge/227441 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341079. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341079 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1341079 into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1341079/+merge/227441 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341079/+merge/227441 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341079. ___ 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-1341674 into lp:widelands
Review: Approve -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341674/+merge/227693 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341674. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341674 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1341674 into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1341674/+merge/227693 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341674/+merge/227693 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341674. ___ 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-1341674_codecheck into lp:widelands
Review: Needs Fixing Diff comments: > === added file 'cmake/codecheck/rules/format_TODO_comments' > --- cmake/codecheck/rules/format_TODO_comments1970-01-01 00:00:00 > + > +++ cmake/codecheck/rules/format_TODO_comments2014-07-23 14:52:16 > + > @@ -0,0 +1,21 @@ > +#!/usr/bin/python > + > +error_msg ="Please use the format \"TODO(): ...\" for your TODO > comments, and don't put them in the doygen comments" Tighten this sentence up a bit. It might be read a lot of time by developers, so it should be short. It also has a typo doygen -> doxygen. Suggestion.: Use "TODO(username): . Do not put TODOs in Doxygen comments." > + > +regexp = > r"""(FIXME|(\s|/|[*])BUG|TODO(?![(])|\Wtodo(?![(])|[*]\s*TODO|///\s*TODO)""" again, I think a python, line by line version would be simpler to read and execute faster. > + > +forbidden = [ > +"// FIXME this is a todo comment", > +"// BUG this is a todo comment", > +"// TODO this is a todo comment", > +"// TODO: This is a todo comment", > +"* TODO: This is a todo comment", > +"/// TODO: This is a todo comment", > +"\TODO: This is a todo comment", > +"\\\todo: This is a todo comment" > +] > + > +allowed = [ > +"// TODO() this is a todo comment", only allow one of these styles. The second one is slightly easier to grep for. > +"// TODO(): This is a todo comment" > +] > -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341674_codecheck/+merge/227936 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341674_codecheck. ___ 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
