Closed Bug 725399 Opened 10 years ago Closed 8 years ago

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

Categories

(DevTools :: Source Editor, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 725235

People

(Reporter: msucan, Assigned: jhk)

References

Details

(Whiteboard: [sourceeditor])

Attachments

(1 file)

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.
Priority: -- → P2
No longer blocks: 687707
Removing prio as there is no planned user for this feature afaik.
Priority: P2 → --
There is, the debugger, see that this blocks bug 725235.
Priority: -- → P2
(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 :/
Attached patch Patch(v1)+TestSplinter Review
Locally tested -> works fine.
Attachment #623977 - Flags: review?(mihai.sucan)
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().
Attachment #623977 - Flags: review?(mihai.sucan) → feedback+
Assignee: nobody → jigneshhk1992
Status: NEW → ASSIGNED
> > +    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!
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!
Keeping open for all :)
Assignee: jigneshhk1992 → nobody
Status: ASSIGNED → NEW
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js] → [sourceeditor]
Depends on: 759837
Moving to Source Editor component.

Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
should probably wait on parser work.

filter on CHOCOLATE
Assignee: nobody → jigneshhk1992
Status: NEW → ASSIGNED
(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.
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.)
Depends on: 732442
Added in bug 725235 and friends.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 725235
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.