478271 - "Network/channel/query view for * opened" and "Now logging to" messages redundant

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[James Ross]

[INFO]	Query view for “Sugnim” opened.
[INFO]	Now logging to <file:///E:/Users/James/AppData/Roaming/Mozilla/ChatZilla/Profiles/eanocf0n.default/chatzilla/logs/moznet/users/Sugnim.2009-02-12.log>.

I don't believe either message is useful, on any view, save perhaps for whether logging is on/off for the view (which we can and should put as an icon in the statusbar and/or menu item).

I'd like both messages gone; the view can identify itself (window title, tab label, background text), so the first message is completely redundant (I know it's used to force the view into existance currently, but that should not be necessary), and the second only has the link, which is rarely useful (you can get the link through /log if needed, as it changes without another message anyway).

[Related note: this is more irritating than usual for me because I'm running with my view-persist plugin, and these extra 2 lines add 50%+ more lines to each channel on returning (5 vs 3, 6 vs 4).]

Comment 1

[James Ross]

Attachment: Remove messages and add logging statusbar icon

For those who can't decode Git binary patches for PNG images in their head, the images are http://twpol.dyndns.org/temp/cz-logging-off.png and http://twpol.dyndns.org/temp/cz-logging-on.png.

Comment 2

[:Gijs (he/him)]

Comment on attachment 430385 [details] [diff] [review]
Remove messages and add logging statusbar icon

r=me, with these caveats:

Are all the updateLoggingIcon calls necessary? Like the one in onDisconnect. I haven't looked at it closely, but offhand I can't think why that'd change anything about whether or not the view is logged.

I'm also not clear why you're removing/adding the wrapping argument (to open/closeLogFile) in some cases. Are we currently getting that wrong?

Comment 3

[James Ross]

I've removed some updateLoggingIcon() calls that looked unnecessary.

The wrapping argument was inverted so the call-sites are being inverted too.

http://hg.mozilla.org/chatzilla/rev/84a5082f689a

Comment 4

[Robert Utasi [:hunboy]]

Removing the initializator lines from the code before appending the table several previous gecko versions render the table badly, but this is an old known behavior with an old known workaround, we just need to add an other initializator line instead of the removed one.

The correct place to initialize the table is the place where we create the table, so the createMessages() function.

I dont want to add a dummy line in to the tbody structure because that disturbs the messages/adding/removing/collapsing stuffs.

Also thead is the recommended way, but the code always gets the tbody object as messages.firstChild and I don't want to modify the code in several points.

So tfoot looks the best acceptable way to add back an initializator line to the table. That doesn't disturb anything at the bottom of the table, message system always use the tbody for the messages.

I prepared 2 patch versions, first one works with html attributes, the second one works with clear css.

The dummy line must be displayed so the chatzilla feature attribute "hidden" doesn't work, but not nessesary. The line has no content and has no height, and also I added css rule to turn the padding off also for the tfoot tabledivisions.

Comment 5

[Robert Utasi [:hunboy]]

Attachment: init the table | attributes version

code contains html attributes to init the width

Comment 6

[Robert Utasi [:hunboy]]

Attachment: init the table | css only version

this one uses css only to init the table with the dummy line

Comment 7

[Robert Utasi [:hunboy]]

based on versioncheck all of clients are affected with this issue until Gecko 1.8.X (<netscape, <seamonkey1.1, <firefox2.0)

the first Gecko where dinamic tablerendering is ok the Gecko 1.9.0 (firefox3.0)

Comment 8

[Robert Utasi [:hunboy]]

While the initializator row is nessesary until adding a real row, after(!) adding one we can delete the initializator footer with the biultin DOM function messages.deteteTFoot() in the addHistory() function. When the first line is initialized, that will be an initializator for the other ones. In this case the attributes version can be better without any css rules, so less code modification, while that will be removed at the first real line adding.

w3c DOM reference says:
deleteTFoot
    Delete the footer from the table, if one exists.
    No Parameters
    No Return Value
    No Exceptions
so calling deleteTFoot uselessly after the tfoot was removed once - no affect.

Comment 9

[Robert Utasi [:hunboy]]

This workaround also can be prospect well, but I have stability worries of it :(
The place is at the end of addHistory()

    if (source.messageCount == 1){
        var mydocument = getContentDocument(source.frame);
        if(mydocument && mydocument.getElementById('output')){
            mydocument.getElementById('output').appendChild(
                mydocument.getElementById('output').firstChild);
        }
    }

It just simply reappends the table when that has 1 row.

Comment 10

[Robert Utasi [:hunboy]]

Attachment: dynamic table rendering testcase

This testcase contains 3 dynamic tables - you can add rows by pressing the button.

the first one is the appendChild (current) version. 1 more button added to represent the reappend workaround method which one fixes this problem.

the second one is the tfoot version, also works as workaround.

the third one is interesting, because works! But... works as a totally REVERSED way than the chatzilla works currently. So this can be only a plan for the future, nothing else, because requires completely re-think the addHistory() method as reversed. I use here the insertRow() DOM function, but i don't use the insertCell(), because seems not nessesary to handle this problem.

Comment 11

[Robert Utasi [:hunboy]]

// original code is
//if (obj)
//    appendTo.appendChild(client.adoptNode(obj, appendTo.ownerDocument));

// new code with insertRow() and repacking childnodes.

if (obj){
    var tr = appendTo.insertRow(-1);
    for(var i = 0; i < obj.attributes.length; i++)
        tr.setAttribute(obj.attributes[i].name,obj.attributes[i].value);
    while(obj.firstChild)
        tr.appendChild(
            client.adoptNode(obj.firstChild, appendTo.ownerDocument));
}

I've skipped re-think the addhistory mechanism, rather created a new TR with insertRow() function, cloned the attributes and moved the content (TD's) to the new tr object. The change at the end of the addHistory(), where the appending happens.

Comment 12

[Robert Utasi [:hunboy]]

Attachment: reverse append of elements at 1st row

This is same way as insertRow() does, and as I mentioned in Comment 11 .
By using cloneNode I saved 1 explicit loop although I need adopt and append in this way. The code is simpler a little and maybe faster. We need do this only at the first row we add. After that it is initialized already, can go in normal way.

(I'm not planning to HG the cross-document reappend-hack, thats too hard however also works (in other way using output-window.html is optional). If nessesary I can re-HG the tfoot way, but the reverse way looks to me the best from the 3 possibilities I worked out to handle this problem.)

Comment 13

[Robert Utasi [:hunboy]]

Attachment: using tfoot

TFoot version. By using the DOM tablefunctions the code is pretty simple, no localvariables, and while Netscape6 (Gecko 0.6.0) already knows them, it's backwards compatible enoughly.

Comment 14

[Robert Utasi [:hunboy]]

Attachment: reverse append the element at 1st row

//******************************************************************************
The original code (addHistory - end section):
//******************************************************************************

    if ("frame" in source)
        needScroll = checkScroll(source.frame);
    if (obj)
        appendTo.appendChild(client.adoptNode(obj, appendTo.ownerDocument));

    if (source.MAX_MESSAGES)
    {
        if (typeof source.messageCount != "number")
            source.messageCount = 1;
        else
            source.messageCount++;

        if (source.messageCount > source.MAX_MESSAGES)
            removeExcessMessages(source);
    }

    if (needScroll)
    {
        scrollDown(source.frame, true);
        setTimeout(scrollDown, 500, source.frame, false);
        setTimeout(scrollDown, 1000, source.frame, false);
        setTimeout(scrollDown, 2000, source.frame, false);
    }

//******************************************************************************
I just use reverse append the table has no rows yet.

The code is here:
//******************************************************************************

    if ("frame" in source)
        needScroll = checkScroll(source.frame);

    // In the Gecko 1.7.X 1.8.X versions when we append an empty table and try
    // to append a completed row, the table-rendering goes wrong, so we need
    // append the first row as a reversed way.
    if (obj)
    {
        if (source.messageCount)
            appendTo.appendChild(client.adoptNode(obj, appendTo.ownerDocument));
        else
        {
            var tr = obj.cloneNode(false);
            appendTo.appendChild(client.adoptNode(tr, appendTo.ownerDocument));
            while(obj.firstChild)
                tr.appendChild(
                    client.adoptNode(obj.firstChild, appendTo.ownerDocument));
        }
    }

    if (source.MAX_MESSAGES)
    {
        if (typeof source.messageCount != "number")
            source.messageCount = 1;
        else
            source.messageCount++;

        if (source.messageCount > source.MAX_MESSAGES)
            removeExcessMessages(source);
    }

    if (needScroll)
    {
        scrollDown(source.frame, true);
        setTimeout(scrollDown, 500, source.frame, false);
        setTimeout(scrollDown, 1000, source.frame, false);
        setTimeout(scrollDown, 2000, source.frame, false);
    }

//******************************************************************************
The problem is, when MAX_MESSAGES is zero it goes always to the reverse append
case, because messageCount never incremented in that case. It works but useless.
That is not nessesary after adding the 1st row.

Ans because I use the messageCount and can't be sure, why the code contains
'if (typeof source.messageCount != "number")' checking, I got out that
incrementing from the removeExcessMessages section, and put before the appending
Now it counts all the time, so we can know all the time how many messages in
the table, although the original code incremented only when MAX_MESSAGES was set
I did not find any conflict in the codebase to increment permanently.
1 difference I found, when the user sets 0 to MAX_MESSAGES and reaches about
1000 messages, and thinks other and sets MAX_MESSAGES=200, the original code
adds new 200 lines to the 1000 existing and fires removeExcessMessages at 1200
lines only, because messageCount starts from zero. The new behavior - while it
counts continuously - fires immediately the removeExcessMessages at the 1001th
line and reduces the list to 200.

I use reverse append way when the messageCount==1

The removeExcessMessages callig "IF" turned to simpler, because I already
handled the messageCount.

The code is here:
//******************************************************************************

    if ("frame" in source)
        needScroll = checkScroll(source.frame);

    if (typeof source.messageCount != "number")
        source.messageCount = 1;
    else
        source.messageCount++;

    // In the Gecko 1.7.X 1.8.X versions when we append an empty table and try
    // to append a completed row, the table-rendering goes wrong, so we need
    // append the first row as a reversed way.
    if (obj)
    {
        if (source.messageCount == 1)
        {
            var tr = obj.cloneNode(false);
            appendTo.appendChild(client.adoptNode(tr, appendTo.ownerDocument));
            while(obj.firstChild)
                tr.appendChild(
                    client.adoptNode(obj.firstChild, appendTo.ownerDocument));
        }
        else
            appendTo.appendChild(client.adoptNode(obj, appendTo.ownerDocument));
    }

    if (source.MAX_MESSAGES && source.messageCount > source.MAX_MESSAGES)
        removeExcessMessages(source);

    if (needScroll)
    {
        scrollDown(source.frame, true);
        setTimeout(scrollDown, 500, source.frame, false);
        setTimeout(scrollDown, 1000, source.frame, false);
        setTimeout(scrollDown, 2000, source.frame, false);
    }

//******************************************************************************
To think forward the modified and the original code, what happens if the
"obj" object is invalid? We don't append anything because of "if (obj)",
but increment the messageCount (in the original code too) and call the
removeExcessMessages if MAX_MESSAGES nonzero.
However in my opinion this is a "no change" issue, so I reput the incrementing
and the removeExcessMessages calling into the "if (obj)" section.

The code is here:
//******************************************************************************

    if ("frame" in source)
        needScroll = checkScroll(source.frame);

    if (obj)
    {
        if (typeof source.messageCount != "number")
            source.messageCount = 1;
        else
            source.messageCount++;

        // In the Gecko 1.7.X 1.8.X versions when we append an empty table
        // and try to append a completed row, the table-rendering goes wrong,
        // so we need append the first row as a reversed way.
        if (source.messageCount == 1)
        {
            var tr = obj.cloneNode(false);
            appendTo.appendChild(client.adoptNode(tr, appendTo.ownerDocument));
            while(obj.firstChild)
                tr.appendChild(
                    client.adoptNode(obj.firstChild, appendTo.ownerDocument));
        }
        else
            appendTo.appendChild(client.adoptNode(obj, appendTo.ownerDocument));

        if (source.MAX_MESSAGES && source.messageCount > source.MAX_MESSAGES)
            removeExcessMessages(source);
    }

    if (needScroll)
    {
        scrollDown(source.frame, true);
        setTimeout(scrollDown, 500, source.frame, false);
        setTimeout(scrollDown, 1000, source.frame, false);
        setTimeout(scrollDown, 2000, source.frame, false);
    }

//******************************************************************************
To see the code, is it possible if the "obj" is invalid at all?
Or that check is about to omit?
The addHistory() is called only from the __display() and as I see the msgRow is
always created (nested table handled).
And addHistory contains inobj=obj and obj=inobj but those are all handled,
the code should die, if obj is invalid at nested tables and collapse mode,
while that uses obj's built-in DOM functions.
I've locked at this point. What did I miss?
//******************************************************************************

Comment 15

[James Ross]

As we're apparently not supporting Gecko <1.9.1 this is not an issue in all supported cases so re-closing.