145798 - Create Filter from Message is not doable when message pane is closed.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[laurel]

Using current trunk/branch

This was never expressly discussed in the spec, has since been mentioned in bug
143683 (duh, I didn't relate that original description to this simple request),
but am opening a more clearly defined bug here.

The main menu item Message|Create Filter from Message is disabled if the message
pane is closed. Therefore, you cannot create a sender filter (say for a spammer)
unless you've opened the message.

1.  Go to main mail window, close message pane.
2.  Select a message in the thread pane --> Message|Create Filter from Message
is disabled.

Result:  can't create a filter from message unless you open the message/load
contents.

Expected:  can create a filter from message (main menu item) without message opened.

Comment 1

[laurel]

Sorry, this is indeed in the spec that it should work with message pane closed.

Comment 2

[laurel]

Sorry, reread the comments for new keywords -- removing inappropriate adt1.0.1

Comment 3

[Michael Buckland]

Discussed in mail news bug meeting.  Decided to minus this bug.

Comment 4

[laurel]

nominating again for next release

Comment 5

[Frank]

Using Mozilla 1.2 Build 20021126 the context-menu supports a "Create Filter from
Message" on *any mail adress* right-clicked in the *mailheader* pane. 
(So it would better be labeled "Create Filter From Adress")
Mark fixed?

Comment 6

[Samir Gehani]

Mail triage team: nsbeta1-

Comment 7

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 8

[Malte Rücker (mbr)]

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

Comment 9

[Mike Cowperthwaite]

This is still true, per the original description; comment 5 is orthogonal to 
this issue.

Reducing severity, resetting milestone.

This would probably be a pretty simple bug for a JavaScript hacker to tackle.

Comment 10

[Mike Cowperthwaite]

Attachment: Attempt at fix (diff -u 3 -w against 1.6 Final)

This seems to do the trick, but it's a little messier than I imagined; to
generate the author's email address from the message URI, there's a bunch of
relatively heavyweight code.

Comment 11

[[e-mail]]

Comment on attachment 141689 [details] [diff] [review]
Attempt at fix  (diff -u 3 -w   against 1.6 Final)

>+    var msgUri = GetFirstSelectedMessage();
>+    var msgService = messenger.messageServiceFromURI(msgUri);
>+    if (msgService) {
>+      var msgHdr = msgService.messageURIToMsgHdr(msgUri);
>+      if (msgHdr) {
Try using gDBView.hdrForFirstSelectedMessage instead.

>+        var headerParser = Components.classes["@mozilla.org/messenger/headerparser;1"].getService(Components.interfaces.nsIMsgHeaderParser);
>+        var authorEmailAddress = new Object;
>+        headerParser.extractHeaderAddressMailboxes(null, msgHdr.author, authorEmailAddress);
>+        emailAddress = authorEmailAddress.value;
Yes, this is ugly. Ideally you would change line 85 of nsIMsgHeaderParser.idl
to string extractHeaderAddressMailboxes (in string charset, in string line); so
that you could write emailAddress =
headerParser.extractHeaderAddressMailboxes(null,
gDBView.hdrForFirstSelectedMessage.author); instead. (You would also have to
fix lines 588-590 of mailCommands.js in a similar way.)

Comment 12

[Mike Cowperthwaite]

Thanks Neil.

> Ideally you would change line 85 of nsIMsgHeaderParser.idl
> to   string extractHeaderAddressMailboxes(...)
>
I'm not likely to address anything in any files that aren't in the chrome 
directory; I'm living with 28.8 dialup most of the time, and I'm not willing to 
start running CVS under those conditions.  If my patch goes thru, I'll open 
another bug for that issue.

Comment 13

[Mike Cowperthwaite]

Attachment: Attempt at fix (diff -u 3 -w against 1.6 Final)

Second pass at a fix, with the first of Neil's comments implemented -- 
that worked fine, of course.

Comment 14

[[e-mail]]

Comment on attachment 141709 [details] [diff] [review]
Attempt at fix (diff -u 3 -w against 1.6 Final)

I can't get your patch to apply, can you attach a -u only version please?

Comment 15

[Mike Cowperthwaite]

Attachment: Attempt at fix (diff -u against 1.6 Final)

Same patch, diff'd without the -w switch.

Comment 16

[[e-mail]]

Comment on attachment 142686 [details] [diff] [review]
Attempt at fix (diff -u against 1.6 Final)

I eventually ended up applying this by hand, there are some subtle whitespace
differences between this patch and the 1.6 release tag...

Comment 17

[[e-mail]]

It would be nice to fix extractHeaderAddressMailboxes before this goes in.

Comment 18

[[e-mail]]

Attachment: Updated patch to the trunk

Comment 19

[David :Bienvenu]

won't this break creating a message filter from the stand-alone message window?

Comment 20

[Mike Cowperthwaite]

Dang, you're right.  Is there a better way to addres that than changing the 
first 'if' to:

    if ((typeof(IsMessageDisplayedInMessagePane) == 'undefined') ||
      IsMessageDisplayedInMessagePane()) {
    ...

?

Comment 21

[timeless]

'IsMessageDisplayedInMessagePane' in window

Comment 22

[Mike Cowperthwaite]

Attachment: Revised patch -- -u against 1.7-0310

OK, here's a patch with timeless's idiom that works for the 3pane and the
standalone; it's diffed against today's build, and shouldn't have any of those
nasty whitespace problems.

Comment 23

[Mike Cowperthwaite]

Comment on attachment 143526 [details] [diff] [review]
Revised patch -- -u against 1.7-0310

Re-requesting reviews (I hope I'm doing this right...)

Comment 24

[[e-mail]]

(In reply to comment #20)
>Dang, you're right.
Double dang :-[
>Is there a better way to address that
Yes! In messageWindow.js, call MsgCreateFilter(true); in
mail3PaneWindowCommands.js, MsgCreateFilter(IsMessageDisplayedInMessagePane());

Comment 25

[Mike Cowperthwaite]

Attachment: Nicer approach

Actually there's a much nicer way [sound of head striking desk]:
Always load the header from the message, don't bother with the crufty logic of
determining the window mode and loading it out of the envelope panel.

Comment 26

[Mike Cowperthwaite]

Comment on attachment 143531 [details] [diff] [review]
Nicer approach

Sorry for all the thrashing about here!

Comment 27

[Mike Cowperthwaite]

Attachment: Nicer approach: the correct file -u against 1.7-0310

This is embarrassing: I uploaded the wrong patch.

Comment 28

[[e-mail]]

Comment on attachment 143533 [details] [diff] [review]
Nicer approach: the correct file   -u against 1.7-0310

Please note that hdrForFirstSelectedMessage never returns null, it will throw
an exception if no message is selected.

Comment 29

[timeless]

and please please don't rely on that behavior. i want to fix callers to not call
it if there aren't any selected messages. those exceptions cause me significant
pain when i use venkman and mail.

Comment 30

[Mike Cowperthwaite]

Interesting.  Is it OK to simply rely on the fact that the menu item is disabled 
if there is no message selected?  If not, what's the nicest way to address this 
-- a try{} around the access to "gDBView.hdrForFirstSelectedMessage"?  Test for 
("hdrForFirstSelectedMessage" in gDBView)?  Or check the selected-message count?

Comment 31

[timeless]

gDBView && gDBView.numSelected

Comment 32

[Mike Cowperthwaite]

(In reply to comment #31)
> gDBView && gDBView.numSelected

Unfortunately, that also fails for the standalone window case.  The only 
potentially useful member of gDBView is currentlyDisplayedMessage -- which is 
set to  (unsigned long)-1  if no message is selected.

I am inclined to let the menu's disabling cover this instance.  If that's not 
sufficient, I'd put a try block there.

Comment 33

[[e-mail]]

Removing dependency as it is no longer relevant.

Most of the exceptions timeless is complaining about are in the checks to see if
the menuitems should be enabled in the first place ;-)

Both versions of doCommand do an extra isCommandEnabled check although I thought
that goDoCommand already does that check...

Comment 34

[Mike Cowperthwaite]

I think the dependency is still valid, since my patch relies on the difference 
in that other bug's patch.

I have a slight update to the patch -- an efficiency tweak in the 
isCommandEnabled() code in mail3PaneWindowCommand.  I'll put it up as soon as I 
get a definite word on how to address timeless's issue.  If I understand Neil 
right, he's advocating not doing anything special; instead, his comment 28 
recommended removing the superfluous 'if' I had in place originally.

Comment 35

[[e-mail]]

The bit of the patch that affects your patch got checked in, so any remaining
issues keeping that bug open shouldn't keep this bug open.

Yes, I am recommending removing the 'if'.

Which isCommandEnabled do you mean? I hope you mean the one that's part of var
DefaultController.

Comment 36

[Mike Cowperthwaite]

Attachment: Better version diff -u against 1.7-0312

Took out the superfluous conditional.  Made the defaultcontroller
isCommandEnabled() a little more compact.

Did not add any protection against the possibility of an exception thrown by
accessing |gDBView.hdrForFirstSelectedMessage|.  This code is not executed
until the menu action has been selected, and if the menu item is enabled, there
shouldn't be an exception.

Comment 37

[[e-mail]]

Comment on attachment 143752 [details] [diff] [review]
Better version   diff -u  against 1.7-0312

>       case "cmd_createFilterFromPopup":
>+      case "cmd_createFilterFromMenu":
>         var loadedFolder = GetLoadedMsgFolder();
>         if (!(loadedFolder && loadedFolder.server.canHaveFilters))
>-          return false;
>-      case "cmd_createFilterFromMenu":
>-        loadedFolder = GetLoadedMsgFolder();
>-        if (!(loadedFolder && loadedFolder.server.canHaveFilters) || !(IsMessageDisplayedInMessagePane()))
>-          return false;
>+          return false;   // else fall thru
Although it's not clear why there was a fall through in the first place...

Comment 38

[Mike Cowperthwaite]

Comment on attachment 143752 [details] [diff] [review]
Better version   diff -u  against 1.7-0312

Requesting approval for this low-risk bug.

Comment 39

[Mike Cowperthwaite]

(In reply to comment #37)
> Although it's not clear why there was a fall through in the first place...

It was a four-stage fall-thru with a redundant clause in the 
cmd_CreateFilterFromPopup; now it's a three-stage fall-thru where there's no 
redundancy, just the canCreateFilter test being shared by the two CreateFilter 
commands.

Comment 40

[chris hofmann]

Comment on attachment 143752 [details] [diff] [review]
Better version   diff -u  against 1.7-0312

a=chofmann for 1.7b

Comment 41

[[e-mail]]

Fix checked in.