Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move TextMatcher from Platform UI to Equinox Common #720

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

ptziegler
Copy link
Contributor

The TextMatcher class is used in the UI component, despite not depending on any UI classes. By moving it to Equinox, it can be used anywhere in the Eclipse Platform.

@ptziegler
Copy link
Contributor Author

Prerequisite to eclipse-platform/eclipse.platform.ui#2567

@vogella
Copy link
Contributor

vogella commented Dec 12, 2024

@tjwatson any concerns here?

@HannesWell
Copy link
Member

I'm really not sure if this is the right place. Equinox common contains code related to the runtime (in the broadest sense).
Having text-handling here seems not to be a good fit.
But while writing this, I see there is already org.eclipse.core.text.StringMatcher, which really surprises me.

@ptziegler
Copy link
Contributor Author

I'm really not sure if this is the right place. Equinox common contains code related to the runtime (in the broadest sense).
Having text-handling here seems not to be a good fit.

The main motivator for all of this are plugin dependencies. Several UI plugins use the TextMatcher and to prevent having to add any new dependencies, I have to find the a common denominator, which is org.eclipse.core.runtime.

But none of the re-exported plugins really have anything to do with text handling.
image

So the reason I picked Equinox is, as you noticed, the StringMatcher, because that's at least somewhat related.

@laeubi
Copy link
Member

laeubi commented Dec 13, 2024

TextMatcher even references StringMatcher in its API doc and as it is more about Strings (in contrast to JFace Documents) I think it is fine to have it here.

@tjwatson
Copy link
Contributor

tjwatson commented Dec 13, 2024

TextMatcher even references StringMatcher in its API doc and as it is more about Strings (in contrast to JFace Documents) I think it is fine to have it here.

The API signature does not reference StringMatcher. The implementation detail does.

@tjwatson any concerns here?

It seems in the spirit of https://bugs.eclipse.org/bugs/show_bug.cgi?id=508611 so I don't have an issue with the functionality existing in this package of Equinox common.

But TextMatcher vs. StringMatcher have arbitrary names that are confusing to one that has never used either of these classes. It seems overkill to add a new slightly differently named class just to add the following public APIs, one which is a static method that could go anywhere:

	/**
	 * Determines whether the given {@code text} matches the pattern.
	 *
	 * @param text String to match; must not be {@code null}
	 * @return {@code true} if the whole {@code text} matches the pattern;
	 *         {@code false} otherwise
	 * @throws IllegalArgumentException if {@code text == null}
	 */
	public boolean match(String text)

	/**
	 * Determines whether the given sub-string of {@code text} from {@code start}
	 * (inclusive) to {@code end} (exclusive) matches the pattern.
	 *
	 * @param text  String to match in; must not be {@code null}
	 * @param start start index (inclusive) within {@code text} of the sub-string to
	 *              match
	 * @param end   end index (exclusive) within {@code text} of the sub-string to
	 *              match
	 * @return {@code true} if the given slice of {@code text} matches the pattern;
	 *         {@code false} otherwise
	 * @throws IllegalArgumentException if {@code text == null}
	 */
	public boolean match(String text, int start, int end)

	/**
	 * Splits a given text into words.
	 *
	 * @param text to split
	 * @return the words of the text
	 */
	public static String[] getWords(String text) {

I think it would be better to enhance StringMatcher to have this functionality instead of introducing a new class.

@ptziegler ptziegler force-pushed the text-matcher branch 2 times, most recently from f678abb to 8ff10ad Compare December 18, 2024 16:11
@ptziegler
Copy link
Contributor Author

I think it would be better to enhance StringMatcher to have this functionality instead of introducing a new class.

That makes sense. Luckily, the TextMatcher effectively extends the StringMatcher, so it was fairly easy to add this new functionality.

@ptziegler ptziegler force-pushed the text-matcher branch 3 times, most recently from 89f7d99 to 4874832 Compare December 18, 2024 17:49
Copy link

github-actions bot commented Dec 18, 2024

Test Results

0 files   -   663  0 suites   - 663   0s ⏱️ - 1h 14m 40s
0 tests  - 2 211  0 ✅  - 2 164  0 💤  -  47  0 ❌ ±0 
0 runs   - 6 777  0 ✅  - 6 634  0 💤  - 143  0 ❌ ±0 

Results for commit 4874832. ± Comparison against base commit 2ae64f5.

♻️ This comment has been updated with latest results.

@ptziegler ptziegler force-pushed the text-matcher branch 2 times, most recently from decd4ed to 66d35bb Compare December 21, 2024 17:44
The TextMatcher class is used in the UI component, despite not depending
on any UI classes. By moving it to Equinox, it can be used anywhere in
the Eclipse Platform.
@laeubi
Copy link
Member

laeubi commented Jan 5, 2025

@tjwatson any more concerns or can we merge this?

@laeubi laeubi merged commit 81972df into eclipse-equinox:master Jan 5, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants