166929 - ChatZilla doesn't have online/offline icon

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Koike Kazuhiko]

ChatZilla doesn't have online/offline icon.
If ChatZilla is launched offline by -chat option, there is no way
to go online.

Comment 1

[Alex Hirzel]

Duping with 236940 because it has more info.


*** This bug has been marked as a duplicate of 236940 ***

Comment 2

[benc]

REOPEN:
Lets make this the UI/frontend bug. At minimum we need to have a button. Often
Pref UI is desired by users (as in MailNews).

Comment 3

[:Gijs (he/him)]

Attachment: Patch

* Add an icon w/ tooltip
* Allow people to go online/offline from the icon
* Send a notification when that happens, and listen for that, ask "are you sure" if you're still connected, allow user to cancel going offline from there.
* listen for and send network:offline-status-changed events so we keep in sync with the platform.
* Sync to/from platform-dependant offline pref based on icon.
* Make sure we error when you try to use a connection that's been silently killed by offline mode in older builds
* Display a nice error message for the error above, and the error thrown by moz 1.0 and current trunk / 1.8 branch. (everything inbetween seems to be pretty silent on the matter).

I think that's about everything this patch does. It also fixes the dep bug about "supporting offline mode", I think.

Comment 4

[:Gijs (he/him)]

So, this patch breaks the normal /disconnect and quit. I don't yet know why. I might figure out tonight, or something, but feel free to tell me why if you know.

Comment 5

[:Gijs (he/him)]

Attachment: Better patch, I believe

I think this is a better patch. It uses a flag set by the exception to make sure we don't get two messages. It seems to help on my laptop running linux, but I saw the problem better on my windows machine. I will try to check there somewhere in the next week.

Also, the icons, though I suppose they could be discussed separately from the patch, I took from:
http://mxr-test.landfill.bugzilla.org/mxr-test/mozilla1.8.0.x/source/toolkit/themes/qute/communicator/icons/online.png
and
http://mxr-test.landfill.bugzilla.org/mxr-test/mozilla1.8.0.x/source/toolkit/themes/qute/communicator/icons/offline.png

These are the online/offline icons that shipped with TB 1.5.0.X, as far as I know. I must say I don't like the new light bulb icon, mostly because I don't think it's clear what it does until you try. SeaMonkey's icon, at least for classic, looks fugly next to our security icon (which we, uh, borrowed from Fx 1.0, I believe).

Comment 6

[:Gijs (he/him)]

I can confirm my last patch no longer breaks disconnect or quit. Yay!

Comment 7

[James Ross]

Comment on attachment 249537 [details] [diff] [review]
Better patch, I believe

>+	skin/modern/chatzilla/images/online.png             (xul/skin/images/online.png)
>+	skin/modern/chatzilla/images/offline.png             (xul/skin/images/offline.png)

Nit: line these up please.

>+    var connectTimeout = new Date() - this.connectDate;

>+
>+        //XXXgijs: on older Mozilla builds, we don't get noticed if the
>+        // connection died because the user went into offline mode. Also, due to
>+        // bug 364741, we can't be sure isAlive() isn't lying just after
>+        // a succesful connect. Give it some leniency:
>+        if ((connectTimeout > SOCKET_ALIVE_TIMEOUT) && this._transport &&
>+            ("isAlive" in this._transport) && !this._transport.isAlive())
>+        {
>+            throw NS_ERROR_ABORT;
>+        }

I'd much rather we didn't do all the Date and isAlive work each time we did a read. The former is easy to fix (put it inside the isAlive check), but I'm not sure about the rest.

>         dd ("*** Caught " + ex + " while reading.");
>+        // Tell the world, so we won't fire onDisconnect twice

Nit: full-stop please.

>+    var connectTimeout = new Date() - this.connectDate;
>     try
>     {
>+        //XXXgijs: see the note in readData
>+        if ((connectTimeout > SOCKET_ALIVE_TIMEOUT) && this._transport &&
>+            ("isAlive" in this._transport) && !this._transport.isAlive())
>+        {
>+            throw NS_ERROR_ABORT;
>+        }

Same as readdata.

>         dd ("*** Caught " + ex + " while sending.");
>+        // Tell the world, so we won't fire onDisconnect twice

Nit: ditto.

> StreamListener.prototype.onStopRequest =
> function sl_stopreq (request, ctxt, status)
> {
>     //dd ("StreamListener::onStopRequest: " + request + ", " + ctxt + ", " +
>     //status);

Nit: blank line here please.

>+    ctxt = ctxt.wrappedJSObject;
>+    if (!ctxt)
>+    {
>+        dd ("*** Can't get wrappedJSObject from ctxt in " +
>+            "StreamListener.onStopRequest ***");

Nit: no space before open parenthesis.

>+        return;
>+    }
>+
>+    // Don't do anything if we already threw an exception (with more info)

Nit: full-stop please.

>+    if (ctxt.threwDCException)
>+        return;
>+    

Nit: blank line.

>-    if ((this.parent.state == NET_CONNECTING) ||
>+    // Don't reconnect if our connection was aborted.
>+    var wasAborted = (e.disconnectStatus == NS_ERROR_ABORT);
>+    if ((this.parent.state == NET_CONNECTING) && !wasAborted ||

Need some parentheses here to make this clear, even if it works currently. A && B || C is not clear.

>         /* fell off while connecting, try again */
>         (this.parent.primServ == this) && (this.parent.state == NET_ONLINE) &&
>-        (!("quitting" in this) && this.parent.stayingPower))
>+        (!("quitting" in this) && this.parent.stayingPower && !wasAborted))
>     { /* fell off primary server, reconnect to any host in the serverList */

>+function initOfflineIcon()
>+{

>+        _getNewIOSvc: function offline_getNewIOSvc()
>+        {
>+            try
>+            {
>+                var ioService = getService("@mozilla.org/network/io-service;1",
>+                                           Components.interfaces.nsIIOService2);

... = getService("@mozilla.org/network/io-service;1", "nsIIOService2");

Also consider putting an IO_SVC constant for the first part at the top of initOfflineIcon.

>+                // If it failed, it's probably just not there. We don't care.
>+                return null;
>+            }
>+            return ioService;

This can be optimised to return from within the try block; no need for a local, just have an empty catch and return null after.

>+        observe: function offline_observe(subject, topic, state)
>+        {
>+            const nsISupportsPRBool = Components.interfaces.nsISupportsPRBool;

Consider moving to the constant to the top of initOfflineIcon.

>+        canGoOffline: function offline_check()
>+        {
>+            const PRBoolCID = "@mozilla.org/supports-PRBool;1";
>+            const nsISupportsPRBool = Components.interfaces.nsISupportsPRBool;
>+            const OS_CID = "@mozilla.org/observer-service;1";
>+            const nsIObserverService = Components.interfaces.nsIObserverService;

Consider moving to the top of initOfflineIcon. Also, neither interface constant is needed.

>+            try
>+            {
>+                var os = getService(OS_CID, nsIObserverService);
>+                var canGoOffline = newObject(PRBoolCID, nsISupportsPRBool);

Use string interfaces names, per previous comment.

>+        updateOfflineFromPref: function offline_syncFromPref()

>+            var isOffline = false;
>+            var prefSvc = getService("@mozilla.org/preferences-service;1",
>+                                     Components.interfaces.nsIPrefBranch);

... = getService("@mozilla.org/preferences-service;1", "nsIPrefBranch");

>+        updatePrefFromOffline: function offline_syncToPref()
>+        {
>+            var isOffline = client.iosvc.offline;
>+            var prefSvc = getService("@mozilla.org/preferences-service;1",
>+                                     Components.interfaces.nsIPrefBranch);

... = getService("@mozilla.org/preferences-service;1", "nsIPrefBranch");

>+    const OS_CID = "@mozilla.org/observer-service;1";
>+    const nsIObserverService = Components.interfaces.nsIObserverService;
>+    try
>+    {
>+        var os = getService(OS_CID, nsIObserverService);

... = getService(OS_CID, "nsIObserverService");

>+function uninitOfflineIcon()
>+{
>+    const OS_CID = "@mozilla.org/observer-service;1";
>+    const nsIObserverService = Components.interfaces.nsIObserverService;
>+    try
>+    {
>+        var os = getService(OS_CID, nsIObserverService);

... = getService(OS_CID, "nsIObserverService");

>+msg.going.offline     = &brandShortName; is trying to go into offline mode. This will disconnect you from ALL the networks and channels you're connected to.
>+msg.really.go.offline = Go Offline Anyway

Use "Go Offline".

>+msg.dont.go.offline   = Don't Go Offline
>+msg.offlinestate.offline = You are offline. Click on the icon to go online.
>+msg.offlinestate.online  = You are online. Click on the icon to go offline.

For both: remove the word "on".

r=silver with those nits fixed.

Comment 9

[:Gijs (he/him)]

Attachment: Patch for the icon only

This patch is identical to the previous, except for:
- not doing any of the compatibility cruft. This means that you will still not get an error in your outputwindow if you go offline (and your connections will claim to still be active until you force them to reconsider their position) iff you're running Mozilla 1.4, 1.7 or Firefox 1.0. The builds between 2003-01-17 and 2004-08-04 will be "broken" in this respect. See bug 176919 and bug 202638.

Firefox 1.5.0.*, 2.0.0.*, Flock, SeaMonkey and XULRunner will display an error and close the connection - ie, they will "work" the way they're supposed to.

The icon will work everywhere, so even on the platforms with broken streams, the icon will indicate what happen (and will also warn you if you have open connections and try to go offline).

- fixing the nits James pointed out (thanks!)

I still want to fix the compat issues (though honestly, nobody is worse off than they are now) but I'd like to do that back in bug 236940. The addition of the icon can be fixed by this patch alone, and will work regardless of the socket hacks, with one exception:

You'll get another warning popup if you went offline earlier in this session, when you had open IRC connections and you try to go online on Firefox 1.0 using the menuitem (I'm not sure about MAS)

This is because Firefox is stupid enough to ask "do you mind if I go offline" when going *online*, and because the socket code will not have told our code that it went offline, so our code will still believe that it has open connections (which it doesn't). See also bug 364611 (fixed on trunk).

James, could I get another stamp of approval for this one? (even though it's mostly the same, as said.

Whoever commits this (probably me) - don't forget the icon files, or the tree will burn. We've done that before... :-)

Comment 10

[James Ross]

Comment on attachment 258363 [details] [diff] [review]
Patch for the icon only

> 	skin/modern/chatzilla/images/secure-broken.png      (xul/skin/images/secure-broken.png)
>+	skin/modern/chatzilla/images/online.png             (xul/skin/images/online.png)
>+	skin/modern/chatzilla/images/offline.png             (xul/skin/images/offline.png)

Nit: line these up.

> 	locale/en-US/chatzilla/chatzillaOverlay.dtd  (xul/locale/en-US/chatzillaOverlay.dtd) 


>+function initOfflineIcon()
>+{
>+    const IOSVC2_CID = "@mozilla.org/network/io-service;1";
>+    const PRBool_CID = "@mozilla.org/supports-PRBool;1";
>+    const OS_CID = "@mozilla.org/observer-service;1";
>+    const nsISupportsPRBool = Components.interfaces.nsISupportsPRBool;
>+    client.offlineObserver = {

Nit: probably cleaner to have a blank line before this one, to separate the constants.

>+        _element: document.getElementById("offline-status"),
>+        _getNewIOSvc: function offline_getNewIOSvc()


>+        updateOfflineFromPref: function offline_syncFromPref()
>+        {
>+            // On toolkit, we might have smart management of offline mode.
>+            // Don't interfere.
>+            var ioSvc2 = this._getNewIOSvc();
>+            if (ioSvc2 && ioSvc2.manageOfflineStatus)
>+                return;
>+
>+            var isOffline = false;
>+            var prefSvc = getService("@mozilla.org/preferences-service;1",
>+                                     "nsIPrefBranch");
>+            // Let the app-specific hacks begin:
>+            try {
>+                if ((client.host == "Firefox") || (client.host == "Flock"))
>+                    isOffline = prefSvc.getBoolPref("browser.offline");
>+                else if (client.host == "XULrunner")
>+                    isOffline = !prefSvc.getBoolPref("network.online");
>+                // This is app-managed, or should be, on startup:
>+                else if (client.host == "Mozilla")
>+                    return;

This if/return can be moved above the var isOffline line.

>+            }
>+            catch (ex) { /* Whatever. */ }
>+
>+            // Actually do it:
>+            client.iosvc.offline = isOffline;
>+        },
>+        updatePrefFromOffline: function offline_syncToPref()
>+        {
>+            var isOffline = client.iosvc.offline;
>+            var prefSvc = getService("@mozilla.org/preferences-service;1",
>+                                     "nsIPrefBranch");
>+            // Let the app-specific hacks begin:
>+            try {
>+                if ((client.host == "Firefox") || (client.host == "Flock"))
>+                    prefSvc.setBoolPref("browser.offline", isOffline);
>+                else if (client.host == "XULrunner")
>+                    prefSvc.setBoolPref("network.online", !isOffline);
>+                // This is app-managed, or should be.
>+                else if (client.host == "Mozilla")
>+                    return;

Same as previous.

>+            }
>+            catch (ex)


r=silver with those fixed.

Comment 11

[:Gijs (he/him)]

Checking in mozilla/extensions/irc/jar.mn;
new revision: 1.32; previous revision: 1.31
Checking in mozilla/extensions/irc/js/lib/connection-xpcom.js;
new revision: 1.45; previous revision: 1.44
Checking in mozilla/extensions/irc/js/lib/irc.js;
new revision: 1.104; previous revision: 1.103
Checking in mozilla/extensions/irc/xul/content/handlers.js;
new revision: 1.149; previous revision: 1.148
Checking in mozilla/extensions/irc/xul/content/menus.xul;
new revision: 1.24; previous revision: 1.23
Checking in mozilla/extensions/irc/xul/content/static.js;
new revision: 1.236; previous revision: 1.235
Checking in mozilla/extensions/irc/xul/locale/en-US/chatzilla.properties;
new revision: 1.132; previous revision: 1.131
Checking in mozilla/extensions/irc/xul/skin/chatzilla.css;
new revision: 1.38; previous revision: 1.37
Checking in mozilla/extensions/irc/xul/skin/images/offline.png;
initial revision: 1.1
Checking in mozilla/extensions/irc/xul/skin/images/online.png;
initial revision: 1.1