GNOME Bugzilla – Bug 643377
telepathyClient: Integrate with TelepathyLogger
Last modified: 2011-03-14 22:54:02 UTC
Created attachment 182007 [details] [review] Patch 1/3. Requires patches from https://bugs.freedesktop.org/show_bug.cgi?id=34770 and https://bugzilla.gnome.org/show_bug.cgi?id=642793.
Created attachment 182008 [details] [review] Patch 2/3.
Created attachment 182009 [details] [review] Patch 3/3.
"Requires 642793" is misphrased. None of the patches require changes from it, I was just lazy and didn't bother rebasing any of the patches, and I change stuff I introduced there.
Review of attachment 182008 [details] [review]: shell-global.c should contain only methods of the ShellGlobal object. Static functions are better off in shell-util.c ::: src/shell-global.c @@ +2111,3 @@ + * @callback: (scope async): User callback to run when the contact is ready + * + * Wrap tpl_log_manager_get_filtered_events_async because gdb cannot support Maybe you meant: gjs @@ +2118,3 @@ + TpContact *contact, + guint num_events, + ShellGetContactEventsCb callback) 1) A callback cannot be invoked from bindings without an user_data parameter (it needs libffi data structures), independent of scope. 2) Since it's just a wrapper for gjs, can you preserve the GAsyncReadyCallback signature, and invoke manager.get_filtered_events_finish from JS?
Review of attachment 182007 [details] [review]: Looks good, but I think it should be squashed into later patches.
Created attachment 182142 [details] [review] Require a recent TelepathyLogger This is a phony commit for now, as telepathy-logger needs the patches from https://bugs.freedesktop.org/show_bug.cgi?id=34770. Additionally, update gnome-shell.modules to use the new git.freedesktop.org repository URLs instead of the old ones at git.collabora.co.uk.
Created attachment 182143 [details] [review] shell-global: Add wrappers for TelepathyLogger gjs can't support more than one callback in the same function, so work around this with yet another shell-global wrapper.
Created attachment 182144 [details] [review] telepathyClient: Add support for TelepathyLogger Add a number of lines of chat history from TelepathyLogger when a source is created.
Comment on attachment 182142 [details] [review] Require a recent TelepathyLogger >This is a phony commit for now, as telepathy-logger needs the patches >from https://bugs.freedesktop.org/show_bug.cgi?id=34770. The upstream bug is closed. Does that mean this is good to go? If so, feel free to commit it with an updated commit message.
Comment on attachment 182143 [details] [review] shell-global: Add wrappers for TelepathyLogger >+void >+shell_get_contact_events (TplLogManager *log_manager, >+ TpAccount *account, >+ TpContact *contact, >+ guint num_events, >+ GAsyncReadyCallback callback) as Giovanni already said, you need a user_data param (He also said that utilities should go in shell-util rather than shell-global, which is true, but this should stay in shell-global for consistency with the other tp wrappers, and we should mass-move the non-ShellGlobal parts of shell-global over to shell-util at a later date.) >+ TplEntity *entity = tpl_entity_new_from_tp_contact (contact, TPL_ENTITY_CONTACT); It's preferable to make the wrappers as thin as possible; could this part be done in the JS instead?
Comment on attachment 182144 [details] [review] telepathyClient: Add support for TelepathyLogger >+ appendMessage: function(message, direction, timestampWanted) { Couldn't you just omit the timestamp field in the log-replayed messages, and key off that? >+ this._append(messageBody, styles, message.timestamp); > }, > >- _append: function(text, styles, timestamp) { >+ _append: function(text, styles, timestamp, timestampWanted) { you're not passing timestampWanted from appendMessage to _append.
Created attachment 182399 [details] [review] Require a recent TelepathyLogger We require a TelepathyLogger version with gobject-introspection support. Additionally, update gnome-shell.modules to use the new git.freedesktop.org repository URLs instead of the old ones at git.collabora.co.uk.
Created attachment 182400 [details] [review] Require a recent TelepathyLogger We require a TelepathyLogger version with gobject-introspection support. Additionally, update gnome-shell.modules to use the new git.freedesktop.org repository URLs instead of the old ones at git.collabora.co.uk. excuse the bugspam, trying to wrangle git-bz bumped required version to 0.2.4, rewrote commit message, rebased
Created attachment 182401 [details] [review] shell-global: Add wrappers for TelepathyLogger gjs can't support more than one callback in the same function, so work around this with yet another shell-global wrapper. rebased, new gobject-introspection from gcampax, added user_data, made wrapper more paper thin
Created attachment 182402 [details] [review] telepathyClient: Add support for TelepathyLogger Add a number of lines of chat history from TelepathyLogger when a source is created.
Created attachment 182740 [details] [review] Require a recent TelepathyLogger We require a TelepathyLogger version with gobject-introspection support. Additionally, update gnome-shell.modules to use the new git.freedesktop.org repository URLs instead of the old ones at git.collabora.co.uk.
Created attachment 182741 [details] [review] shell-global: Add wrappers for TelepathyLogger gjs can't support more than one callback in the same function, so work around this with yet another shell-global wrapper.
Created attachment 182742 [details] [review] telepathyClient: Add support for TelepathyLogger Add a number of lines of chat history from TelepathyLogger when a source is created. There seems to be a bug where the first message from somebody won't show up, so the notification will show the last history item. I'm looking into it.
Created attachment 182779 [details] [review] telepathyClient: Add messages from TelepathyLogger This allows users to see chat history from other contacts with the inline message tray replies.
Created attachment 182789 [details] [review] telepathyClient: Add messages from TelepathyLogger This allows users to see chat history from other contacts with the inline message tray replies. forgot to stage a change
Comment on attachment 182740 [details] [review] Require a recent TelepathyLogger this is ok to commit, but you'll need to update the commit message, since this part has already been committed: >Additionally, update gnome-shell.modules to use the new git.freedesktop.org >repository URLs instead of the old ones at git.collabora.co.uk.
Comment on attachment 182741 [details] [review] shell-global: Add wrappers for TelepathyLogger >+ callback, NULL); If you keep the user_data param, you need to actually pass it there, so it gets passed to the callback. However, it turns out that gjs doesn't actually need a user_data anyway if scope is async. So, either drop the user_data again, or change the NULL to user_data here. Either way, ok to commit.
Comment on attachment 182789 [details] [review] telepathyClient: Add messages from TelepathyLogger >+ * @noTimestamp: Whether to add a timestamp "if %true, no timestamp will be added, regardless of the difference since the last timestamp" good to commit with that change
Attachment 182740 [details] pushed as 12df10b - Require a recent TelepathyLogger Attachment 182741 [details] pushed as 525da01 - shell-global: Add wrappers for TelepathyLogger Attachment 182789 [details] pushed as 5a269db - telepathyClient: Add messages from TelepathyLogger