Last Comment Bug 444209 - Filter only selected messages in a folder
: Filter only selected messages in a folder
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9.1b1
Assigned To: Jeff Beckley
:
Mentors:
: 387870 (view as bug list)
Depends on: 440635
Blocks: 359236 445695
  Show dependency treegraph
 
Reported: 2008-07-08 14:39 PDT by Jeff Beckley
Modified: 2008-11-07 19:20 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Filter Selected Messages UI (39.54 KB, patch)
2008-07-08 14:39 PDT, Jeff Beckley
no flags Details | Diff | Splinter Review
Filter Selected Messages UI - Take 2 (43.01 KB, patch)
2008-07-08 15:04 PDT, Jeff Beckley
no flags Details | Diff | Splinter Review
New patch addressing Neil's comments (42.95 KB, patch)
2008-07-09 19:40 PDT, Jeff Beckley
no flags Details | Diff | Splinter Review
Changes for Neil's latest comments (37.06 KB, patch)
2008-07-17 01:20 PDT, Jeff Beckley
neil: superreview+
Details | Diff | Splinter Review
Neil's remaining comments (37.04 KB, patch)
2008-07-17 12:28 PDT, Jeff Beckley
mnyromyr: review+
beckley: superreview+
Details | Diff | Splinter Review
Final patch (34.64 KB, patch)
2008-08-19 09:47 PDT, Jeff Beckley
beckley: review+
beckley: superreview+
Details | Diff | Splinter Review

Description Jeff Beckley 2008-07-08 14:39:41 PDT
Created attachment 328569 [details] [diff] [review]
Filter Selected Messages UI

Some Classic Eudora users manage their mail by using the ability to manually filter specific messages (e.g. when the message has been dealt with by the user).  This patch gives that functionality to Thunderbird and Sea Monkey.  It adds a "Run Filters on Selected Messages" menu item to the Tools menu, right after the "Run Filters on Folder" item.  It also makes a reduction in code to nsMsgFilter that I noticed wasn't being used.

This patch is dependent on bug 440635 because it needs the manual filter context type.
Comment 1 Jeff Beckley 2008-07-08 15:04:07 PDT
Created attachment 328572 [details] [diff] [review]
Filter Selected Messages UI - Take 2

Some of my local changes didn't get included in the initial patch.  Now they are in there.
Comment 2 neil@parkwaycc.co.uk 2008-07-08 16:03:52 PDT
Comment on attachment 328572 [details] [diff] [review]
Filter Selected Messages UI - Take 2

