586131 - Quickfilter bar has lost OR functionality using | (Pipe character)

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Thomas D. (:thomas8)]

STR

1 Ctrl+F, enter foo|bar into search box

Actual result

- TB looks for "foo|bar" as an exact string
- does not interpret | as an OR operator

Expected result

- interpret | as OR operator, as in TB 2.0

This used to work (special hack in the old quicksearch code) -> regression
http://getsatisfaction.com/mozilla_messaging/topics/search_filters_in_tb_3_1

Related: 
240454 - Add AND / OR searching to quick search (not a duplicate, imo)
SM Bug 177034 - Allow for | and & operators in 3-pane and ab book quick searches

Xref Bug 318060 - Can't Quicksearch to find messages containing logical operator pipeline | as a character (but works for &)

Comment 1

[Ludovic Hirlimann [:Usul]]

(In reply to comment #0)
> STR
> 
> 1 Ctrl+F, enter foo|bar into search box
> 
> Actual result
> 
> - TB looks for "foo|bar" as an exact string
> - does not interpret | as an OR operator
> 
> Expected result
> 
> - interpret | as OR operator, as in TB 2.0
> 
> This used to work (special hack in the old quicksearch code) -> regression

If it's a hack this can't be a regression. How is this different from 240454 which states we couldn't use operators in search. If you can't use Operators, need to hack to have it work in 2.x then this is just a dup.

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to comment #1)
> (In reply to comment #0)
> > STR
> > 
> > 1 Ctrl+F, enter foo|bar into search box
> > 
> > Actual result
> > 
> > - TB looks for "foo|bar" as an exact string
> > - does not interpret | as an OR operator
> > 
> > Expected result
> > 
> > - interpret | as OR operator, as in TB 2.0
> > 
> > This used to work (special hack in the old quicksearch code) -> regression
> 
> If it's a hack this can't be a regression. How is this different from bug 240454
> which states we couldn't use operators in search. If you can't use Operators,
> need to hack to have it work in 2.x then this is just a dup.

I don't follow what bug 240454 has to do with this. Isn't 240454 about the words AND and OR?  And hack or not, | worked, it was implicitly supported, and now it doesn't work = regression.  

There is no workaround which makes this even more egregious, and no bug 240454 fix in sight. Still, this bug is fair game for WONTFIX. (ref bug 318060 comment 9)

water over the dam, but if we have a test for this, and the test was run, would this would have been caught earlier?

Comment 3

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)

> > which states we couldn't use operators in search. If you can't use Operators,
> > need to hack to have it work in 2.x then this is just a dup.
> 
> I don't follow what bug 240454 has to do with this. Isn't 240454 about the
> words AND and OR?  And hack or not, | worked, it was implicitly supported, and
> now it doesn't work = regression.  

Cause bug is not clear enough, it's about adding the use of OR and AND, not the functionality. I just twitched on the word hack that's all.
 
> 
> water over the dam, but if we have a test for this, and the test was run, would
> this would have been caught earlier?

Yes - because it's probably something you use on your filters all the time to causual usage/testing wouldn't catch it.

Comment 4

[Thomas D. (:thomas8)]

(In reply to comment #3)
> (In reply to comment #2)
> > I just twitched on the word hack that's all.

Sorry for the confusion about "hack". It was normal inbuilt functionality that used to work as expected. I once saw the code and somehow it reminded me of a hack that someone had added after the fact to make this work.

> > water over the dam, but if we have a test for this, and the test was run,
> > would this would have been caught earlier?
> Yes - because it's probably something you use on your filters all the time to
> causual usage/testing wouldn't catch it.

I think what Ludo wanted to say is:
Yes [this problem would have been found earlier had there been an automated test case in testsuite], because it's probably something you do NOT use on your filters all the time, so casual usage/testing wouldn't catch it.

Comment 5

[Thomas D. (:thomas8)]

FTR Related Bug 240454 - Add AND / OR searching to quick search (not a dupe, as that's only about adding the words AND and OR as search operators to access the underlying functionality)

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Patch for possible implementation of using | as OR operator.

Comment 8

[:aceman]

Comment on attachment 770860 [details] [diff] [review]
Patch

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

So this seems to work, thanks Suyash.
But there are some corner cases. If you input "word1|word2" is works as requested. But if you use "word1 | word2" it fails (does not find the matches). Also try "|word1" which fails too. Can you look at that? It seems normally the search trims whitespace around the searchstring (try e.g. "   word1"). Maybe you have to do the same inside each pipe delimited substring.

::: mail/base/modules/quickFilterManager.js
@@ +982,5 @@
>          continue;
>        }
>  
> +      let orOperator;
> +      let flag = false;

You should probably find a better name for the 'flag' variable. Like "pipeFound". Instead of string.indexOf(x) != -1 you can use string.contains(x) .

@@ +984,5 @@
>  
> +      let orOperator;
> +      let flag = false;
> +      while ((orOperator = aSearchString.indexOf('|')) != -1) {
> +        addTerm('|' + aSearchString.substring(0, orOperator));

I think double quotes are preferred for strings where possible.

@@ +1013,5 @@
>     *  sender and recipient enabled, we build:
>     * ("foo" sender OR "foo" recipient) AND ("bar" sender OR "bar" recipient)
>     */
>    appendTerms: function(aTermCreator, aTerms, aFilterValue) {
> +    let term, value, flag;

'flag' name again...

@@ +1018,4 @@
>  
>      if (aFilterValue.text) {
>        let phrases = this._parseSearchString(aFilterValue.text);
>        for each (let [, phrase] in Iterator(phrases)) {

If phrases is a normal javascript array, you can rewrite this loop as 'for (let phrase of phrases)'. See bug 824104 on why we do this.

@@ +1018,5 @@
>  
>      if (aFilterValue.text) {
>        let phrases = this._parseSearchString(aFilterValue.text);
>        for each (let [, phrase] in Iterator(phrases)) {
> +        if (phrase[0] == '|') {

phrase.startsWith("|") ?

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Thanks for your feedback sir, made the changes suggested. Works for " |" and/or "| " as well and multiple OR operators are supported.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 770969 [details] [diff] [review]
Patch v2

I no longer do Thunderbird reviews, so one of the 3-pane owners would probably be most appropriate.  I can, however, provide some quick advice:

For removing spaces adjacent to pipes, an alternate way to do that and handle multiple spaces would be to do something like:

aSearchString = aSearchString.replace(
  / *\| */g,
  function (pipeString) { return pipeString.trim(); });


Rather than putting a '|' in the string to indicate that the phrase wants to be OR'ed, I would instead just push an object onto instead.  addTerm could take a second argument which is useOR, and it could then do: terms.push({ term: aTerm, useOR: useOR })


The unit tests for the quick filter are done in mozmill, here:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/quick-filter-bar/test-filter-logic.js


I would think that you would want to clarify in a doc block comment on MessageTextFilter or its appendTerms function how the impact of precedence is addressed within the phrase.  And then in the unit test, you would probably want to ensure that you implemented what you described there.  I know I certainly would be wondering what happens if I type "a b | c" into the search box.

Comment 11

[Thomas D. (:thomas8)]

May I also suggest that we don't repeat old mistakes of hardcoding "|" as OR operator. That will spoil it for users who have scenarios where they actually need to search for "|" as part of a string, or users who'd prefer using other strings as OR operator, like "OR".  Instead, I suppose we should define the logical OR operator string used for quicksearch in a hidden pref.
Alternatively, allow escaping of "|", e.g. via "\|"?

Comment 12

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #11)
> Alternatively, allow escaping of "|", e.g. via "\|"?

(In reply to Andrew Sutherland (:asuth) from comment #10)
> aSearchString = aSearchString.replace(
>   / *\| */g,
>   function (pipeString) { return pipeString.trim(); });

Looking at the above regex, using "\|" as a way of escaping "|" also comes with problems because then it will be more difficult (or impossible?) for users to search for the actual string "\|" as seen in the regex, e.g. in code snippets that are part of the message or subject; but I don't know enough about escaping to judge.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

I revisited the logic after seeing Thomas' comments.  I was a little bit too low-level and quick in my analysis of the patch previously.  Most of the new pipe logic is not really required.  You can just have addTerm recognize a specific string as the delimiting character.

addTerm can do a test to see if the term is the magic string which defaults to "|".  So addTerm sees that, then it knows to add the term as an OR operation.

I don't see any harm in making the string configured by secret preference as long as we don't make any tooltip hints aware of the preference.  As test permutations go, the existence of the secret pref can be ignored.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Er, and what I mean by "addTerm sees that", I mean that it would set a flag that is closed over from its enclosing function which would then be cleared on the next call to addTerm when it actually adds a term with the OR indicator set.

Comment 15

[Thomas D. (:thomas8)]

Bug 318060 Comment 9 discusses this problem and suggests a first-use-dialogue and pref to toggle between Simple and Advanced search mode, where only in Advanced mode | & and ! would be treated as logical operators, but in Simple mode, they are considered simple characters. Having ! as a way of excluding search terms would be really cool; for the best way of marking precedence (e.g. using brackets or & ) , I'm not sure. Can we benefit from similar efforts that have recently happened in filters?

Comment 16

[Thomas D. (:thomas8)]

Andrew, how hard would it be to make addTerm (or whatever is involved) aware of brackets for marking precedence?

Comment 17

[Suyash Agarwal (:sshagarwal)]

So, Thomas D and :asuth, please let me know what all operators and even words like OR, AND are to be implemented here, so that I can think about implementing them :)

Thanks.

Comment 18

[:aceman]

Why are we morphing this bug into creation of another regexp parsing system? :)

I would like OR and AND operators better than some | and " " (space). The current search term "word1 word2" behaves like msg.contains(word1) AND msg.contains(word2) but not as msg.contains("word1 word2") which may also not be intuitive.

So we either keep the current logic (space being AND but not "exact string") and do not add special operators or then clean up the syntax and make the operators explicit (e.g. no space with special meaning but a verbose "AND" required). That is something Thomas proposes in comment 15: having 2 modes of operation. Those modes could even be toggled automatically: whenever the string uses any of the operators {NOT,OR,AND} or {!,|,&} the advanced mode is used. The tooltip of the search field could hint about these modes.

Comment 19

[Wayne Mery (:wsmwk)]

+1 to aceman and using word terms, rather than logic symbols that will be foreign to the common public

Comment 20

[Magnus Melin [:mkmelin]]

Bar and others are shorter, and not English-specific. Also, bar is supported by e.g. google.

Comment 21

[Thomas D. (:thomas8)]

I need more time to think about this. Things to consider:
a) definitely want to keep existing 'google' logic of spaces as implicit AND
b) introducing any operators on top of current exclusive AND requires some way of precedence marking, e.g. with brackets or varying spacing:

Explicit predence indicators (with or without spaces doesn't matter)
a (b|c)    -> a AND (b OR c)
(a b)|c    -> (a AND b) OR c

I'm a bit concerned that parsing brackets as precedence markers will no longer allow searching for simple strings that have brackets as part of the string.
Similar to what aceman suggests, could we make it so that brackets are only parsed as precedence markers when there are other logical operator(s) present?

Implicite precedence by spacing:
a b|c      -> a AND (b OR c)
a b | c    -> (a AND b) OR c

c) how important is it to allow unquoted simple literal searches for strings having !, &, or | and ? Or could we try only parsing |, &, and ! as operators when they occur standalone, like this (spaces on both sides of operator):

foo | bar
foo & bar
foo ! bar

or perhaps also allow this (space only on left side of operator)

foo |bar
foo &bar
foo !bar

but never parse this as operators (space on right side of operator only), instead, parse as simple string:

foo| bar
foo& bar
foo! bar

I'm just trying to understand our options better, might still be incomplete, and no conclusions yet.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

I think it would be reasonable to require that you use quotes if you want to treat less common punctuation as part of the search string.  (In other words, we would never want to overload ",", ".", "!", "?", ";", or ":" in the default search mode.)

So:
"foo | bar"
is literally searching for: "foo" <space> <pipe> <space> "bar"

Whereas:
foo | bar
is searching for "foo" or "bar"

same for:
foo OR bar

In other words, reasonable magic if you don't quote, exactly what you type when you do quote (or at least less magic; it could make sense to make white-space more forgiving of at least newlines).

Probably the most useful thing is describing to the user what we are actually searching for in these cases.  I do like Bryan's blurb that Thomas called out in comment 15 as a means of letting the user then change behaviour, although I think it's preferable as a last resort for implementation complexity reasons.  (As always, your comprehensive coverage of search is very appreciated!)


Note that I don't have a strong opinion as to the future direction of the quick filter bar, but I am interested in trying to maximize consistency between Thunderbird and whatever ends up happening with the desktop version of the Gaia e-mail app.  (I am planning to work on that as a personal side-project at least.)

Comment 23

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #19)
> +1 to aceman and using word terms, rather than logic symbols that will be
> foreign to the common public

Parsing any more logical operators than the single implicit AND which we have now (via spacing) requires a precedence system (probably explicit precedence indicator) and user knowledge about operators AND precedence, at least for those who want to use those new operators. Imho, although we should surely strive to make it as simple as possible, that implies advanced usage and advanced users. I don't see much difference between using...

|, &, and !          vs.
OR, AND, and NOT
(plus precedence indicators like brackets for both)

...you really have to know what you're doing to do those, and read the manual to find out. Atm I see no big problem in principle that we could have both ways (operators as words AND signs), but there might be pragmatic reasons to postpone word operators, see below.

(In reply to Magnus Melin from comment #20)
> Bar and others are shorter, and not English-specific. Also, bar is supported
> by e.g. google.

+1. Bar and others = |, &, !   It may or may not be easier to implement operator signs only at this point and defer operator words to be done later in bug 240454.

If we see any chance to land this on TB24, we definitely need to limit ourselves to the signs, because operator words require strings for which we'll certainly be too late, and those language-specific stringe also require language-specific documentation (whereas for the operator signs like | and & we could still get away with en-US documentation).

(In reply to Andrew Sutherland (:asuth) from comment #22)
> I think it would be reasonable to require that you use quotes if you want to
> treat less common punctuation as part of the search string.  (In other
> words, we would never want to overload ",", ".", "!", "?", ";", or ":" in
> the default search mode.)

I think that's a worthwhile proposal. I took a while to parse this, so allow me to try and make Andrew's points more explicit:

- OK to parse "|" (and "&"?) as logical operators in default search, because they are less common punctuation
- OK to require users who want to search for strings involving literal "|" or "&" to use quoted terms like "foo|" (with quotes).
- Avoid parsing any of ",", ".", "!", "?", ";", or ":" as logical operators or wildcards because they could be consindered "common punctuation" which users are likely to search for in their simple searches.

I think that sounds going into good directions, but I have two comments:

(1) *****

Imo, we really want a NOT operator while we're adding operators anyway. Assuming we agree to implement operator signs, using preceding "!" as in "!foo" (without quotes) is an obvious candidate for NOT operator.

I suggest that we only parse "!" as a logical operator if it is preceded by a space, so that:

foo !bar  -> advanced expression
finds: .contains "foo" AND NOT "bar"
(because a preceding exclamation mark is not a common use case for simple search)

foo! bar  -> simple string
finds: .contains "foo<exclamation mark>" AND "bar"
(because an appended exclamation mark might be common use case for simple searches)

foo!bar -> simple string
finds: .contains "foo<exclamation mark>bar"
(not 100% sure, but I think this is more likely as a simple search, e.g. many special names of events, locations and such contain exclamation marks; for operator search, imo we should require the "!" to be preceded by a space)

foo "!bar" -> quoted string
finds: .contains "foo" AND "<exclamation mark>bar"

I suppose that, as a courtesy, we should also make
foo ! bar  -> advanced expression (exclamation mark with spaces on both sides)
find: .contains "foo" AND NOT "bar"

(2) ******

What about precedence/precedence indicators?
I suggest that we hijack "(" and ")" as precedence indicators ONLY IF we find any other operator sign "|", "&", "!", and only with the distinct use of "!" suggested above. E.g.:

foo (bar|baz)    -> advanced expression, because operator sign found
finds: .contains "foo" AND (bar OR baz)

foo !(bar baz)    -> advanced expression, because operator sign found
finds: .contains "foo" AND NOT "(bar" AND "baz)"

foo (bar baz)    -> simple expression, because no operator sign found
finds: .contains "foo" AND "(bar" AND "baz)"
There's no point to parse this as an advanced expression because implicit AND has precedence anyway.

foo (bar)        -> simple expression, because no operator sign found
finds: .contains "foo" AND "(bar)"
Finding single words in brackets looks like a common enough usecase so that we should not just ignore the brackets, and we can't use them for advanced expression anyway because it they can't change precedence of a single word.

*******************************

> So:
> "foo | bar"
> is literally searching for: "foo" <space> <pipe> <space> "bar"

+1, probably reasonable
 
> Whereas:
> foo | bar
> is searching for "foo" or "bar"

+1, that's the cool part!

> same for:
> foo OR bar

+1, maybe later as explained above
I assume that we DO require capitalization for those word operators, right?

> In other words, reasonable magic if you don't quote, exactly what you type
> when you do quote (or at least less magic; it could make sense to make
> white-space more forgiving of at least newlines).

+1. How does that behave now?

> Probably the most useful thing is describing to the user what we are
> actually searching for in these cases.

+1, documentation is definitely needed.

> I do like Bryan's blurb that Thomas
> called out in comment 15 as a means of letting the user then change
> behaviour, although I think it's preferable as a last resort for
> implementation complexity reasons.

FTR, I just mentioned Bryan's proposal without making recommendations.
I agree with Andrew that that proposal might be a bit complex, especially for implementation. Otoh, we need to make sure that we don't bring too many surprises to default simple searches that might involve our magic characters.

Do we want a visual feedback somewhere when we parse magic characters as an advanced search? Perhaps an (i) aka information icon appearing on the right of "No results | 33 messages" string that we show on quickfilterbar, then when you hover or click the icon, you get information like "Your search was parsed as an advanced search..."

>  (As always, your comprehensive coverage
> of search is very appreciated!)

Thanks :) 

> Note that I don't have a strong opinion as to the future direction of the
> quick filter bar

By all means, I hope we will keep quick filter bar and the powerful search algorithm it has now (and more powerful after this bug).

Global search is by magnitudes inferior wrt design, behaviour, usability and precision for many scenarios, while stronger in scope and speed of full text searches.

> , but I am interested in trying to maximize consistency
> between Thunderbird and whatever ends up happening with the desktop version
> of the Gaia e-mail app.  (I am planning to work on that as a personal
> side-project at least.)

If the Gaia search is powerful and good enough, perhaps yes...

Comment 24

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #23)

There had to be typos somewhere... sorry, too much copy and paste, fixed here:

> (2) ******
> 
> What about precedence/precedence indicators?
> I suggest that we hijack "(" and ")" as precedence indicators ONLY IF we
> find any other operator sign "|", "&", "!", and only with the distinct use
> of "!" suggested above. E.g.:

foo (bar|baz)    -> advanced expression, because operator sign found
finds: .contains "foo" AND ("bar" OR "baz")

foo !(bar baz)    -> advanced expression, because operator sign found
finds: .contains "foo" AND NOT ("bar" AND "baz")

foo (bar baz)    -> simple expression, because no operator sign found
finds: .contains "foo" AND "bar" AND "baz"
There's no point to parse this as an advanced expression because implicit AND has precedence anyway.
 
foo (bar)        -> simple expression, because no operator sign found
finds: .contains "foo" AND "(bar)"
Finding single words in brackets looks like a common enough usecase so that
we should not just ignore the brackets, and we can't use them for advanced
expression anyway because it they can't change precedence of a single word.
 
> *******************************
> 
> > So:
> > "foo | bar"
> > is literally searching for: "foo" <space> <pipe> <space> "bar"
> 
> +1, probably reasonable
>  
> > Whereas:
> > foo | bar
> > is searching for "foo" or "bar"
> 
> +1, that's the cool part!

Comment 25

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #24)
> (In reply to Thomas D. from comment #23)
> 
> There had to be typos somewhere... sorry, too much copy and paste, fixed
> here:

And still wrong... let's try again:

> > (2) ******
> > 
> > What about precedence/precedence indicators?
> > I suggest that we hijack "(" and ")" as precedence indicators ONLY IF we
> > find any other operator sign "|", "&", "!", and only with the distinct use
> > of "!" suggested above. E.g.:
> 
> foo (bar|baz)    -> advanced expression, because operator sign found
> finds: .contains "foo" AND ("bar" OR "baz")
> 
> foo !(bar baz)    -> advanced expression, because operator sign found
> finds: .contains "foo" AND NOT ("bar" AND "baz")
> 
> foo (bar baz)    -> simple expression, because no operator sign found
> finds: .contains "foo" AND "bar" AND "baz"

finds: .contains "foo" AND "(bar" AND "baz)"

> There's no point to parse this as an advanced expression because implicit
> AND has precedence anyway
...and precedence indicators only make sense when there is at least one OR or NOT operator (or "|", "!" respectively.

> foo (bar)        -> simple expression, because no operator sign found
> finds: .contains "foo" AND "(bar)"
> Finding single words in brackets looks like a common enough usecase so that
> we should not just ignore the brackets, and we can't use them for advanced
> expression anyway because it they can't change precedence of a single word.
>  
> > *******************************
> > 
> > > So:
> > > "foo | bar"
> > > is literally searching for: "foo" <space> <pipe> <space> "bar"
> > 
> > +1, probably reasonable
> >  
> > > Whereas:
> > > foo | bar
> > > is searching for "foo" or "bar"
> > 
> > +1, that's the cool part!

It's all much easier than it looks (I hope), but we also need to get the edge cases right to avoid introducing new bugs ;)

Comment 26

[:aceman]

Comment on attachment 770969 [details] [diff] [review]
Patch v2

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

Looks good:)

::: mail/base/modules/quickFilterManager.js
@@ +969,5 @@
>      }
>  
> +    /*
> +     * Look for spaces around | (OR operator) and remove them
> +     *

Surplus * .

Comment 27

[Suyash Agarwal (:sshagarwal)]

Thanks sir.
So, is this acceptable? Or the changes mentioned in the above comments need to be incorporated? If so, please change the bug description and summary in support of it, or else, we can try getting this in :)

Comment 28

[:aceman]

I would try to get this in as is. Just get ui-r from bwinton an a review. Because it is not sure you can build the full operator logic without any heavy infrastructure changes.

Comment 29

[:aceman]

Of course I'd like Thomas to create a new bug for the extended and full operator functionality with a good and polished proposal on how it should work and what operators should be supported.

Comment 30

[Suyash Agarwal (:sshagarwal)]

And I'll interfere in it for sure :P

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 770969 [details] [diff] [review]
Patch v2

So, I can't seem to get this to work, and I don't see any tests that would let me figure out what new functionality you've added here, so I'm going to have to give it a ui-r- until there are some tests for me to try and follow manually.

Thanks,
Blake.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 770969 [details] [diff] [review]
Patch v2

Cancelling review due to r-.

Comment 33

[alta88]

in the meantime, TotalQuickFilter adds this back, as well as implements things like inverted filtering, textfilter term negation, multiple textfilter groupings with OR/AND, etc.

https://addons.mozilla.org/en-US/thunderbird/addon/totalquickfilter/

Comment 34

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3 with tests

Okay so here is the patch with tests (passing).

Comment 35

[:aceman]

Comment on attachment 813278 [details] [diff] [review]
Patch v3 with tests

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

The | works in the QFB. The test also seems sound. Changing any of the fields in the tested messages makes the test fail (as the expressions no longer match them). Just please add a description to the new test function on what it actually tests (the OR operator) and mention this bug number. You have my f+, good work Suyash!

Comment 36

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3 + test

Added the comment.

Thanks.

Comment 37

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 813334 [details] [diff] [review]
Patch v3 + test

(As per comment 10, I don't review Thunderbird/MailNews code anymore.)

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 813334 [details] [diff] [review]
Patch v3 + test

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

I'd like to see this simplified a bit, before testing it

::: mail/base/modules/quickFilterManager.js
@@ +969,5 @@
>      }
>  
> +    /*
> +     * Look for spaces around | (OR operator) and remove them
> +     */

can't all this just be a regexp? something like
aSearchString.replace(/\s*\|\s*/g, "|"));

@@ +1008,5 @@
> +      }
> +      if (pipeFound) {
> +        aSearchString = "|" + aSearchString;
> +        pipeFound = false;
> +      }

... and this more like aSearchString.split("|") and then add for all matches if more than one

@@ +1059,5 @@
> +            term.booleanAnd = !firstClause;
> +            useOR = false;
> +          } else {
> +            term.booleanAnd = firstClause;
> +          }

term.booleanAnd = !useOR && firstClause;

or what?

::: mail/test/mozmill/quick-filter-bar/test-filter-logic.js
@@ +267,5 @@
> +                           setSubject3]);
> +
> +  set_filter_text("foo | junk");
> +  assert_messages_in_view([setSenderFoo, setSubject1,
> +                           setSubject2, setSubject3]);

could you also test the cases with spaces, and cases with more than one bar character?

Comment 39

[Suyash Agarwal (:sshagarwal)]

So, what do you expect "a b | c" to return?

Comment 40

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #39)
> So, what do you expect "a b | c" to return?

Does the current patch provide some way for the user to indicate OR-precedence?
As I explained in comment 21, it is impossible (or at least nonsensical/highly ambiguous) to implement logical OR and accept phrases like "a b | c" without implementing a more or less explicit system of marking OR-precedence (more explicit: using brackets; less explicit: using spaces or absence thereof, i.e. some OR search terms are more closely grouped together - without spaces - than others and we read that as implicit precedence).

There's an inherent ambiguity in shorthand notations like "a b | c". It could mean either of:
1) (a AND b) OR c
2) a AND (b OR c)

Of course we could easily agree that in absence of any precedence markers, we default to an implicit precedence of AND where we find spaces (1), but what if the user *wants/needs* to express a logical precedence of OR (2)? Introducing OR logic without implementing some way of marking OR-precedence to allow searches like 2) is such a serious limitation of use cases that we could just as well forget the whole story. Think of scenarios:

