Closed Bug 11054 (JTK) Opened 25 years ago Closed 16 years ago

Ignore (kill) a Subthread (branch: not the whole thread) (Troll)

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: sspitzer, Assigned: jcranmer)

References

Details

Attachments

(6 files, 12 obsolete files)

2.40 KB, application/octet-stream
Details
39.40 KB, patch
jcranmer
: review+
jcranmer
: superreview+
Details | Diff | Splinter Review
1.14 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
2.18 KB, patch
Details | Diff | Splinter Review
34.79 KB, patch
neil
: review+
clarkbw
: ui-review+
Details | Diff | Splinter Review
1.42 KB, application/zip
Details
Whiteboard: HELP WANTED
Target Milestone: M15
Bulk-resolving requests for enhancement as "later" to get them off the Seamonkey
bug tracking radar. Even though these bugs are not "open" in bugzilla, we
welcome fixes and improvements in these areas at any time. Mail/news RFEs
continue to be tracked on http://www.mozilla.org/mailnews/jobs.html
Reopen mail/news HELP WANTED bugs and reassign to nobody@mozilla.org
Keywords: helpwanted
Summary: [HELP WANTED]Ignore a branch of a thread, not just the whole thread. → Ignore a branch of a thread, not just the whole thread.
Whiteboard: HELP WANTED
Target Milestone: M15
QA-assigned to myself.
QA Contact: lchiang → fenella
QA Contact: fenella → laurel
*** Bug 208137 has been marked as a duplicate of this bug. ***
Adding portions of comments from duped bug because they effectively illistrate
the value of this bug:

For trolls in newsgroups it would be *very* handy to be have the ability to not
only filter out the troll, but also any replies to the troll, replies to the
replies, etc, ...

Currently you can only kill either the troll, or entire threads that the troll
posts to.  However, if the troll interjects his comments into the middle of a
good thread you are royally screwed because filtering out only his comments
doesn't filter out the idiots (AKA troll feeders) that reply to him, yet the
only other option (killing the thread) is overkill because other branches of the
thread could contain worthwhile content.

1. Add keyboard shortcut: SHIFT-k

2. Menu location: Message | Ignore Thread Branch  <-- below "Ignore Thread"

3. Add to "Message Filters": placed immediately after the "Ignore Thread" option

4. We'll need an icon too. It should be on the post that is SHIFT-k'd (visible
only when "View|Threads|Ignored Threads" IS selected. Otherwise that whole
branch would be hidden).
Severity: normal → enhancement
Summary: Ignore a branch of a thread, not just the whole thread. → Ignore (kill) a Branch of a Thread (not just the whole thread) (Troll)
Bug 207124 would be a more positive solution to the problem.

Gerv
I still prefer this bug for my more immediate needs (troll avoidance). Both bugs
have merits.

That bug is like: "watch this thread's branch", 
This bug is like: "ignore this thread's branch".
Alias: JTK
Theoretically, I guess it would filter by a certain message ID in the
"Message-ID", or "References" headers. So I think this bug should be dependant
on Bug 16913 (Filter news based on any headers).
This bug probably deserves alias "andkon"...
*** Bug 251859 has been marked as a duplicate of this bug. ***
Hi;

My Feature Request got listed as a duplicate of this "bug".

However, looking at the comments that are here I would like to mention some
further points.

Currently, there is the "ignore thread" option in the usenet reader which takes
out the entire tree of messages.....the troll, replies, to the troll, and all of
the good messages that maybe in the tree.

It would be nice to be able to have the "ignore branch" feature that would only
take out a branch of messages in a tree authored by the troll.

That would take out the troll's message, messages replying to his message(
feeding the troll ), but it would leave the good messages the user would want to
read alone.


Product: Browser → Seamonkey
Updating summary for searchability.

Does this bug actually belong in the front end, or is the backend work required 
enough to make it a Core bug?  I ask because there has been at least one RFE 
against Thunderbird for the same feature.

Where did the "JTK" alias come from?  (I know *who*, I don't know *why* or what 
it means.)
Summary: Ignore (kill) a Branch of a Thread (not just the whole thread) (Troll) → Ignore (kill) a Subthread (branch: not the whole thread) (Troll)
yes, backend work would be required.
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core
*** Bug 277696 has been marked as a duplicate of this bug. ***
it appears to be a reference to trolling in mozilla.org newsgroups
<http://216.239.63.104/search?q=cache:so_nOiCStb0J:www.geocrawler.com/mail/msg.php3%3Fmsg_id%3D6296838+subthread+jtk&hl=en>