>       case "cmd_applyFilters":
>         if (gDBView)
>           gDBView.getCommandStatus(nsMsgViewCommandType.applyFilters, enabled, checkStatus);
>         return enabled.value;
>+      case "cmd_applyFiltersToSelectedMessages":
>+        return (GetNumSelectedMessages() > 0 );
First, some strange ()s - you can probably drop them. I think you need to copy the check for filter after the fact. (I wonder if we should check for a virtual folder; maybe that already works!) Also, "applyFiltersToSelectedMessages" is is a bit long... maybe "applyFiltersToSelection"?
Comment 3 Jeff Beckley 2008-07-09 13:33:59 PDT
(In reply to comment #2)
> First, some strange ()s - you can probably drop them.

Yeah, not necessary.  Was a copy and paste job from another case in the switch.


> I think you need to copy
> the check for filter after the fact. (I wonder if we should check for a virtual
> folder; maybe that already works!)

No, I didn't think about the virtual folder case, and it turns out that it crashes when running on a virtual folder.  )-;

I might be able to get it to work with virtual folders, but if not then I'll disable it.


> Also, "applyFiltersToSelectedMessages" is is
> a bit long... maybe "applyFiltersToSelection"?

OK, I'll shorten it.
Comment 4 Jeff Beckley 2008-07-09 19:40:13 PDT
Created attachment 328805 [details] [diff] [review]
New patch addressing Neil's comments

Here's a new patch that addresses Neil's comments.  The code to apply filters manually can't deal with messages living in multiple folders, so I just have it use the logic for the Run Filters on Folder menu item, which will disabled the menu item for virtual folders.  I also added a check for preventing the addition of null message headers to the list so that the crash won't occur if that happens again.
Comment 5 neil@parkwaycc.co.uk 2008-07-10 16:10:00 PDT
Comment on attachment 328805 [details] [diff] [review]
New patch addressing Neil's comments

>       case "cmd_applyFilters":
>+      case "cmd_applyFiltersToSelection":

>+      case "cmd_applyFiltersToSelection":
>+        if (GetNumSelectedMessages() <= 0)
>+          return false;   // else fall thru
>       case "cmd_applyFilters":
>         if (gDBView)
>           gDBView.getCommandStatus(nsMsgViewCommandType.applyFilters, enabled, checkStatus);
>         return enabled.value;

>       case "cmd_applyFilters":
>         MsgApplyFilters(null);
>         return;
>+      case "cmd_applyFiltersToSelection":
>+        MsgApplyFiltersToSelection();
>+        return;
I would be very slightly happier if you could be consistent and put cmd_applyFiltersToSelection before cmd_applyFilters.

>+      var msgHdr = folder.GetMessageHeader(gDBView.getKeyAt(indices[i]));
>+      if (msgHdr)
>+        selectedMsgs.AppendElement(msgHdr);
Does this do the right thing when selecting the day grouping headers?

>+      case "cmd_applyFiltersToSelection":
> 				return true;
I think the message window needs to disable this option (c.f. cmd_applyFilters)

>+                      in nsISupportsArray aMsgHdrList,
Should this be an nsIArray?

> NS_IMPL_ISUPPORTS3(nsMsgFilterAfterTheFact, nsIUrlListener, nsIMsgSearchNotify, nsIMsgCopyServiceListener)
> 
>+
>+// nsMsgApplyFiltersToMessages overrides nsMsgFilterAfterTheFact in order to
>+// apply filters to a list of messages, rather than an entire folder
>+class nsMsgApplyFiltersToMessages : public nsMsgFilterAfterTheFact
>+{
>+public:
>+  nsMsgApplyFiltersToMessages(nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList, nsISupportsArray *aFolderList, nsISupportsArray *aMsgHdrList, nsMsgFilterTypeType aFilterType);
>+
>+protected:
>+  virtual   nsresult  RunNextFilter();
>+
>+  nsCOMPtr <nsISupportsArray> m_msgHdrList;
>+  nsMsgFilterTypeType         m_filterType;
>+};
>+
> nsMsgFilterAfterTheFact::nsMsgFilterAfterTheFact(nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList, nsISupportsArray *aFolderList)
This is rather a strange place to declare nsMsgApplyFiltersToMessages.

>+nsresult nsMsgFilterAfterTheFact::ApplyFilter(PRBool *applyMore)
aApplyMore perhaps?

>-  PRBool applyMoreActions = PR_TRUE;
>+  PRBool applyMoreActions;
>+  if (!applyMore)
>+    applyMore = &applyMoreActions;
>+  *applyMore = PR_TRUE;
Personally I'd prefer to output this at the end of the method i.e.
if (aApplyMore)
  *aApplyMore = applyMoreActions;
return rv;

>+nsMsgApplyFiltersToMessages::nsMsgApplyFiltersToMessages(nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList, nsISupportsArray *aFolderList, nsISupportsArray *aMsgHdrList, nsMsgFilterTypeType aFilterType)
>+: nsMsgFilterAfterTheFact(aMsgWindow, aFilterList, aFolderList)
>+{
>+  m_msgHdrList = aMsgHdrList;
>+  m_filterType = aFilterType;
Nit: These should be constructed, not assigned.

>+  while (1)
>+  {
>+    do
>+    {
>+      if (m_curFilterIndex >= m_numFilters)
>+        return AdvanceToNextFolder();
>+
>+      rv = m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = m_curFilter->GetFilterType(&filterType);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = m_curFilter->GetEnabled(&isEnabled);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } while (!(isEnabled && (filterType & m_filterType)));
This looks confusing; my suggested alternative is something like this:
while (m_curFilterIndex < m_numFilters)
{
  // addition of error checking is left as an exercise ;-)
  m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
  m_curFilter->GetFilterType(&filterType);
  if (!(filterType & m_filterType))
    continue;
  m_curFilter->GetEnabled(&enabled);
  if (!enabled)
    continue;
...
  if (!msgHdrCount)
    break;
}
return AdvanceToNextFolder();

>+      PRBool applyMore = PR_TRUE;
>+
>+      rv = ApplyFilter(&applyMore);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      if (applyMore)
>+      {
>+        // If there are more filters to apply, then ApplyFilter() would have
>+        // called RunNextFilter() itself, and so we should exit out afterwards
>+        break;
Would you mind explaining where this breaks to and why we don't call AdvanceToNextFolder in this case?

>+        nsCOMPtr <nsIMsgDBHdr> msgHdr;
>+        m_searchHitHdrs->QueryElementAt(msgIndex, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(msgHdr));
>+        if (msgHdr)
>+          m_msgHdrList->RemoveLastElement(msgHdr);
I can't wait for someone to fix the filter code to use sane array classes.

> <!ENTITY searchAddressesCmd.accesskey "S">
> <!ENTITY filtersCmd.label "Message Filters…">
> <!ENTITY filtersCmd.accesskey "F">
> <!ENTITY filtersApply.label "Run Filters on Folder">
> <!ENTITY filtersApply.accesskey "R">
>+<!ENTITY filtersApplyToSelection.label "Run Filters on Selected Messages">
>+<!ENTITY filtersApplyToSelection.accesskey "g">
You didn't fall into the trap :-)
Comment 6 Jeff Beckley 2008-07-13 20:29:49 PDT
(In reply to comment #5)
> (From update of attachment 328805 [details] [diff] [review])
> I would be very slightly happier if you could be consistent and put
> cmd_applyFiltersToSelection before cmd_applyFilters.

OK. Anything to make you happy, Neil.  ;^>


> >+      var msgHdr = folder.GetMessageHeader(gDBView.getKeyAt(indices[i]));
> >+      if (msgHdr)
> >+        selectedMsgs.AppendElement(msgHdr);
> Does this do the right thing when selecting the day grouping headers?

What are "day grouping headers"?


> >+      case "cmd_applyFiltersToSelection":
> > 				return true;
> I think the message window needs to disable this option (c.f. cmd_applyFilters)

No, actually it works.  It was intentional and I tested it.


> >+                      in nsISupportsArray aMsgHdrList,
> Should this be an nsIArray?

The similar function in that interface, applyFiltersToFolders, uses nsISupportsArray, so I figured I should use that for consistency.  Should I use nsIArray instead?


> This is rather a strange place to declare nsMsgApplyFiltersToMessages.

It was declared right after the class that its derived from, which seems not so strange.  Where do you think it should be?


> >+nsresult nsMsgFilterAfterTheFact::ApplyFilter(PRBool *applyMore)
> aApplyMore perhaps?

Sure, that would be better.


> >-  PRBool applyMoreActions = PR_TRUE;
> >+  PRBool applyMoreActions;
> >+  if (!applyMore)
> >+    applyMore = &applyMoreActions;
> >+  *applyMore = PR_TRUE;
> Personally I'd prefer to output this at the end of the method i.e.
> if (aApplyMore)
>   *aApplyMore = applyMoreActions;
> return rv;

There are early returns in that function, and it needs to be insured that the out parameter gets its value set in each case.  Adding an extra level of indirection seemed preferred to the chance that the out param might not get updated correctly if someone modifies the function in the future.


> >+nsMsgApplyFiltersToMessages::nsMsgApplyFiltersToMessages(nsIMsgWindow *aMsgWindow, nsIMsgFilterList *aFilterList, nsISupportsArray *aFolderList, nsISupportsArray *aMsgHdrList, nsMsgFilterTypeType aFilterType)
> >+: nsMsgFilterAfterTheFact(aMsgWindow, aFilterList, aFolderList)
> >+{
> >+  m_msgHdrList = aMsgHdrList;
> >+  m_filterType = aFilterType;
> Nit: These should be constructed, not assigned.

OK


> >+  while (1)
> >+  {
> >+    do
> >+    {
> >+      if (m_curFilterIndex >= m_numFilters)
> >+        return AdvanceToNextFolder();
> >+
> >+      rv = m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+      rv = m_curFilter->GetFilterType(&filterType);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+      rv = m_curFilter->GetEnabled(&isEnabled);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+    } while (!(isEnabled && (filterType & m_filterType)));
> This looks confusing; my suggested alternative is something like this:
> while (m_curFilterIndex < m_numFilters)
> {
>   // addition of error checking is left as an exercise ;-)
>   m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
>   m_curFilter->GetFilterType(&filterType);
>   if (!(filterType & m_filterType))
>     continue;
>   m_curFilter->GetEnabled(&enabled);
>   if (!enabled)
>     continue;
> ...
>   if (!msgHdrCount)
>     break;
> }
> return AdvanceToNextFolder();

Can't do that because not all exits out of the loop wind up calling AdvanceToNextFolder().  See next comment.


> >+      PRBool applyMore = PR_TRUE;
> >+
> >+      rv = ApplyFilter(&applyMore);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+      if (applyMore)
> >+      {
> >+        // If there are more filters to apply, then ApplyFilter() would have
> >+        // called RunNextFilter() itself, and so we should exit out afterwards
> >+        break;
> Would you mind explaining where this breaks to and why we don't call
> AdvanceToNextFolder in this case?

ApplyFilter() calls RunNextFilter() in the case where there are potentially more filters to match (i.e. filtering is not stopping on the matched filter having its actions applied), and so will have already gone on by the time it returns.  This logic is used due to the asynchronous nature of search in the base class nsMsgFilterAfterTheFact.  This new derived class, nsMsgApplyFiltersToMessages, uses a synchronous search, so it's not a perfect match but it winds up reusing a great majority of the code.


> >+        nsCOMPtr <nsIMsgDBHdr> msgHdr;
> >+        m_searchHitHdrs->QueryElementAt(msgIndex, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(msgHdr));
> >+        if (msgHdr)
> >+          m_msgHdrList->RemoveLastElement(msgHdr);
> I can't wait for someone to fix the filter code to use sane array classes.

Yes, that code is a bit tortured due to the array.


> > <!ENTITY searchAddressesCmd.accesskey "S">
> > <!ENTITY filtersCmd.label "Message Filters…">
> > <!ENTITY filtersCmd.accesskey "F">
> > <!ENTITY filtersApply.label "Run Filters on Folder">
> > <!ENTITY filtersApply.accesskey "R">
> >+<!ENTITY filtersApplyToSelection.label "Run Filters on Selected Messages">
> >+<!ENTITY filtersApplyToSelection.accesskey "g">
> You didn't fall into the trap :-)

Yes, that one I caught as I was copying it over from mail in to suite.
Comment 7 neil@parkwaycc.co.uk 2008-07-14 08:27:37 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 328805 [details] [diff] [review] [details])
> > >+      var msgHdr = folder.GetMessageHeader(gDBView.getKeyAt(indices[i]));
> > >+      if (msgHdr)
> > >+        selectedMsgs.AppendElement(msgHdr);
> > Does this do the right thing when selecting the day grouping headers?
> What are "day grouping headers"?
When you do View - Sort by - Date / Grouped By Sort then all the messages on the same date are grouped with a dummy entry as the header (since there's no natural thread header in this case.)

> > >+      case "cmd_applyFiltersToSelection":
> > > 				return true;
> > I think the message window needs to disable this option (c.f. cmd_applyFilters)
> No, actually it works.  It was intentional and I tested it.
OK, so in the message window it's "run filters on message"?

> > >+                      in nsISupportsArray aMsgHdrList,
> > Should this be an nsIArray?
> The similar function in that interface, applyFiltersToFolders, uses
> nsISupportsArray, so I figured I should use that for consistency.  Should I use
> nsIArray instead?
If possible i.e. if you don't specifically need an nsISupportsArray, otherwise at some point someone will convert the whole interface anyway.

> > This is rather a strange place to declare nsMsgApplyFiltersToMessages.
> It was declared right after the class that its derived from, which seems not so
> strange.  Where do you think it should be?
I think it should either a) be declared and then defined after the definition of nsMsgFilterAfterTheFact or b) be declared and then defined just before the code in nsMsgFilterService that uses it.

> > >-  PRBool applyMoreActions = PR_TRUE;
> > >+  PRBool applyMoreActions;
> > >+  if (!applyMore)
> > >+    applyMore = &applyMoreActions;
> > >+  *applyMore = PR_TRUE;
> > Personally I'd prefer to output this at the end of the method i.e.
> > if (aApplyMore)
> >   *aApplyMore = applyMoreActions;
> > return rv;
> There are early returns in that function, and it needs to be insured that the
> out parameter gets its value set in each case.  Adding an extra level of
> indirection seemed preferred to the chance that the out param might not get
> updated correctly if someone modifies the function in the future.
Hmm... the existing code is somewhat ugly, so I won't ask you to clean it up.
(e.g. might it be better to early return when setting *applyMore to false?)

> > >+  while (1)
> > >+  {
> > >+    do
> > >+    {
> > >+      if (m_curFilterIndex >= m_numFilters)
> > >+        return AdvanceToNextFolder();
> > >+
> > >+      rv = m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+
> > >+      rv = m_curFilter->GetFilterType(&filterType);
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+      rv = m_curFilter->GetEnabled(&isEnabled);
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+    } while (!(isEnabled && (filterType & m_filterType)));
> > This looks confusing; my suggested alternative is something like this:
> > while (m_curFilterIndex < m_numFilters)
> > {
> >   // addition of error checking is left as an exercise ;-)
> >   m_filters->GetFilterAt(m_curFilterIndex++, getter_AddRefs(m_curFilter));
> >   m_curFilter->GetFilterType(&filterType);
> >   if (!(filterType & m_filterType))
> >     continue;
> >   m_curFilter->GetEnabled(&enabled);
> >   if (!enabled)
> >     continue;
> > ...
> >   if (!msgHdrCount)
> >     break;
> > }
> > return AdvanceToNextFolder();
> Can't do that because not all exits out of the loop wind up calling
> AdvanceToNextFolder().  See next comment.
I see only one exit, and I wanted clarification on it below first ;-)

> > >+      PRBool applyMore = PR_TRUE;
> > >+
> > >+      rv = ApplyFilter(&applyMore);
> > >+      NS_ENSURE_SUCCESS(rv, rv);
> > >+      if (applyMore)
> > >+      {
> > >+        // If there are more filters to apply, then ApplyFilter() would have
> > >+        // called RunNextFilter() itself, and so we should exit out afterwards
> > >+        break;
> > Would you mind explaining where this breaks to and why we don't call
> > AdvanceToNextFolder in this case?
> ApplyFilter() calls RunNextFilter() in the case where there are potentially
> more filters to match (i.e. filtering is not stopping on the matched filter
> having its actions applied), and so will have already gone on by the time it
> returns.  This logic is used due to the asynchronous nature of search in the
> base class nsMsgFilterAfterTheFact.  This new derived class,
> nsMsgApplyFiltersToMessages, uses a synchronous search, so it's not a perfect
> match but it winds up reusing a great majority of the code.
D'oh, I overlooked that this is the RunNextFilter that ApplyFilter calls.
This code would be much simpler if ApplyFilter didn't call RunNextFilter.

You would have to tweak OnSearchDone and friends though, something like this:
if (!continueExecution)
  return OnEndExecution(rv);
PRBool applyMore = PR_TRUE;
if (!m_searchHits.IsEmpty())
  rv = ApplyFilter(&applyMore);
if (applyMore)
  rv = RunNextFilter();
return rv;

However, assuming you don't want to rewrite filters, I'd just put an early return into RunNextFilter, and then you can simplify the loop :-)
Comment 8 Jeff Beckley 2008-07-15 13:39:19 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 328805 [details] [diff] [review] [details] [details])
> When you do View - Sort by - Date / Grouped By Sort then all the messages on
> the same date are grouped with a dummy entry as the header (since there's no
> natural thread header in this case.)

There are problems with it in this case.  It doesn't disable the menu item if only a dummy header is selected.  Also, if a dummy header is selected then that selection will return the message header for the first "real" message in the group.  This can cause a particular message to get filtered twice (if the "real" message is selected as well).  It's interesting to note, though, that standard message operations like Reply and Forward exhibit the same bug.  I'll fix this so that the menu item isn't enabled when just a dummy header is selected, and also ensure that the message header associated with the dummy header doesn't get added to the list.


> > > I think the message window needs to disable this option (c.f. cmd_applyFilters)
> > No, actually it works.  It was intentional and I tested it.
> OK, so in the message window it's "run filters on message"?

That's a good point.  The menu item name needs to change when in a message window.  Does TB/SM do on-the-fly menu text changing anywhere?  Is that a UI model that is acceptable?  The other route would be to come up with a name that worked in both situations.  Classic Eudora did that by just generically calling it "Filter Messages", but that's because it didn't have another menu item that it had to clarify against: "Run Filters on Folders".


> > > >+                      in nsISupportsArray aMsgHdrList,
> > > Should this be an nsIArray?
> > The similar function in that interface, applyFiltersToFolders, uses
> > nsISupportsArray, so I figured I should use that for consistency.  Should I use
> > nsIArray instead?
> If possible i.e. if you don't specifically need an nsISupportsArray, otherwise
> at some point someone will convert the whole interface anyway.

I'll change it to nsIArray.


> > > This is rather a strange place to declare nsMsgApplyFiltersToMessages.
> > It was declared right after the class that its derived from, which seems not so
> > strange.  Where do you think it should be?
> I think it should either a) be declared and then defined after the definition
> of nsMsgFilterAfterTheFact or b) be declared and then defined just before the
> code in nsMsgFilterService that uses it.

OK, I'll move the whole thing, class definition and declaration, to just before nsMsgFilterService::ApplyFilter().


> D'oh, I overlooked that this is the RunNextFilter that ApplyFilter calls.
> This code would be much simpler if ApplyFilter didn't call RunNextFilter.
> 
> You would have to tweak OnSearchDone and friends though, something like this:
> if (!continueExecution)
>   return OnEndExecution(rv);
> PRBool applyMore = PR_TRUE;
> if (!m_searchHits.IsEmpty())
>   rv = ApplyFilter(&applyMore);
> if (applyMore)
>   rv = RunNextFilter();
> return rv;
> 
> However, assuming you don't want to rewrite filters, I'd just put an early
> return into RunNextFilter, and then you can simplify the loop :-)

Good idea.  I'll do that.
Comment 9 neil@parkwaycc.co.uk 2008-07-15 15:21:05 PDT
(In reply to comment #8)
>Does TB/SM do on-the-fly menu text changing anywhere?
Not between message and three-pane windows as far as I know; it does for instance change the Undo label depending on what it would undo, but that would be the same for both windows.
Comment 10 neil@parkwaycc.co.uk 2008-07-15 15:22:22 PDT
What might be acceptable is for the default label to be "Run Filters on Message" and then the 3-pane command updater code would change the label to "Run Filters on Selection" if more than one message was selected.
Comment 11 Jeff Beckley 2008-07-17 01:20:29 PDT
Created attachment 329967 [details] [diff] [review]
Changes for Neil's latest comments

Here's fixes based on Neil's latest comments:
- Menu item is only enabled when there is at least one "real" (i.e. non-dummy) message selected
- Selected dummy headers don't get the msgHdr they point to in the list of selected filters
- Menu item says "Run Filters on Message" when only one item selected or being operated on a message window, and "Run Filters on Selected Messages" when more than one message is selected
- nsIMsgFilterService.applyFilters() not uses a nsIArray for the message header list instead of a nsISupportsArray
- nsMsgApplyFiltersToMessages class defined and declared just before the nsIMsgFilterService::ApplyFilters() function that uses it
- Simplified loop logic in nsMsgApplyFiltersToMessages::RunNextFilter()
Comment 12 Jeff Beckley 2008-07-17 01:29:40 PDT
btw, I didn't make the modifications to nsMsgDBView::GetNumSelected() and nsMsgDBView::GetIndicesForSelection() that we talked about in #maildev as there was existing code that depended on them returning dummy headers as well as real messages.  Not to mention extensions out there that might rely on the existing behavior.
Comment 13 neil@parkwaycc.co.uk 2008-07-17 02:04:10 PDT
Comment on attachment 329967 [details] [diff] [review]
Changes for Neil's latest comments

>       case "cmd_reload":
>+      case "cmd_applyFiltersToSelection":
>+        if (command == "cmd_applyFiltersToSelection")
I thought, "why can't you put this first and let it fall through?" and discovered that someone else had got there first :-(

>+        // Getting the URI will tell us if the item is real or a dummy header
>+        var uri = gDBView.getURIForViewIndex(indices[i]);
>+        if (uri.length)
Strictly speaking you don't need to check this because getURIForViewIndex will throw an exception for a dummy header, but if you still want to check I would simply write if (uri)

>+    nsCOMPtr<nsMsgSearchScopeTerm> scope(new nsMsgSearchScopeTerm(nsnull, nsMsgSearchScope::offlineMail, m_curFolder));
sorry for not noticing before., but this should probably be nsRefPtr<nsMsgSearchScopeTerm> (or possibly nsCOMPtr<nsIMsgSearchScopeTerm>). (I think I had a build that detected when you used nsCOMPtr on a concrete class, but the PC I had it on won't build trunk anyway.)

>+      nsCOMPtr<nsIMsgDBHdr> msgHdr(m_msgHdrList[i]);
You can probably get away with an nsIMsgDBHDr* here as the m_msgHdrList's reference will suffice to keep the header alive.

>+      for (PRUint32 msgIndex = 0; msgIndex < m_searchHits.Length(); msgIndex++)
>+      {
>+        nsCOMPtr <nsIMsgDBHdr> msgHdr;
>+        m_searchHitHdrs->QueryElementAt(msgIndex, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(msgHdr));
This confused me, until I noticed that nobody uses m_searchHits except for its length! I'll file a bug on that, and I might fix m_searchHitHdrs to be an nsCOMArray at the same time.
Comment 14 Jeff Beckley 2008-07-17 12:28:39 PDT
Created attachment 330086 [details] [diff] [review]
Neil's remaining comments

Fixes for small issues that Neil pointed out.

Karsten: Where art thou?
Comment 15 Karsten Düsterloh 2008-08-05 16:27:44 PDT
Comment on attachment 330086 [details] [diff] [review]
Neil's remaining comments

Looks good, just some minor nits:
(XUL/JS nits are meant for both /mail and /mailnews, but I only list iot once below.)

>+function MsgApplyFiltersToSelection()
>+{
>+  var filterService = Components.classes["@mozilla.org/messenger/services/filters;1"].getService(Components.interfaces.nsIMsgFilterService);

Please wrap .getService and align the . under the . of classes.

>+    var selectedMsgs = Components.classes["@mozilla.org/array;1"].createInstance(Components.interfaces.nsIMutableArray);

Same here.


r=me with that.
Comment 16 Jeff Beckley 2008-08-19 09:47:23 PDT
Created attachment 334502 [details] [diff] [review]
Final patch

Addressed Karsten's review comments.  All done now, and ready for check in.

Continuing r=mnyromyr, sr=neil.
Comment 17 Mark Banner (:standard8) 2008-08-19 10:12:04 PDT
Checked in, changeset id 143:de88d146b87c
Comment 18 Jeff Beckley 2008-11-07 19:20:48 PST
*** Bug 387870 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.