I'm looking for Peter's message about his holiday in Spain last year. Of course, I won't remember exactly which words he used in subject (could be any of "holiday", OR "trip", OR "journey", OR "Spain"), but I know for sure the message must have "Peter". So what I really want to search for is 

Peter AND (holiday OR trip OR journey OR Spain)

I think that's a really frequent everyday scenario: user knows the name of an involved person, but subject words might vary or be less certain. Or you communicated with a group of people about an exactly defined subject: (Peter OR Paul OR Dave) AND "holiday". Even for such very basic searches, you already need a way of marking OR-precedence, because simple implicit precedence of AND over OR is no longer enough. Fwiw, without implementing ways of indicating OR-precedence, user would have to use this search term for the same result:

"Peter holiday | Peter trip | Peter journey | Peter Spain"
-> (Peter AND holiday) OR (Peter AND trip) OR (Peter AND journey) OR (Peter and Spain)
-> Peter AND (holiday OR trip OR journey OR Spain)

That's certainly better than status quo (without OR search), but it's really annoying and ineffective.
Furthermore, we would be violating ux-error-prevention if we parse
"a b|c" -> (a AND b) OR c (because we don't appreciate the visual fact that b and c are grouped).
(which I think is what the current patch does).

If I don't know if it was Peter or his wife Anne who wrote the message, I need (logically):

(Peter OR Anne) AND (holiday OR trip OR journey OR Spain)

If I want to search for my correspondence with peter on that subject, I need:

Peter AND Thomas AND (holiday OR trip OR journey OR Spain)

Without offering OR-precedence marking to the user, it is not possible to search for any of these common scenarios.

I see two ways of enabling user to mark OR-precedence: either

a) require explicit precedence markers:
User needs to add brackets for marking or-precedence:
1) "a b | c" -> (a AND b) OR c     (with that solution, spaces around | don't matter)
2) "a (b | c)" -> a AND (b OR c)

b) suffice with "implicit" precedence markers (absence of space before | to indicate OR precedence):
User indicates OR-precedence by omitting space before | (so that two terms are grouped together more closely than the rest):
1) "a b | c" -> (a AND b) OR c  (and we'd also parse "a b |c" to mean the same)
2) "a b|c" -> a AND (b OR c)    (it's clear that b|c are grouped closer than the rest, so we take that as precedence; same principle applies in BMO quicksearch when you use foo,bar vs. foo ,bar or foo , bar)

Personally, I'd tend towards variant b) "a b|c" using implicit OR-precedence via "no spaces around |", because
- easier to implement: no extra parsing for brackets
- easier to use: no need for putting brackets
- more versatile, less hassle: users can still comfortably search for brackets and bracketed expressions as literal strings, without having to use quotation marks
- implicit OR-precedence by visual grouping via ommision of space before | blends in nicely with current implicit space = AND

In both variants, we have to deal with *edge cases* (quite literally) like orphaned brackets, or brackets present without an OR operator, or adjacent | with a space only on one side:

a) brackets as precedence markers
"(a b" -> (a AND b) (alternatively, we could parse this bracket as a literal -> "(a" AND b ?
"(a b) c -> a AND b AND c (It's unlikely that this means "(a" AND "b)" AND c...)

b) no space before | as OR-precedence marker
"a b |c" -> (a AND b) OR c (I think that's clear)
"a b| c" -> a AND (b OR c) (?)

Otherwise, if users want to search for
literal brackets "(" ")" in variant a), or
literal "|" in variant a) or b),
I like asuth's suggestion in comment 22 that we require double quotes for parsing anything with special meaning as a literal string:

"(foo)" (sic, with quotes) -> search for "(foo)" (string with brackets).
"a | b" (sic, with quotes) -> search for "a | b" (as one string).

I'd appreciate if others could provide their feedback on this instead of ignoring and then end up with patterns that won't work or bring about new UX problems.

Comment 41

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #40)

> a) require explicit precedence markers:
> User needs to add brackets for marking or-precedence:
> 1) "a b | c" -> (a AND b) OR c     (with that solution, spaces around |
> don't matter)
> 2) "a (b | c)" -> a AND (b OR c)

and on more thought, a) needs b) for the sake of ux-error-prevention (but b) does not need a)):

3) "a b|c" -> a AND (b OR c)

This means imo
b) no-space + | -> OR-preference is a MUST whichever way, and
a) allowing brackets for marking OR-preference is optional (but currently neither needed nor helpful).
We might need brackets if we lateron introduce " !foo" operator for (NOT foo), but unfortunately that's another story again.

> b) suffice with "implicit" precedence markers (absence of space before | to
> indicate OR precedence):
> User indicates OR-precedence by omitting space before | (so that two terms
> are grouped together more closely than the rest):
> 1) "a b | c" -> (a AND b) OR c  (and we'd also parse "a b |c" to mean the
> same)
> 2) "a b|c" -> a AND (b OR c)    (it's clear that b|c are grouped closer than
> the rest, so we take that as precedence; same principle applies in BMO
> quicksearch when you use foo,bar vs. foo ,bar or foo , bar)

