New Bracket support #1718
New Bracket support #1718
Conversation
jukzi
force-pushed
the
Enhancement/865_Bracket_Support
branch
from
8c41fb9
to
6dcad12
Compare
| package org.eclipse.jface.text; | ||
|
|
||
| /** | ||
| * @since 3.25 |
There was a problem hiding this comment.
@Dinesh0723 in principle the code and feature looks good. But i would not add new API for these otherwise unused constants. Please inline them
| { SpecialCharacterConstants.closeParentheses, SpecialCharacterConstants.closeSquareBracket, SpecialCharacterConstants.closeCurlyBracket, | ||
| SpecialCharacterConstants.closeAngleBracket, SpecialCharacterConstants.singleQuote, SpecialCharacterConstants.doubleQuotes } | ||
| }; | ||
| boolean containsSpecialBrackets= Arrays.asList(bracketTypes[0]).contains(text); |
There was a problem hiding this comment.
You can store this List as constant "private final static", please. Hint: better useList.of(...)
| /** | ||
| * @since 3.25 | ||
| */ | ||
| public class SpecialCharacterConstants { |
There was a problem hiding this comment.
| public class SpecialCharacterConstants { | |
| class SpecialCharacterConstants { |
should work as well....
There was a problem hiding this comment.
We don't want to hardcode such feature into JFace, we want JFace to make such feature possible but not to be the host of the edition smartness. And this is usually done by the IAutoEditStrategies registered by the editor on the viewer.
Editors such as the Java one or the Generic Editor already host some smartness regarding finding or inserting matching brackets. We should stay one the same layer instead of hardcoding it into JFace.
Concretely, this feature could be implemented in some IAutoEditStrategy that would be added to the viewer via the SourceViewerConfiguration. JDT already has some CompilationUnitEditor.getPeerCharacter() to reuse; and the other case would be mostly the generic editor, where the "autoEdits" are usually provided by TM4E. So TM4E could be improved to turn "replace foo by (" into replace fooby(foo)`" according to the defined character matching rules.
| * ETAS GmbH - initial API and implementation | ||
| *******************************************************************************/ | ||
|
|
||
| package org.eclipse.jface.text; |
There was a problem hiding this comment.
Please move it to org.eclipse.jface.text.source package, which already hosts some related feature such as ICharacterPairMatcher.
| /** | ||
| * @since 3.25 | ||
| */ | ||
| public class SpecialCharacterConstants { |
There was a problem hiding this comment.
Yes, please keep non-API, and better call it "DefaultCharacterPairs" here and host also the pairs in that class.
There was a problem hiding this comment.
@mickaelistria As per my understanding, a non-API does not include public classes. If we move the class to the org.eclipse.jface.text.source package, it will not be possible to access the constants from DefaultDocumentAdapter.java, which is located in the org.eclipse.jface.text package.
Therefore, Is it better to inline them in DefaultDocumentAdapter.java as
String[][] bracketTypes = {
{ "(", "[", "{", "<", "'", """ },
{ ")", "]", "}", ">", "'", """ }
};
Please correct me if I have misunderstood.
@mickaelistria I will explore on this further if the approach provided in the PR is not acceptable |
Yes please, I think it would be better. |
Introduced a new Bracket support.
Fixes:#865