725399 - Source Editor: add a method to retrieve the token pointed at by an offset

Status: RESOLVED DUPLICATE of bug 725235

(DevTools :: Source Editor, defect, P2)

Description

[Mihai Sucan [:msucan]]

We need to be able to retrieve what constitutes a "token" based on a given offset in the document being edited within the Source Editor. A token is what each mode defines (CSS, JavaScript and HTML).

This should be fairly easy to implement, see prior work in bug 687707.

Comment 1

[Cedric Vivier [:cedricv]]

Removing prio as there is no planned user for this feature afaik.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

There is, the debugger, see that this blocks bug 725235.

Comment 3

[Cedric Vivier [:cedricv]]

(In reply to Panos Astithas [:past] from comment #2)
> There is, the debugger, see that this blocks bug 725235.

Oops, sorry. My mistake, I was looking at Depends instead of Blocks :/

Comment 4

[Jignesh Kakadiya [:jhk]]

Attachment: Patch(v1)+Test

Locally tested -> works fine.

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 623977 [details] [diff] [review]
Patch(v1)+Test

Review of attachment 623977 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Jignesh for taking this bug! Awesome first patch!

Some comments below:

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +467,5 @@
>  
>      return index;
>    },
> +
> +  getTokenAtOffset : function SE_getTokenAtOffset(offset)

nit: please remove the space between the method name and the colon.

Also arguments need to start with "a". s/offset/aOffset/ :)

We also need a jsdoc comment explaining what the method does, the arguments and the return value.

@@ +469,5 @@
>    },
> +
> +  getTokenAtOffset : function SE_getTokenAtOffset(offset)
> +  {
> +    let text = this.getText();

It is more efficient to only retrieve the text at the line where offset is found. So, please do that. You only need to go through the characters in that line.

(see getLineAtOffset() and getLine())

@@ +472,5 @@
> +  {
> +    let text = this.getText();
> +    let start = offset > 0 ? offset - 1 : 0;
> +    let end = offset;
> +    let re = /[\s;:\{\}\[\]\"\']/;

This regex should be specific for each mode. In JS and CSS modes tokens are slightly different. Please take a quick glance at the CSS and ECMAScript specs for the definition of a token. If you want I can help you with this.

For text mode we shall define a token = a word.

@@ +487,5 @@
> +      start : start + 1,
> +      end : end,
> +      text : text.substring(start+1, end),
> +      previousChar : text[start-1] || " ",
> +      nextChar : text[end] || " "

Probably previous/nextChar should be "" when there's no text[start-1] / text[end].

Also, why do we include the previous and next chars in the return object? For what do you think they are needed?

::: browser/devtools/sourceeditor/test/Makefile.in
@@ +63,5 @@
>  		browser_bug729480_line_vertical_align.js \
>  		browser_bug725430_comment_uncomment.js \
>  		browser_bug731721_debugger_stepping.js \
>  		browser_bug729960_block_bracket_jump.js \
> +		browser_bug725399_tokenAtoffset.js \

all lower case, or go for tokenAtOffset.js

::: browser/devtools/sourceeditor/test/browser_bug725399_tokenAtoffset.js
@@ +54,5 @@
> +     is(editor.getTokenAtOffset(i).text, "test" , " test recieved");
> +  }
> +  for (; i < 24; i++) {
> +     is(editor.getTokenAtOffset(i).text, "ing" , " ing recieved");
> +  }

These for-loops will generate many tests with identical messages. Please include the offset you are testing for.

Also, please check that the other properties returned by getTokenAtOffset().

Comment 6

[Jignesh Kakadiya [:jhk]]

> > +    let re = /[\s;:\{\}\[\]\"\']/;
> 
> This regex should be specific for each mode. In JS and CSS modes tokens are
> slightly different. Please take a quick glance at the CSS and ECMAScript
> specs for the definition of a token. If you want I can help you with this.

I need help here. Is there any link OR regex for CSS and JS tokens available? That would be helpful so that I can directly put it here rather than coming up with regex which gives results that we don't wanted. 

Thanks!

Comment 7

[Mihai Sucan [:msucan]]

As discussed on IRC, we need a parser to accurately determine the token found at a given offset. A regex-based approach has limited uses. We will revisit this bug once we have parsers integrated into the source editor. Thanks!

Comment 8

[Jignesh Kakadiya [:jhk]]

Keeping open for all :)

Comment 9

[Rob Campbell [:rc] (:robcee)]

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 10

[Rob Campbell [:rc] (:robcee)]

should probably wait on parser work.

filter on CHOCOLATE

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> should probably wait on parser work.
> 
> filter on CHOCOLATE

should probably use the parser API.

Comment 12

[Victor Porof [:vporof][:vp]]

The way I imagine this working (for JS at least) is pretty straightforward:

1. parse the source with Reflect.jsm

2. from the AST info, find the (single) deepest node for which the definition bounds contain the requested caret offset

3. climb back the tree and return all it's parents (objects, scopes..)
(this may be the only "interesting" part to implement, but we could just remember the path while traversing, push/pop nodes off the stack, etc.)

Comment 13

[Victor Porof [:vporof][:vp]]

Added in bug 725235 and friends.