436759 - "log this view" in Global Settings/ General opens a tab for each previously logged view

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[worms]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.9b4pre) Gecko/2008021302 Mnenhy/0.7.5.0 SeaMonkey/2.0a1pre XpcomViewer/0.9
Build Identifier: 

In the Preferences, Global Settings / tab General, the option "Log this view" is unticked by default.

Ticking it while you have previously logged views will open a tab for each of these views.

Reproducible: Always

Steps to Reproduce:
1. Log views as you wish
2. Open Global settings, check that "log this view" is not ticked in the General tab
3. Tick it
Actual Results:  
ChatZilla opens a tab for each previously logged view (which can be a lot, and can freeze everything)

It doesn't connect, though! :)

Expected Results:  
I don't know!

Sugnim++

Comment 1

[:Gijs (he/him)]

I'm quite confused about why this is happening, actually. We should fix it for 0.9.83 if at all possible. I think we can probably fix it by checking whether view.frame exists before calling view.displayHere in client.openLogFile. But I'd like to understand *why* it breaks so magnificently before I do that...


http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/irc/xul/content/static.js&rev=1.277&mark=4762-4763#4740

Comment 2

[James Ross]

What doesn't make sense to me is that, as "log this view" is not an inherited preference, I can't see how changing it for Global Settings would affect anything other than the *client* tab.

worms, what appears on each view that wasn't there before?

I'm still not exactly clear on the STR for this bug, too. If I've got a bunch of views open and being logged, and I change the setting, how can they open again? Need some more detail in there somewhere...

Comment 3

[worms]

Maybe I wasn't very clear when explaining. 

Chatzilla will open a view for each "view" that you had previously logged in the past. 

I did a new test (Chatzilla on Seamonkey, 0.9.83). Logging is enable for all views (in the "Global Settings / Global" tab. I have "delete channel views on part" in this build, but I'm not sure it's relevant, because on another build this pref. is not.

Test:
- I was in the *client* network, and when I ticked "Log this view", it opened a view for each network I ever connected to.
Each view says:  (example: moznet)

Now logging to <file:///C:/Documents%20and%20Settings/Stupid_User_Name/Application%20Data/Mozilla/SeaMonkey/Profiles/Stupid_Profile_Salted_and_Peppered/chatzilla/logs/moznet/moznet.2008-07-09.log>.

I went to that view to copy the above information, and re-did the test. That time it created all the views, EXCEPT moznet.

(See screenshot attached... later)

Now before closing cZ, I close each view with right-click, and untick "Log this view" in the Global Settings / General tab.
Close cZ.
Relaunch.

Tick "Log this view". If nothing happens, untick/tick a few times, it will happen :-)

I'm not sure why this time it didn't try to open channels and users. Also, it doesn't open a view for the irc.thing. logs... Mystery!

Comment 4

[worms]

Attachment: Log folder / Chatzilla pref / Chatzilla views created

Comment 5

[worms]

Sugnim++

Comment 6

[James Ross]

Wow. This is a fun one, but I think I can explain what's happening.

The PrefManager has a list of "observers" - things that want to know when prefs change. One of these is always the code to turn logging on/off when the "log" pref changes. Also in the observers list are 'child' objects (so client has all the networks listed, networks have channels listed, etc.).

The problem comes when you change a preference that is not inherited (or, in fact, one which is just not set to the inherited value). The chain of observer notifications goes on without caring about this, so you can end up in onPrefChanged observers for child objects, but with the *parent's* newValue and oldValue!

The incorrect values don't matter in this specific case, but notifying the 'child' objects when they simply aren't inheriting the preference is what causes the wash of spurious messages (as it syncs the logging state with the prefs).

I think we need to split the PrefManager observers list into two: one for prefs changing and one for the parent/child relationship, which needs extra care.

Comment 7

[:Gijs (he/him)]

Attachment: Patch

Comment 8

[James Ross]

Comment on attachment 354049 [details] [diff] [review]
Patch

>+    var r;
>+        var oldValue = r.realValue;

Nit: add oldValue to the var at the top.

> function onNetworkPrefChanged(network, prefName, newValue, oldValue)
> {
>+    if (newValue == oldValue)
>+        return;

Nit: put after the stale check, like you did with DCC.

> function onChannelPrefChanged(channel, prefName, newValue, oldValue)
> {
>+    if (newValue == oldValue)
>+        return;

Nit: put after the stale check, like you did with DCC.

> function onUserPrefChanged(user, prefName, newValue, oldValue)
> {
>+    if (newValue == oldValue)
>+        return;

Nit: put after the stale check, like you did with DCC.

Nit: you forgot to add the check to function onPrefChanged (for client). Might as well have consistency.

r=silver with nits fixed.

Comment 9

[:Gijs (he/him)]

Checked in with nits fixed:

Checking in mozilla/extensions/irc/js/lib/pref-manager.js;
/cvsroot/mozilla/extensions/irc/js/lib/pref-manager.js,v  <--  pref-manager.js
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/extensions/irc/xul/content/prefs.js;
/cvsroot/mozilla/extensions/irc/xul/content/prefs.js,v  <--  prefs.js
new revision: 1.57; previous revision: 1.56
done