954183 - System Tray Icon should persist even when Buddy List is open

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, enhancement)

Description

[Instantbot]

*** Original post on bio 749 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-04-10 09:02:00 UTC ***

Currently the icon disappears if the buddy list shows and reappears if it's hidden.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 749 at 2011-04-10 13:01:57 UTC ***

Just wanted to mention that this is /not/ wanted by all people. Some people expect the tray icon only after it's been minimized. I'm not sure how easy this would be with the current code (if it's possible).

Comment 2

[Instantbot]

*** Original post on bio 749 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-04-10 14:33:11 UTC ***

(In reply to comment #1)
This certainly falls under the jurisdiction of a bug given that the original patch provides parity with all major IM software. This bug was filed to fix the usability inconsistency. I'm not against creating an option to have it disappear, but that would be a followup enhancement to this bug.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 749 at 2011-04-15 14:50:20 UTC ***

Just adding "system" to the title since I can never re-find this bug. :)

Comment 4

[Instantbot]

*** Original post on bio 749 by deOmega <jahkae AT gmail.com> at 2011-04-15 19:23:56 UTC ***

I would like to reiterate the need for this feature.  It seems to make sense, considering what is standard.

Comment 5

[Instantbot]

*** Original post on bio 749 by Kissaki <kissaki0 AT googlemail.com> at 2011-07-02 01:30:41 UTC ***

I'd also highly recommend/request this.
At least as an optional setting - whatever the default is then.

Comment 6

[Instantbot]

Attachment: patch v1, always show tray icon (user configurable)

*** Original post on bio 749 as attmnt 741 by maierman AT web.de at 2011-07-12 10:46:00 UTC ***

MinTrayR already includes support for creating "persistent" icons, although the extension does not currently expose this feature.

The attached patch adds the required integration code (js only) and a corresponding <preference> to let the user configure the behavior.

Comment 7

[Instantbot]

Comment on attachment 8352483 [details] [diff] [review]
patch v1, always show tray icon (user configurable)

*** Original change on bio 749 attmnt 741 by maierman AT web.de at 2011-07-12 10:47:06 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 749 at 2011-07-12 17:39:14 UTC ***

Comment on attachment 8352483 [details] [diff] [review] (bio-attmnt 741)
patch v1, always show tray icon (user configurable)

Drive by review...

>diff --git a/instantbird/components/mintrayr/content/mintrayr.js b/instantbird/components/mintrayr/content/mintrayr.js

>+    this._prefs = Services.prefs.getBranch("extensions.mintrayr.");
>+    this._prefs instanceof Ci.nsIPrefBranch2;
Written this way, this line looks like it has no effect. Maybe calling QueryInterface would make the meaning more explicit?


>+  reinitWindow: function() {
>+    if (this._prefs.getBoolPref("alwaysShowTrayIcon")) {
>+      if (!this._icon) {
>+        this._icon = Services.traysvc.createIcon(window);

>+        if (this._icon.restoreOnClose) {
>+          this._icon.close();
>+          this._icon = Services.traysvc.createIcon(window);
>+        }
I don't see the point of these 4 lines. Are they a copy/paste error?



>+  toggle: function MinTrayR_toggle() {
>+    if (this._icon) {
>+      if (this._icon.isMinimized) {
>+        this._icon.restore();
>+      }
>+      else {
>+        this._icon.minimize();
>+      }
>+      return;
>+    }
>+
>+    // !alwaysShow
>+    this.restore();
>   }

This is just a tiny nit, but I think this code would be more readable like this:

  if (!this._icon) {
    // when the tray icon isn't always visible
    this.restore();
    return;
  }

  if (this._icon.isMinimized)
    this._icon.restore();
  else
    this._icon.minimize();



>diff --git a/instantbird/modules/imServices.jsm b/instantbird/modules/imServices.jsm
>--- a/instantbird/modules/imServices.jsm
>+++ b/instantbird/modules/imServices.jsm
>@@ -58,3 +58,8 @@
> XPCOMUtils.defineLazyServiceGetter(Services, "logs",
>                                    "@instantbird.org/logger;1",
>                                    "ibILogger");
>+if ("@tn123.ath.cx/trayservice;1" in Components.classes) {
>+  XPCOMUtils.defineLazyServiceGetter(Services, "traysvc",
>+                                     "@tn123.ath.cx/trayservice;1",
>+                                     "trayITrayService");
>+}

This is used from a single JS file and doesn't exist on all platforms so I would prefer not sharing it in imServices.jsm.

Was there an issue with the this.trayService variable initialized in the init method?


Thanks for looking at this issue and contributing a patch here! :-)

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8352483 [details] [diff] [review]
patch v1, always show tray icon (user configurable)

*** Original change on bio 749 attmnt 741 at 2011-07-17 20:08:55 UTC ***

Per flo's comments. I don't have any in addition. Thanks for taking a look at this.

Comment 11

[Patrick Cloke [:clokep]]

Attachment: patch v1.1

*** Original post on bio 749 as attmnt 762 at 2011-07-29 23:20:00 UTC ***

This is pretty important to get into our next release so I'm going to take this. This is the previous patch from Nils Maier w/ the review comments met and a few changes to match our coding style. Tested on Windows 7.

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8352483 [details] [diff] [review]
patch v1, always show tray icon (user configurable)

*** Original change on bio 749 attmnt 741 at 2011-07-29 23:20:36 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Florian Quèze [:florian]]

*** Original post on bio 749 at 2011-08-17 21:39:20 UTC ***

Comment on attachment 8352504 [details] [diff] [review] (bio-attmnt 762)
patch v1.1

>diff --git a/instantbird/components/mintrayr/content/mintrayr.js b/instantbird/components/mintrayr/content/mintrayr.js

>   init: function() {
>-    window.removeEventListener("load", gMinTrayR.init, true);
>+    window.removeEventListener("load", this.init, true);

I'm afraid this won't work, as the event listener is gMinTrayR.init.bind(gMinTrayR), not this.init.

>+    window.addEventListener("unload", this.uninit.bind(this), false);

The code Nils used in attachment 8352483 [details] [diff] [review] (bio-attmnt 741) to avoid the uninit method seemed OK to me.
Your code works too, except I think the removeEventListener call in the uninit method won't work for the same reason as above.

>+    this._prefs.addObserver("alwaysShowTrayIcon", this, false);

A function can be used as an nsIObserver implementation, so it's possible to avoid the observe method, but doing so may cause an issue with the value of this in the method.

Looks good overwise.

Comment 14

[Patrick Cloke [:clokep]]

Attachment: patch v1.2

*** Original post on bio 749 as attmnt 785 at 2011-08-24 01:31:00 UTC ***

I think this should properly unregister the observers now, is there some way to check this?

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8352504 [details] [diff] [review]
patch v1.1

*** Original change on bio 749 attmnt 762 at 2011-08-24 01:31:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 16

[Florian Quèze [:florian]]

*** Original post on bio 749 at 2011-08-24 10:02:50 UTC ***

(In reply to comment #12)
> Created an attachment (id=785) [details]
> patch v1.2
> 
> I think this should properly unregister the observers now

I don't think so. Each call to bind returns a new different function.
To verify this, you can try this in the error console:

$ alert == alert
-> true

$ alert.bind(null) == alert.bind(null)
-> false

> is there some way to check this?
I'm not completely sure, but I think the load event may fire several times per window. Maybe once for the document then once per image, or something like that. If you put a dump or alert call in your "init" method and it's called several times, you are then sure your removeEventListener call doesn't work. If not, then I don't know :-/.

Comment 17

[Patrick Cloke [:clokep]]

Attachment: patch v1.3

*** Original post on bio 749 as attmnt 789 at 2011-08-25 22:48:00 UTC ***

Instead of messing with bind, I just use the global variable throughout the init and uninit methods.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8352527 [details] [diff] [review]
patch v1.2

*** Original change on bio 749 attmnt 785 at 2011-08-25 22:48:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Patrick Cloke [:clokep]]

Attachment: patch v1.6

*** Original post on bio 749 as attmnt 790 at 2011-08-25 23:20:00 UTC ***

Skipped a few revisions via IRC reviews.

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8352531 [details] [diff] [review]
patch v1.3

*** Original change on bio 749 attmnt 789 at 2011-08-25 23:20:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8352532 [details] [diff] [review]
patch v1.6

*** Original change on bio 749 attmnt 790 at 2011-08-25 23:22:17 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8352532 [details] [diff] [review]
patch v1.6

*** Original change on bio 749 attmnt 790 at 2011-08-25 23:25:22 UTC ***

Looks good, finally :).

Comment 23

[Florian Quèze [:florian]]

*** Original post on bio 749 at 2011-08-26 01:15:35 UTC ***

https://hg.instantbird.org/instantbird/rev/343293bfba9e