Bug 531785 - Auto infix search in Open Resource

[pbodnar]

Basic feature description

A new match rule RULE_SUBSTRING_MATCH is introduced in the
SearchPattern. The constant itself is copied as-is from JDT's variant
of SearchPattern, yet its usage and purpose differs.

When used and prefix search is not enforced, then:

• search is done as if "*" was specified at the pattern start
• CamelCase search doesn't require the 1st pattern char to be the very
  1st char of the matched string

Use this in Resource search dialogs for the default and file name
pattern, while also list "prefix matches" firstly and extend the matched
characters highlighting to still work correctly.
Path (container) and extension searches should not be affected by this.

Further changes:

• users can start a search pattern with ">" to enforce the old
  prefix-only search

For seeing how autoInfixSearch differs from the old one, one can compare "SearchPatternAuto.java" with "InfixSearchPatternAuto.java".

Yet to be investigated / implemented

• present the SearchPattern's autoInfixSearch feature on UI layer: change the Open Resource dialog label
• API: introduce SearchPattern.RULE_SUBSTRING_MATCH or similar instead of SearchPattern.autoInfixSearch
• update corresponding user documentation (see Document new searching behavior of the Open Resource dialog eclipse.platform.common#89)

Historical issues identified that could / should be solved later on

1. SearchPattern.RULE_CASE_SENSITIVE - this flag seems to be ignored by the actual SearchPattern implementation

2. SearchPattern.camelCaseMatch() could be optimized to check strings' lengths firstly. Something like:

    int patternLength = patternEnd - patternStart;
    int nameLength = nameEnd - nameStart;
    if (nameLength < patternLength) {
    	return false;
    }
   

3. Constants like SearchPattern.END_SYMBOL are just private and special chars checking is hard-coded in dependent classes like FilteredResourcesSelectionDialog

4. Inefficient matches highlighting: ResourceItemLabelProvider.getMatchPositions() and related seem to contain some tricky code and logic. -> see Make matches highlighting in Resource dialog more simple and efficient #184

[mickaelistria]

For consistency, couldn't it be a SearchPattern.RULE_INFIX_MATCH or something like that?

[pbodnar]

For consistency, couldn't it be a SearchPattern.RULE_INFIX_MATCH or something like that?

Possibly yes, I will have to look at how it could be incorporated into the existing code base, but also how to make it easy to use for the client code. Maybe a related question to be sure: We want to keep the behavior backwards compatible for existing clients (so that they won't have to change their code in order to keep the old behavior), don't we?

[mickaelistria]

We want to keep the behavior backwards compatible for existing clients (so that they won't have to change their code in order to keep the old behavior), don't we?

As much as possible yes. If some important feature does require to break some behavior, this needs to be discussed with the PMC for a decision.

[pbodnar]

So, as can be seen from the commits, I have implemented some other points from the todo-list in the description. The next big thing in the list is this one:

• make sure found resources are properly highlighted (see existing ResourceItemLabelTest)

After this, I would probably move on to the last:

• present the SearchPattern's autoInfixSearch feature on UI layer somehow

Plus of course, some documentation pages should be updated...

---

BTW The Jenkins build is currently failing on some error I don't quite understand. It is possible that the error is quite unrelated to commits in this PR, but I don't know really...

---

A final thought, after digging into this search topic more, I think that replacing the current "wildcards / camelCase / etc.-based searches" with a brand-new "fuzzy search" as known from the other IDEs out there (i. e. when writing simply & easily "illexc" finds "IllegalArgumentException") is the way to go. Yet, even finishing this one (which is rather an extension of the existing searches) could be a good start before moving on to a more "radical" change like that?

As usual, feedback and hints are welcome. :)

[pbodnar]

This could be hopefully sufficient?

The existing comparator code is written in a way which doesn't seem to let us cover the code with corresponding unit tests easily. But hopefully it doesn't matter that much in this case.

[pbodnar]

No one protested, so assuming this sorting implementation is sufficient and closing this thread.

[pbodnar]

Maybe reopen for a while with a question...

The current comparator implementation part:

				// Prioritize names starting with the pattern
				char patternFirstChar = getFirstFileNameChar(pattern);
				if (patternFirstChar != 0) {
					patternFirstChar = Character.toLowerCase(patternFirstChar);
					m1 = patternFirstChar == Character.toLowerCase(s1.charAt(0));
					m2 = patternFirstChar == Character.toLowerCase(s2.charAt(0));
					if (!m1 || !m2) {
						if (m1) {
							return -1;
						}
						if (m2) {
							return 1;
						}
					}
				}

... is simple, but because of checking just the first letters, it wrongly evaluates for example name "Aabc" as a "name starting with the pattern" if the pattern is "ab". IMO the only 100% working solution would be either to de-facto re-implement the matching here (which is somewhat already done in the matches highlighting code) or to change the whole searching solution to keep metadata for every successful match. Until then, can we say that the solution above is "good enough" for now and maybe add some inline warning comment about it not being perfect?

[pbodnar]

Status update: I've tried some real-world manual testing, finding I need to fix handling of some edge cases in / with the new code. Approximately these:

1. when entering just " " or "> ": ArrayIndexOutOfBoundsException
2. nothing is found when typing-in (not copy-pasting) ">abc", because ">" already finds nothing (and "subFilter concept" kicks in, filtering an empty set on entering ">a" etc.)
3. ResourceFilter should probably work with SearchPattern.initialPattern in order to handle dots (".") in the search string correctly, while pattern is being typed in the search dialog

[pbodnar]

Wow, it looks like everything is fine again :), I guess the previous build failures were given by the June release / freeze period somehow.

Anyway, as can be seen, I have (hopefully) coped with the 3 points described above - by the 3 follow-up commits. So if someone could review and give a feedback, it would be great. Mainly the 3rd point was quite challenging, as I had to drill through many historical tickets to find out what is going on there in the ResourceFilter class & retesting various scenarios manually (no easy tests automation in this area without a huge refactoring, it seems).

I'm also not sure if I have correctly selected to merge the master branch into mine, or if I should do a rebase instead. So please let me know, provided I should do this differently.

[HannesWell]

I'm also not sure if I have correctly selected to merge the master branch into mine, or if I should do a rebase instead. So please let me know, provided I should do this differently.

A rebase is perfectly fine. We try to avoid merge commits.
Usually it does not matter when a change happened at your computer and what rounds you spinned until the change is completed, what matters is what changes were finally committed to the master. And this is best tracked with a simple linear history where each commit is a self-contained change. So unless you have unrelated changes in this PR or large changes that can be understood more easily if applied incrementally you can squash your commits into one or at least few self-contained commits.

[pbodnar]

@HannesWell, thanks for your quick info. So while still probably quite far from being a fully implemented PR, I have at least master-rebased, reorganized & squashed the 8 commits into 3 relatively "self-contained" ones, which I believe can somewhat simplify the reviews.

[pbodnar]

OK, so I have finished some smaller amendments and the code is ready for review & testing now (except for the 2 remaining tasks from PR description). I think I will take a small break from this one. ;)

