GNOME Bugzilla – Bug 627726
Should use Messages interface
Last modified: 2011-04-20 07:50:52 UTC
Once the Messages interface is mandatory [1] we should make use of it. [1] https://bugs.freedesktop.org/show_bug.cgi?id=29376
I started working on this. I think the best approach is to rebase EmpathyTpChat as a subclass of TpTextChannel, but this is blocked by https://bugs.freedesktop.org/show_bug.cgi?id=31583
Created attachment 186198 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/text-channel-no-inherit-627726 libempathy-gtk/empathy-chat.c | 30 ++- libempathy/empathy-chatroom-manager.c | 3 + libempathy/empathy-message.c | 196 +++++++------------ libempathy/empathy-message.h | 19 +-- libempathy/empathy-tp-chat.c | 354 +++++++++++++++------------------ libempathy/empathy-tp-chat.h | 2 +- src/empathy-chat-manager.c | 7 + src/empathy-event-manager.c | 2 +- 8 files changed, 264 insertions(+), 349 deletions(-)
This is a new approach now depending on any tp-glib change. This is the very first step, I'm sure there are more refactoring we could do. I tried to make commits as atomic as possible so it's probably best to review patch by patch.
Created attachment 186201 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/text-channel-no-inherit-627726 libempathy-gtk/empathy-chat.c | 30 ++- libempathy/empathy-chatroom-manager.c | 3 + libempathy/empathy-message.c | 196 +++++++------------ libempathy/empathy-message.h | 19 +-- libempathy/empathy-tp-chat.c | 353 +++++++++++++++------------------ libempathy/empathy-tp-chat.h | 2 +- src/empathy-chat-manager.c | 7 + src/empathy-event-manager.c | 2 +- 8 files changed, 263 insertions(+), 349 deletions(-)
Review of attachment 186201 [details] [review]: At least one timestamp bug that I noticed. You should check varargs usage thoroughly for more, so we don't repeat the mistakes made in tp-logger. I strongly recommend porting all timestamps to gint64. ~ It would be nice for the SEND_ERROR signal to include the dbus-error, but this can be done in a later branch. It would also be nice to include a SUCCESS signal for things like SMSes, but again this can be done later. ::: libempathy/empathy-message.c @@ +259,3 @@ + priv->timestamp = g_value_get_long (value); + if (priv->timestamp <= 0) + priv->timestamp = empathy_time_get_current (); For the same reason as in tp-logger, I strongly advice the eradication of time_t. You can get the updated utilities from tp-logger (they originally came from Empathy). @@ +633,3 @@ + "body", body, + "type", tp_message_get_message_type (tp_msg), + "timestamp", tp_message_get_received_timestamp (tp_msg), tp_message_get_received_timestamp() returns gint64 but "timestamp" is a long. This is a sizing mismatch on 32-bit. You either must explicitly cast to long (remember varargs don't do implicit casts), or better, make timestamp a gint64. Either way, you'll need to check all varargs usage. @@ +644,3 @@ + "pending-message-id", NULL); + + priv = GET_PRIV (message); Assign priv higher up, people tend to assume it will be valid.
(In reply to comment #5) > Review of attachment 186201 [details] [review]: > > At least one timestamp bug that I noticed. You should check varargs usage > thoroughly for more, so we don't repeat the mistakes made in tp-logger. I > strongly recommend porting all timestamps to gint64. Yeah I think you're right. I started porting to gint64 but that's a pretty big change, so I'll do it in a new branch based on this one. > It would be nice for the SEND_ERROR signal to include the dbus-error, but this > can be done in a later branch. It would also be nice to include a SUCCESS > signal for things like SMSes, but again this can be done later. Yeah, my main focus here was to do a feature equivalent port. > ::: libempathy/empathy-message.c > @@ +259,3 @@ > + priv->timestamp = g_value_get_long (value); > + if (priv->timestamp <= 0) > + priv->timestamp = empathy_time_get_current (); > > For the same reason as in tp-logger, I strongly advice the eradication of > time_t. You can get the updated utilities from tp-logger (they originally came > from Empathy). > @@ +633,3 @@ > + "body", body, > + "type", tp_message_get_message_type (tp_msg), > + "timestamp", tp_message_get_received_timestamp (tp_msg), > > tp_message_get_received_timestamp() returns gint64 but "timestamp" is a long. > This is a sizing mismatch on 32-bit. > > You either must explicitly cast to long (remember varargs don't do implicit > casts), or better, make timestamp a gint64. > > Either way, you'll need to check all varargs usage. I just casted it for now as said above. > @@ +644,3 @@ > + "pending-message-id", NULL); > + > + priv = GET_PRIV (message); > > Assign priv higher up, people tend to assume it will be valid. I changed to assign it as soon as the message is created.
Created attachment 186260 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/text-channel-no-inherit-627726 libempathy-gtk/empathy-chat.c | 30 ++- libempathy/empathy-chatroom-manager.c | 3 + libempathy/empathy-message.c | 197 +++++++------------ libempathy/empathy-message.h | 19 +-- libempathy/empathy-tp-chat.c | 353 +++++++++++++++------------------ libempathy/empathy-tp-chat.h | 2 +- src/empathy-chat-manager.c | 7 + src/empathy-event-manager.c | 2 +- 8 files changed, 264 insertions(+), 349 deletions(-)
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 186201 [details] [review] [details]: > > > > At least one timestamp bug that I noticed. You should check varargs usage > > thoroughly for more, so we don't repeat the mistakes made in tp-logger. I > > strongly recommend porting all timestamps to gint64. > > Yeah I think you're right. I started porting to gint64 but that's a pretty big > change, so I'll do it in a new branch based on this one. I opened bug #648188 for this.
Review of attachment 186260 [details] [review]: ::: libempathy/empathy-message.c @@ +325,3 @@ + "body", body, + "is-backlog", TRUE, + "timestamp", (glong) tpl_event_get_timestamp (logevent), I think you also need to CLAMP to prevent overflow? I don't know what the compiler does for you when casting down... "timestamp", (glong) CLAMP (..., G_MINLONG, G_MAXLONG)
I just merged my branch fixing bug #648188 which was a superset of this branch and was fixing this issue so we're good. :) This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.