Closed Bug 1250295 Opened 4 years ago Closed 4 years ago

Run selected filters - default folder

Categories

(Thunderbird :: Filters, defect)

38 Branch
x86_64
Windows 7
defect
Not set

Tracking

(thunderbird45+ fixed, thunderbird46 fixed, thunderbird47 fixed, thunderbird48 fixed)

RESOLVED FIXED
Thunderbird 48.0
Tracking Status
thunderbird45 + fixed
thunderbird46 --- fixed
thunderbird47 --- fixed
thunderbird48 --- fixed

People

(Reporter: John, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Severity: normal → minor
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
I noticed this too and find it quite annoying. When did we break this?
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Filters
I would think somewhere around bug 878805.
So can you fix it, or is this desired behaviour?
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.
OK, then let me look at it:)
Assignee: nobody → acelists
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.
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.
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.
(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!!
Attached patch patch (obsolete) — Splinter Review
OK, try this.
Attachment #8727474 - Flags: feedback?(mozilla)
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? ;-)
Attachment #8727474 - Flags: feedback?(mozilla) → feedback+
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.
Attachment #8727474 - Flags: review?(squibblyflabbetydoo)
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 on attachment 8727474 [details] [diff] [review]
patch

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

r- for now per the above comment.
Attachment #8727474 - Flags: review?(squibblyflabbetydoo) → review-
(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().
(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.
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.
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?
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!
(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.
Attached patch patch v2Splinter Review
Attachment #8727474 - Attachment is obsolete: true
Attachment #8729118 - Flags: review?(squibblyflabbetydoo)
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.
Attachment #8729118 - Flags: review?(squibblyflabbetydoo) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
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.
Attachment #8729118 - Flags: approval-comm-esr45?
Attachment #8729118 - Flags: approval-comm-beta?
Attachment #8729118 - Flags: approval-comm-aurora+
https://hg.mozilla.org/comm-central/rev/3d9fb0a3003f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
Duplicate of this bug: 1263475
You need to log in before you can comment on or make changes to this bug.