Closed Bug 954183 Opened 7 years ago Closed 7 years ago

System Tray Icon should persist even when Buddy List is open

Categories

(Instantbird :: Other, enhancement)

All
Windows XP
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: clokep)

References

Details

Attachments

(1 file, 4 obsolete files)

*** 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.
Blocks: 953598
*** 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).
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: All → Windows XP
*** 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.
*** 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. :)
Summary: Tray Icon should persist even when Buddy List is open → System Tray Icon should persist even when Buddy List is open
*** 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.
*** 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.
*** 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.
Attachment #8352483 - Flags: review?
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 ***
Attachment #8352483 - Flags: review? → review?(clokep)
*** 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 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.
Attachment #8352483 - Flags: review?(clokep) → review-
Duplicate of this bug: 954383
Attached patch patch v1.1 (obsolete) — Splinter Review
*** 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.
Attachment #8352504 - Flags: review?(florian)
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 ***
Attachment #8352483 - Attachment is obsolete: true
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** 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.
Attached patch patch v1.2 (obsolete) — Splinter Review
*** 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?
Attachment #8352527 - Flags: review?(florian)
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 ***
Attachment #8352504 - Attachment is obsolete: true
Attachment #8352504 - Flags: review?(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 :-/.
Attached patch patch v1.3 (obsolete) — Splinter Review
*** 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.
Attachment #8352531 - Flags: review?(florian)
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 ***
Attachment #8352527 - Attachment is obsolete: true
Attachment #8352527 - Flags: review?(florian)
Attached patch patch v1.6Splinter Review
*** Original post on bio 749 as attmnt 790 at 2011-08-25 23:20:00 UTC ***

Skipped a few revisions via IRC reviews.
Attachment #8352532 - Flags: review?(florian)
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 ***
Attachment #8352531 - Attachment is obsolete: true
Attachment #8352531 - Flags: review?(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 ***
Attachment #8352532 - Attachment is patch: true
Attachment #8352532 - Attachment mime type: application/octet-stream → text/plain
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 :).
Attachment #8352532 - Flags: review?(florian) → review+
*** Original post on bio 749 at 2011-08-26 01:15:35 UTC ***

https://hg.instantbird.org/instantbird/rev/343293bfba9e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
You need to log in before you can comment on or make changes to this bug.