Comment 42

[Blake Winton (:bwinton) (:☕️)]

Having "a b | c" be different than "a b|c" seems like a recipe for disaster to me.

Yeah, it's a clever way of distinguishing precedence, but on the downside, it's a clever way of distinguishing precedence.  ;)

Comment 43

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #39)
> So, what do you expect "a b | c" to return?

I think it should be a AND (b OR c) - that's also what google does, it seems. 
Let's not make whitespace around operators significant...

Comment 44

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #43)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #39)
> > So, what do you expect "a b | c" to return?
> 
> I think it should be a AND (b OR c) - that's also what google does, it

Fair enough... But with that, how can users express (a AND b) or c?

https://support.google.com/websearch/answer/136861?p=adv_operators&hl=en
https://support.google.com/mail/answer/7190

Comment 45

[Jim Porter (:squib)]

The easy way to do this would be to decide if we should be left-associative or right-associative, and let users change the behavior by changing the order of the operands. Left-associative is probably the more sensible choice here. Then you'd do the following:

(a AND b) OR c  =>  a b | c
a AND (b OR c)  =>  b | c a

This probably falls apart for really complex cases, but I have serious doubts that people will often run into that.

Comment 46

[Thomas D. (:thomas8)]

(In reply to Jim Porter (:squib) from comment #45)
> The easy way to do this would be to decide if we should be left-associative
> or right-associative, and let users change the behavior by changing the
> order of the operands. Left-associative is probably the more sensible choice
> here. Then you'd do the following:
> 
> (a AND b) OR c  =>  a b | c
> a AND (b OR c)  =>  b | c a

Interesting. So instead of AND-precedence, or OR-precedence, that's left-associative precedence (first come, first served, from left to right, left wins).

Jim, how would you parse a b|c?
According to left-associative, should it be (a AND b) OR c?
But according to visual intuition and ux-error prevention, it looks much more like a AND (b OR c)?
That's the tricky part imo... and we have to deal with it simply because it's possible that users do that...

> This probably falls apart for really complex cases, but I have serious
> doubts that people will often run into that.

For full flexibility/complexity, we probably need brackets (even google has them, at least in the mail search). But we can do that in another bug, perhaps together with introducing some NOT operator (Google has "-").

Comment 47

[Jim Porter (:squib)]

(In reply to Thomas D. from comment #46)
> Jim, how would you parse a b|c?
> According to left-associative, should it be (a AND b) OR c?
> But according to visual intuition and ux-error prevention, it looks much
> more like a AND (b OR c)?
> That's the tricky part imo... and we have to deal with it simply because
> it's possible that users do that...

Whitespace should never matter for operator precedence. In any mathematical expression, I can abuse whitespace to make you think it'll parse a certain way when it does not, in fact, do so. For example: 2 * 3+4. (Anyone who remembers their order of operations will tell you that's 10.)

Besides, users (probably) won't even know about the existence of the | operator without being told, so when they're told about it, they can also be told that it's left-associative.

I mean, the real solution to this is to define a formal grammar for searches and use it wherever appropriate in Thunderbird, but that's well beyond the scope of this bug, I think.

Comment 48

[Thomas D. (:thomas8)]

Using left-associative, (how) can I express things like

a AND (b OR c OR d)?
(a AND b) OR (c AND d)?
(a OR b) AND (c OR d)?

which is a common usecase for TB, e.g.
Person AND (subjectword1 OR subjectword2 OR subjectword3)

Is google left-associative? (I think not, they seem to have general OR-precedence, only brackets can override that)

Comment 49

[Jim Porter (:squib)]

You're not going to get all of this in a single field unless you write an actual grammar. Period. Honestly, I think a lot of the examples you're posting are beyond the scope of a "quick filter", and belong in either faceted search or the old Ctrl+Shift+F search. I'm really not entirely convinced that we should even have "|" in the quick filter to begin with, but I wouldn't mind seeing a real discussion about defining a formal grammar for search queries across TB.

Again though, that's beyond the scope of this bug and should probably be discussed on tb-planning until we can decide exactly how we want to handle that.

(The first one is really easy though: "b | c | d a".)

Comment 50

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #49)
> You're not going to get all of this in a single field unless you write an
> actual grammar. Period.

Ok, to be fair, you could probably get this without writing a "grammar" per se, but I'd r- it given the chance, since I can't imagine it being maintainable in that case.

Comment 51

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 813334 [details] [diff] [review]
Patch v3 + test

So, there are a lot of questions left, but while we discuss them, I think that we can land this patch.  Particularly since the vast majority of people who type "|" in the quickfilter bar are likely going to expect it to mean "or".

So, ui-r=me, and I look forward to the continuing discussion.

Comment 52

[alta88]

(In reply to Suyash Agarwal (:sshagarwal) via email)

My thoughts on this:

1. TotalQuickFilter did not initially implement a textfilter syntax/grouping of AND and OR since I think it's overkill in the vast majority of cases; the terms a simply passed without grouping with the booleanAnd flag set per term based on what precedes it.  But I may do it for the exercise eventually.

2. TotalQuickFilter implements immediate feedback error state in the box and it should be done here if syntax is implemented.  Further, it parses the input in great detail to support quoted phrases (have you considered that?), and escaped quotes, and backslashes used to escape, and pipes in quoted phrases, etc etc.  And explains that "word1| word2" means "word1|" AND "word2" while "word1 | word2" means OR.

3. The backend supports any grouping you construct from a syntax, but you will find that the current code, when it ripped out OR, also specifically crippled groupings by flattening the query in _groupifyTerms(), regardless of your beginsGrouping/endsGrouping in the nsITerm array.

4. TotalQuickFilter rips out _groupifyTerms().

5. Translating a grouping syntax into a usable UI is hard.  I did the barest of attempts by turning OR and NOT into toggleable buttons and turning a word/phrase into a discrete unit indicated by a button.  So as a string is entered, the UI is built.  You can't know if A B | C means
-> (A+B)|C
-> A+(B|C)
so you have to autobuild a UI with an assumption, maybe background hilight a grouping, and let the user tune it if it's not what's meant.

jmo.

Comment 53

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4 + test

Okay, so this works in all possible ways as tried in the test cases.
It works for an input search string of the form:
a | b | c d e | f | g h | i j k.....
treating this input string as =>
(a OR b OR c) AND d AND (e OR f OR g) AND (h OR i) AND j AND k...
So, though this isn't complete grammar and there can be varied cases, but this works for most of the possible queries one can fire.

Please let me know if this isn't correct.
Moreover, I have tried to cover most of the functionality in the test, please let me know if the test coverage isn't complete.

Thanks.

Comment 54

[:aceman]

Comment on attachment 8377122 [details] [diff] [review]
Patch v4 + test

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

The patch looks very nice, just a small change code-wise :)
I tried some combinations and it seems to work in my tests.
Congrats for including a mozmill test!

