1085205 - Create filter from - Chooses wrong field

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[steven.hartland]

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.

Comment 1

[:aceman]

Probably the code doesn't care which field you opened "Create filter from" on. But it hopefully could.

Comment 2

[:aceman]

Attachment: patch

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?

Comment 3

[Magnus Melin [:mkmelin]]

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

Comment 4

[Kent James (:rkent)]

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

Comment 5

[:aceman]

Attachment: patch v1.1

Thanks, done.

Comment 6

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/4da6759d4f47