718279 - Collapsed Thread lines colored according to tags inside

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jan]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

I updated Thunderbird (do not know from which version) to 9.0.1. 


Actual results:

Suddenly, collapsed threads are colored according to the tags of the messages they contain. i.e. if I have a thread which contains a tagged message (but the first message is not tagged), the thread will show in the tags color when collapsed. When expanded, the first message is correctly shown in black.


Expected results:

The thread should stay black even if it is collapsed. I use tags with filters to highlight own messages in mailing lists and to color troll posts in a light grey. This change means that threads look like they are posted by a troll just because one of them posted somewhere inside.

If this behaviour is intentional, there needs to be a way to disable this "feature", and it should be possible to find it...

Comment 1

[Hashem Masoud]

> Suddenly, collapsed threads are colored according to the tags of the
> messages they contain. i.e. if I have a thread which contains a tagged
So, before the update you didn't have this issue?

Comment 2

[Jan]

(In reply to Hashem Masoud from comment #1)
> So, before the update you didn't have this issue?


No. Before the update, a collapsed thread was colored only according to the tags of the first message, i.e. if that message had no tag, it was black.

Comment 3

[Hashem Masoud]

(In reply to Jan from comment #2)
This looks to be intentional then. Will have to wait for an answer from developers.

Comment 4

[Ludovic Hirlimann [:Usul]]

confirming on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a2) Gecko/20120118 Thunderbird/11.0a2. But I'm not sure this is a regression. Protz did we change something that would affect this behavior ?

Comment 5

[Jonathan Protzenko [:protz]]

I feel like this has always been the case. Jan, are you sure you were using unthreaded mode before? Also, this really does sound like a matter of personal preference to me, so I don't see a strong reason to change the current behavior (I can see bugs popping up asking to revert the change if we were to do such a change.)

Comment 6

[Jan]

What do you mean by "unthreaded mode"? I was grouping by thread before the update, and I am still grouping by thread (otherwise there would be no collapsed threads, thus no problem).

I installed TB 6.0 portable and tested. I will attach a screenshot showing the comparison between TB 6.0 (which does what I want) and TB 9.0.1 (which behaves differently).

Although it is annoying that an update changes behavior in a way that totaly breaks my highlighting (just because a mail is from a troll does not mean the thread is from a troll), it wouldn't be a problem if there was a setting to disable it. I can understand that some people will prefer this new behavior (one mail in thread marked important -> collapsed thread marked important), so I think the best choice would be to provide a setting for it.

Comment 7

[Jan]

Attachment: Comparison between 9.0.1 (collapsed thread colored according to childs tag) and 6.0 (collapsed thread not colored)

Comment 8

[Mark Banner (:standard8)]

I've been seeing this as well actually. Whilst it could be a matter of personal preference, I'm wondering if we did really mean to change this and what Blake's thoughts are on it.

Comment 9

[Jim Porter (:squib)]

(In reply to Mark Banner (:standard8) from comment #8)
> I've been seeing this as well actually. Whilst it could be a matter of
> personal preference, I'm wondering if we did really mean to change this and
> what Blake's thoughts are on it.

This was intentional; see bug 289411, which has a patch with ui-review from bwinton.

I think it would make sense to disable this when mail.operate_on_msgs_in_collapsed_threads is false though. Alternately, we could make a new pref to disable "rows for collapsed threads show a summary of the entire thread's contents". This will probably become more noticeable to users as we move to doing this sort of thing for all columns; see bug 622779.

Comment 10

[Jan]

I assume that although the change should be trivial (additional if), this will not be fixed until TB 11, right?

Comment 11

[RDL]

Thanks for raising this bug, Jan.

The fix for bug 289411 unfortunately assumed that tags were always used to indicate message priority and therefore in a collapsed thread always allows the lowest numbered tag in the thread to override the expected colouring (whether from another tag or none) on the visible (first) message. However, as already pointed out here, tags can legitimately be used, alternatively, to simply classify messages. To support that usage, all tag names may be amended/chosen by the user.

As further illustration of such usage and the sort of effect the fix for bug 289411 can have, I include here the information request I made in  bug 289411.

I hope that the proposed fix for this bug will provide an answer to my request.

Thank you.

> Please advice on how original behaviour may be obtained where users have
> chosen to use tags and their colours to distinguish eg 'input' and 'output'.
> Originally use of labels to classify mails rather than prioritize them was
> an obvious possibility and nowhere discouraged.
> 
> For me, the current effect of your change is to show colours indicating that
> all collapsed threads which I originated as instead apparently originating
> from a respondent. Changing round my labels would invert the problem but not
> cure it.
> 
> Of course the 'From' field shows the truth but the colour coding up to now
> gave me an immediately visible indicator of 'my' threads and those started
> by others. Keeping the threads expanded to provide the picture leads to a
> confusing picture.
> 
> I really don't want to have to raise a new bug report just to (eventually
> and only perhaps) be able to regain a simple, natural and helpful technique.
> 
> Is there a preference to prevent the new behaviour, or can you suggest an
> alternative way to obtain the previous picture?

Comment 12

[Magnus Melin [:mkmelin]]

I think ideally whether or not tags apply to the whole thread should be a per tag setting.

Comment 13

[Jan]

(In reply to Magnus Melin from comment #12)
> I think ideally whether or not tags apply to the whole thread should be a
> per tag setting.

That sounds like a good idea. However, I (and I am sure most other affected users) would prefer having something that allows reverting to the old behaviour now to something that is perfect in 6 months. So please do a quick fix (hidden setting to disable this/binding to operate_on_msgs_in_collapsed_threads) as soon as possible, and then start debating and preparing a perfect solution.

Comment 14

[Jan]

Is there any progress happening on this, e.g. is someone implementing the fix suggested by Jim in comment 9?

Comment 15

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jan from comment #14)
> Is there any progress happening on this, e.g. is someone implementing the
> fix suggested by Jim in comment 9?

Not that I know of.  Would you like to give it a try?

Thanks,
Blake.

Comment 16

[Jan]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #15)
>
> Not that I know of.  Would you like to give it a try?

Nope, sorry - I have zero experience with the Mozilla development process and don't have a build environment. I just want to know if I have to delete my filters and live with the trolls or if there is any hope of this being fixed in the next 2-3 months.

I am pretty sure that the only required change is an additional condition to the if-statement "if (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay)". See the patch https://bug289411.bugzilla.mozilla.org/attachment.cgi?id=556117 for the changes that introduced the unwanted behaviour.

Comment 17

[Jim Porter (:squib)]

(In reply to Jan from comment #14)
> Is there any progress happening on this, e.g. is someone implementing the
> fix suggested by Jim in comment 9?

If someone In The Know (e.g. bwinton, standard8, bienvenu, or protz) picks one of the two options I posted in comment 9, I'll do it (though probably not for a week or so).

Comment 19

[Maxim Makarenko]

Attachment: Graphical explanation of the issue

Comment 20

[Maxim Makarenko]

Ian, you may install TB 8.0. The difference in functionality is not as big, but the version has the correct behavior. 

It looks like I will have to use 8.0 till the bug is fixed because I track status of tickets from Jira using the tags and wrong color of collapsed threads gives me wrong information about status of the ticket.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

I don't often say this, but while we're waiting for the better solution Magnus proposed in comment 12, I would be fine with a hidden pref to let people revert this.

Later,
Blake.

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to Jim Porter (:squib) from comment #9)
> 
> This was intentional; see bug 289411, which has a patch with ui-review from
> bwinton.
> 
> I think it would make sense to disable this when
> mail.operate_on_msgs_in_collapsed_threads is false though. 

I'd rather see the alternate, new pref, then the above.  I use  mail.operate_on_msgs_in_collapsed_threads=false and would rather not see other features conflated into it.

> Alternately, we
> could make a new pref to disable "rows for collapsed threads show a summary
> of the entire thread's contents". This will probably become more noticeable
> to users as we move to doing this sort of thing for all columns; see bug
> 622779.

Comment 23

[Maxim Makarenko]

I would like to remind about the issue which is still open for 14 months now. I personally still has to use 8.0 version of Thunderbird, this is ridiculous...

Comment 24

[Andrew Buehler]

Attachment: thunderbird-collapsedthread-tagcolors-v1.diff

Since I lack the ability - or at least, in the short term, the constitutional fortitude - to implement the solution suggested in comment 12, here's a patch which should add a hidden pref for this behavior.

I'm not at all attached to (or really satisfied with) the pref name - better suggestions are welcome - but it's at least vaguely descriptive. I chose it with an eye to what someone searching for terms related to this behavior might use as search terms; there are probably better names even for that goal. The internal variable name is more descriptive of the objective behavior.

This is not yet even build-tested, because I have a horribly inconvenient build and test environment, but I've been over the code several times and I at least don't see any errors. I'll get to testing it eventually if (as seems likely) no one beats me to it.

Requesting review from :bwinton, as someone who's ACKed the idea of such a patch already, and who is currently listed as having an empty review queue. If someone else would be a better choice, feel free to transfer the review request over.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8478037 [details] [diff] [review]
thunderbird-collapsedthread-tagcolors-v1.diff

I'm _really_ not qualified to review this, so I'm going to redirect to Magnus, since he was suggested by the link, and has left the most relevant comment in the bug.  ;)

Comment 26

[Andrew Buehler]

Acknowledged, and sorry to have bothered you with it, in that case.

Though I do wonder why the "suggested reviewers" drop-down listed you as the second of three candidates, right behind :mconley, and IIRC didn't list Magnus at all...

Comment 27

[Blake Winton (:bwinton) (:☕️)]

No worries!  I suspect I was listed because I touched the code back in the day, and have a really small review queue.  ;)

I also suspect the code that runs is based off of https://github.com/bwinton/Mozilla-Tools/blob/master/getReviewer.py so you could probably see what it's doing, if you were interested.

Comment 28

[Andrew Buehler]

I have a copy of that locally, and when I pass that patch to it, it suggests a nearly completely different list of reviewer candidates; it lists neil, bienvenu, and standard8 at the top (with 14, 13 and 13 respectively), mconley near the bottom (with 2), and you only in the section for ui-reviewer candidates - not in the one for reviewer candidates at all.

So if the code that runs is based off of that, it apparently either is using a rather different data source, or has diverged so far that examining that would probably not be useful for figuring out what's going on.

I'd be interested to keep discussing that if anyone else is, but it's certainly not on topic for this bug.

Comment 29

[Andrew Buehler]

Attachment: thunderbird-collapsedthread-tagcolors-v3.diff

The previous patch didn't build; this one does, although the overall build still fails on my machine due to bug 890877, so the patch isn't run-tested yet.

Comment 30

[Andrew Buehler]

This is now tested - in a newsgroup, not with a mail thread, but it should be the same either way - and appears to work perfectly. After the pref is toggled (in either direction), the color of any given collapsed thread doesn't change until mouseover of that thread with the window in focus, but that shouldn't be much of an issue.

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 8481777 [details] [diff] [review]
thunderbird-collapsedthread-tagcolors-v3.diff

Review of attachment 8481777 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review lag here. It seems to work, and could be useful, thx!
Looks good to me, with a few nits. I'm not a mailnews/ reviewer though, so only f+

::: mailnews/base/src/nsMsgDBView.cpp
@@ +1278,5 @@
>      return NS_MSG_INVALID_DBVIEW_INDEX;
>    }
>  
>    nsCString keywordProperty;
> +  bool mCascadeKeywordsUp = true;

