Bug 90796 - Change semantics of TokenSequence.move() and TS.moveIndex()
Change semantics of TokenSequence.move() and TS.moveIndex()
Status: RESOLVED FIXED
Product: editor
Classification: Unclassified
Component: Lexer
6.x
PC Linux
: P2 (vote)
: 6.x
Assigned To: Miloslav Metelka
issues@editor
: API_REVIEW_FAST
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-07 23:03 UTC by Miloslav Metelka
Modified: 2007-01-29 14:28 UTC (History)
3 users (show)

See Also:
Issue Type: TASK
:


Attachments
Diff of the change (70.11 KB, patch)
2007-01-18 08:54 UTC, Miloslav Metelka
Details | Diff
Diff of the change (80.67 KB, patch)
2007-01-24 12:57 UTC, Miloslav Metelka
Details | Diff
Diff of the change #3 (106.41 KB, patch)
2007-01-26 21:44 UTC, Miloslav Metelka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miloslav Metelka 2006-12-07 23:03:50 UTC
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.
Comment 1 Miloslav Metelka 2007-01-17 17:14:37 UTC
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.
Comment 2 Pavel Flaska 2007-01-17 19:46:32 UTC
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().

Comment 3 Miloslav Metelka 2007-01-18 08:54:36 UTC
Created attachment 37475 [details]
Diff of the change
Comment 4 Miloslav Metelka 2007-01-24 12:57:08 UTC
Created attachment 37621 [details]
Diff of the change
Comment 5 Miloslav Metelka 2007-01-24 13:13:18 UTC
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.
Comment 6 Vitezslav Stejskal 2007-01-24 23:29:50 UTC
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 &gt;=0 index of the current token or <code>-1</code>
-     *  if this token sequence is initially located in front of the first token.
+     * @return &gt;=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?

Comment 7 Marek Fukala 2007-01-25 12:54:48 UTC
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.
Comment 8 Miloslav Metelka 2007-01-25 12:55:12 UTC
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.
Comment 9 Miloslav Metelka 2007-01-26 21:43:35 UTC
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.
Comment 10 Miloslav Metelka 2007-01-26 21:44:34 UTC
Created attachment 37742 [details]
Diff of the change #3
Comment 11 Miloslav Metelka 2007-01-29 14:28:56 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo