- simplify and clarify dc_msg caching for Message object

- merge state class into Message object proper -- one less intermediate object to worry about for callers

python/src/deltachat/chatting.py

@@ -155,7 +155,8 @@ class Chat(object):
         :returns: the resulting :class:`deltachat.message.Message` instance
         """
         msg = self.prepare_message_file(path=path, mime_type=mime_type)
-        return self.send_prepared(msg)
+        self.send_prepared(msg)
+        return msg
 
     def send_image(self, path):
         """ send an image message and return the resulting Message instance.
@@ -166,10 +167,11 @@ class Chat(object):
         """
         mime_type = mimetypes.guess_type(path)[0]
         msg = self.prepare_message_file(path=path, mime_type=mime_type, view_type="image")
-        return self.send_prepared(msg)
+        self.send_prepared(msg)
+        return msg
 
     def prepare_message(self, msg):
-        """ create a new message.
+        """ create a new prepared message.
 
         :param msg: the message to be prepared.
         :returns: :class:`deltachat.message.Message` instance.
@@ -177,6 +179,8 @@ class Chat(object):
         msg_id = lib.dc_prepare_msg(self._dc_context, self.id, msg._dc_msg)
         if msg_id == 0:
             raise ValueError("message could not be prepared")
+        # invalidate passed in message which is not safe to use anymore
+        msg._dc_msg = msg.id = None
         return Message.from_db(self.account, msg_id)
 
     def prepare_message_file(self, path, mime_type=None, view_type="file"):
@@ -191,7 +195,7 @@ class Chat(object):
         :raises ValueError: if message can not be prepared/chat does not exist.
         :returns: the resulting :class:`Message` instance
         """
-        msg = Message.new(self.account, view_type)
+        msg = Message.new_empty(self.account, view_type)
         msg.set_file(path, mime_type)
         return self.prepare_message(msg)
 
@@ -201,12 +205,19 @@ class Chat(object):
         :param message: a :class:`Message` instance previously returned by
                         :meth:`prepare_file`.
         :raises ValueError: if message can not be sent.
-        :returns: a :class:`deltachat.message.Message` instance with updated state
+        :returns: a :class:`deltachat.message.Message` instance as sent out.
         """
-        msg_id = lib.dc_send_msg(self._dc_context, 0, message._dc_msg)
+        assert message.id != 0 and message.is_out_preparing()
-        if msg_id == 0:
+        # get a fresh copy of dc_msg, the core needs it
+        msg = Message.from_db(self.account, message.id)
+
+        # pass 0 as chat-id because core-docs say it's ok when out-preparing
+        sent_id = lib.dc_send_msg(self._dc_context, 0, msg._dc_msg)
+        if sent_id == 0:
             raise ValueError("message could not be sent")
-        return Message.from_db(self.account, msg_id)
+        assert sent_id == msg.id
+        # modify message in place to avoid bad state for the caller
+        msg._dc_msg = Message.from_db(self.account, sent_id)._dc_msg
 
     def set_draft(self, message):
         """ set message as draft.
@@ -229,7 +240,7 @@ class Chat(object):
         if x == ffi.NULL:
             return None
         dc_msg = ffi.gc(x, lib.dc_msg_unref)
-        return Message.from_dc_msg(self.account, dc_msg)
+        return Message(self.account, dc_msg)
 
     def get_messages(self):
         """ return list of messages in this chat.

python/src/deltachat/message.py

@@ -15,16 +15,14 @@ class Message(object):
     You obtain instances of it through :class:`deltachat.account.Account` or
     :class:`deltachat.chatting.Chat`.
     """
-    def __init__(self, account, id=None, dc_msg=None):
+    def __init__(self, account, dc_msg):
         self.account = account
         self._dc_context = account._dc_context
-        if dc_msg is not None:
-            self._cache_dc_msg = self._dc_msg_volatile = dc_msg
-            id = lib.dc_msg_get_id(dc_msg)
-        assert id is not None
-        self.id = id
         assert isinstance(self._dc_context, ffi.CData)
-        assert int(id) >= 0
+        assert isinstance(dc_msg, ffi.CData)
+        self._dc_msg = dc_msg
+        self.id = lib.dc_msg_get_id(dc_msg)
+        assert self.id is not None and self.id >= 0, repr(self.id)
 
     def __eq__(self, other):
         return self.account == other.account and self.id == other.id
@@ -32,46 +30,22 @@ class Message(object):
     def __repr__(self):
         return "<Message id={} dc_context={}>".format(self.id, self._dc_context)
 
-    @property
-    def _dc_msg(self):
-        if self.id > 0:
-            if not hasattr(self, "_cache_dc_msg"):
-                self._cache_dc_msg = ffi.gc(
-                    lib.dc_get_msg(self._dc_context, self.id),
-                    lib.dc_msg_unref
-                )
-            return self._cache_dc_msg
-        return self._dc_msg_volatile
-
     @classmethod
     def from_db(cls, account, id):
-        assert hasattr(account, "_dc_context")
         assert id > 0
-        return cls(account, id)
+        return cls(account, ffi.gc(
-
+                lib.dc_get_msg(account._dc_context, id),
-    @classmethod
-    def from_dc_msg(cls, account, dc_msg):
-        assert hasattr(account, "_dc_context")
-        return cls(account, dc_msg=dc_msg)
-
-    @classmethod
-    def new(cls, account, view_type):
-        """ create a non-persistent message. """
-        dc_context = account._dc_context
-        msg = cls(account=account, id=0)
-        view_type_code = MessageType.get_typecode(view_type)
-        msg._dc_msg_volatile = ffi.gc(
-            lib.dc_msg_new(dc_context, view_type_code),
                 lib.dc_msg_unref
-        )
+        ))
-        return msg
 
-    def get_state(self):
+    @classmethod
-        """ get the message in/out state.
+    def new_empty(cls, account, view_type):
-
+        """ create a non-persistent message. """
-        :returns: :class:`deltachat.message.MessageState`
+        view_type_code = MessageType.get_typecode(view_type)
-        """
+        return Message(account, ffi.gc(
-        return MessageState(self)
+            lib.dc_msg_new(account._dc_context, view_type_code),
+            lib.dc_msg_unref
+        ))
 
     @props.with_doc
     def text(self):
@@ -81,7 +55,7 @@ class Message(object):
     def set_text(self, text):
         """set text of this message. """
         assert self.id > 0, "message not prepared"
-        assert self.get_state().is_out_preparing()
+        assert self.is_out_preparing()
         lib.dc_msg_set_text(self._dc_msg, as_dc_charpointer(text))
 
     @props.with_doc
@@ -197,76 +171,17 @@ class Message(object):
         contact_id = lib.dc_msg_get_from_id(self._dc_msg)
         return Contact(self._dc_context, contact_id)
 
-
+    #
-class MessageType(object):
+    # Message State query methods
-    """ DeltaChat message type, with is_* methods. """
+    #
-    _mapping = {
-            const.DC_MSG_TEXT: 'text',
-            const.DC_MSG_IMAGE: 'image',
-            const.DC_MSG_GIF: 'gif',
-            const.DC_MSG_AUDIO: 'audio',
-            const.DC_MSG_VIDEO: 'video',
-            const.DC_MSG_FILE: 'file'
-    }
-
-    def __init__(self, _type):
-        self._type = _type
-
-    def __eq__(self, other):
-        return self._type == getattr(other, "_type", None)
-
-    @classmethod
-    def get_typecode(cls, view_type):
-        for code, value in cls._mapping.items():
-            if value == view_type:
-                return code
-        raise ValueError("message typecode not found for {!r}".format(view_type))
-
-    @props.with_doc
-    def name(self):
-        """ human readable type name. """
-        return self._mapping.get(self._type, "")
-
-    def is_text(self):
-        """ return True if it's a text message. """
-        return self._type == const.DC_MSG_TEXT
-
-    def is_image(self):
-        """ return True if it's an image message. """
-        return self._type == const.DC_MSG_IMAGE
-
-    def is_gif(self):
-        """ return True if it's a gif message. """
-        return self._type == const.DC_MSG_GIF
-
-    def is_audio(self):
-        """ return True if it's an audio message. """
-        return self._type == const.DC_MSG_AUDIO
-
-    def is_video(self):
-        """ return True if it's a video message. """
-        return self._type == const.DC_MSG_VIDEO
-
-    def is_file(self):
-        """ return True if it's a file message. """
-        return self._type == const.DC_MSG_FILE
-
-
-class MessageState(object):
-    """ Current Message In/Out state, updated on each call of is_* methods.
-    """
-    def __init__(self, message):
-        self.message = message
-
-    def __eq__(self, other):
-        return self.message == getattr(other, "message", None)
-
     @property
     def _msgstate(self):
-        if self.message.id == 0:
+        if self.id == 0:
-            return lib.dc_msg_get_state(self.message._dc_msg)
+            dc_msg = self.message._dc_msg
+        else:
+            # load message from db to get a fresh/current state
             dc_msg = ffi.gc(
-            lib.dc_get_msg(self.message._dc_context, self.message.id),
+                lib.dc_get_msg(self._dc_context, self.id),
                 lib.dc_msg_unref
             )
         return lib.dc_msg_get_state(dc_msg)
@@ -323,3 +238,57 @@ class MessageState(object):
         state, you'll receive the event DC_EVENT_MSG_READ.
         """
         return self._msgstate == const.DC_STATE_OUT_MDN_RCVD
+
+
+class MessageType(object):
+    """ DeltaChat message type, with is_* methods. """
+    _mapping = {
+            const.DC_MSG_TEXT: 'text',
+            const.DC_MSG_IMAGE: 'image',
+            const.DC_MSG_GIF: 'gif',
+            const.DC_MSG_AUDIO: 'audio',
+            const.DC_MSG_VIDEO: 'video',
+            const.DC_MSG_FILE: 'file'
+    }
+
+    def __init__(self, _type):
+        self._type = _type
+
+    def __eq__(self, other):
+        return self._type == getattr(other, "_type", None)
+
+    @classmethod
+    def get_typecode(cls, view_type):
+        for code, value in cls._mapping.items():
+            if value == view_type:
+                return code
+        raise ValueError("message typecode not found for {!r}".format(view_type))
+
+    @props.with_doc
+    def name(self):
+        """ human readable type name. """
+        return self._mapping.get(self._type, "")
+
+    def is_text(self):
+        """ return True if it's a text message. """
+        return self._type == const.DC_MSG_TEXT
+
+    def is_image(self):
+        """ return True if it's an image message. """
+        return self._type == const.DC_MSG_IMAGE
+
+    def is_gif(self):
+        """ return True if it's a gif message. """
+        return self._type == const.DC_MSG_GIF
+
+    def is_audio(self):
+        """ return True if it's an audio message. """
+        return self._type == const.DC_MSG_AUDIO
+
+    def is_video(self):
+        """ return True if it's a video message. """
+        return self._type == const.DC_MSG_VIDEO
+
+    def is_file(self):
+        """ return True if it's a file message. """
+        return self._type == const.DC_MSG_FILE

python/tests/test_account.py

@@ -147,11 +147,11 @@ class TestOfflineChat:
             chat1.send_text("msg1")
 
     def test_prepare_message_and_send(self, ac1, chat1):
-        msg = chat1.prepare_message(Message.new(chat1.account, "text"))
+        msg = chat1.prepare_message(Message.new_empty(chat1.account, "text"))
         msg.set_text("hello world")
         assert msg.text == "hello world"
         assert msg.id > 0
-        msg = chat1.send_prepared(msg)
+        chat1.send_prepared(msg)
         assert "Sent" in msg.get_message_info()
         str(msg)
         repr(msg)
@@ -165,11 +165,10 @@ class TestOfflineChat:
         message = chat1.prepare_message_file(p)
         assert message.id > 0
         message.set_text("hello world")
-        assert message.get_state().is_out_preparing()
+        assert message.is_out_preparing()
         assert message.text == "hello world"
-        msg = chat1.send_prepared(message)
+        chat1.send_prepared(message)
-        s = msg.get_message_info()
+        assert "Sent" in message.get_message_info()
-        assert "Sent" in s
 
     def test_message_eq_contains(self, chat1):
         msg = chat1.send_text("msg1")
@@ -188,14 +187,13 @@ class TestOfflineChat:
         assert not msg.view_type.is_gif()
         assert not msg.view_type.is_file()
         assert not msg.view_type.is_image()
-        msg_state = msg.get_state()
+        assert not msg.is_in_fresh()
-        assert not msg_state.is_in_fresh()
+        assert not msg.is_in_noticed()
-        assert not msg_state.is_in_noticed()
+        assert not msg.is_in_seen()
-        assert not msg_state.is_in_seen()
+        assert msg.is_out_pending()
-        assert msg_state.is_out_pending()
+        assert not msg.is_out_failed()
-        assert not msg_state.is_out_failed()
+        assert not msg.is_out_delivered()
-        assert not msg_state.is_out_delivered()
+        assert not msg.is_out_mdn_received()
-        assert not msg_state.is_out_mdn_received()
 
     def test_create_chat_by_message_id(self, ac1, chat1):
         msg = chat1.send_text("msg1")
@@ -299,7 +297,7 @@ class TestOfflineChat:
             ac1.initiate_key_transfer()
 
     def test_set_get_draft(self, chat1):
-        msg = Message.new(chat1.account, "text")
+        msg = Message.new_empty(chat1.account, "text")
         msg1 = chat1.prepare_message(msg)
         msg1.set_text("hello")
         chat1.set_draft(msg1)
@@ -397,7 +395,7 @@ class TestOnlineAccount:
         evt_name, data1, data2 = ev
         assert data1 == chat.id
         assert data2 == msg_out.id
-        assert msg_out.get_state().is_out_delivered()
+        assert msg_out.is_out_delivered()
 
         lp.sec("wait for ac2 to receive message")
         ev = ac2._evlogger.get_matching("DC_EVENT_MSGS_CHANGED")
@@ -426,7 +424,7 @@ class TestOnlineAccount:
         lp.step("1")
         ac1._evlogger.get_matching("DC_EVENT_MSG_READ")
         lp.step("2")
-        assert msg_out.get_state().is_out_mdn_received()
+        assert msg_out.is_out_mdn_received()
 
     def test_saved_mime_on_received_message(self, acfactory, lp):
         lp.sec("starting accounts, waiting for configuration")
@@ -466,7 +464,7 @@ class TestOnlineAccount:
         evt_name, data1, data2 = ev
         assert data1 == chat.id
         assert data2 == msg_out.id
-        assert msg_out.get_state().is_out_delivered()
+        assert msg_out.is_out_delivered()
 
         lp.sec("wait for ac2 to receive message")
         ev = ac2._evlogger.get_matching("DC_EVENT_MSGS_CHANGED")

python/tests/test_increation.py

@@ -19,7 +19,7 @@ class TestInCreation:
         lp.sec("create a message with a file in creation")
         path = data.get_path("d.png")
         prepared_original = chat.prepare_message_file(path)
-        assert prepared_original.get_state().is_out_preparing()
+        assert prepared_original.is_out_preparing()
         wait_msgs_changed(ac1, chat.id, prepared_original.id)
 
         lp.sec("forward the message while still in creation")
@@ -34,24 +34,23 @@ class TestInCreation:
             forwarded_id = wait_msgs_changed(ac1, chat2.id)
             assert forwarded_id
         forwarded_msg = ac1.get_message_by_id(forwarded_id)
-        assert forwarded_msg.get_state().is_out_preparing()
+        assert forwarded_msg.is_out_preparing()
 
         lp.sec("finish creating the file and send it")
-        sent_original = chat.send_prepared(prepared_original)
+        assert prepared_original.is_out_preparing()
-        assert sent_original.id == prepared_original.id
+        chat.send_prepared(prepared_original)
-        state = sent_original.get_state()
+        assert prepared_original.is_out_pending() or prepared_original.is_out_delivered()
-        assert state.is_out_pending() or state.is_out_delivered()
+        wait_msgs_changed(ac1, chat.id, prepared_original.id)
-        wait_msgs_changed(ac1, chat.id, sent_original.id)
 
         lp.sec("expect the forwarded message to be sent now too")
         wait_msgs_changed(ac1, chat2.id, forwarded_id)
-        state = ac1.get_message_by_id(forwarded_id).get_state()
+        fwd_msg = ac1.get_message_by_id(forwarded_id)
-        assert state.is_out_pending() or state.is_out_delivered()
+        assert fwd_msg.is_out_pending() or fwd_msg.is_out_delivered()
 
         lp.sec("wait for the messages to be delivered to SMTP")
         ev = ac1._evlogger.get_matching("DC_EVENT_MSG_DELIVERED")
         assert ev[1] == chat.id
-        assert ev[2] == sent_original.id
+        assert ev[2] == prepared_original.id
         ev = ac1._evlogger.get_matching("DC_EVENT_MSG_DELIVERED")
         assert ev[1] == chat2.id
         assert ev[2] == forwarded_id