Bug 224328

Created attachment 129775 [details]
IDE log

I think I noticed that as well...

This is not a bug in implementation, but it may be a bug in design.

If regular expression checkbox is unchecked, the text pattern still supports simple wildcard characters - "*" for any string and "?" for any character. They can be escaped with "\". Info about this is displayed only in tooltip of the "Containing Text" field (which may be the problem).

See related bug 207913.

I guess that the cause of the problem is some of this:

 - Info about wildcards in simple pattern should be more visible.
 - Simple patterns should not be supported at all.
 - Users should be able to choose between: Literal, Simple Pattern, Regexp.

What do you think?

My two cents:

I like to use Find in Projects to search for literal strings in my data files, and those * and ? characters often show up. So after I copy&paste text into the search field, I have to go back and manually add \ to escape those characters. That can be annoying if I'm searching for longer strings. So I'd like to be able to turn all pattern-matching off entirely, to make it easier to copy-and-paste.

I wouldn't personally miss this simple regex matching feature, as I'm comfortable with the full Regular Expression syntax and would just use that.

(In reply to comment #5)
> I wouldn't personally miss this simple regex matching feature, as I'm
> comfortable with the full Regular Expression syntax and would just use that.
Thank you for your feedback.
I'm afraid that removing this feature would be a regression.

I can think of three solutions:

1) Add a checkbox to disable wildcard characters in simple patterns.
2) Add a button to escape all wildcards characters in the field
   (the button can be enabled/visible only if the field contains one
    or more wildcards).
3) Provide three match types: exact, simple, regexp.

I would vote for number 2. The UI change is very small and the algorithm doesn't have to be modified at all. One extra click is required, but I think it is acceptable.

Do you have any opinion? Thanks.

Created attachment 130456 [details]
UI prototypes

#3 seems by far the most elegant to me; it draws attention to the existence of the simple-expression feature in a really intuitive way -- especially if that is the default option (I mean the first option in the combobox), so that the informational line above it "(* = any string, etc.)" applies to it by default. 

The #1 option has the problem of what happens if the simple-expression and the regular-expression checkboxes are both checked. (As a user, from that GUI I would assume the regex checkbox takes precedent, largely due to how it's placed... but I'm not sure that everyone would read it that way.)

The #2 option has the gotcha that it's not repeatable: if I end up clicking it twice it will result in a bad search (or else you'd have to make make it assume that \* is already an escaped * and not turn it into \\*, which creates its own special cases.)

So I would vote for #3!

(In reply to comment #8)
> #3 seems by far the most elegant to me; it draws attention to the existence of
> the simple-expression feature in a really intuitive way -- especially if that
> is the default option (I mean the first option in the combobox), so that the
> informational line above it "(* = any string, etc.)" applies to it by default. 
> 
> The #1 option has the problem of what happens if the simple-expression and the
> regular-expression checkboxes are both checked. (As a user, from that GUI I
> would assume the regex checkbox takes precedent, largely due to how it's
> placed... but I'm not sure that everyone would read it that way.)
> 
> The #2 option has the gotcha that it's not repeatable: if I end up clicking it
> twice it will result in a bad search (or else you'd have to make make it assume
> that \* is already an escaped * and not turn it into \\*, which creates its own
> special cases.)
> 
> So I would vote for #3!

I fully agree with this comment. From the options offered in Comment #6 the option 3 is by far the cleanest from UEX perspective (it always and unambiguously communicates what it is all about) while the option 2 is actually the worst (context-dependent display of button, unintuitive necessity to manually add escape characters to pasted text, non-repeatability + non-revertability of escape characted adding, etc. , that all would add to user confusion). So I vote for option 3 as well.

Thank you for your comments.
I agree that #3 is the cleanest solution.
(I was just afraid that it makes the UI more complicated.)
So I think #3 is the winner.

A small additional idea:
The dialog becomes quite wide. What about moving the "(test)" link next to the label under the field. If type regular expression is selected, it would be:
"Use java.util.regex.Pattern syntax (test)".

Created attachment 130486 [details]
Mockup wildcard type selection

What would you say to the simple solution as attached ? Tooltips over the radio buttons would give explanation what each choice means. I guess presenting the options using radiobuttons with explanatory tooltips makes the functionality more discoverable than a solution based on dropdown list, while keeping the UI relatively uncluttered. Maybe the wording in radiobutton names can be improved..

(In reply to comment #11)
> Created attachment 130486 [details]
> Mockup wildcard type selection
> What would you say to the simple solution as attached?
It's nice and clean, but overall number of controls in the dialog is becoming quite high - two new (radio) buttons. I would also say that the info text under the field is quite handy. On the other hand, mixing checkboxes and combo box in one line doesn't look very well. So, I'm not very sure what is better, but I'm still slightly inclined to #3.

It's close between #3 and the new mockup (radiobuttons), but I vote for #3 again; the reason is that it's hard to parse the radio buttons and the checkboxes in such close proximity.

This time it was almost a tie for me, so I had a colleague who doesn't use Netbeans weigh in and she came away with the same conclusion, so I'll vote 3 again :)

However as a side note, her comment was that the term "basic wildcards" (from mockup #4) is a much better descriptor than "simple pattern" (which is what you called it in #3)

Hope that helps!

Created attachment 130730 [details]
Screenshot

Screenshot of modified dialog, solution #3.

> However as a side note, her comment was that the term "basic wildcards" (from
> mockup #4) is a much better descriptor than "simple pattern" (which is what 
> you called it in #3)
>
> Hope that helps!
Sure, it helps, thank you.

> A small additional idea:
> The dialog becomes quite wide. What about moving the "(test)" link next to the
> label under the field. If type regular expression is selected, it would be:
> "Use java.util.regex.Pattern syntax (test)".
In the screenshot, I used this layout, but I guess it will be better to place it next to the "Type:" combo box. It will make the dialog wider, but it is probably acceptable. Right?

(At first I thought that the "test" button should be near the pattern value, but it is not clear that its enabled/disabled state depends on the check box/combo box. And the placement of the "test" button in "Text to Find" and 
File Name Patterns" sections should be consistent.)

Created attachment 130792 [details]
Proposed Patch

Patch on top of patch from bug 222406

Please review the following API change.

Search pattern should specify exact match type:
Regular Expression, Basic Wildcards or Literal.

(Basic wildcards are not supported by Editor search, but the
fallback to Literal Search is transparent for the editor).

Added items:

 - Enum SearchPattern.MatchType which contains all supported match types
 - Method SearchPattern.create(String searchExpression,
      boolean wholeWords, boolean matchCase, MatchType matchType)
 - Method SearchPattern.getMatchType()
   and SearchPattern.changeMatchType(MatchType)
 - Enum SearchPatternController.MultiOption for
   listing SearchPattern properties that can be bound to  combo boxes
 - Method SearchPatternController.bind(MultiOption, JComboBox) for binding 
   SearchPattern properties to combo boxes

Thank you.

R01: The default match type has been changed from Basic Wildcards to Literal on line SearchPattern:159 
R02: For the same method, SearchPattern#create, the javadoc does not specify what happens if regexp is false
R03: I don't think SearchPattern:323 would still work

Created attachment 130823 [details]
Proposed Patch v2

Created attachment 130824 [details]
Difference between patch v1 and v2

(In reply to comment #17)
> R01: The default match type has been changed from Basic Wildcards to 
> Literal on line SearchPattern:159 
Fixed.

> R02: For the same method, SearchPattern#create, the javadoc does not specify
> what happens if regexp is false
Fixed.

> R03: I don't think SearchPattern:323 would still work
Thank you very much for catching this!

I'm still thinking about the default match type. It's true that in previous releases the default in Find in Projects was BASIC, so it should be respected by this API change.
On the other hand, Find in editor performs LITERAL and REGEXP searches only. So, if a non-regexp search pattern, e.g. "** Important comment" is created by editor and then stored, it should be used as LITERAL too in Find in Projects (there is a shared storage of recently used search patterns).

I guess majority of clients of SearchPattern understands non-regexp searches as LITERAL, not BASIC (simpler implementation, more intuitive).
So, maybe it would be more practical to use LITERAL by default.

What do you think?

Thanks for your comments.

> So, maybe it would be more practical to use LITERAL by default.
> 
> What do you think?
> 

I slightly incline to agree that LITERAL might be the more natural default (as you say, when people copy/paste search string from elsewhere then LITERAL is more likely to be the expected mode. There might be people who are used to the current default behavior though who might disagreee - but for them this should be only slightly annoying, provided the choice is persisted. Do you plan to persist this ? 

One more suggestion. Now the label (if I understand it right) is Type:. I guess the better label would be Match:, because it would state more precisely what the choice is about, instead of just Type what might mean anything.

(In reply to comment #20)
> I slightly incline to agree that LITERAL might be the more natural default (as
> you say, when people copy/paste search string from elsewhere then LITERAL is
> more likely to be the expected mode. 

There are two different questions which are not the same, but it is probably better if they are consistent.

1) Default value in the API, used if the SearchPattern is created with factory 
   method that only specifies that the pattern is not regular expression: 

   assertEquals("If not specified exactly, match type should be BASIC",
              MatchType.BASIC, 
              SearchPattern.create("test", false, false, false).getMatchType()
   );

   It is currently (in Proposed Patch v2) BASIC, but I think it 
   would be better to change it to LITERAL.
   Ralph, would it be OK for you?

2) Default value in the UI. I agree it should be LITERAL, too.

> There might be people who are used to the current default behavior though 
> who might disagreee - but for them this should be only slightly annoying,
> provided the choice is persisted. Do you plan to persist this? 
Yes, it will be persisted.

> One more suggestion. Now the label (if I understand it right) is Type:. 
> I guess the better label would be Match:, because it would state more
> precisely what the choice is about, instead of just Type what might 
> mean anything.
Good idea.

Thank you for your comments.

I check API methods which my module uses as a client and I am happy with its implementation. Good work.

(In reply to comment #21)
>    assertEquals("If not specified exactly, match type should be BASIC",
>               MatchType.BASIC, 
>               SearchPattern.create("test", false, false, false).getMatchType()
>    );
> 
>    It is currently (in Proposed Patch v2) BASIC, but I think it 
>    would be better to change it to LITERAL.
>    Ralph, would it be OK for you?

That's ok for me :)

Created attachment 131040 [details]
Proposed Patch v3

(In reply to comment #21)
> 1) It is (the default value in the API) currently BASIC, but I think it
>    would be better to change it to LITERAL.
> 2) Default value in the UI. I agree it should be LITERAL, too.
Both fixed.

> > One more suggestion. Now the label (if I understand it right) is Type:.
> > I guess the better label would be Match:, because it would state more
> > precisely what the choice is about, instead of just Type what might
> > mean anything.
> Good idea.
Fixed.

I've realized that there could be problems with SearchPatternControllers:
1) If the combo box doesn't contain mandatory REGEXP and LITERAL items.
2) If we try to set a SearchPattern (method setSearchPattern) with MatchType 
   that is not contained in the bound combo box.
3) If the combo box contains items that are not instances of MatchType.

Proposed Patch v3 fixes this (some checks added).

Please note that API of SearchPatternController has changed:

 - Enum MultiOption (with a single value) was removed
 - Method bind(MultiOption, JComboBox) was replaced with 
   bindMatchTypeComboBox(textToFindType).

I'm sorry for changing the proposed API. I hope it will be beneficial.
Please review it.

Thank you.

Created attachment 131041 [details]
Difference between patch v2 and v3

Created attachment 131053 [details]
Proposed Patch v4

Updating apichanges.xml.

If there are no objections, I'll integrate tomorrow. Thanks.

Integrated as http://hg.netbeans.org/core-main/rev/3f81b5976967
Thank you for your help.

Integrated into 'main-golden', will be available in build *201302152300* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/3f81b5976967
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #224328: Find in Projects always using regular expressions