Source Editor should provide a line-based API

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: msucan)

Tracking

({dev-doc-complete})

Trunk
Firefox 11
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor][fixed-in-fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

The Source Editor API does not currently have an API to set the caret at a specific line. For the script debugger and the style editor we need such an API to focus the right line when the user clicks on a stack frame or a CSS rule. Something like setCaretAtLine(aLine) and getCaretLine() would be OK I think.
Using column information is important as well.

getCaretPosition() => {line:, column:}
setCaretPosition(aLine, aColumn)
Summary: Source Editor should provide a line API → Source Editor should provide a line-based API
getOffsetAtPosition(aLine, optional aColumn) => number
(Assignee)

Comment 3

6 years ago
I had this in the initial API for the SourceEditor. it was getCaretLine() and setCaretLine().

During review it was pointed out that the implementation in the textarea fallback was slow (it had to count the new lines, \n). For that reason the two methods were taken out.

To bring the two methods back we are required to do an optimally-performing implementation in the textarea fallback code. That preeetty much means doing the same as Orion does - it would mean rewriting parts of Orion. It means we need to track the whole source code text changes, to keep an internal model that maps offsets to line numbers and so on.

I want the textarea fallback removed. :)
That's sad. For the script debugger we wouldn't even have that performance hit, since it's a read-only editor and no tracking would be necessary.

Comment 5

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #3)
> I had this in the initial API for the SourceEditor. it was getCaretLine()
> and setCaretLine().
> 
> During review it was pointed out that the implementation in the textarea
> fallback was slow (it had to count the new lines, \n). For that reason the
> two methods were taken out.

But we really *must* have this for the Style Editor.

How slow is it actually to count new lines? Doesn't seem likely to be *that* slow, though it's not something you'd want to do in a tight loop. But for the purpose we're talking about here, it may not even matter.

> To bring the two methods back we are required to do an optimally-performing
> implementation in the textarea fallback code. That preeetty much means doing
> the same as Orion does - it would mean rewriting parts of Orion. It means we
> need to track the whole source code text changes, to keep an internal model
> that maps offsets to line numbers and so on.
> 
> I want the textarea fallback removed. :)

That's problematic, until we're sure that the a10y and i18n concerns with Orion are fully addressed.
(Assignee)

Comment 6

6 years ago
(In reply to Kevin Dangoor from comment #5)
> (In reply to Mihai Sucan [:msucan] from comment #3)
> > I had this in the initial API for the SourceEditor. it was getCaretLine()
> > and setCaretLine().
> > 
> > During review it was pointed out that the implementation in the textarea
> > fallback was slow (it had to count the new lines, \n). For that reason the
> > two methods were taken out.
> 
> But we really *must* have this for the Style Editor.
> 
> How slow is it actually to count new lines? Doesn't seem likely to be *that*
> slow, though it's not something you'd want to do in a tight loop. But for
> the purpose we're talking about here, it may not even matter.

It is potentially very slow. If one wants a status bar to show the "current line number", the number of lines would be recomputed for any text change. (yikes)

Hence the actual need of an internal model for the source code where we track changes, where we put the code split into lines, and so on. Like a real editor.

This is why I'd like the textarea fallback removed. Adding more APIs, like having a breakpoints ruler, or more features needed for the Style Editor or for the debugger, etc... will only deepen the differences between the textarea fallback and the Orion version. If we attempt to bring each API to the fallback, we'll end up rewriting Orion. Each with its own bugs.


> > To bring the two methods back we are required to do an optimally-performing
> > implementation in the textarea fallback code. That preeetty much means doing
> > the same as Orion does - it would mean rewriting parts of Orion. It means we
> > need to track the whole source code text changes, to keep an internal model
> > that maps offsets to line numbers and so on.
> > 
> > I want the textarea fallback removed. :)
> 
> That's problematic, until we're sure that the a10y and i18n concerns with
> Orion are fully addressed.

True.

Comment 7

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #6)
> (In reply to Kevin Dangoor from comment #5)
> > (In reply to Mihai Sucan [:msucan] from comment #3)
> > > I had this in the initial API for the SourceEditor. it was getCaretLine()
> > > and setCaretLine().
> > > 
> > > During review it was pointed out that the implementation in the textarea
> > > fallback was slow (it had to count the new lines, \n). For that reason the
> > > two methods were taken out.
> > 
> > But we really *must* have this for the Style Editor.
> > 
> > How slow is it actually to count new lines? Doesn't seem likely to be *that*
> > slow, though it's not something you'd want to do in a tight loop. But for
> > the purpose we're talking about here, it may not even matter.
> 
> It is potentially very slow. If one wants a status bar to show the "current
> line number", the number of lines would be recomputed for any text change.
> (yikes)
> 
> Hence the actual need of an internal model for the source code where we
> track changes, where we put the code split into lines, and so on. Like a
> real editor.

Yeah, I agree that's a bad path. So maybe the APIs we use internally for now need to do just what's necessary to get the features we need.

For example, setCaretAtLine can be implemented without incident, right? Since I'm particularly keen on the click-and-get-to-a-certain-line case, and can live without a status bar with column number, we could have just the setter without the getter...
Blocks: 687093
Blocks: 585563
(Assignee)

Comment 8

6 years ago
(In reply to Kevin Dangoor from comment #7)
> For example, setCaretAtLine can be implemented without incident, right?
> Since I'm particularly keen on the click-and-get-to-a-certain-line case, and
> can live without a status bar with column number, we could have just the
> setter without the getter...

We can have a setCaretAtLine(), sure.

Shall we do this here?
(Assignee)

Updated

6 years ago
Whiteboard: [sourceeditor]
(Assignee)

Comment 9

5 years ago
Created attachment 579811 [details] [diff] [review]
proposed patch

Proposed patch.

I added get/setCaretLine() and getLineCount() for source-editor-orion.jsm, and only setCaretLine() to the textarea fallback.

Thoughts:

- the new API is fast and it's only intended to be used with the Orion component.
- I added setCaretLine() as a minimal compat for the textarea fallback. This is slow, but it won't be called often.

Should I not add it to the textarea fallback at all? Should I make stubs in the textarea.jsm?

Looking forward to your review. Thank you!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #579811 - Flags: review?(rcampbell)
(Assignee)

Comment 10

5 years ago
This patch depends on the patch from bug 702331.
Depends on: 702331
(In reply to Mihai Sucan [:msucan] from comment #9)
> I added get/setCaretLine() 

I'm not sure if Orion provides a way to set the column right now, but could we have a more 'extensible' API like get/setCaretPosition({line:, column:}) even if the column property is not supported yet?

Users of get/setCaretLine most likely will want to jump the caret at a specific column as well for a better end-user experience.
(Assignee)

Comment 12

5 years ago
(In reply to Cedric Vivier [cedricv] from comment #11)
> (In reply to Mihai Sucan [:msucan] from comment #9)
> > I added get/setCaretLine() 
> 
> I'm not sure if Orion provides a way to set the column right now, but could
> we have a more 'extensible' API like get/setCaretPosition({line:, column:})
> even if the column property is not supported yet?
> 
> Users of get/setCaretLine most likely will want to jump the caret at a
> specific column as well for a better end-user experience.

That is possible. Will do!
(Assignee)

Comment 13

5 years ago
Created attachment 580154 [details] [diff] [review]
[in-fx-team] updated patch - column support

As suggested by Cedric, I have added support for columns. You can now get the current line and column, and you can jump to a specific line and column.
Attachment #579811 - Attachment is obsolete: true
Attachment #579811 - Flags: review?(rcampbell)
Attachment #580154 - Flags: review?(rcampbell)
Comment on attachment 580154 [details] [diff] [review]
[in-fx-team] updated patch - column support

very nice.
Attachment #580154 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 15

5 years ago
Comment on attachment 580154 [details] [diff] [review]
[in-fx-team] updated patch - column support

https://hg.mozilla.org/integration/fx-team/rev/7cf966da2e93

Thank you Rob for the r+!
Attachment #580154 - Attachment description: updated patch - column support → [in-fx-team] updated patch - column support
(Assignee)

Updated

5 years ago
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7cf966da2e93
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
(Assignee)

Comment 17

5 years ago
The addition of the new line-based API should be documented (Firefox 11). Thanks!
Keywords: dev-doc-needed
This API is documented in the source-editor.jsm doc. Examples and the like will be covered under other bugs.

https://developer.mozilla.org/en/JavaScript_code_modules/source-editor.jsm

Mentioned on Firefox 11 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.