variable names starting with m are reserved for class _m_embers

@@ +1280,5 @@
>  
>    nsCString keywordProperty;
> +  bool mCascadeKeywordsUp = true;
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  prefs->GetBoolPref("mailnews.color_collapsed_threads_like_replies",

You should add this pref (with a default value and documentation) in mailnews.js

Comment 32

[Andrew Buehler]

Attachment: thunderbird-collapsedthread-tagcolors-v4.diff

While addressing your comments, I noticed an apparently better way to do this, which keeps the relevant code all in one central place rather than duplicating it in two. It builds fine (after the bustage fix from yesterday), and seems to work exactly the same way the previous version did; that approach is in the revised patch.

> Sorry for the review lag here. It seems to work, and could be useful, thx!
> Looks good to me, with a few nits. I'm not a mailnews/ reviewer though, so
> only f+

Requesting review from :neil, then, as a known-good fallback - despite his relatively huge review-queue backlog. I was hoping that this could be addressed among the people who had already discussed it back nearer the time when it was filed, but if not, then so be it.

> variable names starting with m are reserved for class _m_embers

Ah, I'd missed that nuance. Changed.

> You should add this pref (with a default value and documentation) in
> mailnews.js

There was a reason why I hadn't done that, but I don't recall clearly what it was, and none of the possible rationales I can think of seem to make sense at this point. Done.

No suggestions for an alternate pref name? I'm really not all that happy with this one, but I haven't been able to think of anything that seems obviously better...

Comment 33

[[e-mail]]

Comment on attachment 8497448 [details] [diff] [review]
thunderbird-collapsedthread-tagcolors-v4.diff

>+  prefs->GetBoolPref("mailnews.color_collapsed_threads_like_replies",
Suggestion: mailnews.display_tags_from_collapsed_threads

>+  if ((m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay ) &&
Nit: extraneous space before )

P.S. Haven't actually tested this assuming mkmelin's f+ covers that.

P.P.S. That nested conditional is gnarly but I think that simplification should be done in a separate bug.

Comment 34

[Andrew Buehler]

Attachment: thunderbird-collapsedthread-tagcolors-v5.diff

(In reply to [e-mail] from comment #33)

> Suggestion: mailnews.display_tags_from_collapsed_threads

I went with mailnews.display_reply_tag_colors_for_collapsed_threads. That includes "reply", "color", "collapsed", and "thread", as well as "tag" (though not "tags"), which is pretty much all the keywords I think someone trying to find this behavior might search for; it also accurately describes the behavior. It's not a perfect name, but I think it's better.

> Nit: extraneous space before )

Fixed. Not quite sure how that snuck in there.

> P.S. Haven't actually tested this assuming mkmelin's f+ covers that.

I've tested all versions, and I see no functionality difference between the one with the f+ and the one you approved. On that basis, setting checkin-needed with your r+ and his f+ for the updated patch. As always, this comes with the disclaimer that anyone may unset it if I've jumped the gun or overstepped my bounds.

> P.P.S. That nested conditional is gnarly but I think that simplification
> should be done in a separate bug.

Agreed on both counts.

Comment 35

[Andrew Buehler]

(In reply to [e-mail] from comment #33)

> P.P.S. That nested conditional is gnarly but I think that simplification
> should be done in a separate bug.

I have a candidate patch for that now, but I'm not sure which component to file the bug against (MailNews Core?); I've never filed a Mozilla bug that wasn't about a user-visible behavior. If you want to file it and assign it to me, I can take it up from there, or I'd be glad to learn appropriate procedures.

Comment 36

[Andrew Buehler]

Ping?

It's been nearly four weeks since the revised post-review patch was posted and checkin-needed was added, with no apparent action. Is there any known reason for the delay?

Comment 37

[Kent James (:rkent)]

(In reply to Andrew Buehler from comment #36)
> Ping?
> 
> It's been nearly four weeks since the revised post-review patch was posted
> and checkin-needed was added, with no apparent action. Is there any known
> reason for the delay?

No, it's just that we have had fewer people doing checkin-needed recently. No other reason.

Comment 38

[Andrew Buehler]

Okay. It never took more than two or three days from checkin-needed to checkin for any of my previous accepted patches, so well over three weeks seemed sufficiently anomalous to be worth asking about.

Since there's no deadline on this nearer than TB38 (if that, since this is only a nice-to-have option), there is of course no rush. I just wanted to make sure it hadn't slipped through the cracks, possibly because of a procedural mistake on my part.

Comment 39

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/fab1a84ce70c -> FIXED
(the tree is finally in a bit better shape!)