134814 - Collapsing messages will collapse too much

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

With COLLAPSE_MSGS set to true and NEW_TAB_THRESHOLD set to 1,

If IRCMonkey is in a channel and caillon does the following:

foo
/msg IRCMonkey foo
/me foo

It shows up in XChat as:

<caillon> foo
>caillon< foo
* caillon foo

and shows up in Chatzilla as:

<caillon> foo
          foo
          foo

There is no way to distinguish what is to the whole channel, via msg only, an
action, etc.  There should be no collapsing in cases like this.  I have a patch
coming up.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v1.0

Also compare the msg-type and msg-dest of the two last messages before assuming
we should collapse.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

taking bug.  rginda would you review?

Comment 3

[Samuel Sieb]

There is a distinction between the regular message to the channel and the action
message to the channel (which is italic and different color).  The private
message problem is probably related to bug 114104.

Comment 4

[Robert Ginda]

ssieb's right, actions should be distinguishable by color or italic text.  Is
there a motif where that distinction isn't made?

/msg's on the other hand, don't have any changes to font or color in any of the
motifs.  Maybe /msgs shouldn't collapse with other message types.  Bug 114104 is
about *sending* /msgs to user A from a query view opened for user B.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Bah, midair.  I'm not aware of one but I will write one soon.  To be honest, I
hate that the font styles change (italic and or color).  It's an action.  It
should be distinguishable by the * nick as opposed to <nick> -- besides they are
always in different contexts.

<nick> I didn't think so
* nick also is tired and goes to bed

Even _with_ color changes, it looks kind of stupid IMO to have it display as

<nick> I didn't think so
       also is tired and goes to bed

It makes it appear like <nick> is stupid on a quick glance.  "I didn't think so
also is tired and goes to bed" is bad grammar.  The fact is they are two
different contexts and should not be treated the same by collapsing.  Especially
since you aren't guaranteed they are always going to be colored differently (or
that the person has bad eyesight, is colorblind, etc).

I missed the part with sending messages but I can include a fix there too.

Comment 6

[Robert Ginda]

The thing is, "collapse" was written for use with the faces motifs, where every
new face takes up at least 50 pixels of horizontal space.  In this situation, we
want to collapse as much as possible.  The faces motifs also use a different
message border for /msgs and actions.

Comment 7

[Daniel Bachran]

rginda, collapsing messages does (IMHO) also make sense w/o the faces motifs, at
least that's how I use this... ;)

Perhaps, then, only the faces motifs need to be changed to _not_ show an image
on an action? Thus, you'd have faces as now with messages collapsed and only
actions stick out: w/o face and with nick being put before the action (like it
should ;).

Comment 8

[Daniel Bachran]

Attachment: Screenshot showing off the proposed patch

I really love this patch! I have just had the possibility to test it and I must
say it makes multiple statements mixed with actions look _much_ better when
collapse messages is on.

The current behaviour matches the implementation of the face motifs. But the
face motifs could be changed to not display an image when an action is
performed (like I did with my motif ;). So I think this might be an acceptable
solution for all...?

Comment 9

[Daniel Bachran]

Attachment: Improved patch (IMHO), based on "proposed fix v1.0"

Hello Everybody,

I have been using the proposed fix v1.0 for nearly 3 months now, without noting
any problem (but of course only using my own motif! ;-). One thing bugged me
though: when performing more than one action directly after another, the nick
is still only shown for the first adjacent action.

----------snip----------
<g0ph3r> I really like the proposed fix v1.0!
* g0ph3r likes it _really_ a lot
	 finds out that the fix still makes him look a bit stupid sometimes
----------snip----------

This still looks quite stupid, so I just decided to try fixing this. This is my
first patch for Mozilla so I neither have CVS access nor do I know if my fix
fits in a certain code guide line (if present).

Having said this, this patch works fine for me, though. It is based on the
proposed fix v1.0 but adds yet another variable 'notActionMsgType' which is
being checked in the if() clause. This variable is TRUE when the message type
is no "ACTION". If this variable is TRUE the message is not collapsed. It's as
easy as this. ;)

Thus, adjacent actions now look like this:

----------snip----------
<g0ph3r> I'm going to fix this!
* g0ph3r fixes adjacent actions, which bugged him a lot
* g0ph3r tests his fix
----------snip----------

Please have a look at this fix and let me know what you think of it!

Thanks a lot and have a nice week-end!

bye, daniel ;)

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

over to rginda

Comment 11

[Daniel Bachran]

Attachment: Updated patch to CZ 0.9.28

I updated "my" patch to the CZ 0.9.x series, it is now based on 0.9.28 but does
still apply to 0.9.33 as well (although with some offset).

Nevertheless, this patch will not get integrated as it is (according to rginda
some time ago) because it would make the faces motifs not look good. So perhaps
there is a smarter way to make both rginda and me happy...?

Comment 12

[James Ross]

While the collapsing on multiple "/me"s could just be a pref, we really (IMHO)
need some sort of "setting" in the motif to indicate if we should use aggressive
collapsing (collapse normal and "/me"s as we currently do) or non-agressive
(collapse normal and "/me"s, but not between).

