Panos Astithas (he/him) [:past] (please ni?) Reporter
	Description • 13 years ago

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.

	Cedric Vivier [:cedricv]
	Comment 1 • 13 years ago

Using column information is important as well.

getCaretPosition() => {line:, column:}
setCaretPosition(aLine, aColumn)

	Cedric Vivier [:cedricv]
	Comment 2 • 13 years ago

getOffsetAtPosition(aLine, optional aColumn) => number

	Mihai Sucan [:msucan] Assignee
	Comment 3 • 13 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. :)

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 4 • 13 years ago

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.

	Kevin Dangoor
	Comment 5 • 13 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.

	Mihai Sucan [:msucan] Assignee
	Comment 6 • 13 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.

	Kevin Dangoor
	Comment 7 • 13 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...

	Mihai Sucan [:msucan] Assignee
	Comment 8 • 13 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?

	Mihai Sucan [:msucan] Assignee
	Comment 9 • 13 years ago

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!

	Mihai Sucan [:msucan] Assignee
	Comment 10 • 13 years ago

This patch depends on the patch from bug 702331.

	Cedric Vivier [:cedricv]
	Comment 11 • 13 years ago

(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.

	Mihai Sucan [:msucan] Assignee
	Comment 12 • 13 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!

	Mihai Sucan [:msucan] Assignee
	Comment 13 • 13 years ago

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.

	Rob Campbell [:rc] (:robcee)
	Comment 14 • 13 years ago

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

very nice.

	Mihai Sucan [:msucan] Assignee
	Comment 15 • 13 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+!

	Rob Campbell [:rc] (:robcee)
	Comment 16 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/7cf966da2e93

	Mihai Sucan [:msucan] Assignee
	Comment 17 • 13 years ago

The addition of the new line-based API should be documented (Firefox 11). Thanks!

	Eric Shepherd [:sheppy]
	Comment 18 • 13 years ago

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.