So @mickaelistria, @vogella or whoever is watching this issue, can you please have a look? It would be great to have some concrete suggestions before moving on, so that we avoid a major rework after everything is implemented. Thanks in advance. :)

[mickaelistria]

I'm quite busy with a handful of high priority tasks. I'll try to review it next week if no other committer does look at it in the meantime (but having someone else to review would be very welcome!)

[vogella]

I will be soon on vacation for a few weeks so I cannot review at the moment. I suggest to squash the commits into one commit using interfactive rebase and force push to your branch. Also I would suggest to remove the WIP from the PR, WIP is usually something committers ignore as WIP indicates that the change is not yet complete but to me it looks (on a quick glance) that this change is actually ready for review and that you expecting feedback on it.

Thanks @pbodnar for working on this, it is really nice to see an issue being worked on that makes IntelliJ better than Eclipse

[vogella]

@pbodnar this one needs manual rebase.

[pbodnar]

OK, freshly rebased on the latest master, squashed into 1 commit, updated the description and removed prefix "WIP". All ready for a review, hopefully. :)

[mickaelistria]

This should really be a MatchRule like others. Keeping this consistencyy would reduce the amount of new API to be created.

[mickaelistria]

Also, I've looked a bit on the web to find a formal definiton of infix search and couldn't find something relatively standard. Can you please properly define it, or use another more standard term?

[pbodnar]

This should really be a MatchRule like others. Keeping this consistencyy would reduce the amount of new API to be created.

OK, I can see we could remove those 2 new constructors and (currently unused) isAutoInfixSearch() afterwards, right? I expect that for backwards compatibility reasons, we won't make the new rule the default one in the no-argument constructor, so can someone please suggest the best "Eclipse code-base" way of how to make the API developer-friendly, so that for example RULE_EXACT_MATCH | RULE_PREFIX_MATCH | RULE_PATTERN_MATCH | RULE_CAMELCASE_MATCH | RULE_BLANK_MATCH | RULE_INFIX_MATCH doesn't have to be copied to every client code which would like to use the new behavior? Maybe introduce a dedicated public constant(s) for the various rule sets? I can also imagine introducing a builder for the SearchPattern class, but that would probably go above the scope of this PR...?

Also, I've looked a bit on the web to find a formal definiton of infix search and couldn't find something relatively standard. Can you please properly define it, or use another more standard term?

