Closed Bug 1085205 Opened 5 years ago Closed 5 years ago

Create filter from - Chooses wrong field

Categories

(Thunderbird :: Filters, defect)

31 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 37.0

People

(Reporter: steven.hartland, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

8.52 KB, patch
aceman
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.101 Safari/537.36

Steps to reproduce:

Select "Create filter from" from the context menu of the "To" field of an email.


Actual results:

It actually creates one with "From" field selected hence the filter doesn't work.




Expected results:

It should create a filter with "To" field selected.

Manually altering the field in the dialog fixes the issue.
Probably the code doesn't care which field you opened "Create filter from" on. But it hopefully could.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Component: Untriaged → Filters
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
So I added a wrapper for the internal table of strings vs. attributes as a method of nsIMsgSearchTerm so that it is accessible from JS. Or is there a better way to do this?
Attachment #8510625 - Flags: review?(mkmelin+mozilla)
Attachment #8510625 - Flags: review?(kent)
Comment on attachment 8510625 [details] [diff] [review]
patch

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

::: mail/base/content/mailWindowOverlay.js
@@ +2262,5 @@
> + * @param emailAddress  An email address to use as value in the first search term.
> + * @param folder        The filter will be created in this folder's filter list.
> + * @param aFieldName    Search field string, from nsMsgSearchTerm.cpp::SearchAttribEntryTable.
> + */
> +function MsgFilters(emailAddress, folder, aFieldName)

mix of aParams and not here. I don't really like the aParam pattern in js though...
Anyway, make it consistent.

::: mailnews/base/search/content/FilterEditor.js
@@ +91,5 @@
>          var term = gFilter.createTerm();
>  
> +        if ("fieldName" in args) {
> +          // fieldName should contain the name of the field in which to search,
> +          // from nsMsgSearchTerm.cpp::SearchAttribEntryTable, e.g. "to" or "cc"

I don't think it's nice for js to have to know values inside a cpp file...

@@ +94,5 @@
> +          // fieldName should contain the name of the field in which to search,
> +          // from nsMsgSearchTerm.cpp::SearchAttribEntryTable, e.g. "to" or "cc"
> +          term.attrib = term.getAttributeFromString(args.fieldName);
> +        } else
> +          term.attrib = Components.interfaces.nsMsgSearchAttrib.Sender;

braces please

::: mailnews/base/search/public/nsIMsgSearchTerm.idl
@@ +146,5 @@
>       * @return      true if message matches
>       */
>      boolean matchCustom(in nsIMsgDBHdr msg);
> +
> +    nsMsgSearchAttribValue getAttributeFromString(in string aAttribName);

needs documentation

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +695,5 @@
>      nsAutoCString customId;
>      nsresult rv = NS_MsgGetAttributeFromString(inStream, &attributeVal, m_customId);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      *attrib = (nsMsgSearchAttribValue) attributeVal;

this cast can go then i assume
Attachment #8510625 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8510625 [details] [diff] [review]
patch

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

r+=me with my one nit, and Magnus' comments handled.

::: mailnews/base/search/content/FilterEditor.js
@@ +91,5 @@
>          var term = gFilter.createTerm();
>  
> +        if ("fieldName" in args) {
> +          // fieldName should contain the name of the field in which to search,
> +          // from nsMsgSearchTerm.cpp::SearchAttribEntryTable, e.g. "to" or "cc"

It's a little odd, yes, but the whole point of that table is to convert internal integer ids into externally-understandable strings, such as are used when the terms are stored in a text file. So I think the usage here is fine.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +182,5 @@
>  
>    return NS_OK;
>  }
>  
> +nsresult nsMsgSearchTerm::GetAttributeFromString(const char *string,

NS_IMETHODIMP nsMsgSearchTerm ... please
Attachment #8510625 - Flags: review?(kent) → review+
Attached patch patch v1.1Splinter Review
Thanks, done.
Attachment #8510625 - Attachment is obsolete: true
Attachment #8530889 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Target Milestone: Thunderbird 38.0 → Thunderbird 37.0
Depends on: 1110095
Blocks: 1110389
You need to log in before you can comment on or make changes to this bug.