65190 - query form: add comparison type "all words as substrings"

Status: VERIFIED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Andreas Franke (gone)]

Both "(case-insensitive) substring" and "all words" comparison types are not
optimal solutions if you want to query for several words:

>> The problem with the Simple one ["(case-insensitive) substring"] 
>> is that, if people put in a few words relevant to their bug, it's 
>> reasonably unlikely that those words will appear in that exact order 
>> in the summary. Could we do "all words" instead?
>
>i've never found "all words" to be useful, because it won't let you type
>"bookmarklet" and get hits for both "bookmarklet" and "bookmarklets".

C.Begle's SimpleSearch <http://www.mozilla.org/quality/help/simplesearch.html>
solves this problem by generating boolean charts on the fly. The javascript for
this page is located at <http://www.mozilla.org/quality/help/bugreport.js>. 
(A more structured and further developed version can be found in bug 61561.)

It would greatly simplify things if you had an "all words as substrings" option
in all comparison type dropdowns on the default query page, too. Probably it
should even be the default.

Similarly, there could be an "any words as substrings" option, too.

Comment 1

[Stephan Niemz]

Good idea ... I would like that feature, too.

Comment 2

[Andreas Franke (gone)]

Attachment: diff -u buglist.cgi

Comment 3

[Andreas Franke (gone)]

I don't know what I'm doing, but it seems to work... :)

Comment 4

[Andreas Franke (gone)]

Attachment: diff -uw query.cgi

Comment 5

[Andreas Franke (gone)]

Gerv, if you like, you can clean up that patch...

Comment 6

[Andreas Franke (gone)]

Attachment: diff -u buglist.cgi (works with special chars, too)

Comment 7

[Gervase Markham [:gerv]]

I don't think we should have "any words as substrings" It has the potential to
produce loads and loads of wrong results, if people use short words which happen
to appear lots in longer words. 

I can't see much use for "none of the words as (case-insens.) substring" either
- I think we are in danger of bloating that drop-down even more.

"All words as substrings" is a fine idea, though.

Re: the patches: 

GetByWordList1 is a bad name for a function. What makes it different from
GetByWordList? Explain that a bit in the name.

Why not "foreach $word ..." rather than foreach $w... $word = $w?

You need to remove the old code you left in comments.

Have you tested this patch yourself?

Gerv

Comment 8

[Andreas Franke (gone)]

Re: "any", "no": I can drop them, although I think they can be useful:
- "any" can be useful if combined with other restrictions if you have
alternative names for one thing (like thumb/slider or address/location/url)
- "no" can be useful if you have many things you want to exclude (don't have an
example at hand)
I added them just to have parity with "all words", "any words" and "no words".

I think the functionality for "any" and "no" could stay in the backend
(buglist.cgi) and just be removed from the frontend (query.cgi).
And that's only one line per option you want to get rid off. 
Not sure if that needs a new patch for query.cgi, but I can make one if you tell
me which ones to drop exactly. The first part of the changes in query.cgi is for
the Summary/StatusWhiteboard/URL/Description dropdowns, the second part for the
boolean charts feature.

> - I think we are in danger of bloating that drop-down even more.

That's true, but then let's ask if we need all the other existing options, too.

> GetByWordList1 is a bad name for a function. What makes it different from
> GetByWordList? Explain that a bit in the name.

Well, I just wanted to make it work. So I copied existing code from
GetByWordList (that is used for the ordinary "all words" case) and tried to make
it produce the same output the ordinary "substring" would do (for every word).

I'll post a new patch shortly.

The main question I have is: Are the names for the options ok? "allsubstr" etc.?
Or should it be "allwordssubstr" or "allwordsassubstrings" or something else?
This probably can't be changed easily in the future...

> Have you tested this patch yourself?

Yes, sort of. I tried 5 or so different combinations, and they seemed to work.
They included different options (all/any), the ordinary "Summary" field vs.
boolean charts, one vs. multiple words for the value, and plain [a-zA-z] words
vs. a sequence of some special chars.
The problem is that I don't know enough perl to understand what I'm doing.
Yesterday I didn't even know that @ was a naming convention for lists ...

Comment 9

[Gervase Markham [:gerv]]

OK, let's compromise. Remove "no words" from the front-end, in all instances :-)

Do you think we need all the existing options?