::: mail/base/modules/quickFilterManager.js
@@ +986,5 @@
>          continue;
>        }
>  
> +      let searchTerms = aSearchString.split(" ");
> +      for (let index = 0; index < searchTerms.length; ++index) {

I'd like index++, but maybe there is a reason to be as it is.

::: mail/test/mozmill/quick-filter-bar/test-filter-logic.js
@@ +271,5 @@
> +  set_filter_text("test | bar");
> +  assert_messages_in_view([setToBar, setSubject1, setSubject2,
> +                           setSubject3, setMail1, setMail2]);
> +
> +  set_filter_text("foo | test");

Please also have a case where there are multiple spaces before |.

Comment 55

[Jim Porter (:squib)]

Comment on attachment 8377122 [details] [diff] [review]
Patch v4 + test

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

Seems ok to me. If you could file a followup bug for us to write a proper syntax for this (something that supports parens, etc), that'd be nice.

::: mail/base/modules/quickFilterManager.js
@@ +986,5 @@
>          continue;
>        }
>  
> +      let searchTerms = aSearchString.split(" ");
> +      for (let index = 0; index < searchTerms.length; ++index) {

index++ is preferred, unless you're in C++, which ironically prefers ++index. :)

@@ +1011,5 @@
>          let firstClause = true;
>          term = null;
> +        let splitPhrases = groupedPhrases.split("|");
> +        for (let phrase of splitPhrases) {
> +          for each (let [tfName, tfValue] in Iterator(aFilterValue.states)) {

Please remove the "each". We don't need that anymore.

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 8377122 [details] [diff] [review]
Patch v4 + test

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

::: mail/base/modules/quickFilterManager.js
@@ +967,5 @@
>        if (aTerm)
>          terms.push(aTerm);
>      }
>  
> +    /*

A documentation starter is /**. Otherwise it's just a multi-line comment.

@@ +970,5 @@
>  
> +    /*
> +     * Look for spaces around | (OR operator) and remove them.
> +     */
> +    aSearchString = aSearchString.replace(/\s*\|\s*/g, "|");

shouldn't this preferably be \s+ instead of \s*

Comment 57

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8377122 [details] [diff] [review]
Patch v4 + test

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

::: mail/base/modules/quickFilterManager.js
@@ +970,5 @@
>  
> +    /*
> +     * Look for spaces around | (OR operator) and remove them.
> +     */
> +    aSearchString = aSearchString.replace(/\s*\|\s*/g, "|");

No, using \s* is the way to go because use that operator and with + it would only replace if whitespaces can be found on both sides of |

@@ +975,3 @@
>      while (aSearchString) {
>        if (aSearchString.startsWith('"')) {
>          let endIndex = aSearchString.indexOf(aSearchString[0], 1);

Can you simplify that to:
let endIndex = aSearchString.indexOf('"', 1);

@@ +986,5 @@
>          continue;
>        }
>  
> +      let searchTerms = aSearchString.split(" ");
> +      for (let index = 0; index < searchTerms.length; ++index) {

You could use searchTerms.forEach(searchTerm => addTerm(searchTerm)); instead.

::: mail/test/mozmill/quick-filter-bar/test-filter-logic.js
@@ +271,5 @@
> +  set_filter_text("test | bar");
> +  assert_messages_in_view([setToBar, setSubject1, setSubject2,
> +                           setSubject3, setMail1, setMail2]);
> +
> +  set_filter_text("foo | test");

What do you think about adding a test with other whitespace characters (like tabulator)?

@@ +281,5 @@
> +  assert_messages_in_view([setSubject1, setSubject2, setMail2]);
> +
> +  set_filter_text("test | foo  bar |logic");
> +  assert_messages_in_view([setSubject1, setSubject2,
> +                           setMail1, setMail2]);

IMHO, you should also always run |assert_messages_not_in_view|, http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1741

Comment 58

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5 + test

Made the suggested changes.
Re-requesting review from mconley as I don't know whether review cancellation was intentional.

Thanks.

Comment 59

[Thomas D. (:thomas8)]

Comment on attachment 8381275 [details] [diff] [review]
Patch v5 + test

I think there's not much left for me to say, after all what I've said before. Search grammars aren't easy to define nor to agree on, so we agreed on the lowest common denominator to allow/speed up landing this. Fair enough. It's a great improvement over status quo, allowing for a large range of important usecases. So I'm thankful for that, to Suyash and everyone who contributed here.

We need someone to write up / complement existing user documentation for this, and I'd appreciate if somebody else could do that.
I wonder if there's any way we could hint about this functionality inside the UI.

I haven't fully checked the patch code, but it looks like it does what it's meant to do.
I haven't looked at the tests.
Given the discussion here, and the complexity of the matter, I think we should be a bit more explicit in code and test comments.

>+        let splitPhrases = groupedPhrases.split("|");

Perhaps there should be an explanatory comment above this line?
// This is where we start parsing | as OR operator. OR operators get precedence: foo bar | baz is parsed as foo AND (bar OR baz). This isn't a full grammar yet, but goes a long way to make quick filter more powerful (see discussion on bug 586131).

> /**
>+ * Verify that the quickfilter bar has OR functionality using
>+ * | (Pipe character) - Bug 586131
>+ */

Imo, this comment should be more clear about the currently implemented behaviour which is not obvious from here or bug 586131.
/**
 * Verify that the quickfilter bar has OR functionality using
 * | (Pipe character). OR operators get precedence: foo bar | baz
 * is parsed as foo AND (bar OR baz). This isn't a full grammar yet,
 * but goes a long way to make quick filter more powerful
 * (see discussion on Bug 586131)
 */

Comment 60

[:aceman]

Comment on attachment 8381275 [details] [diff] [review]
Patch v5 + test

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

My nits were addressed, thank.

Comment 61

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8381275 [details] [diff] [review]
Patch v5 + test

Cool.  It would be nice if we could handle this everywhere, but for now, this seems good to me.

(As a side note, using mozilla.governance for my tests was a really depressing idea.  ;)

Comment 62

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8381275 [details] [diff] [review]
Patch v5 + test

Carrying over review from squib, mkmelin and ui-r from bwinton.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

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

Comment 64

[Matt]

(In reply to Thomas D. from comment #59)

> 
> We need someone to write up / complement existing user documentation for
> this, and I'd appreciate if somebody else could do that.

I will have a look at the existing documentation and see what can be done.

> I wonder if there's any way we could hint about this functionality inside
> the UI.

Perhaps we take a leaf from the changes to remote images and incorporate a link to more information.  Firefox has a handy blue question mark icon that would fit nicely into the quick filter bar

Note that the previous in application link "filelink" is the most accessed Thunderbird support document by a factor of 5.  Offering to link to relevant task based information at the time is supportive and something Thunderbird should do more of. Based on the filelink experience our users agree.

Comment 65

[Thomas D. (:thomas8)]

[tb31features][relnote]

This is another powerful feature addition (given that it was gone since TB 3.1, and is more powerful now), allowing users to combine AND and OR searches in message quick filter bar, hence catering for a whole range of new use cases. It should definitely be mentioned and explained in relnotes so that users can discover this new feature (which is otherwise completely undiscoverable, until we do something along comment 64 and provide an in-app link to help page e.g. (?) icon in qfb, or an informative tooltip at least).

Matt, any progress on documentation for this feature (your comment 64)?