Searching by "Priority" is missing an "Isn't" operator

RESOLVED FIXED in Thunderbird 24.0

Status

MailNews Core
Search
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: sshagarwal)

Tracking

Trunk
Thunderbird 24.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=aceman][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
If searching messages by the Priority field, the available operators are "Is", "Is Lower than", "Is Higher than". There is no "Isn't" operator.

The use-case could be:
Search all messages where Priority isn't Normal. It is currently possible to simulate that via "(Priority, is higher than, Normal) OR (Priority, is lower than, Normal)", but that forces the OR operator on all other search terms in the same search (or filter).

So this operator needs to be added to:
nsMsgSearchTerm.cpp > nsMsgSearchTerm::MatchPriority

and:
mailnews/base/search/src/nsMsgImapSearch.cpp (in the lines like: "SetAvailable (nsMsgSearchAttrib::Priority, nsMsgSearchOp::Is, 1);") . It seems the UI picks it up from there.
(Reporter)

Updated

4 years ago
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 747970 [details] [diff] [review]
Patch

Sir,

I have tried to make the changes as per the description.
Attachment #747970 - Flags: review?(kent)
Attachment #747970 - Flags: feedback?(acelists)

Comment 2

4 years ago
Could you add a test for this new attribute in mailnews/base/test/unit/test_search.js?
(Assignee)

Comment 3

4 years ago
Created attachment 748229 [details] [diff] [review]
Patch v2

Sir,

I have added the test.
Attachment #747970 - Attachment is obsolete: true
Attachment #747970 - Flags: review?(kent)
Attachment #747970 - Flags: feedback?(acelists)
Attachment #748229 - Flags: review?(kent)
Attachment #748229 - Flags: feedback?(acelists)
(Reporter)

Comment 4

4 years ago
Comment on attachment 748229 [details] [diff] [review]
Patch v2

The change and the test works great! Thanks.
Attachment #748229 - Flags: feedback?(acelists) → feedback+

Comment 5

4 years ago
Comment on attachment 748229 [details] [diff] [review]
Patch v2

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

Overall, looks good and great job. Please just fix the one nit.

::: mailnews/base/test/unit/test_search.js
@@ +207,5 @@
>        customHeader: "withspace",
>        count: 1},
> +    
> +    //test for priority
> +    { testString: Components.interfaces.nsMsgPriority.lowest,

Nit: In xpcshell tests, standard abbreviations are predefined, and should be used. So please change these five instances of Components.interfaces. to Ci.
Attachment #748229 - Flags: review?(kent) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 749207 [details] [diff] [review]
Patch v2 (revised)
Attachment #749207 - Flags: review+
Attachment #749207 - Flags: feedback?(acelists)
(Assignee)

Comment 7

4 years ago
Sir,
Thanks for your review and feedback.
I have made the changes.
Carrying over review from rkent.
(Assignee)

Updated

4 years ago
Attachment #748229 - Attachment is obsolete: true
(Reporter)

Comment 8

4 years ago
Comment on attachment 749207 [details] [diff] [review]
Patch v2 (revised)

Yes, that's it.
Attachment #749207 - Flags: feedback?(acelists) → feedback+
(Reporter)

Comment 9

4 years ago
Congratulations to your first user visible change in TB and Seamonkey:)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/e232ca630f82
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
You need to log in before you can comment on or make changes to this bug.