This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
| Summary: | Change semantics of TokenSequence.move() and TS.moveIndex() | ||
|---|---|---|---|
| Product: | editor | Reporter: | Miloslav Metelka <mmetelka> |
| Component: | Lexer | Assignee: | Miloslav Metelka <mmetelka> |
| Status: | RESOLVED FIXED | ||
| Severity: | blocker | CC: | mfukala, pflaska, vstejskal |
| Priority: | P2 | Keywords: | API_REVIEW_FAST |
| Version: | 6.x | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Issue Type: | TASK | Exception Reporter: | |
| Attachments: |
Diff of the change
Diff of the change Diff of the change #3 |
||
Pavle, I've got a problem with the code generation with the attached diff that
implements new semantics of ts.move() and ts.moveIndex().
I've patched some code generation classes where the TS is used but now the
generation fails e.g. if I type new Runnable() { } and inside braces I attempt
to implement the run() method by completion then there are multiple copies of
the code created. Although I've tried to refactor the code in the right way I
had to make some error but I'm unable to find what's wrong. The TS.move() now
moves before the token that "contains" the given offset so to fetch the next
token it's necessary to call TS.moveNext(). The ts.move() will always move the
TS if the offset is too high it will be moved after the last token. The
moveIndex() is done in a similar way - it always moves and a subsequent
moveNext() is necessary to move to the token that contains the offset.
If possible could you please tell me what should I check with the knowledge
that the code is being duplicated (where should I put a breakpoint)? Thanks.
Perhaps when moving on sequence, it went to far to the beginning. Not sure, can you attach the diff? -- I'll run my test and will look at it. The place where anonymous class is matches is CasualDiff:diffNewClass(). Created attachment 37475 [details]
Diff of the change
Created attachment 37621 [details]
Diff of the change
The last diff should be the complete one after we fixed the java code generation with Pavel yesterday. I would like to ask for fasttrack review, especially Pavel F., Marek F. and Vita. Probably ok, just a few things:
+ <summary>Changed TokenSequence.move() and moveIndex() use</summary>
+ <version major="1" minor="13"/>
+ <date day="16" month="1" year="2007"/>
+ <author login="mmetelka"/>
+ <compatibility source="incompatible" deletion="yes" addition="yes"
modification="yes"/>
+ <description>
+ <p>
+ Changed the <code>TokenSequence.move()</code> to position
<b>before</b> the particular token
+ that "contains" the token (or after the last token if the given offset
VS: typo, should read '... that "contains" the offset ...'
/**
- * Get the index of the current token in the complete list of tokens.
+ * Get an index of token to which (or before which) this TS is currently
positioned.
+ * <br/>
+ * Zero is returned after TS creation.
*
- * @return >=0 index of the current token or <code>-1</code>
- * if this token sequence is initially located in front of the first token.
+ * @return >=0 index of the current token.
*/
public int index() {
VS: should not this throw ISE too if the sequence is 'not positioned'? It looks
strange that after TS.moveIndex(7) you are allowed to call TS.index(), which
returns something else then 7. (assuming there is enough tokens in the sequence)
@@ -302,48 +312,62 @@
/**
* Move to the next token in this token sequence.
* <br/>
+ * This method can be used in case when TS is positioned before a token
+ * (after its creation or after use of {@link #move(int)} or {@link
#moveIndex(int)})
+ * or in case the TS already points to a concrete token <code>{@link
#token()} != null</code>
VS: Isn't this just saying that TS.moveNext can be called anytime. If not, then
in what situations you are not allowed to call it?
+ * - in such case it moves to next token and {@link #index()} gets
increased by one.
VS: Isn't this just an implementation detail? Why is it mentioned here?
+ * <br/>
* The next token may not necessarily start at the offset where
- * the current token ends (there may be gaps between tokens
- * caused by use of a token id filter).
+ * the previous token ends (there may be gaps between tokens
+ * caused by token filtering).
*
* @return true if the sequence was successfully moved to the next token
- * or false if stays on the original token because there are no more tokens
+ * or false if it was not moved before there are no more tokens
VS: typo, should read '... or false if it was not moved because ...'
/**
- * Move to the previous token in this token sequence.
+ * Move to a previous token in this token sequence.
+ * <br/>
+ * This method can be used in case when TS is positioned after a token
+ * (after its creation or after use of {@link #move(int)} or {@link
#moveIndex(int)})
+ * or in case the TS already points to a concrete token <code>{@link
#token()} != null</code>.
VS: the same question as for moveNext
+ * <br/>
+ * In both cases {@link #index()} gets decreased by one.
VS: ditto
Index: openide/text/src/org/openide/text/CloneableEditorSupport.java
===================================================================
RCS file: /cvs/openide/text/src/org/openide/text/CloneableEditorSupport.java,v
retrieving revision 1.28
diff -u -r1.28 CloneableEditorSupport.java
--- openide/text/src/org/openide/text/CloneableEditorSupport.java 12 Jan 2007
10:16:57 -0000 1.28
+++ openide/text/src/org/openide/text/CloneableEditorSupport.java 24 Jan 2007
10:59:59 -0000
@@ -1,3 +1,4 @@
+
/*
* The contents of this file are subject to the terms of the Common Development
* and Distribution License (the License). You may not use this file except in
VS: bogus change?
I am OK with the new semantic, but I miss some of my usages in the diff. They are (hopefuly) just in html/editor/lib and web/jspsyntax. I am going to commit some changes to the affected classes in about an hour, so please update then and fix the usages. Thanks. Vito, thanks for the comments. The change in CES was accidental -> rollback. Regarding index() etc. I understand that I need much better explanation so I'm going to rewrite the TS javadocs. Thanks to Marek F. as well - I need to double check jsp and html stuff again there seem to be some unrefactored usages remaining. I'm going to do one more iteration and I will attach a new diff. TS javadoc rewritten. I have recollected usages and fixed additional occurrences. I have tested editing of the mimetypes where I know the lexer is used including html and jsp. Code completion seems to work fine (at least the basic stuff). Final diff attached. Created attachment 37742 [details]
Diff of the change #3
Fixed: Checking in editor/lib2/src/org/netbeans/modules/editor/lib2/highlighting/SyntaxHighlighting.java; /cvs/editor/lib2/src/org/netbeans/modules/editor/lib2/highlighting/SyntaxHighlighting.java,v <-- SyntaxHighlighting.java new revision: 1.7; previous revision: 1.6 done Checking in html/editor/lib/src/org/netbeans/editor/ext/html/HTMLCompletionQuery.java; /cvs/html/editor/lib/src/org/netbeans/editor/ext/html/HTMLCompletionQuery.java,v <-- HTMLCompletionQuery.java new revision: 1.37; previous revision: 1.36 done Checking in html/editor/lib/src/org/netbeans/editor/ext/html/HTMLLexerFormatter.java; /cvs/html/editor/lib/src/org/netbeans/editor/ext/html/HTMLLexerFormatter.java,v <-- HTMLLexerFormatter.java new revision: 1.2; previous revision: 1.1 done Checking in html/editor/lib/src/org/netbeans/editor/ext/html/HTMLSyntaxSupport.java; /cvs/html/editor/lib/src/org/netbeans/editor/ext/html/HTMLSyntaxSupport.java,v <-- HTMLSyntaxSupport.java new revision: 1.34; previous revision: 1.33 done Checking in html/editor/lib/src/org/netbeans/editor/ext/html/parser/SyntaxParser.java; /cvs/html/editor/lib/src/org/netbeans/editor/ext/html/parser/SyntaxParser.java,v <-- SyntaxParser.java new revision: 1.2; previous revision: 1.1 done Checking in html/editor/src/org/netbeans/modules/editor/html/HTMLCompletionProvider.java; /cvs/html/editor/src/org/netbeans/modules/editor/html/HTMLCompletionProvider.java,v <-- HTMLCompletionProvider.java new revision: 1.10; previous revision: 1.9 done Checking in java/editor/src/org/netbeans/modules/editor/java/JavaCompletionProvider.java; /cvs/java/editor/src/org/netbeans/modules/editor/java/JavaCompletionProvider.java,v <-- JavaCompletionProvider.java new revision: 1.76; previous revision: 1.75 done Checking in java/editor/src/org/netbeans/modules/editor/java/Utilities.java; /cvs/java/editor/src/org/netbeans/modules/editor/java/Utilities.java,v <-- Utilities.java new revision: 1.14; previous revision: 1.13 done Checking in java/hints/src/org/netbeans/modules/java/hints/JavaHintsProvider.java; /cvs/java/hints/src/org/netbeans/modules/java/hints/JavaHintsProvider.java,v <-- JavaHintsProvider.java new revision: 1.66; previous revision: 1.65 done Checking in java/source/src/org/netbeans/api/java/source/TreeUtilities.java; /cvs/java/source/src/org/netbeans/api/java/source/TreeUtilities.java,v <-- TreeUtilities.java new revision: 1.14; previous revision: 1.13 done Checking in java/source/src/org/netbeans/modules/java/source/save/CasualDiff.java; /cvs/java/source/src/org/netbeans/modules/java/source/save/CasualDiff.java,v <-- CasualDiff.java new revision: 1.54; previous revision: 1.53 done Checking in java/source/src/org/netbeans/modules/java/source/save/PositionEstimator.java; /cvs/java/source/src/org/netbeans/modules/java/source/save/PositionEstimator.java,v <-- PositionEstimator.java new revision: 1.3; previous revision: 1.2 done Checking in java/source/src/org/netbeans/modules/java/source/save/TokenUtilities.java; /cvs/java/source/src/org/netbeans/modules/java/source/save/TokenUtilities.java,v <-- TokenUtilities.java new revision: 1.2; previous revision: 1.1 done Checking in java/source/src/org/netbeans/modules/java/source/save/TreeDiff.java; /cvs/java/source/src/org/netbeans/modules/java/source/save/TreeDiff.java,v <-- TreeDiff.java new revision: 1.7; previous revision: 1.6 done Checking in lexer/arch.xml; /cvs/lexer/arch.xml,v <-- arch.xml new revision: 1.9; previous revision: 1.8 done Checking in lexer/manifest.mf; /cvs/lexer/manifest.mf,v <-- manifest.mf new revision: 1.14; previous revision: 1.13 done Checking in lexer/api/apichanges.xml; /cvs/lexer/api/apichanges.xml,v <-- apichanges.xml new revision: 1.11; previous revision: 1.10 done Checking in lexer/editorbridge/src/org/netbeans/modules/lexer/editorbridge/LexerLayer.java; /cvs/lexer/editorbridge/src/org/netbeans/modules/lexer/editorbridge/LexerLayer.java,v <-- LexerLayer.java new revision: 1.10; previous revision: 1.9 done Checking in lexer/nbbridge/test/unit/src/org/netbeans/modules/lexer/nbbridge/test/MimeLookupLanguageProviderTest.java; /cvs/lexer/nbbridge/test/unit/src/org/netbeans/modules/lexer/nbbridge/test/MimeLookupLanguageProviderTest.java,v <-- MimeLookupLanguageProviderTest.java new revision: 1.5; previous revision: 1.4 done Checking in lexer/src/org/netbeans/api/lexer/Token.java; /cvs/lexer/src/org/netbeans/api/lexer/Token.java,v <-- Token.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/src/org/netbeans/api/lexer/TokenSequence.java; /cvs/lexer/src/org/netbeans/api/lexer/TokenSequence.java,v <-- TokenSequence.java new revision: 1.8; previous revision: 1.7 done Checking in lexer/src/org/netbeans/lib/lexer/LexerUtilsConstants.java; /cvs/lexer/src/org/netbeans/lib/lexer/LexerUtilsConstants.java,v <-- LexerUtilsConstants.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/src/org/netbeans/lib/lexer/SubSequenceTokenList.java; /cvs/lexer/src/org/netbeans/lib/lexer/SubSequenceTokenList.java,v <-- SubSequenceTokenList.java new revision: 1.7; previous revision: 1.6 done Checking in lexer/test/unit/src/org/netbeans/api/lexer/TokenSequenceTest.java; /cvs/lexer/test/unit/src/org/netbeans/api/lexer/TokenSequenceTest.java,v <-- TokenSequenceTest.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/test/unit/src/org/netbeans/lib/lexer/LanguageManagerTest.java; /cvs/lexer/test/unit/src/org/netbeans/lib/lexer/LanguageManagerTest.java,v <-- LanguageManagerTest.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/test/unit/src/org/netbeans/lib/lexer/test/LexerTestUtilities.java; /cvs/lexer/test/unit/src/org/netbeans/lib/lexer/test/LexerTestUtilities.java,v <-- LexerTestUtilities.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/test/unit/src/org/netbeans/lib/lexer/test/inc/TokenHierarchySnapshotTest.java; /cvs/lexer/test/unit/src/org/netbeans/lib/lexer/test/inc/TokenHierarchySnapshotTest.java,v <-- TokenHierarchySnapshotTest.java new revision: 1.6; previous revision: 1.5 done Checking in lexer/test/unit/src/org/netbeans/lib/lexer/test/simple/SimpleLexerBatchTest.java; /cvs/lexer/test/unit/src/org/netbeans/lib/lexer/test/simple/SimpleLexerBatchTest.java,v <-- SimpleLexerBatchTest.java new revision: 1.7; previous revision: 1.6 done Checking in lexer/test/unit/src/org/netbeans/lib/lexer/test/simple/SimpleLexerIncTest.java; /cvs/lexer/test/unit/src/org/netbeans/lib/lexer/test/simple/SimpleLexerIncTest.java,v <-- SimpleLexerIncTest.java new revision: 1.7; previous revision: 1.6 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JSPHyperlinkProvider.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JSPHyperlinkProvider.java,v <-- JSPHyperlinkProvider.java new revision: 1.13; previous revision: 1.12 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JspParserErrorAnnotation.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JspParserErrorAnnotation.java,v <-- JspParserErrorAnnotation.java new revision: 1.5; previous revision: 1.4 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JspSyntaxSupport.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/JspSyntaxSupport.java,v <-- JspSyntaxSupport.java new revision: 1.90; previous revision: 1.89 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/SimplifiedJSPServlet.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/SimplifiedJSPServlet.java,v <-- SimplifiedJSPServlet.java new revision: 1.3; previous revision: 1.2 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/ELExpression.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/ELExpression.java,v <-- ELExpression.java new revision: 1.14; previous revision: 1.13 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/JavaJSPCompletionProvider.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/JavaJSPCompletionProvider.java,v <-- JavaJSPCompletionProvider.java new revision: 1.18; previous revision: 1.17 done Checking in web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/JspCompletionProvider.java; /cvs/web/jspsyntax/src/org/netbeans/modules/web/core/syntax/completion/JspCompletionProvider.java,v <-- JspCompletionProvider.java new revision: 1.11; previous revision: 1.10 done Checking in xml/tageditorsupport/src/org/netbeans/modules/editor/structure/formatting/TagBasedLexerFormatter.java; /cvs/xml/tageditorsupport/src/org/netbeans/modules/editor/structure/formatting/TagBasedLexerFormatter.java,v <-- TagBasedLexerFormatter.java new revision: 1.4; previous revision: 1.3 done |
I'm not satisfied with the present semantics of TS.move(int offset) and TS.moveIndex(int index). Both methods attempt to position TS to either the given offset or index but if the given offset/index is not available they either remain positioned at the present position or move to the token with nearest available offset (e.g. when an offset is too high the move(offset) uses the highest available token) which is fine but it's problematic in certain situations as described later. The present usage example of move(offset) from javadoc: * int diff = tokenSequence.move(targetOffset); * // diff equals to (targetOffset - tokenSequence.token().offset()) * if (diff >= 0 && diff < tokenSequence.token().length()) { * // Within token bounds - tokenSequence.token() starts at or "contains" targetOffset * * } else if (diff == Integer.MAX_VALUE) { * // No tokens in the token sequence at all. * // Token sequence is not positioned to any token. * * } else { * // 1. diff >= tokenSequence.token().length() * // a) targetOffset is above the end of the last token in the sequence. * // Token sequence is positioned to the last token in the sequence. * // b) there are text areas not covered by any tokens * // due to skipped tokens (skipTokenIds was used * // in TokenHierarchy.create()) and the targetOffset points to such gap. * // Token sequence is positioned to the preceding token. * // * // 2. diff < 0 * // a) targetOffset < 0 * // b) targetOffset >= 0 but there is a text area * // at the begining that is not covered by any tokens * // (skipTokenIds was used in TokenHierarchy.create()) * // Token sequence is positioned to the first token in the sequence. * } Semantics of moveIndex(index): * @param index index of the token to which this sequence * should be positioned. * @return <code>true</code> if the sequence was moved to the token * with the given index. Returns <code>false</code> * if <code>index < 0</code> or <code>index < tokenCount</code>. * In such case the current token sequence's position stays unchanged. Both methods position the TS so that ts.token() can immediately be called. The present approach is problematic in case a TS should be pre-positioned to certain index or offset e.g. I would like to modify semantics of TokenHierarchyEvent.tokenChange().currentTokenSequence() to point to the first token that was added (currently it positions to index 0). However if the modification only removed all the characters till the end of the document and there were no added tokens then the TS would have to be positioned to the last token in the document which is however not the first token that was added (or the one that followed the modification place). This is awkward as the clients would likely have to distinguish this situation specially. I propose to change TS.move(offset) to move the TS to the position between the token that "contains" the offset and the one that precedes it (if any). If the offset would be too high the TS would be positioned after the last token. When the offset would be too low the TS would be positioned before the first token (if it exist). The returned value semantics would generally remain similar i.e. the number of charaters between the requested offset and the start of the token to which the TS was positioned. Before starting of the iteration after move(offset) it would be necessary to do ts.moveNext() to position to the token that "contains" the offset (just like when starting the iteration from the first token). It would have an advantage that unlike now the iteration would always have the same pattern: while (ts.moveNext()) { ts.token()... } Currently when using move(offset): ts.move(offset); // already positions the TS do { // ts.token() already valid => must use do { } cycle ts.token()... } while (ts.moveNext()); The semantics of moveIndex(index) would be changed in a similar way. It would also position between the tokens and its return value would be an integer like for move(offset) it would just operate in the indexes semantics (not offsets) i.e. it would return 0 if the token with the given index exists or number of tokens between requested index and tokenCount() (similar when index < 0 would be provided). The present moveFirst() and moveLast() should be removed and replaced by moveIndex(0) and moveIndex(tokenCount()) in the code. IMHO the proposed semantics would be more natural and it would cover the problematic usecase with currentTokenSequence() nicely. Also there would be an advantage of the same pattern of the TS token iteration. I will start to prototype the solution to see how this will fit with an existing code and whether I will hit any problematic usecases.