GNOME Bugzilla – Bug 654159
Display a notification when IM accounts fail to connect
Last modified: 2011-08-29 16:05:53 UTC
As part of bug #653938 the Shell should generated a notification when an IM account fail to connect or get disconnected. I think it should be similar to Empathy's infobar displayed in the contact list, including the "reconnect" and "edit" buttons.
Created attachment 191738 [details] javascript example using TpAccountManager Empathy's code is in Empathy/src/empathy-main-window.c. I tried to reuse the same logic in javascript with TpAccountManager but I hit a problem with the attached code: TpAccount's "status-changed" signal has a "GHashTable details" parameter. JS ERROR: !!! Exception was: Error: Unable to introspect element-type of container in GValue The error comes from gjs/gi/value.c::gjs_value_from_g_value_internal(): it explicitly rejects G_TYPE_HASH_TABLE. Any idea?
My guess would be that support for the various array types hasn't been implemented yet, and hash tables are problematic as they don't map to a corresponding type in javascript. The canonical solution of adding help methods in shell-foo obviously doesn't apply for signal handlers, so I think the best we can do for now is to display a generic error message, possibly with an action to invoke the online-accounts panel.
The hash table here is annotated as being utf8->GValue (http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-account.html#TpAccount-status-changed), so it could be a JS object, and I'm pretty sure we support that. But (a) I'm not sure we look at annotations on signals, and (b) I'm not sure we handle GValue.
GValue itself is supported. The problem is we know the GValue contains an array (I'm guessing a GPtrArray), but we don't know the element-type of the array itself (aka the container in the GValue). My eventual solution for Telepathy is to only expose GVariants in the introspection API, but I was waiting on GVariant support in gjs. Since dbus-glib actually knows the full variant type, you can convert a GValue from dbus-glib into a GVariant with dbus_g_value_build_g_variant() -- and go the other way around with dbus_g_value_parse_g_variant().
(In reply to comment #4) > My eventual solution for Telepathy is to only expose GVariants in the > introspection API, but I was waiting on GVariant support in gjs. Since > dbus-glib actually knows the full variant type, you can convert a GValue from > dbus-glib into a GVariant with dbus_g_value_build_g_variant() -- and go the > other way around with dbus_g_value_parse_g_variant(). Is it a change in telepathy-glib (would it break the API?), gjs, or in my program? Is there an open bug about it?
My vague plan was the (skip) the GValue APIs and add GVariant APIs with renames to make them appear with useful names. So it would be new API in tp-glib + an API break in the TelepathyGLib g-i package (for which we currently make no API guarantees). Here are some bugs: https://bugs.freedesktop.org/show_bug.cgi?id=30422 https://bugs.freedesktop.org/show_bug.cgi?id=30423 https://bugs.freedesktop.org/show_bug.cgi?id=30427
Created attachment 191966 [details] [review] telepathyClient: send notifications for connection failures Generates a notification when an IM account fail to connect or get disconnected. It behaves like Empathy's infobar displayed in the contact list and it has "reconnect", "edit" and "close" buttons.
Review of attachment 191966 [details] [review]: ::: js/ui/telepathyClient.js @@ +127,3 @@ + + let connectionError = account.connection_error; + if (connectionError && connectionError Drop the "connectionError &&". @@ +938,3 @@ +// create_errors_to_message_hash() +function errors_to_message(dbus) { + var hash = new Array(); Don't use arrays for this, even though it "works". I'd create this variable in global scope too, otherwise we're recreating the object every time we call the function. @@ +941,3 @@ + + hash[Tp.error_get_dbus_name(Tp.Error.NETWORK_ERROR)] + = _("Network error"); These seem like a lot of strings. Can we share the translation effort with Empathy by gettext trickery? Should we put these in their own gettext context? @@ +1020,3 @@ + switch (action) { + case 'reconnect': + account.reconnect_async(null, null); What happens when it fails again? Do we pop up another notification? You should talk to designers about this... @@ +1023,3 @@ + break; + case 'edit': + let cmd = "/usr/bin/empathy-accounts" Use single quotes for non-translatable strings... And do you not ship a .desktop file for empathy-accounts (or gnome-online-accounts if you want to use that)?
Review of attachment 191966 [details] [review]: We shouldn't stack errors of the same IM account. If the account is disabled the error should go away (as in Empathy). ::: js/ui/telepathyClient.js @@ +925,3 @@ + + createNotificationIcon: function() { + Not sure about this icon? network-server maybe? @@ +1009,3 @@ + null, + { customContent: true }); + = _("Certificate self-signed"); Do we really want this? @@ +1023,3 @@ + break; + case 'edit': + hash[Tp.error_get_dbus_name(Tp.Error.CONNECTION_REFUSED)] We do, so yeah you should use that.
Created attachment 192044 [details] [review] telepathyClient: send notifications for connection failures New patch with the fixes: (In reply to comment #8) > Review of attachment 191966 [details] [review]: > > ::: js/ui/telepathyClient.js > @@ +127,3 @@ > + > + let connectionError = account.connection_error; > + if (connectionError && connectionError > > Drop the "connectionError &&". Done > @@ +938,3 @@ > +// create_errors_to_message_hash() > +function errors_to_message(dbus) { > + var hash = new Array(); > > Don't use arrays for this, even though it "works". > > I'd create this variable in global scope too, otherwise we're recreating the > object every time we call the function. Done > @@ +941,3 @@ > + > + hash[Tp.error_get_dbus_name(Tp.Error.NETWORK_ERROR)] > + = _("Network error"); > > These seem like a lot of strings. Can we share the translation effort with > Empathy by gettext trickery? Should we put these in their own gettext context? TODO > @@ +1020,3 @@ > + switch (action) { > + case 'reconnect': > + account.reconnect_async(null, null); > > What happens when it fails again? Do we pop up another notification? You should > talk to designers about this... Yes, a new notification appears with the new error. > @@ +1023,3 @@ > + break; > + case 'edit': > + let cmd = "/usr/bin/empathy-accounts" > > Use single quotes for non-translatable strings... Done. > And do you not ship a .desktop file for empathy-accounts (or > gnome-online-accounts if you want to use that)? Yes, there is a desktop file. The problem is to give the parameter. I tried this: let app_info = Gio.DesktopAppInfo.new('empathy-accounts.desktop'); app_info.launch_uris(['--select-account=%s'.format(account.get_path_suffix())], null); But it seems launch_uris() drops the parameter as it is not a proper URI. So empathy-accounts starts but not on the right account.
Review of attachment 192044 [details] [review]: ::: js/ui/telepathyClient.js @@ +79,3 @@ + this._accountSource = new AccountSource(); + Main.messageTray.add(this._accountSource); Won't this mean there will be a lingering summary item around "in case" accounts don't connect? This should only really be added to the message tray on first notify if it's not already added. It should also go away if there's no notifications left, just like a regular notification source. @@ +116,3 @@ + } + + var accountList = this._tpAccountManager.get_valid_accounts(); 'let'. @@ +117,3 @@ + + var accountList = this._tpAccountManager.get_valid_accounts(); + for each (var account in accountList) { 'for each' statement is poor because it unexpectedly iterates out of order. Try: accountList.forEach(Lang.bind(this, function(account) { })); or a regular for loop with an index var. If do that, use 'let', not 'var'. @@ +737,3 @@ timestamp = currentTime; + Nope. @@ +1087,3 @@ + + "cryptography library"); + +function errors_to_message(dbus) { "dbus" is a bad parameter name. @@ +1108,3 @@ + this.setResident(true); + + this.addBody(errors_to_message(connectionError)); errors_to_message isn't used outside of here, so I'd just put it inline. @@ +1118,3 @@ + case 'reconnect': + // If it fails again, a new notification will pop up with the + // new error. Is this what the designers want?
(In reply to comment #11) > @@ +1087,3 @@ > + + "cryptography library"); > + > +function errors_to_message(dbus) { > > "dbus" is a bad parameter name. Also our coding style is to use camelCase in javascript (errorsToMessage). > @@ +1118,3 @@ > + case 'reconnect': > + // If it fails again, a new notification will pop up with the > + // new error. > > Is this what the designers want? I don't think any designer has actively be involved here, I'd say getting an OK for the UI additions here is a requirement before landing the patch.
*** Bug 652719 has been marked as a duplicate of this bug. ***
This branch does not listen to new accounts being created. Also in bug #653941 I added a TpAccountManager for contact subscription stuff, so we should share a bit of code. Setting this bug as depend on the other. I'm updating the code based on Alban's work.
Created attachment 194813 [details] [review] telepathyClient: Add notification for account connection errors Based on initial work from Alban Crequy
Created attachment 194948 [details] [review] telepathyClient: Add notification for account connection errors Based on initial work from Alban Crequy and Xavier Claessens Rebased and fixed up style. Tested and seems to work fine.
Why did you drop signal disconnect in AccountNotification? If there is a good reason then surely it should be done for the patch on bug #653941 too, no?
(In reply to comment #17) > Why did you drop signal disconnect in AccountNotification? GObjects automatically disconnect all signal handlers when destroyed, and signal handlers only keep a weakref, no? > If there is a good > reason then surely it should be done for the patch on bug #653941 too, no?
But account object is not destroyed when the AccountNotification is. at least not on C level, dunno if gjs changes anything to this.
Created attachment 194982 [details] [review] telepathyClient: Add notification for account connection errors Based on initial work from Alban Crequy and Xavier Claessens After a bit more investivation, I was incorrect. Added the destroy handler back.
Xavier said on irc: 'it's fine for me'
Attachment 194982 [details] pushed as bd9455e - telepathyClient: Add notification for account connection errors