i'm inclined to remove it unless someone feels that it's really meaningful and
defends it before the next time i encounter this bug. there are about 3 google
hits for this which is fairly poor if you want to claim something is meaningful.
(In reply to comment #15)
> i'm inclined to remove it 

I don't understand -- remove what from where?  If you are somehow suggesting 
this RFE's feature is not useful, I disagree -- I think this feature would be 
very useful on the newsgroups.
the alias.
Hi;

I am one of the people who filed a bug report on this in July of last year.

I was curious to know if it is fixed, or being worked on.

If I can help, let me know

Would an offer of a few hundred $$ USD to the committer of the implementation 
of this feature help move this RFE along?

I want to combine this with the killfile/message filters, so that anytime 
a person in my "killfile" replies to a thread, that message and all followups
to that message (that is, the subthread rooted in that message) present and future, are killed.

trn was doing this over a decade ago.  This RFE itself is now ~7 years old.
Maybe some dead presidents would help move it along?
I'd like to contribute money too.

Another thing: how about posting the assignment on elance.com or something? Maybe there will be some takers there.
I'll kick down $100 upon successful completion of this function.


Bob
Does the scope of this bug include marking a subthread as read one-time, or is that a separate issue?
(In reply to comment #8)
> Theoretically, I guess it would filter by a certain message ID in the
> "Message-ID", or "References" headers. So I think this bug should be dependant
> on Bug 16913 (Filter news based on any headers).

This really describes an *action* which could be triggered by a filter test, but is useful in its own right.  The ability to filter on more headers header fields would make things even better, but this bug in no way depends on the also highly desired bug 16913.

(In reply to comment #22)
> Does the scope of this bug include marking a subthread as read one-time, or is
> that a separate issue?

I guess it should should do whatever Ignore thread does.  (Ignore thread is just a special case of this...?)  

Since it has not been mentioned here, I'll add that another important use of the ability to ignore a subthread is dealing with long threads that sprout off topic branches.  The ability to manually prune away branches of the thread that wander off a topic that one wishes to follow would be valuable.

This and better filtering (bug 16913!) would go a long way towards making SM & TB much more capable usenet readers.  :)  
> Does the scope of this bug include marking a subthread as read one-time, or is
> that a separate issue?

Separate issue, far easier to solve - it'd require "only" some recursive "mark as read"s.

(In reply to comment #5)
>..
> 
> 1. Add keyboard shortcut: SHIFT-k

alternatives to shift+k:
Prune (but "P" is taken)
amputate "A"
excise "E" or "X"
lop "L

related bug 311642
Can we Prune, Amputate, Excise or Lop the word Troll from the bug summary?
Please?  
While we're at it, Prune would be better than ignore or kill in the summary.
(In reply to comment #27)
> Can we Prune, Amputate, Excise or Lop the word Troll from the bug summary?

Why? See comment #5

> While we're at it, Prune would be better than ignore or kill in the summary.

I suspect that different English-speaking cultures (American vs. British) use different terms for this. If you can verify that there is a large group of users that use "prune", then we could ADD it to the summary.
Just ½ a cents worth on the words prune and ignore. Prune might be misunderstood to mean something will be deleted from the NG, as in the NG *server*. Ignore isn't as easily misunderstood that way.

See e. g.:
https://bugzilla.mozilla.org/show_bug.cgi?id=311642
Could someone please add "See comment #5 for this bug's description" to the Whiteboard?
Whiteboard: see comment #5 for a detailed description
I very much like the "prune" suggestion for the name of this function and I don't feel it would be misunderstood as "delete" any more than "kill" is.

I'd sure love to see someone take this on.  I don't have the expertise.

Question: could this function be implemented as an add-on or do parts of it require base code modification?
At a cursory glance of the source code, ignoring is implemented at the thread level (i.e., the thread itself is ignored). However, message-level ignoring does exist but all instances are immediately transformed into thread-level ignoring. It appears that the message-level ignoring may be (?) necessary to work properly.

(Therefore, in reply to comment 31, it cannot be implemented as an add-on easily.)
Requirements for back-end, as I see it:
1. Create a new flag (MSG_FLAG_PRUNED is my vote) for nsMsgHdr and possibly nsMsgThread (for performance reasons).
2. Wherever MSG_FLAG_IGNORED is used for threads, MSG_FLAG_PRUNED should be used at message level.
3. Where nsMsgHdr's are pushed/pulled into/from trees, MSG_FLAG_PRUNED attributes should be properly propagated.

In addition, the front-end issues from comment 5, included here for completeness, would also need to be implemented.
4. Add keyboard shortcut: SHIFT-k
5. Menu location: Message | Ignore Thread Branch  <-- below "Ignore Thread"
6. Add to "Message Filters": placed immediately after the "Ignore Thread" option
7. Make an icon. It should be on the post that is SHIFT-k'd (visible
only when "View|Threads|Ignored Threads" IS selected. Otherwise that whole
branch would be hidden).

My vote is also for prune as a name for this action.

I would work on this but a) I'm no good with frontend and b) I'm struggling to get TB to compile.
I forgot to mention that the backend code seems to be all in mozilla/mailnews/db/msgdb.
Attached patch Partial implementation (obsolete) — Splinter Review
Known problems:
1. No support for SeaMonkey front-end.
2. The icon for the pruned thread doesn't show up. Not tested enough to know why
3. Marking a thread as pruned marks the thread as read as opposed to the sub-thread.
4. The check as to whether or not the thread is pruned should probably cache the result.
5. Inconsistent tabulation problems (vim doesn't accept the emacs string at the top).
6. Filters not tested.

To create the prune icon, I took the ignored icon, cut it in half, moved it to the bottom-right corner and overlayed that over the thread icon. The end result is that it sort of looks like the bottom part is ignored but the others are not.
Nice to see someone finally working on that! :-)

Some comments, though:
- The Mozilla word for hiding a thread is "killing a thread". Calling killing a subthread "pruning" is just ... confusing.
- I wonder if the actual distinction between killing a thread (=marking the thread) and killing a subthread (=marking all messages) is something we need to keep. Killing the thread is equivalent to killing the subthread which begins with the thread's first posting. Checking the "thread killed?" state is equivalent to checking the first message's "message killed?" state etc., so filter perf shouldn't be an issue. (I'm not sure we should tag all subthread messages as killed, and you don't seem to do that anyway).
Attached image Concept for the pruned image (obsolete) —
This is my concept icon for the thread-pruned image. Happily accepting any other icons, as long as I don't have to make them myself!

In other news, I figured out why the CSS wasn't working. My changes were in the pinstripe theme but the qute theme was being used. I now officially hate double negatives.
I'm not crazy about using a very limited resource (msg flags) for a feature that I don't think is going to be used very much in general. It would be nicer just to use a msg header property, and live with the fact that rebuilding the index would lose the prunedness.
> a feature that I don't think is going to be used very much in general.

Well, maybe not in general, but it's a killer feature for newsreading.

> It would be nicer just to use a msg header property, and live with the 
> fact that rebuilding the index would lose the prunedness.

Hm, that's far from ideal, but losing newsgroup msf is already havoc anyway. :|

I'm actually more concerned about having two thread killing mechanisms in parallel, which I don't think is necessary.
David, I suspect you underestimate the popularity of pruning in news groups.
I predict this will really catch on.  
Even so, it takes up a flag for all kinds of messages, imap, local, rss, not just news. And the feature is easily implementable w/o using a msg flag. And, I'm an idiot for not realizing til now that deleting the .msf file is going to lose the pruned flag for newsgroups anyway, even if it is stored as a flag and not a property, because we only persist flags for local folders, and imap messages (if the server supports that).
I think I can get rid of the pruned flag and use the MSG_FLAG_IGNORED on messages.

In addition, I think that killing a thread could be changed into pruning on the root.

I'll look into purging the pruned flag this afternoon while I can test the filtering action.
> I think I can get rid of the pruned flag and use the MSG_FLAG_IGNORED on
> messages. In addition, I think that killing a thread could be changed into 
> pruning on the root.

Yes, that's what I meant above: we could morph the current "kill thread" into "kill subthread" and change the working of "K" to "kill sub-thread of thread starter message".

BTW: When looking into our code, I found that we #define the message flags both in nsMsgMessageFlags.h and in MailNewsTypes.h. This is confusing at best.
re changing K to just kill sub-thread, I think that's going to make people unhappy - up til now, that's killed the whole thread. With that proposed change, you'd need to select the root message in the thread and press K in order to kill the whole thread, which pretty much defeats the purpose of having a single key to kill a whole thread.
> re changing K to just kill sub-thread, I think that's going to make people
> unhappy

I didn't propose that. ;-)
K would still kill the thread, but not because it kills "the thread", but because it kills the subthread starting at the thread's first posting.

Changing K would indeed make many people unhappy, including me.

I'd propose Shift-K for killing just the subthread (which would effectively be the same at the start posting).
ah, sorry I misunderstood. That sounds fine.
Attached patch Almost complete implementation (obsolete) — Splinter Review
Nearly complete, except that there are no SeaMonkey components (I think...)

Requesting that someone with SeaMonkey experience point me to which files in mailnews/ would correspond to mail/

Now to see if I can get rid of MSG_FLAG_PRUNED...
Attachment #278651 - Attachment is obsolete: true
the file names tend to be the same between mail and mailnews, just the directories are different. Look in mailnews/base/resources/content

The theme stuff is in mozilla/suite/themes, iirc.
Attached image All used images (obsolete) —
The three images I added. In clockwise starting from upper-left, they are:
mail/themes/pinstripe/mail/icons/thread-closed-prune.png
mail/themes/pinstripe/mail/icons/thread-new-closed-prune.png
[ Blank space ]
mail/themes/qute/mail/icons/thread-pruned.png

Each icon is 16x16.
Attachment #278859 - Attachment is obsolete: true
Attachment #279000 - Attachment is patch: false
Attachment #279000 - Attachment mime type: text/plain → image/png
What is the difference between the top two icons ?  They look identical to me. Given that the most common color perception defect is red/green, icon coloring shifts should be distinctive enough to be detectable.

 
Their names. I took the ignored icons as the basis. Since there were two, one with the -new- and one without, I thought that the pruned variant might as well have two in addition. So the correct question, "what is the difference between the two ignored icons in the pinstripe theme?"
I understand your reasoning, see it in the Sky Pilot theme too.  I do think the design concept is clear, the 'Ball' denotes the pruning point.
I find this icon difficult to see.  The features are too small.
I'd like to suggest: less vine and bigger berry. 
Or even, less vine, and scissors (pruning shears).

  |               |
  +--+            |    \ /
     |      -->   +---- X
     +--          |    / \
     |    _       |
     +-- (_)      +----
Attached patch Working + Tested implementation (obsolete) — Splinter Review
This is a working implementation that has been tested, but is not yet ready to be committed onto the tree for the following reasons:
1. No changes to seamonkey frontend (I don't have suite/ on my build tree... yet)
2. I have some cleaning up to do.
3. I would like to cache the headers, but that mayn't be possible...
Attachment #278985 - Attachment is obsolete: true
as I said in my newsgroup post:

   I would not add a member variable  m_pruned to every nsMsgHdr - it's equivalent to m_flags & MSG_FLAG_IGNORED, and there's no sense in making every nsMsgHdr object bigger to cache this value. I think it's also complicating your code, so changing that might simplify things. Setting a msg hdr as pruned should just set the MSG_FLAG_IGNORED flag, and checking if it's pruned should just check  MSG_FLAG_IGNORED.
do we really think people are going to define filters to prune sub-threads? Or did you just do that to match what you can do with ignore?
Personally, I would ignore sub-threads in a filter if I had the chance. Trolls have this nasty habit of attaching onto important threads to start flame wars.

Re my current work:
1. I have giving up on trying to cache the pruned status. The process of invalidation would be as expensive as just checking each time, and trying to avoid that would require really mucking up nsIMsgDBHdr or just make GetIsPruned awkward (GetIsPruned(PRBool invalidate, PRBool* isPruned) ? )
2. Things work but with some potential long times for long threads -- marking a subthread as ignored takes O(N^2) where N is the number of posts in the thread. Caching would bring it down O(N) but see 1 for that part.
3. Yes, pruning is basically put everywhere ignoring is for parity.
I guess the prune sub-thread option would only appear for newsgroups, like ignore thread. So you'd prune sub-threads based on the author, I guess, in your filter.

if you prune a sub-thread, are you going to remove it from the view, like we do for killed threads? Or just mark all the messages read?
> I guess the prune sub-thread option would only appear for newsgroups, like
> ignore thread.

Actually, I'd like to have both for mailing lists, too, but that's (of course) not in the scope of this bug. 

> if you prune a sub-thread, are you going to remove it from the view, 
> like we do for killed threads? Or just mark all the messages read?

I'd vote for 'remove'.
(Having eg. Shift-T do "mark subthread read" would be another goody not part of this bug here *g*)
In reply to comment 55:
I guarantee you I will define filters to prune sub-threads in newsgroups.
Attached patch Full, tested, working patch (obsolete) — Splinter Review
Fully working, except that SeaMonkey front-end is not supported.

In response to comment 57:
Pruning is pretty much killing except for subthreads only.

In response to comment 58:
When going through the killing threads portion, it appears to be implemented for POP and IMAP servers in the back-end but not the front-end.
Attachment #279151 - Attachment is obsolete: true
Attachment #279169 - Flags: superreview?(mscott)
Attachment #279169 - Flags: review?(bienvenu)
Request that when this lands it is to both Tb 2 branch and the trunk.  We will gain real world feedback earlier. I suggest Tb 2.0.0.8 to get themes on board.
Won't be in tb2 for, but I sure look forward to trying it out on trunk!
Assignee: nobody → Pidgeot18
Keywords: helpwanted
Status: NEW → ASSIGNED
+            if (msgHdr)
+            {
+              m_curFolderDB->MarkHeaderPruned(msgHdr, PR_TRUE, nsnull);
+            }

don't need extra braces here

same here:

+  if (bIgnored)
+  {
+    msgFlags |= MSG_FLAG_IGNORED;
+  }
+  else
+    msgFlags &= ~MSG_FLAG_IGNORED;

Other than that, it looks OK - I haven't been able to apply and run with this patch, yet, though.
Comment on attachment 279169 [details] [diff] [review]
Full, tested, working patch

this looks OK to me, thx for the patch!
Attachment #279169 - Flags: review?(bienvenu) → review+
Attached file The current images
Still actively soliciting PNG images from anyone with any degree of skill making icons. A number of people have expressed approval for Nelson's design in comment 52.

The proper file paths for the images (only the last part of the filenames were retained in the zip) are:
mail/themes/pinstripe/mail/icons/thread-closed-prune.png
mail/themes/pinstripe/mail/icons/thread-new-closed-prune.png
mail/themes/qute/mail/icons/thread-pruned.png
Attachment #279000 - Attachment is obsolete: true
I've never attempted to patch anything before, but my interest in this one is such that I'm motivated to make this a learning experience.  ( <http://groups.google.com/group/mozilla.dev.apps.seamonkey/browse_thread/thread/2ae0b9b9d4703150/5369b5313aad4ee5> )

Based on conversations with developers in #seamonkey, it looks as if you've done  the core work as well as some of the SM patches too.  The files in mailnews/base/resources/content are suite files. (Once bug 367034 lands, those files will be moved to suite/.)

If I understand correctly, then the only thing remaining is to apply the  mail/locales patches to the corresponding suite/locales files and hit the themes files in suite/themes/classic/messenger and suite/themes/modern/messenger. mailWindow1.css is renamed to threadPane.css in the suite.

With the zip of the image files you posted, I should be able to try this now.  ("Now" being sometime later this week, I hope. :/ )


I must agree, this is going to be a killer newsgroup feature.
Thanks, Joshua!
As a user (NOT developer) of Firefox must I await the appearance of this feature in an official release?  I haven't a clue about applying or working with patches but if there is some simple thing I, as a naive user, can do I'd really appreciate a specification of the steps needed to use it.

There are a couple of very noisy, troll infested newsgroups I'm in (e.g. sci.physics) where this function would be hugely popular and, if it's not premature, I'd love to announce its availability and how to get it.


Many Thanks,

Bob (One of the original requesters for this function a couple of years ago.)
In answer to recent comments from people eager to give this a try:
At some point soon, Joshua's patch will be good enough that he will 
request that it be reviewed by other mailnews (Thunderbird) developers.  
When it has been positively reviewed, the patch will be committed to the
"trunk", from which new new advanced "alpha" builds of the next release 
are built nightly.  At that point, no-one will need to know anything 
about applying patches or building Thunderbird to try this out.  People
will only need to download the latest "nightly trunk" build to try it.

Of course, that "nightly trunk" build will have MANY new features and 
changes in it, any of which may be in a not-yet-final possibly-pretty-buggy
state.  So testing of nightly trunk builds is not for the faint of heart,
and is recommended only for those who backup their "profiles" regularly.
But I use the trunk builds every day, and intend to jump on this feature
as soon as it's in a nightly build.  My offer of comment 19 still stands.
(In reply to comment #68)
>
> My offer of comment 19 still stands.
> 

As does mine of comment 21.  To whom and how?


Bob
(In reply to comment #68)
> At that point, no-one will need to know anything about applying
> patches or building Thunderbird to try this out.

Bob Cain could be in a hurry, but Rich Gray is talk about SeaMonkey here, not TB.

He wants to make it possible to make it fully available on SM the same time that it's available to TB.

Probably all of backend apply easily to both, but there is some work to do that only can be done testing with the backend changes applied.

pinging for sr, for Joshua
Comment on attachment 279169 [details] [diff] [review]
Full, tested, working patch

>Index: mailnews/imap/src/nsImapMailFolder.cpp
>===================================================================
>+        case nsMsgFilterAction::PruneThread:
>+          msgHdr->OrFlags(MSG_FLAG_WATCHED, &newFlags);
>           break;

Should be MSG_FLAG_IGNORED, not MSG_FLAG_WATCHED.

>Index: mailnews/local/src/nsParseMailbox.cpp
>===================================================================
>+      case nsMsgFilterAction::PruneThread:
>+        msgHdr->OrFlags(MSG_FLAG_WATCHED, &newFlags);
>         break;

Same here.

Almost two months and nobody noticed this... I'm glad I was just popping around IMAP and POP filters today!
As suggested by bienvenu over IRC, I split the patch up into a backend portion of code and a frontend for TB. This is the backend portion.

This should have both bienvenu's nits and the fix for the bug I mentioned in my last comment.
Attachment #279169 - Attachment is obsolete: true
Attachment #286613 - Flags: review?(bienvenu)
Attachment #279169 - Flags: superreview?(mscott)
Comment on attachment 286613 [details] [diff] [review]
Patch for the backend portion of the code

thx, Joshua. One nit - the indentation doesn't look right here:

NS_IMETHODIMP nsMsgHdr::GetThreadParent(nsMsgKey *result)
 {
   nsresult res;
   if(!(m_initedValues & THREAD_PARENT_INITED))
   {
+  if (!(m_initedValues & FLAGS_INITED))
+    InitFlags();
Attachment #286613 - Flags: review?(bienvenu) → review+
I know this bug is almost "done", but I'd like to bring up (one last?) argument one the "prune" vs. "ignore" terminology:

Pruning is that act *cutting off* unwanted branches(1). These branches can not be put back on. In Thunderbird, we are not cutting off a sub-thread, we are merely *ignoring* it. Calling this "cutting off" gives the false impression that the sub-thread is being deleted and can not be re-attached. "Ignore" seems to better correlate with what is actually happening. The user will better understand/intuit that he can "stop ignoring" a previously ignored sub-thread.

(1) http://dictionary.cambridge.org/define.asp?key=prune*1+0&dict=A

> killing a thread could be changed into pruning on the root.

Which is the logical consequence of using the term "prune" for sub-threads. Unfortunately, it is even less intuitive for ignoring the root: When someone cuts off the trunk or main branch of a tree, *nobody* would call that "pruning"!

Sorry for bringing this up again, but once these things appear in the nightlies/milestones/releases, they tend to be set in stone (see bug 273008).
Attachment #286613 - Flags: superreview?(neil)
Comment on attachment 286613 [details] [diff] [review]
Patch for the backend portion of the code

>+
>+  const nsMsgNavigationTypeValue toggleThreadPruned = 23;
Nit: extraneous blank line

>             <xul:menuitem label="&ignoreThread.label;" value="ignorethread" enablefornews="true"/>
>+            <xul:menuitem label="&pruneThread.label;" value="prunethread" enablefornews="true"/>
>             <xul:menuitem label="&watchThread.label;"  value="watchthread" enablefornews="true"/>
Heh, it seems as if there was a vague attempt at alignment here

>+      PRBool pruned;
>+      msgHdr->GetIsPruned(&pruned);
>+      if (pruned && !(m_viewFlags & nsMsgViewFlagsType::kShowIgnored))
>+        continue;
GetIsPruned is comparatively expensive - mightn't it be better to test the flag first, and not check for pruning when we're showing ignored messages?

>+  // Ignored state is toggled based on the first selected thread
s/thread/message/

>+  // Process threads in reverse order
>+  // Otherwise collapsing the threads will invalidate the indices
You're not collapsing threads, so this makes no sense

>+    rv = m_db->GetThreadContainingMsgHdr(header,getter_AddRefs(thread));
Nit: space after comma

>+    for (current=0; current < children; current++)
Nit: spaces around =

>+    for (; current < children; current++)
>+    {
>+       nsCOMPtr <nsIMsgDBHdr> nextHdr;
>+       PRBool isPruned;
>+       
>+       thread->GetChildHdrAt(current, getter_AddRefs(nextHdr));
>+       nextHdr->GetIsPruned(&isPruned);
>+       
>+       // Ideally, the messages should stop processing here.
>+       // However, the children are ordered not by thread...
>+       if (isPruned)
>+         nextHdr->MarkRead(PR_TRUE);
>+    }
This marks the current header read too, right?

> nsresult nsMsgThreadedDBView::AddMsgToThreadNotInView(nsIMsgThread *threadHdr, nsIMsgDBHdr *msgHdr, PRBool ensureListed)
> {
>   nsresult rv = NS_OK;
>   PRUint32 threadFlags;
>+  PRBool msgPruned;
>   threadHdr->GetFlags(&threadFlags);
>-  if (!(threadFlags & MSG_FLAG_IGNORED))
>+  msgHdr->GetIsPruned(&msgPruned);
>+  if (!(threadFlags & MSG_FLAG_IGNORED) && !msgPruned)
>     rv = AddHdr(msgHdr);
>   return rv;
> }
Again, no point testing for a pruned header on an ignored thread (or vice versa... I'm not sure which is cheaper)

>+      if (!parentHdr)
>+        return NS_OK;
>+      parentHdr->GetIsPruned(isPruned);
if (parentHdr)
  return parentHdr->GetIsPruned(isPruned);

>+  PRBool isPruned;
>+  child->GetIsPruned(&isPruned);
>+  if (m_flags & MSG_FLAG_IGNORED && m_mdbDB && isPruned)
>     m_mdbDB->MarkHdrRead(child, PR_TRUE, nsnull);
This will only mark pruned headers in ignored threads!

>+          threadFlags |= MSG_FLAG_IGNORED;
>+          thread->SetFlags(threadFlags);
Nit: could inline this, as you only use it the once
Attached patch Back-end patch, version 2 (obsolete) — Splinter Review
Whoops, 2 front-end files slipped into the first patch!

This version should not have them, and all of Neil's comments should be fixed.
Attachment #286613 - Attachment is obsolete: true
Attachment #287416 - Flags: review?(bienvenu)
Attachment #286613 - Flags: superreview?(neil)
Comment on attachment 287416 [details] [diff] [review]
Back-end patch, version 2

>     // skip ignored threads.
>     if ((threadFlag & MSG_FLAG_IGNORED) && !(m_viewFlags & nsMsgViewFlagsType::kShowIgnored))
>       continue;
>+    
>+    // skip ignored subthreads
>+    nsCOMPtr <nsIMsgDBHdr> msgHdr;
>+    m_db->GetMsgHdrForKey(pKeys[i], getter_AddRefs(msgHdr));
>+    if (!(m_viewFlags & nsMsgViewFlagsType::kShowIgnored))
>+    {
>+      PRBool pruned;
>+      msgHdr->GetIsPruned(&pruned);
>+      if (pruned)
>+        continue;
>+    }
At first I thought "Hmm, can't you move the nsCOMPtr inside the if block?"
Then, I thought "Hmm, can't you move the threadFlag test inside too?"
Or am I overlooking something?

>+  PRBool isPruned;
>+  child->GetIsPruned(&isPruned);
>+  if ((m_flags & MSG_FLAG_IGNORED || isPruned) && m_mdbDB)
>     m_mdbDB->MarkHdrRead(child, PR_TRUE, nsnull);
If I could think of a good way to avoid testing for a pruned header in an ignored thread then I'd suggest it.
Comment on attachment 287416 [details] [diff] [review]
Back-end patch, version 2

isn't pruneThread really killSubthread or ignoreSubthread? I think the code might be more readable with that name change. What do you think? That would make code like this more readable:

+nsresult nsMsgDBView::SetThreadPruned(nsIMsgDBHdr *header, nsMsgViewIndex msgIndex, PRBool ignored)

where it's called SetThreadPruned, but it's really ignoring a subthread, and the boolean is called "ignored".
I'm trying to figure out how to get this bug/patch moving forward again.
It appears to me that David's r+ for attachment 286613 [details] [diff] [review] could and should
be "carried forward" to attachment 287413 [details] [diff] [review], and that attachment should 
move forward to the next step?  Do I understand that correctly?
Still some unanswered/unaddressed review comments in comment 79.
Priority: P3 → --
QA Contact: laurel → backend
(In reply to comment #80)
> I'm trying to figure out how to get this bug/patch moving forward again.

I assure you that I am still working on it. However, the way my patch tree works, I want/need bug 16913 to go through before I can work on this some more.

> It appears to me that David's r+ for attachment 286613 [details] [diff] [review] could and should
> be "carried forward" to attachment 287413 [details] [diff] [review], and that attachment should 
> move forward to the next step?  Do I understand that correctly?

He brought up a point in comment 79 that I should rename some methods, and Neil also had some objections in comment 78 that I haven't had time to look at yet.

I'll finish my bug when my priorities allow me to--16913 comes first in my opinion, and then this bug. Both of these patches being checked in would simplify my tree immensely.
Attached patch Backend patch, version 3 (obsolete) — Splinter Review
Ugh, I've had some serious bitrot since the last version that took way too long to work out.

Anyways, this is an updated patch fixing bienvenu's nit.
Attachment #287416 - Attachment is obsolete: true
Attachment #310036 - Flags: review?(bienvenu)
Attachment #287416 - Flags: review?(bienvenu)
Comment on attachment 310036 [details] [diff] [review]
Backend patch, version 3

this shouldn't be needed, right?

+  if (!(m_initedValues & FLAGS_INITED))
+    InitFlags();
Attached patch Backend patch, version 4 (obsolete) — Splinter Review
Update, yet again. Fixed bienvenu's comment and another problem Neil pointed out to me over IRC.
Attachment #310036 - Attachment is obsolete: true
Attachment #311152 - Flags: review?(bienvenu)
Attachment #310036 - Flags: review?(bienvenu)
Attached patch Real version #4 (obsolete) — Splinter Review
The nsTArray changes is having real bitrotting effects on these patches...
Attachment #311152 - Attachment is obsolete: true
Attachment #311153 - Flags: review?(bienvenu)
Attachment #311152 - Flags: review?(bienvenu)
In light of bug 419143, how confident are you that the new implementation 
of this feature (killing a subthread) works as a filter action?  
(In reply to comment #87)
> In light of bug 419143, how confident are you that the new implementation 
> of this feature (killing a subthread) works as a filter action?  

As I've mentioned on bug 419143 (including here only for those who are not watching that bug), this actually fixes the problem causing that bug. So I am very confident. :)
Comment on attachment 311153 [details] [diff] [review]
Real version #4

thx, Joshua
Attachment #311153 - Flags: review?(bienvenu) → review+
Attachment #311153 - Flags: superreview?(neil)
Comment on attachment 311153 [details] [diff] [review]
Real version #4

>+    readonly attribute boolean isKilled;
Need to bump the IID for this change. sr=me with this fixed.

>   { nsMsgFilterAction::KillThread,      nsMsgFilterType::All,   0,  "Ignore thread"},
>+  { nsMsgFilterAction::KillSubthread,     nsMsgFilterType::All,   0,  "Ignore subthread"},
Odd spacing here.

>+  // Ignored state is toggled based on the first selected message
>+  if (numIndices > 1)
>+    NS_QuickSort(indices, numIndices, sizeof(nsMsgViewIndex), CompareViewIndices, nsnull);
We don't need to do this - I removed most of the other cases already.

>+  // Process messages in reverse order
>+  // Otherwise the indices may be invalidated...
>+  nsMsgViewIndex msgIndex = nsMsgViewIndex_None;
>+  while (numIndices)
>+  {
>+    numIndices--;
>+    if (indices[numIndices] < msgIndex)
>+    {
>+      msgIndex = indices[numIndices];
In the toggle thread killed case, we process them in reverse order because we collapse each killed thread. This invalidates all the indices after the thread index. We therefore retrieve the first index of the thread, and ignore any other indices in the selection that were part of that thread. You, however, don't have to worry about that.

>+          nsCOMPtr < nsIMsgThread > thread;
Nit: nsCOMPtr<nsIMsgThread>
Attachment #311153 - Flags: superreview?(neil) → superreview+
Fixes Niel's nits. r/sr carried over from last patch.
Attachment #311153 - Attachment is obsolete: true
Attachment #313805 - Flags: superreview+
Attachment #313805 - Flags: review+
Keywords: checkin-needed
Whiteboard: see comment #5 for a detailed description → Do not resolve on checkin
Comment on attachment 313805 [details] [diff] [review]
Backend patch, to check in
[Checkin: Comment 92]

{{
2008-04-06 16:31	neil%parkwaycc.co.uk 	...  	Bug 11054 Back end support for ignoring (killing) a subthread (branch) [Troll] p=pidgeot18@gmail.com r=bienvenu sr=me
}}
Attachment #313805 - Attachment description: Backend patch, to check in → Backend patch, to check in [Checkin: Comment 92]
Keywords: checkin-needed
Whiteboard: Do not resolve on checkin
Attached patch Front-end patch, version 1 (obsolete) — Splinter Review
The images needed for this patch are located in attachment 284083 [details].

This is the first version of the frontend patch, per the decision made in IRC and summarized in comment 73.
Attachment #314195 - Flags: superreview?(neil)
Attachment #314195 - Flags: review?
Attachment #314195 - Flags: review? → review?(neil)
Depends on: 428614
Can I confirm: Is this patch intended to a) allow *individual* messages to be selectively deleted from the local message store (i.e. just as you already can with POP3), or b) is it only intended for ignoring (deleting) entire subthreads?

Apologies for asking this question but it's not clear to me from the comments above (possibly due to the TB-specific terminology with which I am not yet familiar) which one is intended.
a) bug 250141
b) this bug
Thanks Chris.
Attached patch handle failureSplinter Review
Attachment #319736 - Flags: superreview?(neil)
Attachment #319736 - Flags: review?(Pidgeot18)
Three of those reported crashed were mine. I had set the a filter and after the patch was backed out the next nightly after the filter setting was still active and there was no code in the nightly.  I deleted the filter setting and stopped crashing.  So my three reports are *invalid*, caused by user error.
I'd say the mailnews client should not crash because of bogus filters.
It should disable the bogus filters and continue.
Attachment #319736 - Flags: superreview?(neil) → superreview+
Comment on attachment 319736 [details] [diff] [review]
handle failure

Looks OK to me.
Attachment #319736 - Flags: review?(Pidgeot18) → review+
Comment on attachment 314195 [details] [diff] [review]
Front-end patch, version 1

bienvenu suggests asking clarkbw for ui-r? (I would have r? philor otherwise).

>RCS file: /cvsroot/mozilla/.cvsignore,v
This won't go down very well ;-)

>+  <key id="key_killSubthread" key="&killSubthreadMenu.key;"          oncommand="goDoCommand('cmd_killSubthread')" modifiers="shift" />

>+  <key id="key_killSubthread" key="&killSubthreadMenu.key;"              oncommand="goDoCommand('cmd_killSubthread')" />
The mailnews version doesn't have modifiers="shift" :-(
Also the spacing is wrong. r+sr=me with that fixed.

Note: This patch as is will break suite as you have not provided the equivalent locale changes. This may have been an oversight when creating the patch, but if not, I made the appropriate changes locally and can attach a diff if you need one. You also have not provided any theme changes but they don't break as hard!
Attachment #314195 - Flags: superreview?(neil)
Attachment #314195 - Flags: superreview+
Attachment #314195 - Flags: review?(neil)
Attachment #314195 - Flags: review+
Comment on attachment 314195 [details] [diff] [review]
Front-end patch, version 1

(In reply to comment #103)
> >RCS file: /cvsroot/mozilla/.cvsignore,v
> This won't go down very well ;-)

Oops, I thought I destroyed that from my patch. Rest assured that it won't find itself in later patches.

> Note: This patch as is will break suite as you have not provided the equivalent
> locale changes. This may have been an oversight when creating the patch, but if
> not, I made the appropriate changes locally and can attach a diff if you need
> one. You also have not provided any theme changes but they don't break as hard!
> 

rbgray had offered to port the changes to SM, which is why I didn't include them; I'll have the changes by the next patch.
Attachment #314195 - Flags: ui-review?(clarkbw)
Updated patch, with suite changes
Attachment #314195 - Attachment is obsolete: true
Attachment #324661 - Flags: superreview?(neil)
Attachment #324661 - Flags: review?(neil)
Attachment #314195 - Flags: ui-review?(clarkbw)
Comment on attachment 324661 [details] [diff] [review]
Front-end patch, version 2

Forgot to set ui-r for clarkbw...
Attachment #324661 - Flags: ui-review?(clarkbw)
Attachment #324661 - Flags: superreview?(neil)
Attachment #324661 - Flags: superreview+
Attachment #324661 - Flags: review?(neil)
Attachment #324661 - Flags: review+
Comment on attachment 324661 [details] [diff] [review]
Front-end patch, version 2

The patch seems ok, however i'm concerned that the indicator of a pruned thread are going to be sufficient.  The icon is quite small and a bit hard to distinguish it's meaning.  Though I'm not sure that a different icon would capture the meaning any better.

As a last thing, which isn't necessary right now for this feature, I'd like to see more thought about user feedback.  This feature is a tied up with the killThread one so they might both require a change.  

In the minimal case is a person who for some reason hits the key combo by accident.  How does that person understand what just happened, and how would they easily undo it?
Attachment #324661 - Flags: ui-review?(clarkbw) → ui-review+
BIG BOLD STATEMENT TO WATCHERS:
If anyone has better images than the ones shown as of comment 65, please post them here (or send them to me and I'll post them) by tomorrow morning.

(In reply to comment #108)
> (From update of attachment 324661 [details] [diff] [review])
> The patch seems ok, however i'm concerned that the indicator of a pruned thread
> are going to be sufficient.  The icon is quite small and a bit hard to
> distinguish it's meaning.  Though I'm not sure that a different icon would
> capture the meaning any better.

Under the default settings, one would never see it for long. I've solicited image designs from anyone with experience and have yet received no response (see comment 65). I'll hold off committing the changes until tomorrow to see if anyone has any last minute icons to add.

> As a last thing, which isn't necessary right now for this feature, I'd like to
> see more thought about user feedback.  This feature is a tied up with the
> killThread one so they might both require a change.  
> 
> In the minimal case is a person who for some reason hits the key combo by
> accident.  How does that person understand what just happened, and how would
> they easily undo it?

What happens is that the (sub)thread is marked as read and the icon is attached (for whole threads, the thread itself is collapsed) regardless of whether or not the view asks for killed threads. If more unread messages are present, the selection moves to the next unread message. The killing part can be undone by highlighting the thread/message and then using the (context) menu item and selecting Ignore (sub)thread or using the keyboard shortcut (Shift-)K.
Joshua, 
two of the 3 images are identical. (The files have the same checksum).
Is there supposed to be a difference?
What do they respectively signify?
(In reply to comment #110)
> Joshua, 
> two of the 3 images are identical. (The files have the same checksum).
> Is there supposed to be a difference?
> What do they respectively signify?
> 

See comment 48, comment 49, and comment 50.
Joshua, those comments don't tell me under what circumstances one of those
two images is used, and what other circumstances the other image is used.
Without that info, one cannot suggest (or make) alternative images that 
differentiate the two.
The two images are apparently supposed to be different ignored images depending on whether or not the thread has new messages. But they're the same image, and it appears that one is not even used in the CSS file at all.
Since Shredder is themed by Pinstripe I reworked those icons.  Personally I am not to hot on the Pinstripe threading indicator icon or its minuscule twistie.
Front-end patch checked in:
Checking in mail/base/content/hiddenWindow.js;
/cvsroot/mozilla/mail/base/content/hiddenWindow.js,v  <--  hiddenWindow.js
new revision: 1.10; previous revision: 1.9
done
Checking in mail/base/content/mail3PaneWindowCommands.js;
/cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v  <--  mail3PaneWindowCommands.js
new revision: 1.53; previous revision: 1.52
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.210; previous revision: 1.209
done
Checking in mail/base/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.252; previous revision: 1.251
done
Checking in mail/base/content/messageWindow.js;
/cvsroot/mozilla/mail/base/content/messageWindow.js,v  <--  messageWindow.js
new revision: 1.70; previous revision: 1.69
done
Checking in mail/locales/en-US/chrome/messenger/FilterEditor.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/FilterEditor.dtd,v  <--  FilterEditor.dtd
new revision: 1.11; previous revision: 1.10
done
Checking in mail/locales/en-US/chrome/messenger/filter.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/filter.properties,v  <--  filter.properties
new revision: 1.8; previous revision: 1.7
done
Checking in mail/locales/en-US/chrome/messenger/messenger.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.dtd,v  <--  messenger.dtd
new revision: 1.78; previous revision: 1.77
done
Checking in mail/themes/pinstripe/mail/jar.mn;
/cvsroot/mozilla/mail/themes/pinstripe/mail/jar.mn,v  <--  jar.mn
new revision: 1.21; previous revision: 1.20
done
Checking in mail/themes/pinstripe/mail/mailWindow1.css;
/cvsroot/mozilla/mail/themes/pinstripe/mail/mailWindow1.css,v  <--  mailWindow1.css
new revision: 1.15; previous revision: 1.14
done
RCS file: /cvsroot/mozilla/mail/themes/pinstripe/mail/icons/message-closed-kill.png,v
done
Checking in mail/themes/pinstripe/mail/icons/message-closed-kill.png;
/cvsroot/mozilla/mail/themes/pinstripe/mail/icons/message-closed-kill.png,v  <--  message-closed-kill.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mail/themes/pinstripe/mail/icons/message-new-closed-kill.png,v
done
Checking in mail/themes/pinstripe/mail/icons/message-new-closed-kill.png;
/cvsroot/mozilla/mail/themes/pinstripe/mail/icons/message-new-closed-kill.png,v  <--  message-new-closed-kill.png
initial revision: 1.1
done
Checking in mail/themes/qute/mail/jar.mn;
/cvsroot/mozilla/mail/themes/qute/mail/jar.mn,v  <--  jar.mn
new revision: 1.25; previous revision: 1.24
done
Checking in mail/themes/qute/mail/mailWindow1.css;
/cvsroot/mozilla/mail/themes/qute/mail/mailWindow1.css,v  <--  mailWindow1.css
new revision: 1.22; previous revision: 1.21
done
RCS file: /cvsroot/mozilla/mail/themes/qute/mail/icons/message-ignored.png,v
done
Checking in mail/themes/qute/mail/icons/message-ignored.png;
/cvsroot/mozilla/mail/themes/qute/mail/icons/message-ignored.png,v  <--  message-ignored.png
initial revision: 1.1
done
Checking in mailnews/base/resources/content/mail3PaneWindowCommands.js;
/cvsroot/mozilla/mailnews/base/resources/content/mail3PaneWindowCommands.js,v  <--  mail3PaneWindowCommands.js
new revision: 1.161; previous revision: 1.160
done
Checking in mailnews/base/resources/content/mailWindowOverlay.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.273; previous revision: 1.272
done
Checking in mailnews/base/resources/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.355; previous revision: 1.354
done
Checking in mailnews/base/resources/content/messageWindow.js;
/cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v  <--  messageWindow.js
new revision: 1.122; previous revision: 1.121
done
Checking in mailnews/base/search/resources/content/FilterEditor.js;
/cvsroot/mozilla/mailnews/base/search/resources/content/FilterEditor.js,v  <--  FilterEditor.js
new revision: 1.101; previous revision: 1.100
done
Checking in mailnews/base/search/resources/content/searchWidgets.xml;
/cvsroot/mozilla/mailnews/base/search/resources/content/searchWidgets.xml,v  <--  searchWidgets.xml
new revision: 1.17; previous revision: 1.16
done
Checking in suite/locales/en-US/chrome/mailnews/FilterEditor.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/FilterEditor.dtd,v  <--  FilterEditor.dtd
new revision: 1.39; previous revision: 1.38
done
Checking in suite/locales/en-US/chrome/mailnews/filter.properties;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/filter.properties,v  <--  filter.properties
new revision: 1.24; previous revision: 1.23
done
Checking in suite/locales/en-US/chrome/mailnews/messenger.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/messenger.dtd,v  <--  messenger.dtd
new revision: 1.226; previous revision: 1.225
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Product: Core → MailNews Core
This RFE is now marked resolved/fixed, so the feature should be available.
Can someone tell me how to use this feature in SM trunk?
I just haven't found it anywhere in SM trunk yet.

Is it in some menu?
Is there a keyboard shortcut?
Is it available as a filter action?
Depends on: 460036
Ditto #116 a year later.  Is this in any release yet?  If not, when?  If so, how?
It's available in tb3 (and seamonkey) betas. 
When in a newsgroup: Message | Ignore subthread
Ditto #116 a year later.  Is this in any release yet?  If not, when?  If so, how?
Thanks to all!
It seems that this feature (both k and shift+k) has some issues when it's used in a filter, however it works well when it's used manually (see https://bugzilla.mozilla.org/show_bug.cgi?id=659617 )
You need to log in before you can comment on or make changes to this bug.