widelands-dev team mailing list archive

[Benedikt Straub <benedikt-straub@xxxxxx>]

Looks good to me. Some comments in the diff.

Diff comments:

> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc	2019-06-23 11:41:17 +0000
> +++ src/logic/playercommand.cc	2019-06-24 22:00:34 +0000
> @@ -226,7 +200,7 @@
>  }
>  
>  void CmdBulldoze::serialize(StreamWrite& ser) {
> -	ser.unsigned_8(PLCMD_BULLDOZE);
> +	write_id(ser);
>  	ser.unsigned_8(sender());

These two lines are used pretty much everywhere. Pull out into a parent class method?

>  	ser.unsigned_32(serial);
>  	ser.unsigned_8(recurse);
> @@ -1263,6 +1237,7 @@
>  }
>  
>  void CmdChangeTargetQuantity::serialize(StreamWrite& ser) {
> +	// Does not implement id(), so we don't have any id header to serialize

Why doesn´t it implement it? Looks like a bug to me…

>  	ser.unsigned_8(sender());
>  	ser.unsigned_32(economy());
>  	ser.unsigned_8(ware_type());
> 
> === modified file 'src/logic/queue_cmd_ids.h'
> --- src/logic/queue_cmd_ids.h	2019-06-09 10:33:51 +0000
> +++ src/logic/queue_cmd_ids.h	2019-06-24 22:00:34 +0000
> @@ -33,17 +33,21 @@
>  
>  namespace Widelands {
>  
> -enum class QueueCommandTypes {
> +// The command types are used by the QueueCmdFactory, for network serialization
> +// and for savegame compatibility.
> +// DO NOT change the order
> +// TODO(GunChleoc): Whenever we break savegame compatibility, clean this up.
> +enum class QueueCommandTypes : uint8_t {

Since you´re already changing it, perhaps we should make it uint16_t so we won´t run out of IDs some day

>  
>  	/* ID zero is reserved and must never be used */
>  	kNone = 0,
>  
>  	/* PLAYER COMMANDS BELOW */
>  	kBuild,
> -	kFlag,
> +	kBuildFlag,
>  	kBuildRoad,
>  	kFlagAction,
> -	kStopBuilding,
> +	kStartStopBuilding,
>  	kEnhanceBuilding,
>  	kBulldoze,
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup_playercommand_enums/+merge/369267
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/cleanup_playercommand_enums into lp:widelands.