widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Code LGTM, but I have added some comments. Please have a look :)


Diff comments:

> 
> === modified file 'src/scripting/lua_game.cc'
> --- src/scripting/lua_game.cc	2016-01-28 05:24:34 +0000
> +++ src/scripting/lua_game.cc	2016-02-12 17:03:35 +0000
> @@ -1132,19 +1132,19 @@
>  };
>  
>  LuaMessage::LuaMessage(uint8_t plr, MessageId id) {
> -	m_plr = plr;
> -	m_mid = id;
> +	player_number_ = plr;
> +	message_id_    = id;
>  }
>  
>  void LuaMessage::__persist(lua_State * L) {
> -	PERS_UINT32("player", m_plr);
> -	PERS_UINT32("msg_idx", get_mos(L)->message_savers[m_plr - 1][m_mid].value());
> +	PERS_UINT32("player", player_number_);

PERS_UINT8? (See comment about PlayerNumber below)

We will break savegame compatibility in this releas cycle, so if we want to change this, we have to do it now.

> +	PERS_UINT32("msg_idx", get_mos(L)->message_savers[player_number_ - 1][message_id_].value());
>  }
>  void LuaMessage::__unpersist(lua_State * L) {
> -	UNPERS_UINT32("player", m_plr);
> +	UNPERS_UINT32("player", player_number_);

UNPERS_UINT8? (See comment about PlayerNumber below)

>  	uint32_t midx = 0;
>  	UNPERS_UINT32("msg_idx", midx);
> -	m_mid = MessageId(midx);
> +	message_id_ = MessageId(midx);
>  }
>  
>  /*
> 
> === modified file 'src/scripting/lua_game.h'
> --- src/scripting/lua_game.h	2015-12-04 18:27:36 +0000
> +++ src/scripting/lua_game.h	2016-02-12 17:03:35 +0000
> @@ -144,15 +144,15 @@
>  };
>  
>  class LuaMessage : public LuaGameModuleClass {
> -	uint32_t m_plr;
> -	Widelands::MessageId m_mid;
> +	uint32_t             player_number_; // TODO(Hasi50): in CTor this is uint8_t, well

We actually have a named data type PlayerNumber for this, which is defined in logic/widelands.h. I think we should use that instead.

> +	Widelands::MessageId message_id_;
>  
>  public:
>  	LUNA_CLASS_HEAD(LuaMessage);
>  	virtual ~LuaMessage() {}
>  
>  	LuaMessage(uint8_t, Widelands::MessageId);
> -	LuaMessage() : m_plr(0), m_mid(0) {}
> +	LuaMessage() : player_number_(0), message_id_(0) {}
>  	LuaMessage(lua_State * L) {
>  		report_error(L, "Cannot instantiate a '%s' directly!", className);
>  	}
> 
> === modified file 'src/scripting/lua_interface.cc'
> --- src/scripting/lua_interface.cc	2015-03-21 13:18:02 +0000
> +++ src/scripting/lua_interface.cc	2016-02-12 17:03:35 +0000
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2006-2010, 2013 by the Widelands Development Team
> + * Copyright (C) 2006-2016, 2013 by the Widelands Development Team

Delete the 2013

>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> 
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc	2016-02-11 06:50:56 +0000
> +++ src/scripting/lua_map.cc	2016-02-12 17:03:35 +0000
> @@ -4864,11 +4864,11 @@
>  };
>  
>  void LuaPlayerSlot::__persist(lua_State * L) {
> -	PERS_UINT32("player", m_plr);
> +	PERS_UINT32("player", player_number_);

uint8?

>  }
>  
>  void LuaPlayerSlot::__unpersist(lua_State * L) {
> -	UNPERS_UINT32("player", m_plr);
> +	UNPERS_UINT32("player", player_number_);

uint8?

>  }
>  
>  /*
> 
> === modified file 'src/scripting/lua_ui.cc'
> --- src/scripting/lua_ui.cc	2015-08-06 17:14:34 +0000
> +++ src/scripting/lua_ui.cc	2016-02-12 17:03:35 +0000
> @@ -122,10 +122,10 @@
>  	}
>  }
>  int LuaPanel::get_buttons(lua_State * L) {
> -	assert(m_panel);
> +	assert(panel_);
>  
>  	lua_newtable(L);
> -	_put_all_visible_buttons_into_table(L, m_panel);
> +	_put_all_visible_buttons_into_table(L, panel_);

put_all_visible_buttons_into_table(L, panel_); - we should lose the initial _ as well. I already have a branch for that, but I might have missed this one.

>  
>  	return 1;
>  }
> @@ -153,10 +153,10 @@
>  	}
>  }
>  int LuaPanel::get_tabs(lua_State * L) {
> -	assert(m_panel);
> +	assert(panel_);
>  
>  	lua_newtable(L);
> -	_put_all_tabs_into_table(L, m_panel);
> +	_put_all_tabs_into_table(L, panel_);

We should lose the initial _ as well. I already have a branch for that, but I might have missed this one.

>  
>  	return 1;
>  }
> @@ -185,10 +185,10 @@
>  	}
>  }
>  int LuaPanel::get_windows(lua_State * L) {
> -	assert(m_panel);
> +	assert(panel_);
>  
>  	lua_newtable(L);
> -	_put_all_visible_windows_into_table(L, m_panel);
> +	_put_all_visible_windows_into_table(L, panel_);

We should lose the initial _ as well. I already have a branch for that, but I might have missed this one.

>  
>  	return 1;
>  }


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