Could we use a "special" CSS selector, like SETTING.collapse > aggressive {}, or
is that *waaaay* to hacky? :)   Perhaps applying a style to an element you can't
see in the output window, and getComputedStyle on it?  Either way, I think it
needs to be a motif-specific setting.

Comment 13

[Max Alekseyev]

*** Bug 237460 has been marked as a duplicate of this bug. ***

Comment 14

[Wanderbug]

When /msg'ing two or more consecutive but different recepients Chatzilla does
not distinguish which message was sent to which recepient past the first if
collapse consecutive messages is active.

Example:
-text entered as:
/msg NickServ identify password
/msg SomeOtherBot login name password
Any text here that goes to channel, fwiw.

-appears as:
to(NickServ) identify password
             login name password
             Any text here that goes to channel, fwiw.

The only visible difference is that odd/even lines are appropriately marked, yet
there is no distinction of what is said in channel or privately past the first line.

Comment 15

[:Gijs (he/him)]

Attachment: Patch to solve the 3 problems pointed out

This patch attempts to solve 3 things:
- Messages/notices/actions done to different persons were collapsed.
- Some people didn't want actions to collapse to other actions, but wanted
notices and messages to collapse to eachother. There is now a deferred
preference (collapseActions) to accomplish that.
- Some people didn't want different message types to collapse to eachother (so
notices shouldn't collapse to messages shouldn't collapse to actions and any
other way around). This setting is now motif-specific. Motifs can now specify
'settings' by using a "CZSETTINGS.foo" selector, such as
"CZSETTINGS.collapsemore", which is used in this case. All settings are stored
on a motifSettings object, which is stored on the view. This way, plugins /
different themes can introduce different settings. All settings are booleans,
so if they're present they're true, otherwise they're undefined. Right now the
only one really used is 'collapsemore', more may be added in future, or used by
plugins.

Note that if you set the collapseActions preference to false, and your motif
specifies collapsemore, action messages will never collapse, not to other
action messages but also not to notices or normal messages (that is to say, the
preference overrides the motif in this respect).

Finally, if you don't collapse messages, this patch shouldn't affect you *at
all*. Simple, eh? :-)

Comment 16

[Daniel Bachran]

Tested the attached patch against ChatZilla 0.9.68.5 (had to manually apply some
parts, since conference mode prefs and associated changes got in between...).

Latest patch (from Hannibal) is working fine for me in all aspects!

ciao, daniel :)

Comment 17

[James Ross]

Comment on attachment 185771 [details] [diff] [review]
Patch to solve the 3 problems pointed out

>Index: mozilla/extensions/irc/xul/content/output-window.js

>+function setMotifSettings(existingTimeout)
>+{
>+    // Try... catch with a repeat to cope with the style sheet not being loaded
>+    try
>+    {
>+        existingTimeout += 100;
>+        view.motifSettings = getMotifSettings();
>+    }
>+    catch(ex) 
>+    {
>+        if (existingTimeout >= 30000) // Stop after trying for 30 seconds
>+            return;
>+        if (ex.name == "NS_ERROR_DOM_INVALID_ACCESS_ERR") // not loaded yet.
>+            setTimeout(setMotifSettings, 100, existingTimeout); // try again.
>+        else // something else, panic!
>+            dd(ex);
>+    }
>+}

Might be nice to put those values at the top as |const Foo = 42;|, so at least
the two 100s are gurenteed to be the same.


>+function getMotifSettings()
>+{
>+    var re = new RegExp("czsettings\\.(\\w*)", "i");
>+    var rules = document.getElementById("main-css").sheet.cssRules;
>+    var rv = new Object();
>+    var ary;
>+    // Copy any settings, which are available in the motif using the
>+    // "CZSETTINGS" selector. We only store the regexp match after checking
>+    // the rule type because selectorText is not defined on other rule types.
>+    for (var i = 0; i < rules.length; i++)
>+    {
>+        if (rules[i].type == rules[i].STYLE_RULE &&
>+            ((ary = rules[i].selectorText.match(re)) != null))

You wrapped the != but not the ==. I'd prefer both wrapped.

>+        {
>+            rv[ary[1]] = true;
>+        }
>+    }
>+    return rv;
>+}


>Index: mozilla/extensions/irc/xul/content/prefs.js

>          ["collapseMsgs",       false,    "appearance.misc"],
>+         ["collapseActions",    true,     "appearance.misc"],

Strictly speaking, the pref lists are meant to be alphabetical.


>Index: mozilla/extensions/irc/xul/content/static.js

>     importFromFrame("updateHeader");
>     importFromFrame("setHeaderState");
>     importFromFrame("changeCSS");
>+    importFromFrame("setMotifSettings");

I think we talked about this on IRC; maybe "updateMotifSettings" would be a
better name for this imported function?


