-
Notifications
You must be signed in to change notification settings - Fork 180
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
Performance improvement for find & replace all in Batch Mode. #2268
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,4 +100,23 @@ | |
* @param text the substitution text | ||
*/ | ||
void replaceSelection(String text); | ||
|
||
/** | ||
* @return true, if its able to handle batch replacements. | ||
*/ | ||
Check warning on line 106 in bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/IFindReplaceTarget.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
Check warning on line 106 in bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/IFindReplaceTarget.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
|
||
default boolean canBatchReplace() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not required after extracting a new FindReplaceTargetExtension. Instead, we assume that any extender of FindReplaceTargetX can perform all of it's operations |
||
return false; | ||
} | ||
|
||
/** | ||
* @param findString the string to find. | ||
* @param replaceString the string to replace found occurrences. | ||
* @param wholeWordSearch search for whole words. | ||
* @param caseSensitiveSearch case sensitive search. | ||
* @param regexSearch RegEex search. | ||
* @param incrementalSearch search in selected lines. | ||
*/ | ||
Check warning on line 118 in bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/IFindReplaceTarget.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
Check warning on line 118 in bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/IFindReplaceTarget.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
|
||
default int batchReplace(String findString, String replaceString, boolean wholeWordSearch, boolean caseSensitiveSearch, boolean regexSearch, boolean incrementalSearch) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure why a method declaration in an interface has a method body and why that even is legal... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats because it shouldn't break any other implementers of this interface. By default I've disabled the batch replace in the interface to avoid breaking / compilation errors in all other implementers.. |
||
return -1; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package org.eclipse.jface.text.internal; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a copyright-header (in all other files too) |
||
|
||
import org.eclipse.osgi.util.NLS; | ||
|
||
Check warning on line 4 in bundles/org.eclipse.text/src/org/eclipse/jface/text/internal/Messages.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
|
||
public class Messages extends NLS { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Messages is not a helpful name - why are we even extracting this into a separate class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this pattern is how most of the nls support is provided by the wizard. |
||
private static final String BUNDLE_NAME= Messages.class.getPackageName() + ".messages"; //$NON-NLS-1$ | ||
|
||
public static String RegExUtils_0; | ||
|
||
public static String RegExUtils_IllegalPositionForRegEx; | ||
static { | ||
// initialize resource bundle | ||
NLS.initializeMessages(BUNDLE_NAME, Messages.class); | ||
} | ||
|
||
private Messages() { | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
package org.eclipse.jface.text.internal; | ||
|
||
import java.util.regex.Pattern; | ||
import java.util.regex.PatternSyntaxException; | ||
|
||
Check warning on line 5 in bundles/org.eclipse.text/src/org/eclipse/jface/text/internal/RegExUtils.java Jenkins - Eclipse Platform / Compiler and API ToolsOther
|
||
public class RegExUtils { | ||
|
||
/** | ||
* Converts a non-regex string to a pattern | ||
* that can be used with the regex search engine. | ||
* | ||
* @param string the non-regex pattern | ||
* @return the string converted to a regex pattern | ||
*/ | ||
public static String asRegPattern(String string) { | ||
StringBuilder out= new StringBuilder(string.length()); | ||
boolean quoting= false; | ||
|
||
for (int i= 0, length= string.length(); i < length; i++) { | ||
char ch= string.charAt(i); | ||
if (ch == '\\') { | ||
if (quoting) { | ||
out.append("\\E"); //$NON-NLS-1$ | ||
quoting= false; | ||
} | ||
out.append("\\\\"); //$NON-NLS-1$ | ||
continue; | ||
} | ||
if (!quoting) { | ||
out.append("\\Q"); //$NON-NLS-1$ | ||
quoting= true; | ||
} | ||
out.append(ch); | ||
} | ||
if (quoting) | ||
out.append("\\E"); //$NON-NLS-1$ | ||
|
||
return out.toString(); | ||
} | ||
|
||
/** | ||
* Substitutes \R in a regex find pattern with {@code (?>\r\n?|\n)} | ||
* | ||
* @param findString the original find pattern | ||
* @return the transformed find pattern | ||
* @throws PatternSyntaxException if \R is added at an illegal position (e.g. in a character set) | ||
* @since 3.4 | ||
*/ | ||
public static String substituteLinebreak(String findString) throws PatternSyntaxException { | ||
int length= findString.length(); | ||
StringBuilder buf= new StringBuilder(length); | ||
|
||
int inCharGroup= 0; | ||
int inBraces= 0; | ||
boolean inQuote= false; | ||
for (int i= 0; i < length; i++) { | ||
char ch= findString.charAt(i); | ||
switch (ch) { | ||
case '[': | ||
buf.append(ch); | ||
if (! inQuote) | ||
inCharGroup++; | ||
break; | ||
|
||
case ']': | ||
buf.append(ch); | ||
if (! inQuote) | ||
inCharGroup--; | ||
break; | ||
|
||
case '{': | ||
buf.append(ch); | ||
if (! inQuote && inCharGroup == 0) | ||
inBraces++; | ||
break; | ||
|
||
case '}': | ||
buf.append(ch); | ||
if (! inQuote && inCharGroup == 0) | ||
inBraces--; | ||
break; | ||
|
||
case '\\': | ||
if (i + 1 < length) { | ||
char ch1= findString.charAt(i + 1); | ||
if (inQuote) { | ||
if (ch1 == 'E') | ||
inQuote= false; | ||
buf.append(ch).append(ch1); | ||
i++; | ||
|
||
} else if (ch1 == 'R') { | ||
if (inCharGroup > 0 || inBraces > 0) { | ||
String msg= Messages.RegExUtils_IllegalPositionForRegEx; | ||
throw new PatternSyntaxException(msg, findString, i); | ||
} | ||
buf.append("(?>\\r\\n?|\\n)"); //$NON-NLS-1$ | ||
i++; | ||
|
||
} else { | ||
if (ch1 == 'Q') { | ||
inQuote= true; | ||
} | ||
buf.append(ch).append(ch1); | ||
i++; | ||
} | ||
} else { | ||
buf.append(ch); | ||
} | ||
break; | ||
|
||
default: | ||
buf.append(ch); | ||
break; | ||
} | ||
|
||
} | ||
return buf.toString(); | ||
} | ||
|
||
/** | ||
* Creates the Pattern according to the flags. | ||
* @param findString the find string. | ||
* @param wholeWord find whole words. | ||
* @param caseSensitive search case sensitive. | ||
* @param regExSearch is RegEx search | ||
* @return a Pattern which can directly be used. | ||
*/ | ||
public static Pattern createRegexSearchPattern(String findString, boolean wholeWord, boolean caseSensitive, boolean regExSearch) { | ||
int patternFlags = 0; | ||
if (regExSearch) { | ||
patternFlags |= Pattern.MULTILINE; | ||
findString = RegExUtils.substituteLinebreak(findString); | ||
} | ||
|
||
if (!caseSensitive) | ||
patternFlags |= Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE; | ||
|
||
if (!regExSearch) | ||
findString = RegExUtils.asRegPattern(findString); | ||
|
||
if (wholeWord) | ||
findString = "\\b" + findString + "\\b"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
|
||
return Pattern.compile(findString, patternFlags); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
RegExUtils_IllegalPositionForRegEx=Illegal position for \\\\R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous to do as many other projects implement the IFindReplaceTarget (EMF implements this too, for example) @merks
The current approach taken is to implement a new
IFindReplaceTargetExtension
. We are at target extension 4, it's time to break the tradition of counting up, please give it a meaningful name (eg.IFindReplaceTargetBatchCountExtension
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, This gives me an idea.. Maybe I can fix the IMarker blocker with creating this new extension