← Back to team overview

widelands-dev team mailing list archive

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

 

I think this looks good codewise now. Do you have plans to add the packet to the currently shipped maps or will this be delayed till after b18? Note that we might have this for some maps then (which are changed in the next few days) but not for others - not a big problem as long as all formats still load of course. So what are your plans for this now?

I have a few more nits. Feel free to merge this to trunk when you have addressed those. 

map_version -> typenames are camlcased in Widelands. Either Map_Version, but much rather MapVersion (the first style is still all over Widelands, but is rather silly and very uncommon in the rest of the world).
m_map_source_url.clear() -> the clears in the constructor are unnecessary.
class map_version -> should be a struct MapVersion as you have no member functions and it really is a plain struct (minus the constructor, which is acceptable imho). If you have plans to make this more complex in the future though it might make sense to add getters/setters right now. Your call.

Thanks for taking ownership of this problem - I gladly see it go away.
-- 
https://code.launchpad.net/~widelands-dev/widelands/map_revision_data/+merge/184940
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/map_revision_data.


References