>+            // What was the span last time?
>+            lastRowSpan = Number(nickColumns[0].getAttribute("rowspan"));
>+            // Are we the same user as last time?
>+            sameNick = (lastRow.getAttribute("msg-user") ==
>                         thisUserCol.parentNode.getAttribute("msg-user"));
>+            // Do we have the same destination as last time?
>+            sameDest = (lastRow.getAttribute("msg-dest") ==
>+                        thisUserCol.parentNode.getAttribute("msg-dest"));
>+            // Is this message the same type as the last one?
>+            haveSameType = (lastRow.getAttribute("msg-type") ==
>+                            thisUserCol.parentNode.getAttribute("msg-type"));
>+            // Is either of the messages an action? We may not want to collapse
>+            // depending on the collapseActions pref
>+            isAction = ((thisUserCol.parentNode.getAttribute("msg-type") ==
>+                        "ACTION") || 
>+                        lastRow.getAttribute("msg-type") == "ACTION");

|thisUserCol.parentNode| is used a lot here, maybe worth keeping it in a local?
Also, you wrapped one == but not the other again. :)

>+            // Do we collapse actions?
>+            collapseActions = source.prefs["collapseActions"];
>+
>+            // Does the motif collapse everything, regardless of type?
>+            // NOTE: the collapseActions pref can override this for actions
>+            needSameType = !source.motifSettings.collapsemore;

Does this get a strict warning if the option does not exist? Also, you said
that it /existing/ was what mattered, not its value.


>Index: mozilla/extensions/irc/xul/locale/en-US/chatzilla.properties

>+pref.collapseActions.label      = Also collapse actions (only takes effect when "Collapse messages" is turned on)
>+pref.collapseActions.help       = Makes multiple actions from one person only show their nickname against the first, which can look cleaner than having the nickname repeated.

That label looks long and cumbersome. Could you come up with something shorter

Comment 18

[:Gijs (he/him)]

Attachment: [checked-in] Patch v2, attempt to take care of Silver's comments [UNTESTED]

Patch v2, attempt to address earlier comment by Silver.

inobj == thisUserCol.parentNode, so I used that instead of creating another
intermediate :-)

Comment 19

[James Ross]

Comment on attachment 193927 [details] [diff] [review]
[checked-in] Patch v2, attempt to take care of Silver's comments [UNTESTED]

>+            // Does the motif collapse everything, regardless of type?
>+            // NOTE: the collapseActions pref can override this for actions
>+            needSameType = ("collapsemore" in source.motifSettings);

This test is now backwards, is it not? (having 'collapsemore' should mean you
DON'T need the same type)

r=silver with that corrected

Comment 20

[James Ross]

Checked in --> FIXED.

Some motifs will need updating for this.

Comment 21

[:Gijs (he/him)]

Attachment: Patch for faces script

Patch for rginda's faces script so it continues to have the same behaviour as
it has always had. I'm not requesting review because this change doesn't go
into the tree, but I daresay rginda will want to have a look over it seeing as
I don't know Perl ;-). It worked fine locally, however :-).

Comment 22

[James Ross]

Attachment: Updated facesrv for the new motif settings

rginda, I think this should all work (had a little issue making the thing at
all), and it also fixes the "border: 1px inherit solid;" to be three separate
properties, so it does not a) get ignored in Gecko, and b) cause the CSS
validator to have a fit.

Comment 23

[[e-mail]]

(In reply to comment #22)
>"border: 1px inherit solid;"
To my untrained eye that should be "border: 1px solid inherit;"

Comment 24

[James Ross]

According to the CSS spec, the following is true about 'border':
 - The order is not important.
 - The entire property may be 'inherit'.
 - Neither the border width nor the border style may be 'inherit'.
 - The border colour may be 'inherit'.

So, firstly, swapping the order should not affect it (and I believe Gecko still
rejects it, and the CSS validator still has a fit - though the size of its fit
depends on the order). Secondly, the value of "1px inherit solid" is valid.

Whether it is a bug in the CSS spec or not, I don't really care - putting
inherit anywhere makes Gecko ignore it and the CSS validator break, so splitting
it into the three components is the only solution.

Comment 25

[:Gijs (he/him)]

What's the reason this bug isn't closed as FIXED again? :-)

Comment 26

[James Ross]

Waiting for the update to the faces motif to complete the fix.

Comment 27

[:Gijs (he/him)]

Attachment: Patch to account for source.motifSettings not existing in certain situations

A case that wouldn't happen quite often, but is still possible. Apologies for a late fix (knew of this since saturday), I was away for the weekend.

Comment 28

[James Ross]

Comment on attachment 200548 [details] [diff] [review]
Patch to account for source.motifSettings not existing in certain situations

>-            needSameType = !("collapsemore" in source.motifSettings);
>+            needSameType = !(("motifSettings" in source) && source.motifSettings
>+                           && ("collapsemore" in source.motifSettings));

According to style rules, it should be:

            needSameType = !(("motifSettings" in source) && source.motifSettings &&
                             ("collapsemore" in source.motifSettings));

Otherwise, r=silver

Comment 29

[Samuel Sieb]

checked in