allwordssubstr looks good. Try and be as consistent as possible with the current 
names. And GetByWordListSubstr().

Don't worry about not knowing Perl - that's exactly the way I started hacking on 
Bugzilla ;-)

Gerv

Comment 10

[Andreas Franke (gone)]

Attachment: diff -u buglist.cgi, version 2, hopefully provably correct

Comment 11

[Andreas Franke (gone)]

Attachment: diff -uw query.cgi, version 2

Comment 12

[Andreas Franke (gone)]

Ok, I have attached version 2 for both diffs. Assuming the buglist.cgi script
was correct before the patch, it should be provably correct with the latest
patch applied.

buglist.cgi: 

1. Is it true that the following lines are equivalent? (word.toLowerCase)
$word =~ tr/A-Z/a-z/;
$word = lc($word);

2. Is lc(SqlQuote($v)) is the same as SqlQuote(lc($v))? (commutativity)

3. What does metaquote mean in SqlQuote(metaquote($v)) ?

BTW, I learned that changing "allsubstr" to "allwordssubstr" can have unexpected
consequences: suddenly "allwords" matched instead of "allwordssubstr", so I had
to place the new code _before_ the old one.

query.cgi:

I have removed the "no words as substrings" option.

1. What about the wording of the options? I have left out "case-insensitive" on
the hardcoded Summary/Description/URL/StatusWhiteboard dropdown because it would
increase the width of the selectbox a lot, however I have included it in the
boolean charts option descriptions.
1a. What about grammar & spelling? Is the "s" at the end correct?

2. What about the order of the options in the selectbox? Currently the order of
"case-sensitive substring" and "case-insensitive substring" in the main part of
the query form is different from the oder in the boolean charts dropdown,
probably because case-insensitive substring was the default. Should this be
changed? Should the "wordssubstr" options occur before the plain "words" options
or after them, or should they even be at the top?

Please review the latest patches, I will do some testing now and report back.

Comment 13

[Andreas Franke (gone)]

Testing looks fine. If you want to check it in as-is, you have my blessing :)

Comment 14

[timeless]

search for subject:plugin and description that
"doesn't have any of these words"|"contains none of the following"
: real, flash, shockwave
search for description that "contains one of the following"
: crash, hang, slows down, freezes
notice that i use , to express string separation, i'm not sure it's possible to 
ask buglist to excluse the substring 'slows down' and exclude the substring 
'hang', but that's how i might use it.

Comment 15

[Tara Hernandez]

This is potentially interesting, but is not a 2.12 candidate.

Comment 16

[Andreas Franke (gone)]

timeless: 

I'm not sure if I'm understanding what you're saying here:
> search for subject:plugin and description that
> "doesn't have any of these words"|"contains none of the following"
> : real, flash, shockwave
Is this a request to put the "no words as substrings" option back in (that Gerv
wanted to be taken out)? If so, where do you want it: in both the main part of
the query form and the boolean charts, or in the boolean charts only?

> search for description that "contains one of the following"
> : crash, hang, slows down, freezes
> notice that i use , to express string separation, i'm not sure it's possible 
> to ask buglist to excluse the substring 'slows down' and exclude the substring 
> 'hang', but that's how i might use it.
Currently the input is split into words at non-empty sequences of (space or
comma): /[\s,]+/ . This is the same splitting mechanism used for "all words",
"any words" etc. If you want that to be changed, please submit a separate RFE. 
[ Note that the QuickSearch tool described in bug 61561 supports quoting. There
it's possible to type 'desc:crash,desc:hang,desc:"slows down",desc:freezes' to
express your query. ]

Comment 17

[timeless]

> Is this a request to put the "no words as substrings" option back in
> (that Gerv wanted to be taken out)? 
yes.
but i'm not familiar w/ how this query system works so i'm not going to press 
any of my comments.

Comment 18

[Andreas Franke (gone)]

Tara: Why is this not a candidate? According to Dave, this is the "better fix"
for bug 66184 which was nominated for 2.12 by Gerv. Nominating for 222.12 in the
hope that is evaluated before the end of this century... ;)

Comment 19

[Andreas Franke (gone)]

If you are concerned that people may have different opinions about the UI part
(query.cgi), then what about just checking in the backend part (buglist.cgi)?
- The backend patch doesn't touch existing functionality, it just adds some.
- I think I can prove that the added functionality is correct: For treating of
the input value as multiple words, it uses exactly the same mechanisms that are
used for the "allwords" case. And building the SQL fragment for each word uses
exactly the same mechanism that is used for the "substr" case.

