← Back to team overview

widelands-dev team mailing list archive

Re: [Merge] lp:~widelands-dev/widelands/fh1-multitexture into lp:widelands

 

Thanks for the review! I have hopefully fixed the compiler warnings now - we'll just have to wait what clang says.

I also addressed your points from a few posts up and added some more documentation to the RendererText header file - I hope it will make the code a bit more clear. Yes, it's a bit complicated due to the cropping involved.

2 notes:

1. CropMode::kVertical doesn't exist because CropMode::kHorizontal was a stupid name. I went with kSelf now. Btter ideas are welcome :)

2. Re. better replace true / false by kBackGroundColorSet and !kBackGroundColorSet or such: While I generally prefer enum classes over bools myself, I think it would actually make the code harder to read in this instance, because there is no "else" branch when it is checked. This bool is only ever assigned in the private constructors, so it shouldn't cause any interface readability issues.

-- 
https://code.launchpad.net/~widelands-dev/widelands/fh1-multitexture/+merge/323903
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fh1-multitexture.


References