widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #02675
Re: [Merge] lp:~widelands-dev/widelands/bug-988831 into lp:widelands
Review: Needs Fixing
Diff comments:
> === modified file 'src/game_io/game_loader.cc'
> --- src/game_io/game_loader.cc 2014-07-28 14:23:03 +0000
> +++ src/game_io/game_loader.cc 2014-09-11 17:46:54 +0000
> @@ -32,7 +32,6 @@
> #include "game_io/game_player_info_data_packet.h"
> #include "game_io/game_preload_data_packet.h"
> #include "io/filesystem/layered_filesystem.h"
> -#include "logic/cmd_expire_message.h"
> #include "logic/game.h"
> #include "logic/player.h"
> #include "map_io/widelands_map_map_object_loader.h"
> @@ -103,13 +102,6 @@
> Message* m = temp_message.second;
> Message_Id m_id = temp_message.first;
>
> - // Renew expire commands
> - Duration const duration = m->duration();
> - if (duration != Forever()) {
> - m_game.cmdqueue().enqueue
> - (new Cmd_ExpireMessage
> - (m->sent() + duration, p, m_id));
> - }
> // Renew MapObject connections
> if (m->serial() > 0) {
> MapObject* mo = m_game.objects().get_object(m->serial());
>
> === modified file 'src/logic/CMakeLists.txt'
> --- src/logic/CMakeLists.txt 2014-07-28 17:12:07 +0000
> +++ src/logic/CMakeLists.txt 2014-09-11 17:46:54 +0000
> @@ -76,8 +76,8 @@
> checkstep.h
> cmd_calculate_statistics.cc
> cmd_calculate_statistics.h
> - cmd_expire_message.cc
> - cmd_expire_message.h
> + cmd_delete_message.cc
> + cmd_delete_message.h
> cmd_incorporate.cc
> cmd_incorporate.h
> cmd_luacoroutine.cc
>
> === modified file 'src/logic/building.cc'
> --- src/logic/building.cc 2014-08-01 12:57:17 +0000
> +++ src/logic/building.cc 2014-09-11 17:46:54 +0000
> @@ -910,8 +910,8 @@
> rt_description += "</p></rt>";
>
> Message * msg = new Message
> - (msgsender, game.get_gametime(), 60 * 60 * 1000,
> - title, rt_description, get_position(), (link_to_building_lifetime ? m_serial : 0));
> + (msgsender, game.get_gametime(), title, rt_description,
> + get_position(), (link_to_building_lifetime ? m_serial : 0));
>
> if (throttle_time)
> owner().add_message_with_timeout
>
> === renamed file 'src/logic/cmd_expire_message.cc' => 'src/logic/cmd_delete_message.cc'
> --- src/logic/cmd_expire_message.cc 2013-07-26 20:19:36 +0000
> +++ src/logic/cmd_delete_message.cc 2014-09-11 17:46:54 +0000
> @@ -17,15 +17,15 @@
> *
> */
>
> -#include "logic/cmd_expire_message.h"
> +#include "logic/cmd_delete_message.h"
>
> #include "logic/game.h"
> #include "logic/player.h"
>
> namespace Widelands {
>
> -void Cmd_ExpireMessage::execute(Game & game) {
> - game.player(player).messages().expire_message(message);
> +void Cmd_DeleteMessage::execute(Game & game) {
> + game.player(player).messages().delete_message(message);
> }
>
> }
>
> === renamed file 'src/logic/cmd_expire_message.h' => 'src/logic/cmd_delete_message.h'
> --- src/logic/cmd_expire_message.h 2014-07-26 10:43:23 +0000
> +++ src/logic/cmd_delete_message.h 2014-09-11 17:46:54 +0000
> @@ -17,8 +17,8 @@
> *
> */
>
> -#ifndef WL_LOGIC_CMD_EXPIRE_MESSAGE_H
> -#define WL_LOGIC_CMD_EXPIRE_MESSAGE_H
> +#ifndef WL_LOGIC_CMD_DELETE_MESSAGE_H
Is delete correct? Should it not be archive?
Edit: Nevermind, delete is correct.
> +#define WL_LOGIC_CMD_DELETE_MESSAGE_H
>
> #include <memory>
>
> @@ -27,7 +27,7 @@
>
> namespace Widelands {
>
> -/// Expires the player's message.
> +/// Delete the player's message.
> ///
> /// \note This is not a GameLogicCommand because it should not be saved.
this comment seems no longer correct. Why should there be a delete command for each message?
> /// Instead, the commands are recreated separately on load (when both command
> @@ -35,8 +35,8 @@
> /// command and then when loading, checking that one exists for each message
> /// and if not, warn and recreate it. Such redundancy would also waste space in
> /// the savegame.
> -struct Cmd_ExpireMessage : public Command {
> - Cmd_ExpireMessage
> +struct Cmd_DeleteMessage : public Command {
> + Cmd_DeleteMessage
> (int32_t const t, Player_Number const p, Message_Id const m)
> : Command(t), player(p), message(m)
> {}
> @@ -51,4 +51,4 @@
>
> }
>
> -#endif // end of include guard: WL_LOGIC_CMD_EXPIRE_MESSAGE_H
> +#endif // end of include guard: WL_LOGIC_CMD_DELETE_MESSAGE_H
>
> === modified file 'src/logic/cmd_luacoroutine.cc'
> --- src/logic/cmd_luacoroutine.cc 2014-07-28 14:17:07 +0000
> +++ src/logic/cmd_luacoroutine.cc 2014-09-11 17:46:54 +0000
> @@ -50,8 +50,7 @@
> for (int i = 1; i <= game.map().get_nrplayers(); i++) {
> Widelands::Message & msg =
> *new Widelands::Message
> - ("Game Logic", game.get_gametime(),
> - Forever(), "Lua Coroutine Failed", e.what());
> + ("Game Logic", game.get_gametime(), "Lua Coroutine Failed", e.what());
> game.player(i).add_message(game, msg, true);
> }
> game.gameController()->setDesiredSpeed(0);
>
> === modified file 'src/logic/message.h'
> --- src/logic/message.h 2014-07-28 14:23:03 +0000
> +++ src/logic/message.h 2014-09-11 17:46:54 +0000
> @@ -46,7 +46,6 @@
> Message
> (const std::string & msgsender,
> uint32_t sent_time,
> - Duration d,
> const std::string & t,
> const std::string & b,
> Widelands::Coords const c = Coords::Null(),
> @@ -57,7 +56,6 @@
> m_title(t),
> m_body (b),
> m_sent (sent_time),
> - m_duration(d),
> m_position(c),
> m_serial (ser),
> m_status (s)
> @@ -65,7 +63,6 @@
>
> const std::string & sender() const {return m_sender;}
> uint32_t sent () const {return m_sent;}
> - Duration duration() const {return m_duration;}
> const std::string & title() const {return m_title;}
> const std::string & body () const {return m_body;}
> Widelands::Coords position() const {return m_position;}
> @@ -78,7 +75,6 @@
> std::string m_title;
> std::string m_body;
> uint32_t m_sent;
> - Duration m_duration; /// will expire after this duration
> Widelands::Coords m_position;
> Widelands::Serial m_serial; // serial to map object
> Status m_status;
>
> === modified file 'src/logic/message_queue.h'
> --- src/logic/message_queue.h 2014-07-28 14:23:03 +0000
> +++ src/logic/message_queue.h 2014-09-11 17:46:54 +0000
> @@ -121,12 +121,11 @@
>
> /// Expire the message with the given id so that it no longer exists.
> /// Assumes that a message with the given id exists.
> - void expire_message(const Message_Id& id) {
> + void delete_message(const Message_Id& id) {
> assert_counts();
> iterator const it = find(id);
> if (it == end()) {
> - // Messages can be expired when the timeout runs out, or when the linked
> - // MapObject is removed, or both. In this later case, two expire commands
> + // Messages can be expired when the linked MapObject is removed. Two expire commands
s/expire/deleted. Also grep -i expire through the codebase.
> // will be executed, and the message will not be present for the second one.
> // So we assume here that the message was removed from an earlier expire cmd.
> return;
>
> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc 2014-07-28 16:59:54 +0000
> +++ src/logic/player.cc 2014-09-11 17:46:54 +0000
> @@ -36,7 +36,7 @@
> #include "io/filewrite.h"
> #include "logic/building.h"
> #include "logic/checkstep.h"
> -#include "logic/cmd_expire_message.h"
> +#include "logic/cmd_delete_message.h"
> #include "logic/cmd_luacoroutine.h"
> #include "logic/constants.h"
> #include "logic/constructionsite.h"
> @@ -309,14 +309,7 @@
> Message_Id Player::add_message
> (Game & game, Message & message, bool const popup)
> {
> - // Expire command
> Message_Id id = messages().add_message(message);
> - Duration const duration = message.duration();
> - if (duration != Forever()) {
> - game.cmdqueue().enqueue
> - (new Cmd_ExpireMessage
> - (game.get_gametime() + duration, player_number(), id));
> - }
>
> // MapObject connection
> if (message.serial() > 0) {
> @@ -359,14 +352,14 @@
>
> void Player::message_object_removed(Message_Id m_id) const
> {
> - // Send expire command
> + // Send delete command
> upcast(Game, game, &m_egbase);
> if (!game) {
> return;
> }
>
> game->cmdqueue().enqueue
> - (new Cmd_ExpireMessage
> + (new Cmd_DeleteMessage
> (game->get_gametime(), m_plnum, m_id));
> }
>
>
> === modified file 'src/logic/ship.cc'
> --- src/logic/ship.cc 2014-07-28 16:59:54 +0000
> +++ src/logic/ship.cc 2014-09-11 17:46:54 +0000
> @@ -914,8 +914,7 @@
> rt_description += "</p></rt>";
>
> Message * msg = new Message
> - (msgsender, game.get_gametime(), 60 * 60 * 1000,
> - title, rt_description, get_position(), m_serial);
> + (msgsender, game.get_gametime(), title, rt_description, get_position(), m_serial);
>
> get_owner()->add_message(game, *msg);
> }
>
> === modified file 'src/logic/soldier.cc'
> --- src/logic/soldier.cc 2014-07-28 16:59:54 +0000
> +++ src/logic/soldier.cc 2014-09-11 17:46:54 +0000
> @@ -1571,7 +1571,7 @@
> (game,
> *new Message
> ("game engine",
> - game.get_gametime(), Forever(),
> + game.get_gametime(),
> _("Logic error"),
> buffer,
> get_position(),
> @@ -1580,7 +1580,7 @@
> (game,
> *new Message
> ("game engine",
> - game.get_gametime(), Forever(),
> + game.get_gametime(),
> _("Logic error"),
> buffer,
> opponent.get_position(),
>
> === modified file 'src/logic/worker.cc'
> --- src/logic/worker.cc 2014-08-26 17:25:00 +0000
> +++ src/logic/worker.cc 2014-09-11 17:46:54 +0000
> @@ -947,7 +947,7 @@
> (game,
> *new Message
> ("geologist " + rdescr->name(), // e.g. "geologist gold"
> - game.get_gametime(), 60 * 60 * 1000,
> + game.get_gametime(),
> rdescr->descname(),
> message,
> position,
> @@ -1845,7 +1845,7 @@
> (game,
> *new Message
> ("game engine",
> - game.get_gametime(), Forever(),
> + game.get_gametime(),
> _("Worker got lost!"),
> buffer,
> get_position()),
>
> === modified file 'src/map_io/widelands_map_players_messages_data_packet.cc'
> --- src/map_io/widelands_map_players_messages_data_packet.cc 2014-07-28 14:23:03 +0000
> +++ src/map_io/widelands_map_players_messages_data_packet.cc 2014-09-11 17:46:54 +0000
> @@ -56,7 +56,7 @@
> if (begin != messages.end()) {
> log
> ("ERROR: The message queue for player %u contains a message "
> - "before any messages have been loade into it. This is a bug "
> + "before any messages have been loaded into it. This is a bug "
> "in the savegame loading code. It created a new message and "
> "added it to the queue. This is only allowed during "
> "simulation, not at load. The following messge will be "
> @@ -64,7 +64,6 @@
> "\tsender : %s\n"
> "\ttitle : %s\n"
> "\tsent : %u\n"
> - "\tduration: %u\n"
> "\tposition: (%i, %i)\n"
> "\tstatus : %u\n"
> "\tbody : %s\n",
> @@ -72,7 +71,6 @@
> begin->second->sender ().c_str(),
> begin->second->title ().c_str(),
> begin->second->sent (),
> - begin->second->duration(),
> begin->second->position().x, begin->second->position().y,
> begin->second->status (),
> begin->second->body ().c_str());
> @@ -96,30 +94,7 @@
> "message is sent in the future: sent at %u but "
> "gametime is only %u",
> sent, gametime);
> - uint32_t duration = Forever(); // default duration
> - if (Section::Value const * const dv = s->get_val("duration")) {
> - duration = dv->get_positive();
> - if (duration == Forever())
> - throw game_data_error
> - (
> - "the value %u is not allowed as duration; it is "
> - "a special value meaning forever, which is the "
> - "default; omit the duration key to make the "
> - "message exist forever",
> - Forever());
> - if (sent + duration < sent)
> - throw game_data_error
> - (
> - "duration %u is too large; causes numeric "
> - "overflow when added to sent time %u",
> - duration, sent);
> - if (sent + duration < gametime)
> - throw game_data_error
> - (
> - "message should have expired at %u; sent at %u "
> - "with duration %u but gametime is already %u",
> - sent + duration, sent, duration, gametime);
> - }
> +
> Message::Status status = Message::Archived; // default status
> if (char const * const status_string = s->get_string("status")) {
> try {
> @@ -147,15 +122,12 @@
> (*new Message
> (s->get_string ("sender", ""),
> sent,
> - duration,
> s->get_name (),
> s->get_safe_string("body"),
> get_coords("position", extent, Coords::Null(), s),
> serial,
> status));
> - // Expiration is scheduled for all messages (with finite
> - // duration) after the command queue has been loaded (in
> - // Game_Loader::load_game).
> +
> previous_message_sent = sent;
> } catch (const _wexception & e) {
> throw game_data_error
> @@ -183,44 +155,11 @@
> message_saver.add (temp_message.first);
> const Message & message = *temp_message.second;
> assert(message.sent() <= static_cast<uint32_t>(egbase.get_gametime()));
> - assert
> - (message.duration() == Forever() ||
> - message.sent() < message.sent() + message.duration());
> - if
> - (message.duration() != Forever() &&
> - message.sent() + message.duration()
> - <
> - static_cast<uint32_t>(egbase.get_gametime()))
> - log
> - ("ERROR: Trying to save a message that should have expired:\n"
> - "\tsent = %u, duration = %u, expiry = %u, gametime = %u\n"
> - "\tsender = \"%s\"\n"
> - "\ttitle: %s\n"
> - "\tbody: %s\n"
> - "\tposition: (%i, %i)\n"
> - "\tstatus: %s\n",
> - message.sent(), message.duration(),
> - message.sent() + message.duration(), egbase.get_gametime(),
> - message.sender().c_str(), message.title().c_str(),
> - message.body().c_str(),
> - message.position().x, message.position().y,
> - message.status() == Message::New ? "new" :
> - message.status() == Message::Read ? "read" :
> - message.status() == Message::Archived ? "archived" : "ERROR");
> - assert
> - (message.duration() == Forever() ||
> - static_cast<uint32_t>(egbase.get_gametime())
> - <=
> - message.sent() + message.duration());
> +
> Section & s = prof.create_section_duplicate(message.title().c_str());
> if (message.sender().size())
> s.set_string("sender", message.sender ());
> s.set_int ("sent", message.sent ());
> - {
> - Duration const duration = message.duration();
> - if (duration != Forever()) // The default duration. Do not write.
> - s.set_int("duration", duration);
> - }
> s.set_string ("body", message.body ());
> if (Coords const c = message.position())
> set_coords("position", c, &s);
>
> === modified file 'src/scripting/lua_game.cc'
> --- src/scripting/lua_game.cc 2014-07-28 16:59:54 +0000
> +++ src/scripting/lua_game.cc 2014-09-11 17:46:54 +0000
> @@ -297,18 +297,12 @@
> std::string title = luaL_checkstring(L, 2);
> std::string body = luaL_checkstring(L, 3);
> Coords c = Coords::Null();
> - Duration d = Forever();
> Message::Status st = Message::New;
> std::string sender = "ScriptingEngine";
> bool popup = false;
>
> if (n == 4) {
> // Optional arguments
> - lua_getfield(L, 4, "duration");
fix documentation?
> - if (!lua_isnil(L, -1))
> - d = luaL_checkuint32(L, -1);
> - lua_pop(L, 1);
> -
> lua_getfield(L, 4, "field");
> if (!lua_isnil(L, -1))
> c = (*get_user_class<L_Field>(L, -1))->coords();
> @@ -344,7 +338,6 @@
> *new Message
> (sender,
> game.get_gametime(),
> - d,
> title,
> body,
> c,
> @@ -1065,7 +1058,6 @@
> PROP_RO(L_Message, title),
> PROP_RO(L_Message, body),
> PROP_RO(L_Message, sent),
> - PROP_RO(L_Message, duration),
> PROP_RO(L_Message, field),
> PROP_RW(L_Message, status),
> {nullptr, nullptr, nullptr},
> @@ -1131,20 +1123,6 @@
> }
>
> /* RST
> - .. attribute:: duration
> -
> - (RO) The time in milliseconds before this message is invalidated or nil if
> - this message has an endless duration.
> -*/
> -int L_Message::get_duration(lua_State * L) {
> - uint32_t d = get(L, get_game(L)).duration();
> - if (d == Forever())
> - return 0;
> - lua_pushuint32(L, d);
> - return 1;
> -}
> -
> -/* RST
> .. attribute:: field
>
> (RO) The field that corresponds to this Message.
>
> === modified file 'src/scripting/lua_game.h'
> --- src/scripting/lua_game.h 2014-07-26 10:43:23 +0000
> +++ src/scripting/lua_game.h 2014-09-11 17:46:54 +0000
> @@ -165,7 +165,6 @@
> int get_sent(lua_State * L);
> int get_title(lua_State * L);
> int get_body(lua_State * L);
> - int get_duration(lua_State * L);
> int get_field(lua_State * L);
> int get_status(lua_State * L);
> int set_status(lua_State * L);
>
> === modified file 'test/maps/lua_testsuite.wmf/scripting/messages.lua'
> --- test/maps/lua_testsuite.wmf/scripting/messages.lua 2014-01-12 19:06:22 +0000
> +++ test/maps/lua_testsuite.wmf/scripting/messages.lua 2014-09-11 17:46:54 +0000
> @@ -1,52 +1,47 @@
> -- =======================================================================
> --- Testing Messages System
> +-- Testing Messages System
> -- =======================================================================
>
> messages_tests = lunit.TestCase("Messages")
> -function messages_tests:setup()
> +function messages_tests:setup()
> for i,m in ipairs(player1.inbox) do
> m.status = "archived"
> end
> end
>
> -function messages_tests:test_defaults()
> +function messages_tests:test_defaults()
> local m = player1:send_message("Hallo", "World!")
> assert_equal("Hallo", m.title)
> assert_equal("World!", m.body)
> assert_equal("ScriptingEngine", m.sender)
> assert_equal(0, m.sent)
> assert_equal(0, m.sent)
> - assert_equal(nil, m.duration)
> assert_equal(nil, m.field)
> assert_equal("new", m.status)
> end
> -function messages_tests:test_status_read()
> +function messages_tests:test_status_read()
> local m = player1:send_message("Hallo", "World!", {status="read"})
> assert_equal("read", m.status)
> end
> -function messages_tests:test_status_archived()
> +function messages_tests:test_status_archived()
> local m = player1:send_message("Hallo", "World!", {status="archived"})
> assert_equal("archived", m.status)
> end
> -function messages_tests:test_status_illegal()
> +function messages_tests:test_status_illegal()
> assert_error("Illegal status!", function()
> player1:send_message("Hallo", "World!", {status="nono"})
> end)
> end
> -function messages_tests:test_sender()
> +function messages_tests:test_sender()
> local m = player1:send_message("Hallo", "World!", {sender="i am you"})
> assert_equal("i am you", m.sender)
> end
> -function messages_tests:test_field()
> +function messages_tests:test_field()
> local f = map:get_field(23,28)
> local m = player1:send_message("Hallo", "World!", {field = f})
> assert_equal(f, m.field)
> end
> -function messages_tests:test_duration()
> - local m = player1:send_message("Hallo", "World!", {duration = 2000})
> - assert_equal(2000, m.duration)
> -end
> -function messages_tests:test_changing_status()
> +function messages_tests:test_changing_status()
> local m = player1:send_message("Hallo", "World!")
> m.status = "read"
> assert_equal("read", m.status)
>
--
https://code.launchpad.net/~widelands-dev/widelands/bug-988831/+merge/233885
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-988831.
References
