Please use the Apache issue tracking system for new NetBeans issues (https://issues.apache.org/jira/projects/NETBEANS0/issues) !!
Bug 224328 - Find in Projects always using regular expressions
Find in Projects always using regular expressions
Status: RESOLVED FIXED
Product: utilities
Classification: Unclassified
Component: Search
7.2.1
Macintosh Mac OS X
: P3 (vote)
: 7.4
Assigned To: Jaroslav Havlin
issues@utilities
: API_REVIEW_FAST, UI, USABILITY
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-31 04:51 UTC by heimburg
Modified: 2013-02-16 01:53 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
IDE log (97.48 KB, text/plain)
2012-12-31 04:52 UTC, heimburg
Details
UI prototypes (37.75 KB, image/png)
2013-01-21 16:32 UTC, Jaroslav Havlin
Details
Mockup wildcard type selection (3.21 KB, image/png)
2013-01-22 13:10 UTC, Petr Somol
Details
Screenshot (32.61 KB, image/png)
2013-01-28 15:44 UTC, Jaroslav Havlin
Details
Proposed Patch (116.28 KB, patch)
2013-01-29 15:15 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch v2 (118.42 KB, patch)
2013-01-30 09:42 UTC, Jaroslav Havlin
Details | Diff
Difference between patch v1 and v2 (4.71 KB, patch)
2013-01-30 10:03 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch v3 (121.75 KB, patch)
2013-02-05 16:06 UTC, Jaroslav Havlin
Details | Diff
Difference between patch v2 and v3 (16.62 KB, patch)
2013-02-05 16:08 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch v4 (121.54 KB, patch)
2013-02-06 09:36 UTC, Jaroslav Havlin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description heimburg 2012-12-31 04:51:56 UTC
The "Find in Projects" window seems to always use regular expressions even if the checkbox is not checked.

Repro:
- With one or more projects open, choose Edit->Find in Projects... from the menu
- Type "**" (without quotes) in the Containing Text field.
- Uncheck the "Regular Expression" checkbox if it is checked
- Enter valid search patterns in the File Name Patterns box
- Click Find

Expected: a listing of occurrences of two consecutive asterisk characters in the open project source files.

Actual Result: the search window will claim that it has "Found 5000 matches in 1 file so far." (Regardless of the content of said file.) The search is aborted.


Product Version = NetBeans IDE 7.2.1 (Build 201210100934)
Operating System = Mac OS X version 10.8.2 running on x86_64
Java; VM; Vendor = 1.7.0_09
Runtime = Java HotSpot(TM) 64-Bit Server VM 23.5-b02
Comment 1 heimburg 2012-12-31 04:52:04 UTC
Created attachment 129775 [details]
IDE log
Comment 2 Marian Mirilovic 2013-01-02 17:05:06 UTC
I think I noticed that as well...
Comment 3 Jaroslav Havlin 2013-01-03 14:06:55 UTC
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.
Comment 4 Jaroslav Havlin 2013-01-07 11:01:02 UTC
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?
Comment 5 heimburg 2013-01-07 12:30:03 UTC
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.
Comment 6 Jaroslav Havlin 2013-01-21 16:30:43 UTC
(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.
Comment 7 Jaroslav Havlin 2013-01-21 16:32:17 UTC
Created attachment 130456 [details]
UI prototypes
Comment 8 heimburg 2013-01-22 07:44:45 UTC
#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!
Comment 9 Petr Somol 2013-01-22 08:44:35 UTC
(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.
Comment 10 Jaroslav Havlin 2013-01-22 09:01:30 UTC
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)".
Comment 11 Petr Somol 2013-01-22 13:10:57 UTC
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..
Comment 12 Jaroslav Havlin 2013-01-22 13:53:53 UTC
(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.
Comment 13 heimburg 2013-01-22 22:27:40 UTC
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!
Comment 14 Jaroslav Havlin 2013-01-28 15:44:47 UTC
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.)
Comment 15 Jaroslav Havlin 2013-01-29 15:15:10 UTC
Created attachment 130792 [details]
Proposed Patch

Patch on top of patch from bug 222406
Comment 16 Jaroslav Havlin 2013-01-29 15:24:44 UTC
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.
Comment 17 Ralph Ruijs 2013-01-29 19:22:28 UTC
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
Comment 18 Jaroslav Havlin 2013-01-30 09:42:51 UTC
Created attachment 130823 [details]
Proposed Patch v2
Comment 19 Jaroslav Havlin 2013-01-30 10:03:54 UTC
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.
Comment 20 Petr Somol 2013-01-31 11:49:01 UTC
> 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.
Comment 21 Jaroslav Havlin 2013-02-04 13:48:57 UTC
(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.
Comment 22 Milutin Kristofic 2013-02-04 15:00:07 UTC
I check API methods which my module uses as a client and I am happy with its implementation. Good work.
Comment 23 Ralph Ruijs 2013-02-04 15:02:09 UTC
(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 :)
Comment 24 Jaroslav Havlin 2013-02-05 16:06:45 UTC
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.
Comment 25 Jaroslav Havlin 2013-02-05 16:08:20 UTC
Created attachment 131041 [details]
Difference between patch v2 and v3
Comment 26 Jaroslav Havlin 2013-02-06 09:36:00 UTC
Created attachment 131053 [details]
Proposed Patch v4

Updating apichanges.xml.
Comment 27 Jaroslav Havlin 2013-02-13 10:57:50 UTC
If there are no objections, I'll integrate tomorrow. Thanks.
Comment 28 Jaroslav Havlin 2013-02-14 16:35:19 UTC
Integrated as http://hg.netbeans.org/core-main/rev/3f81b5976967
Thank you for your help.
Comment 29 Quality Engineering 2013-02-16 01:53:08 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo