Last Comment Bug 687160 - Source Editor should provide a line-based API
: Source Editor should provide a line-based API
Status: RESOLVED FIXED
[sourceeditor][fixed-in-fx-team]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Mihai Sucan [:msucan]
:
:
Mentors:
Depends on: 702331
Blocks: 585563 687093
  Show dependency treegraph
 
Reported: 2011-09-16 14:24 PDT by Panos Astithas [:past]
Modified: 2012-03-24 09:58 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (5.64 KB, patch)
2011-12-07 13:11 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[in-fx-team] updated patch - column support (6.88 KB, patch)
2011-12-08 12:49 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-09-16 14:24:40 PDT
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.
Comment 1 Cedric Vivier [:cedricv] 2011-09-16 14:51:47 PDT
Using column information is important as well.

getCaretPosition() => {line:, column:}
setCaretPosition(aLine, aColumn)
Comment 2 Cedric Vivier [:cedricv] 2011-09-16 14:54:30 PDT
getOffsetAtPosition(aLine, optional aColumn) => number
Comment 3 Mihai Sucan [:msucan] 2011-09-17 01:15:31 PDT
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. :)
Comment 4 Panos Astithas [:past] 2011-09-17 07:35:50 PDT
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 Kevin Dangoor 2011-09-19 12:57:16 PDT
(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.
Comment 6 Mihai Sucan [:msucan] 2011-09-20 05:29:01 PDT
(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 Kevin Dangoor 2011-09-20 07:35:52 PDT
(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...
Comment 8 Mihai Sucan [:msucan] 2011-09-20 07:58:14 PDT
(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?
Comment 9 Mihai Sucan [:msucan] 2011-12-07 13:11:45 PST
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!
Comment 10 Mihai Sucan [:msucan] 2011-12-07 13:13:34 PST
This patch depends on the patch from bug 702331.
Comment 11 Cedric Vivier [:cedricv] 2011-12-08 07:42:18 PST
(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.
Comment 12 Mihai Sucan [:msucan] 2011-12-08 07:47:36 PST
(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!
Comment 13 Mihai Sucan [:msucan] 2011-12-08 12:49:38 PST
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.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-12-09 07:53:33 PST
Comment on attachment 580154 [details] [diff] [review]
[in-fx-team] updated patch - column support

very nice.
Comment 15 Mihai Sucan [:msucan] 2011-12-13 08:58:44 PST
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+!
Comment 16 Rob Campbell [:rc] (:robcee) 2011-12-14 06:34:46 PST
https://hg.mozilla.org/mozilla-central/rev/7cf966da2e93
Comment 17 Mihai Sucan [:msucan] 2012-02-09 11:39:55 PST
The addition of the new line-based API should be documented (Firefox 11). Thanks!
Comment 18 Eric Shepherd [:sheppy] 2012-03-24 09:58:36 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.