Bug 184688 - org.eclipse.ui.dialogs.SearchPattern has a few painful glitches
Summary: org.eclipse.ui.dialogs.SearchPattern has a few painful glitches
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.3 RC1   Edit
Assignee: Krzysztof Michalski CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks: 178549 179485 182726
  Show dependency tree
 
Reported: 2007-04-30 08:05 EDT by Markus Keller CLA
Modified: 2007-06-05 14:30 EDT (History)
4 users (show)

See Also:
Tod_Creasey: review+

Attachments
Fix (1.96 KB, patch)
2007-04-30 08:07 EDT, Markus Keller CLA 		no flags Details | Diff
Patch rollback all inconvenient changes (6.81 KB, patch)
2007-05-07 12:14 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Rollback + fixed usages of String#trim() (3.39 KB, patch)
2007-05-08 04:59 EDT, Markus Keller CLA 		no flags Details | Diff
Patch to SearchPattern test cases (3.84 KB, patch)
2007-05-08 06:39 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Patch is respone for Szymon comment. (10.59 KB, patch)
2007-05-09 09:24 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2007-04-30 08:05:33 EDT 
HEAD

org.eclipse.ui.dialogs.SearchPattern as a few painful glitches that need to be corrected ASAP, since they cause subtle problems in the Open Type dialog.

I'll attach a fix for these problems:
- calls String#trim() when only last character should be deleted
- should detect string ending in " " or "<" as RULE_EXACT_MATCH, not RULE_CAMELCASE_MATCH
- isValidCamelCaseChar(..) always returns true

To reproduce the problems of the blocked bugs, you usually have to type very quickly (or paste the whole pattern).
Comment 1 Markus Keller CLA 2007-04-30 08:07:13 EDT 
Created attachment 65376 [details]
Fix
Comment 2 Tod Creasey CLA 2007-04-30 08:27:57 EDT 
Krzysztof please test this against the bugs Markus has outlined on the fastest machine you have
Comment 3 Krzysztof Michalski CLA 2007-04-30 09:32:07 EDT 
This bug is result of enhancement described in bug#174349. Tod, Markus, if it necessary we could rollback all our changes.
Comment 4 Markus Keller CLA 2007-04-30 11:47:41 EDT 
That brings us again knee-deep into the problems of bug 176017.

The fix for bug 174349 only seems to work in the ui.dialogs.SearchPattern but not in the jdt.core version. This can be seen with the following scenarios:

a) type "HM", wait for the search to finish, and the append a "<"
    => looks like bug 174349 is fixed (since ui.dialogs.SearchPattern refilters the matches for HM)

b) new Open Type dialog, paste "HM<"
    => no matches (since jdt.core's SearchPattern does not support that pattern)

I'm not sure what else the "fix" for bug 174349 and later changes changed in SearchPattern, so it's hard to tell for me whether reverting is better than just releasing my fix. The calls to String#trim() need to be removed even if you decide to revert.
Comment 5 Krzysztof Michalski CLA 2007-05-07 12:14:42 EDT 
Created attachment 66138 [details]
Patch rollback all inconvenient changes

I reverted all incorrect enhancements described in bug 174349. Moreover inside patch i attached changes to SearchPatern test cases. Markus could you test it and give us a feedback.
Comment 6 Markus Keller CLA 2007-05-08 04:59:33 EDT 
Created attachment 66263 [details]
Rollback + fixed usages of String#trim()

The "rollback" patch fixes most problems, but still leaves the wrong calls to String#trim() where only the last character should be removed. It's e.g. strange that " ArrayList " finds matches, but " ArrayList" does not.

Here's my (final?) patch based on Krzysztof's rollback, with the trim() problems fixed in initializePatternAndMatchRule(..).

Krzysztof, please review again.
Comment 7 Krzysztof Michalski CLA 2007-05-08 06:39:30 EDT 
Created attachment 66269 [details]
Patch to SearchPattern test cases

I reviewed Markus corrections. Everything looks good. Moreover i attached patch to test cases.
Comment 8 Markus Keller CLA 2007-05-08 06:52:09 EDT 
> Moreover i attached patch to test cases.
Oops, I'm sorry. Forgot that I skipped the tests because I didn't have the in my workspace.
Comment 9 Tod Creasey CLA 2007-05-08 14:56:05 EDT 
+1 from me. I'll need a second committer to commit this - Szymon would you check please?
Comment 10 Szymon Brandys CLA 2007-05-09 08:57:29 EDT 
There are wrong docs for methods in SearchPatternAuto class. Look at testExactMatch1 and testExactMatch2.
Strings from comments "abcd" are different than these used in test. You made some typos in docs too, like
"funtionality".

Otherwise looks good.
Comment 11 Krzysztof Michalski CLA 2007-05-09 09:24:29 EDT 
Created attachment 66455 [details]
Patch is respone for Szymon comment.

Patch contains fixes for SearchPattern and SearchPatternAuto.
Comment 12 Szymon Brandys CLA 2007-05-09 09:33:03 EDT 
+1 from me
Comment 13 Tod Creasey CLA 2007-05-09 09:55:16 EDT 
Krzysztof please make the updates Szymon suggests and then we can re-review
Comment 14 Szymon Brandys CLA 2007-05-09 10:43:46 EDT 
+1 from me for the latest patch.
Comment 15 Tod Creasey CLA 2007-05-09 10:52:35 EDT 
Patch released to HEAD for build >20070416