GNOME Bugzilla – Bug 645760
Extension for the Gajim IM client
Last modified: 2011-04-01 16:40:58 UTC
If Gajim is running the chats could be managed with the Shell infrastructure, like Empathy does. I have a branch of gnome-shell-extensions with that new gajim extension: https://github.com/philn/gnome-shell-extensions Will also attach the patch here. Please review it here or on github :)
Created attachment 184304 [details] [review] new gajim extension. Requires gajim-nightly >= 20110326
Review of attachment 184304 [details] [review]: Looks mostly fine, just minor style nits. ::: extensions/gajim/extension.js @@ +21,3 @@ + "ghijklmnopqrstuv" + + "wxyz0123456789+/" + + "="; In JS, string concatenation is not done at compile time. Better to write it fully. @@ +25,3 @@ +function decode64(input) { + let output = ""; + let chr1, chr2, chr3 = ""; Why only chr3 is initalized to = ""? @@ +114,3 @@ + + _gotContactInfos: function(result, excp) { + global.log("contact infos received"); Please remove debug logs (both log and global.log) @@ +115,3 @@ + _gotContactInfos: function(result, excp) { + global.log("contact infos received"); + this.title = result['FN']; Setting title after the Source is added has no effect, currently. @@ +155,3 @@ + }, + + _notificationClicked: function(notification) { You should not override _notificationClicked. Instead you should override open(), and clicking on the notification should open gajim. @@ +188,3 @@ + + MessageTray.Source.prototype.notify.call(this, this._notification); + }, Can you kill this function? You're changing signature for no obvious reason. (Yes, TelepathyClient has this, but it is cleaner to use this.notify(this._notification) everywhere instead, as notify() can be called from context that expect a generic Source) @@ +252,3 @@ +let Gajim = DBus.makeProxyClass(GajimIface); + +function gajimClient() { Use UpperCamelCase for classes.
(In reply to comment #2) > Review of attachment 184304 [details] [review]: > > Looks mostly fine, just minor style nits. > Thanks for the quick review! > ::: extensions/gajim/extension.js > @@ +21,3 @@ > + "ghijklmnopqrstuv" + > + "wxyz0123456789+/" + > + "="; > > In JS, string concatenation is not done at compile time. Better to write it > fully. > OK. > @@ +25,3 @@ > +function decode64(input) { > + let output = ""; > + let chr1, chr2, chr3 = ""; > > Why only chr3 is initalized to = ""? > Oh well, I think it's fine to leave them uninitialized as they will be assigned anyway in the following loop. > @@ +114,3 @@ > + > + _gotContactInfos: function(result, excp) { > + global.log("contact infos received"); > > Please remove debug logs (both log and global.log) > OK. > @@ +115,3 @@ > + _gotContactInfos: function(result, excp) { > + global.log("contact infos received"); > + this.title = result['FN']; > > Setting title after the Source is added has no effect, currently. > The source is added later on below. So I think setting the title here is fine. No? > @@ +155,3 @@ > + }, > + > + _notificationClicked: function(notification) { > > You should not override _notificationClicked. Instead you should override > open(), and clicking on the notification should open gajim. > Hum what do you mean by "open gajim"? This extension works only if it's already running anyway. Otherwise the gajim name won't appear on the session bus. Or am I missing something? Implementing open() would be nice indeed. > @@ +188,3 @@ > + > + MessageTray.Source.prototype.notify.call(this, this._notification); > + }, > > Can you kill this function? You're changing signature for no obvious reason. > (Yes, TelepathyClient has this, but it is cleaner to use > this.notify(this._notification) everywhere instead, as notify() can be called > from context that expect a generic Source) > OK. > @@ +252,3 @@ > +let Gajim = DBus.makeProxyClass(GajimIface); > + > +function gajimClient() { > > Use UpperCamelCase for classes. OK. Will attach an updated patch when the open() question and other remaining doubts are cleared out :)
(In reply to comment #3) > > @@ +115,3 @@ > > + _gotContactInfos: function(result, excp) { > > + global.log("contact infos received"); > > + this.title = result['FN']; > > > > Setting title after the Source is added has no effect, currently. > > > > The source is added later on below. So I think setting the title here is fine. > No? Sorry, I oversaw it as a signal handler. It's ok this way. > > > @@ +155,3 @@ > > + }, > > + > > + _notificationClicked: function(notification) { > > > > You should not override _notificationClicked. Instead you should override > > open(), and clicking on the notification should open gajim. > > > > Hum what do you mean by "open gajim"? This extension works only if it's already > running anyway. Otherwise the gajim name won't appear on the session bus. Or am > I missing something? > > Implementing open() would be nice indeed. I mean "app.activate()", where app is a ShellApp representing gajim, which will change workspace/window to gajim. Even better if you get a MetaWindow representing the actual chat window, but I don't know if that is feasible.
Created attachment 184513 [details] [review] new gajim extension. Requires gajim-nightly >= 20110326
Review of attachment 184513 [details] [review]: ::: extensions/gajim/extension.js @@ +171,3 @@ + // display main Gajim window. + if (gajimWindow) + Main.activateWindow(gajimWindow); This is a no-no. I'm fine with using HACKS to find the chat window, if you have no better methods (but consider that title is locale dependent, can't you use the instance part of wm_class or the role?). But the fallback should be using ShellApp. If gajim is not tracked (as I guess from a window class of Gajim.py), then fix it using GLib.set_prgname at startup. There is no reason to reimplement window matching here.
(In reply to comment #6) > Review of attachment 184513 [details] [review]: > > ::: extensions/gajim/extension.js > @@ +171,3 @@ > + // display main Gajim window. > + if (gajimWindow) > + Main.activateWindow(gajimWindow); > > This is a no-no. I'm fine with using HACKS to find the chat window, if you have > no better methods (but consider that title is locale dependent, can't you use > the instance part of wm_class or the role?). Alright, thanks for the advice! Here's what I have now: if (metaWindow.get_wm_class_instance() == "gajim" && metaWindow.get_role() == "messages") { Main.activateWindow(metaWindow); > But the fallback should be using > ShellApp. If gajim is not tracked (as I guess from a window class of Gajim.py), > then fix it using GLib.set_prgname at startup. There is no reason to > reimplement window matching here. I tried that previously yes. Gajim calls gobject.set_prgname("gajim") at startup already. But when I use the AppSystem to get gajim.desktop and activate it's like starting a second instance of the program and I don't want to do that. Here's the code I use: let app = Shell.AppSystem.get_default().get_app('gajim.desktop'); app.activate(-1);
Created attachment 184827 [details] [review] new gajim extension. Requires gajim-nightly >= 20110326
Review of attachment 184827 [details] [review]: Assuming it is tested (and considering that is not enabled by default, so no risk for testers), looks good to commit. Remember to replace custom base64 implementation with GLib.base64_decode when GJS supports (out) arrays.
Thanks Giovanni! Would you or another committer mind land this? I should request an account to ease future maintenance of the extension.
(In reply to comment #10) > Thanks Giovanni! Would you or another committer mind land this? > I should request an account to ease future maintenance of the extension. For now it is ok if you just ask me. If in the future more maintenance is needed, I'll be happy to vouch for you for an account.