Regarding the infix search term, there are many pages using this term, but I can see it doesn't have to be necessarily the best in our context (and it would deserve an explanatory sentence in the javadoc). What to use instead though? Page What is it called when you search the middle of a string instead of the beginning? suggests unanchored search, which comes from the world of regex, but maybe contains search is a "more natural" candidate, although it has a verb in its name?

[mickaelistria]

JDT's SearchPattern calls it SUBSTRING_MATCH if I understand correctly. That's a good term to reuse.

[pbodnar]

OK, I will try to refactor the code - to introduce a new flag RULE_SUBSTRING_MATCH...

[vogella]

@pbodnar did you update your API baseline? I downloaded https://download.eclipse.org/eclipse/downloads/drops4/S-4.26RC2-202211231800/ and set my workspace to use this a API baseline in which I did not get the error you described. Lets see what CI thinks of this change.

[vogella]

@pbodnar unfortunately this change now conflicts with your Javadoc changes. Can you rebase your change onto master?

[pbodnar]

@vogella, thanks for your assistance. :)

So I have rebased, squashed and moved version / @since to 3.128.0 / 3.128. Not sure about the GitHub build (seems like a random error?), but the Jenkins build fails on this now:

21:00:02.429 [ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.0-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release)
on project org.eclipse.ui.ide: Only qualifier changed for (org.eclipse.ui.ide/3.20.0.v20221208-2047).
Expected to have bigger x.y.z than what is available in baseline (3.20.0.v20221027-2208) -> [Help 1]

Can you please help once again?

[mickaelistria]

The version of org.eclipse.ui.ide in the MANIFEST.MF should be increased by +0.0.100 to signify some changes have happened compare to last release. Also, by the way, its requirement on org.eclipse.ui.workbench should set 3.128.0 as minimal version as it uses the new APIs there.

[mickaelistria]

OK, but what about the org.eclipse.ui.internal.navigator.resources.actions.GotoResourceDialog which extends the extended org.eclipse.ui.dialogs.FilteredResourcesSelectionDialog? No need for "explicit" dependency here?

No, because it doesn't consume new API methods or fields. It's still compatible with the older one.

Apart from that, it looks like this will be necessary according to the latest build failure (as tests have been modified as well):

You got it ;)

... and hopefully that could be all?

Hopefully yes.

I wonder if these versions checks could be automated on localhost (dev's machine) somehow...

Some of those checks can already be seen in the IDE if you configure a PDE baseline referencing latest release.
Some others are currently not available in the IDE; and a local build isn't really faster than a remote one, so it's OK to just push to PR to test.

[pbodnar]

@mickaelistria, thanks for the tips. So I have done the 2 changes in manifests as proposed + another forced rebase. And finally "All checks have passed". :)

[mickaelistria]

Thank you!
Can you please add a note to the N&N document as well ( https://github.com/eclipse-platform/www.eclipse.org-eclipse-news )?

[vogella]

@pbodnar the new funcationality is super nice and easy to use, thanks a bunch for this.

One of my clients mentioned that it would also be very nice if the * in the middle of a work could be avoid, e.g. in the following example:

the match for TaskColumnPropertyAccessor continues if:

• I continue to write column, e.g. taskcolumn
• I write TCP

but not if I write taskproperti have to add an * to match this one, e.g. task*propert

Would you be interested in also implementing the automatic * in the middle of the search term? According to my client this is standard behavior in VSCode and IntelliJ.

[pbodnar]

@vogella, thanks for your feedback. I have actually kinda proposed this kind of change at https://bugs.eclipse.org/bugs/show_bug.cgi?id=531785#c13 - I called it "fuzzy search" there. :)

Anyway, yes, it seems doable (with matching algorithm itself being quite trivial?), with some major refactorings (API and UI changes would have to be consulted?), given I already somewhat understand the searching code behind. I'm just not sure if it still pays off when Eclipse Theia is on its way, and it is also harder for me to find some extra time for Eclipse contributions these days... Maybe when there will be more votes on this? ;)

[vogella]

Anyway, yes, it seems doable (with matching algorithm itself being quite trivial?),

If the matching algorithm can be adjusted easily why not change it without any API changes? I think it would be OK for our search to become better without offering API to go back to the "not so nice" behavior.

[pbodnar]

@vogella, good point, just extending the behavior could probably be the simplest way, provided it would by widely accepted. Question is, what with all those existing SearchPattern's flags like RULE_EXACT_MATCH, RULE_PREFIX_MATCH, RULE_PATTERN_MATCH etc., moreover when we wouldn't introduce any new one...?

Besides that, matching, sorting and highlighting of the matches are currently done independently of each other. As seen already within this MR, this has got some limits (mainly in regards to sorting and coping with duplicated executions of +- the same stuff). I can imagine a major redesign where output from matching wouldn't be just a boolean - instead, it would be some structured data which could be directly used within sorting and highlighting. But once again, this is not quite trivial to do, IMHO...