Bug 369289

Comment 1 Jay Arthanareeswaran 2012-01-31 09:48:32 EST

I have a patch at hand. I will post it after adding tests and testing a bit more.

Comment 2 Jay Arthanareeswaran 2012-02-02 07:00:35 EST

Created attachment 210440 [details]
Proposed fix

Path doesn't include tests yet. I will include the tests for the codemirror styler soon.

Comment 3 Mark Macdonald 2012-02-03 08:45:34 EST

Thanks Jay! I'll take a look at the patch.

Comment 4 Jay Arthanareeswaran 2012-02-03 11:45:23 EST

(In reply to comment #3)
> Thanks Jay! I'll take a look at the patch.

Mark, I can't figure out an easier way to get the line index, though I believe there must be a better way to do it. Thanks for taking time to look at the patch.

Comment 5 Mark Macdonald 2012-02-09 19:49:34 EST

Hi again... I haven't forgotten about this one, just got pulled away by other stuff. I started reviewing/testing the patch.

Can you please post a comment here confirming that you authored this code? Something like this:

> "I wrote all this code and have the rights to contribute it to Eclipse under the
> eclipse.org web site terms of use."

We need this for legal reasons. Thanks!

Comment 6 Jay Arthanareeswaran 2012-02-09 23:13:02 EST

(In reply to comment #2)
> Created attachment 210440 [details]
> Proposed fix
> 
> Path doesn't include tests yet. I will include the tests for the codemirror
> styler soon.

I wrote all this code and have the rights to contribute it to Eclipse under the eclipse.org web site terms of use.

Comment 7 Mark Macdonald 2012-02-14 02:10:26 EST

I am going to commit the patch, with a second change to fix up a few things, namely:
- The "Syntax Error" string must be externalized, since the editor now uses i18n
- Minor style changes
- The logic can be simplified somewhat by exploiting the fact that every element of 'rangesAndErrors' is guaranteed to fall within the line being styled (e.lineIndex in the code). This lets us remove the 'for' loop and some min/max counting, and instead just replace the annotations between getLineStart(e.lineIndex) and getLineEnd(e.lineIndex). The annotations for any subsequent lines will be handled by later onLineStyle() calls.

Comment 8 Jay Arthanareeswaran 2012-02-14 02:31:32 EST

(In reply to comment #7)
> I am going to commit the patch, with a second change to fix up a few things,
> namely:
> - The "Syntax Error" string must be externalized, since the editor now uses
> i18n
> - Minor style changes

Thanks, Mark! 

> - The logic can be simplified somewhat by exploiting the fact that every
> element of 'rangesAndErrors' is guaranteed to fall within the line being styled
> (e.lineIndex in the code). This lets us remove the 'for' loop and some min/max
> counting, and instead just replace the annotations between
> getLineStart(e.lineIndex) and getLineEnd(e.lineIndex). The annotations for any
> subsequent lines will be handled by later onLineStyle() calls.

I did think about it but wasn't very sure, particularly about the old annotations on subsequent lines being removed. If you can confirm it works for that case, it's fine then.

Comment 9 Mark Macdonald 2012-02-14 15:12:54 EST

(In reply to comment #8)
> I did think about it but wasn't very sure, particularly about the old
> annotations on subsequent lines being removed. If you can confirm it works for
> that case, it's fine then.

Yes, the TextView API guarantees that our onLineStyle() listener will be called for subsequent lines before they are rendered, which will give us a chance to update their annotations (and styles) to whatever they should be.

I've tested and it works ok.

Adding Silenio for review since this is editor/styler related. The commit is in orion/bug369289:
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?h=bug369289&id=b5d7be1c09acaf6231a0b31880649b6c9ac1dc5c

Comment 10 Silenio Quarti 2012-02-15 10:37:39 EST

The changes are fine.

But note that the highlight error annotations are not shown in the rulers (annotation and overview). If they where shown in the overview ruler, onLineStyle() would not be the right place to add the annotations, since it is generally only called for visible lines.  The user would not see the annotations until the lines containing the annotations were drawn.

Comment 11 Mark Macdonald 2012-02-15 15:06:11 EST

Merged commits into master:
[1] http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=1d6d6c8323f4a0f5a0a8ec93f1b668a376088e97
[2] http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b5d7be1c09acaf6231a0b31880649b6c9ac1dc5c