widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

[redundant widelands* in file and library names]
I agree about removing them. They were sometimes added in the past to work around having the same names in system header files. But since our include paths are now much cleaner and we always include the full local path renaming should not be problematic. Just make sure to use bzr mv so that bzr pick up on the renames.

> A single header file in its own library is a little bit of an overkill. Then again it might be good for the initial refactoring and modularization and we can merge libraries that are often used together into one.

I disagree - libraries should be small, logical units. And sometimes a single header with one feature (like container_iterate) doesn't really belong anywhere else. With the build_deps.py script it is also no too inconvenient to keep the build files up to date. The only think that is unfortunate is that cmake does not support header only libraries - so I had to add a bunch of dummy .cc files. For that reason I can see bunching up small libraries into bigger catch-alls would make sense.

> Thanks for all the work. I really like the way the Widelands code is developing right now: easier to understand, better structured and more fun to work with :) 

I am glad that you approve of the changes. I feel I spend too much time refactoring (one_world is also just a refactor basically) and not adding enough new value to the project. Also the latest changes (especially this one) does not really make the live of developers easier - we now have to maintain our build files too instead of just dumping a .cc file someplace and it will get compiled. I still think this makes more sense in the long run - but only when we follow through and reduce coupling in the code base. I hope everybody will help out with this.

> I hope to contribute more in the future myself but currently I am a little afraid to do big changes because everything is changed so quickly.

1) There are now plenty of 10-60 minutes task that can be done - peeling away ball_of_mud into smaller libraries or splitting logic into smaller, less coupled pieces. I also filed a bunch of bugs with tag cleanups (https://bugs.launchpad.net/widelands/+bugs?field.tag=cleanups). Most of them should be fairly quick too. Feel free to work on that :)
2) I maintained 2 huge changes (one_world and Guns wrapping of MapObjectDescr) in the last few months, and I was very surprised how well bzr handled merging of the huge changes. Sure, they were merge conflicts, but if you always start out with merging trunk when you sit down to work on anything, you are pretty fine.
3) Striving for smaller changes is always better - for example feel free to do refactorings in smaller pieces. For example, introduce a better API first and start using it in only some places and then change the users one by one in trunk later and only remove the old API when done completely is fine. I think we should move away from big branches as much as possible (of course it is not always possible). If you have something that you want to work on specifically, feel free to reach out and we can come up with a strategy that does not take you too far away from trunk at any time.

[compilers]
Teppo, thanks for fixing the gcc builds. I think we can now state that widelands compiles on gcc >= 4.7 and clang >= 3.3. I feel good about that and consider the c++11 transition a success. 

[older cmake versions]
I am usually not too concerned about debian, but others usually are. I will go ahead and merge this branch now as is, but I am fine with somebody adding one more macro that does target_include_directories and SYSTEM for never cmakes and adding a simple -I for all targets when running on older cmakes. I think this can happen in trunk. 

[Closing remarks].
This branch is a huge step forward for better code structure in Widelands. I also think that it was a wonderful collaboration example with 6 people participating in the discussion and 4 people contributing code. I wish we could have more of those branches in the future - if somebody has suggestions for procedure changes how to make that happen more often, I am very open.
-- 
https://code.launchpad.net/~widelands-dev/widelands/cmake-reworked/+merge/222455
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmake-reworked.