1250295 - Run selected filters - default folder

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[John Schneider]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160210153822

Steps to reproduce:

Select Tools -> Message Filters.  "The Run selected filter(s) on:" is defaulting to "Choose Folder...".  This has always defaulted to the current folder in focus.


Actual results:

Default selection is now "Choose Folder...".


Expected results:

This has always defaulted to the current folder in focus on the folder tree on the left side of the window.  I am not exactly sure when this behavior changed, but believe it was in one of the recent v38 releases.

Comment 1

[Jorg K (CEST = GMT+2)]

I noticed this too and find it quite annoying. When did we break this?

Comment 2

[:aceman]

I would think somewhere around bug 878805.

Comment 3

[Jorg K (CEST = GMT+2)]

So can you fix it, or is this desired behaviour?

Comment 4

[Jorg K (CEST = GMT+2)]

This didn't break due to bug 878805. That bug was landed on TB 29.
When I create a new filter from a message, I have a default folder to run it on set in TB 31 but not TB 38.

Comment 5

[:aceman]

OK, then let me look at it:)

Comment 6

[:aceman]

Jorg, can you actually see this in current trunk? It seems to me the current folder is selected.
It is NOT selected when the current selection in folder pane is the whole account. That could be OK I think. There is also NO folder selected int he case of "create filter from message". I can look at that case. It could be that bug 1198744 improved this since TB38.

Comment 7

[Jorg K (CEST = GMT+2)]

I'm using TB 46 (Earlybird) and trunk (still TB 47).

Go to the inbox or a local folder. In the thread pane, click on a row so the message is displayed in the message pane. Click on the sender or recipient address in the message header and select "Create Filter From...". Create the filter. When it's done, you want to run it on the folder, but that folder is not selected.

Comment 8

[:aceman]

Yes, that is what I also observed. However, those steps are different from the original report. I'll look at improving that.

But with Tools -> Message Filters I can no longer see the problem.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #8)
> But with Tools -> Message Filters I can no longer see the problem.
I can.

Click on a local folder, no message is selected, Tools -> Message Filters, defaults to this folder.

Click on a local folder, select a message, Tools -> Message Filters, does not default!!

Comment 10

[:aceman]

Attachment: patch

OK, try this.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8727474 [details] [diff] [review]
patch

OK, tested three cases:

Folder, no message selected. Tools > Message Filters: Run box pre-populated. Good.

Folder, with message selected. Tools > Message Filters: Run box pre-populated. Good.

Click on message header displayed in message pane. Create Filter From. Create the filter. When done, you get to the filter list and the run box is pre-populated. Good.

So, all good. When can you ship? ;-)

Comment 12

[:aceman]

Comment on attachment 8727474 [details] [diff] [review]
patch

I don't particularly like getting the displayed message via gFolderDisplay.selectedMessage. I do not trust that the current selection always represents the message that is shown. Squib, is there a better way to do this? Do we store the msghdr of the displayed message at some better place?
I mean for the CreateFilter() calls that originate from the popup in the message header display.

Comment 13

[Jim Porter (:squib)]

I'm a bit confused here...

1) I'm not sure why you don't trust that property. What cases are you imagining where you'd have an inconsistent result?

2) If you want the *displayed* message (I'm not convinced you do), the relevant property is `gMessageDisplay.displayedMessage`.

3) In either case, you should handle the scenario where no message is selected/displayed. Currently, it looks like your code will throw an exception.

4) From comment 0, it sounds more like you want the folder selected in the folder pane, so I'd recommend `gFolderTreeView.getSelectedFolders()[0]`. Although note that it's possible (but rare) to have no selected folders, so you'll have to handle that.

Comment 14

[Jim Porter (:squib)]

Comment on attachment 8727474 [details] [diff] [review]
patch

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

r- for now per the above comment.

Comment 15

[:aceman]

(In reply to Jim Porter (:squib) from comment #13)
> I'm a bit confused here...
> 
> 1) I'm not sure why you don't trust that property. What cases are you
> imagining where you'd have an inconsistent result?

The case when gMessageDisplay.displayedMessage is not equal to gFolderDisplay.selectedMessage.
Because I actually want gMessageDisplay.displayedMessage :)

> 2) If you want the *displayed* message (I'm not convinced you do), the
> relevant property is `gMessageDisplay.displayedMessage`.

For the "create filter from" menuitem in the popup from the displayed message header I want gMessageDisplay.displayedMessage . That seems 

> 3) In either case, you should handle the scenario where no message is
> selected/displayed. Currently, it looks like your code will throw an
> exception.

Yes, fixed.

> 4) From comment 0, it sounds more like you want the folder selected in the
> folder pane, so I'd recommend `gFolderTreeView.getSelectedFolders()[0]`.
> Although note that it's possible (but rare) to have no selected folders, so
> you'll have to handle that.

Should be covered by the "gFolderDisplay.selectedMessage.folder" inside a try{}catch{} block, then if it fails it gets GetFirstSelectedMsgFolder().

Comment 16

[Jim Porter (:squib)]

(In reply to :aceman from comment #15)
> Should be covered by the "gFolderDisplay.selectedMessage.folder" inside a
> try{}catch{} block, then if it fails it gets GetFirstSelectedMsgFolder().

Are you sure that's correct? Consider the flow when you have a cross-folder search selected: I'm pretty sure `gFolderDisplay.selectedMessage.folder` will return the real folder that contains the message, not the virtual search folder. As a user, I'd expect the filter to run on the virtual search folder.

Comment 17

[:aceman]

I'm not sure we want to touch the logic that is already there. 
The point of this bug is to at least obey the folder that is sent to the filter list dialog.
And in some cases nothing is sent so I chose some folders (the displayed message, when operating on its header). This was just previously hidden by the fact that the default folder defaulted to Inbox if nothing was sent. But after the cited bug there is now the "choose folder" item selected.

Comment 18

[Jim Porter (:squib)]

Actually, I'm a bit confused still. Comment 0 seems to be about *running* filters, whereas this patch only seems to address *creating* filters. Am I missing something?

Comment 19

[John Schneider]

I just tried creating a new filter and it also does not default to the current folder.

1) Right-click on the "From" field in a message in the preview pane
2) Select Create Filter From...
3) Fill in the appropriate values
4) Click OK
5) You are then taken to the Message Filters window and the current folder is not selected

I appreciate the effort you guys are putting in on this little annoying bug!

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Jim Porter (:squib) from comment #18)
> Actually, I'm a bit confused still. Comment 0 seems to be about *running*
> filters, whereas this patch only seems to address *creating* filters. Am I
> missing something?
As the reporter clarified: While *creating* a filter, the option to *run* it on the folder it has just been created on has been made harder, since this folder is no longer preselected. The bug is about re-establishing the preselection.

Comment 21

[:aceman]

Attachment: patch v2

Comment 22

[Jim Porter (:squib)]

Comment on attachment 8729118 [details] [diff] [review]
patch v2

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

I'm not entirely convinced this is the right behavior, but the patch itself looks good.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8729118 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): we don't know, but this worked before in TB 31.
User impact if declined: Little annoyance that the folder for the new rule to run on isn't set.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low risk.

Landed on Aurora (TB 47) for further testing and because we need it for TB 45.
https://hg.mozilla.org/releases/comm-aurora/rev/0513bb332127

C-C is currently closed.

Comment 24

[Kent James (:rkent)]

Comment on attachment 8729118 [details] [diff] [review]
patch v2

http://hg.mozilla.org/releases/comm-beta/rev/db7619bd68ba
http://hg.mozilla.org/releases/comm-esr45/rev/07d945673613

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/3d9fb0a3003f