The frontend part of this patch is trivial and could be release noted.

Comment 20

[Dave Miller [:justdave]]

If you look at the timestamps, you'll notice Tara removed the 2.12 AFTER bug 
66184 was closed.  She probably would have removed that one, too, if it had still 
been open.  Not to say it's a bad patch (looks like a nice one to me) just we're 
attempting to get 2.12 out the door ReallySoonNow and trying to avoid the attack 
of creeping featuritis which keeps sneaking up on us :)

Comment 21

[Andreas Franke (gone)]

So the problem is that this fix is a feature whereas bug 66184 is not? Then
let's reopen bug 66184 and fix that one ;)

Comment 22

[Dave Miller [:justdave]]

dunno...  Tara's calling the shots here...  Tara?

Comment 23

[Andreas Franke (gone)]

I'm not sure if Tara is looking at any bugs without 2.12 in the status
whiteboard... Well, the thing is this: if you want to release today, then I can
accept that it's too late. Or if you release a 2.14 one week after 2.12 with all
the patches included that didn't make it into 2.12, that's fine, too. But
looking at what happened in the past there can be more than half a year between
two releases of Bugzilla, there can be really long times when the installation
at mozilla.org is not upgraded. Now I do care less about all the other bugzilla
installations out there that could use this feature (maybe except of my own one,
but that's already patched). So just tell me (assuming the patch doesn't have
any problems) that mozilla.org's bugzilla will have this before the end of
February, and I will happily shut up :)

Comment 24

[Andreas Franke (gone)]

I think the lack of responses has proven my theory that bugs without 2.12 in the
status whiteboard are off the radar. So I'm putting 2.12 back in order to get a
response from Tara.

Here are the reasons for accepting at least the backend support (buglist.cgi):
- The "all words as substrings" is more useful than both "all words" and 
  "(case-insensitive) substring" in most cases.
- Currently it can only be realized with JS. The change to buglist.cgi makes it 
  possible to build non-JS based query forms with this feature.
- I believe the patch is provably correct.
- The backend patch is the major change here. The UI change (query.cgi) is
  trivial and can be release noted if controversial (e.g. with a link to the 
  patch attached here:
  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23354 ).
  Every bugzilla maintainer should be able to apply it by hand.

Comment 25

[Andreas Franke (gone)]

This is my whinemail for the week Feb. 12th - Feb. 18th. This bug has a patch
that is waiting for review and checkin.

Comment 26

[Tara Hernandez]

Okay, Andreas has convinced me.  Accepting it back on 2.12 list.

Comment 27

[Dave Miller [:justdave]]

moving to real milestones...

Comment 28

[Tara Hernandez]

reassigning...

Comment 29

[Chris Yeh]

moving to 2.14 after discussion with Andreas.

Comment 30

[Dave Miller [:justdave]]

query.cgi patch v2 r= justdave

buglist.cgi patch v1 w/spec chars and patch v2 both have my r=, but I need to 
know which one to check in.  Can we get some people to make some more comments 
and settle this argument?  :-)

Do we or don't we want "none of these words as substrings" on the string match 
type popup?

Comment 31

[Andreas Franke (gone)]

For buglist.cgi, it's definitely version 2 (attachment 23353 [details] [diff] [review]) that should go in
(I think it's even provably correct, under the assumption that "allwords" and
"substr" are correct).

For query.cgi, my suggestion would be to check in version 2 (attachment 23354 [details] [diff] [review])
as-is, update b.m.o to the tip, and see if anyone complains. Then query.cgi can
easily be adapted according to what the majority wants.

I can't see any other way to get the broad usability testing that is needed to
optimize / fine-tune the order of options in the menus. People need to be able
to try them out first.

You can try out these patches on http://bugzilla.mathweb.org/query.cgi , but of
course that doesn't have the huge mass of bugs that is available on mozilla.org.

Comment 32

[Dave Miller [:justdave]]

haven't seen any other comment, v2 it is for both.

checked in.

Comment 33

[Dave Miller [:justdave]]

Moving to Bugzilla product

Comment 34

[Andreas Franke (gone)]

Verified fixed. Tested on b.m.o.