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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
