Here is the opinion document:
http://openide.netbeans.org/tutorial/reviews/opinions_88471.html

Created attachment 35715 [details]
editor/lib2 architecture document

Created attachment 35717 [details]
editor/lib2 javadoc (see the o.n.spi.editor.highlighting.*)

I'd like to ask for an inception review of the new editor Highlighting SPI. The
arch and javadoc are attached to this issue. There is also a link to the
opinions document with short overview and the instructions how to get sources
and build and run a working prototype. Thank you.

Ouch. Now I know why you wanted to change the background color. My eyes are
swimming in tears. Tip for other reviewers: comment out defs of body and
.TableRowColor in javadoc-generic.css.


BTW spelling: "supersede" despite "cede" and "exceed" (this is a common spelling
competition word!).


Comments on general packages (are these really supposed to be public?!):


EditorDocument.get(Object) is pretty mysterious.


Behavior of ED.atomicUndo if not under atomic lock? Can atomic locks be nested?
If so, does aU undo all levels, or just the "current" one?


EditorUtilities is empty, pls. delete.


DialogFactory - where does an instance come from? Why is this in the API anyway
- what's wrong with DialogDisplayer etc.?


EditorImplementationProvider: "independently of", not "... on"


Unclear who is supposed to use EditorImplementationProvider and for what purpose.


EditorImplementationProvider.getResourceBundle - ???


EditorImplementationProvider.getGlyphGutterActions says it should be removed,
but there it is. You will not be able to remove it if it is in a public SPI.


Now to the SPI package which I think you meant to have reviewed: looks basically
OK. Would like to see some complete module which adds a nontrivial draw layer as
an example, though. (Hopefully) simple example which would actually be useful:
boldface summary sentences of Javadoc comments, using the Java lexer as a
starting point.


Purpose of HighlightsLayer.layerTypeId is unclear.


Shouldn't highlights which are outside of accepted bounds, or which overlap,
cause some sort of warning to be logged, besides quietly trimming them?


HL(String,ZOrder) constructor does not document what defaults it uses for
fixedSize and lineHighlighting.


Discussion of ZOrder in Javadoc for HighlightsLayerFactory is probably unnecessary.


What is the recommended way of extracting file info from
HighlightsLayerFactory.Context.document? (E.g. FileObject)


I am surprised that bold and italic font attributes affect metrics. Is it
discouraged for performance reasons to use these attributes?


"ConcurrentModificationExceptoin" - typo


Not clear to me what OffsetsBag and PositionsBag are for. Who makes them? When?
HLF seems to require a HL, not general HighlightsContainer.


Who would call ZOrder.sort? Does this need to be in the API at all? Could this
be replaced with a static Comparator<HighlightsLayer> (or make HL impl
Comparable<HL>)?

> BTW spelling: "supersede" despite "cede" and "exceed" (this is a common
> spelling competition word!).

Shame! :-(

> Comments on general packages (are these really supposed to be public?!):

They are in a very early stage of development. Please do not review them, stuff
in there is just a sketch and is likely to be changed a lot.

> Now to the SPI package which I think you meant to have reviewed: looks
> basically OK. Would like to see some complete module which adds
> a nontrivial draw layer as an example, though. (Hopefully) simple
> example which would actually be useful:
> boldface summary sentences of Javadoc comments, using the Java lexer as a
> starting point.

There are several layers already implemented in o.n.m.editor.lib2.highlighting.
Most of them are trivial like CaretRowHighlighting.java, but the more complex
one is SyntaxHighlighting.java. It implements syntax highlighting based on new
lexer (including embedded languages). If you follow the instructions in the
opinions document it's this layer that will colorify java files.

Anyway, I'll write a module as you suggest, it should be pretty simple.

> Purpose of HighlightsLayer.layerTypeId is unclear.

I'll try to describe it better. It's basically a unique ID of a layer and it's
used for ordering (in connection with ZOrder). I call it layerTypeId, because
there is likely to be many instances of one 'layer type' (i.e. one instance for
each document). So it identifies a class of layers rather then one particular
instance. An example: all instances of CaretRowHighlighting has the same
layerTypeId, which must be different from the layerTypeId of any instance of
SyntaxHighlighting.

> Shouldn't highlights which are outside of accepted bounds, or which overlap,
> cause some sort of warning to be logged, besides quietly trimming them?

Maybe. The javadoc of HC.getHighlights should probably be more strict, saying
that the three rules it mentions must be obeyed. Then any violation can be
logged with the infrastructure trying its best to render something meaningful
and if not possible scratching the rest of the offending sequence.

> HL(String,ZOrder) constructor does not document what defaults it uses for
> fixedSize and lineHighlighting.

Ok, will add this.

> Discussion of ZOrder in Javadoc for HighlightsLayerFactory is probably
> unnecessary.

Ok, I was going to write a bit more about ZOrder to the package overview, so
I'll move it there.

> What is the recommended way of extracting file info from
> HighlightsLayerFactory.Context.document? (E.g. FileObject)

Good question, no idea. :-) The editor infra should be able to provide that and
I suppose we could add HL.Context.getFileObject() to make it easy for SPI
clients. But then, would it return a primary file or should it return
FileObject[] or perhaps DataObject?

> I am surprised that bold and italic font attributes affect metrics. Is it
> discouraged for performance reasons to use these attributes?

Well, I know pretty much nothing about the views hierarchy and rendering, but I
think it does matter for proportional fonts.

> "ConcurrentModificationExceptoin" - typo

Ok, will fix.

> Not clear to me what OffsetsBag and PositionsBag are for. Who makes them?
> When? HLF seems to require a HL, not general HighlightsContainer.

They are supposed to be used by implementors of HL. This part is probably
clumsy, but HLF produces HL, which is pretty much HighlightsContainer plus some
additional info (i.e. ZOrder, etc). Both OffsetsBag and PositionsBag are
writable implementations of HC that can be used as an internal storage inside a
HighlightsLayer. Also note that OffsetsBag or PositionsBag can be passed into
HL's constructor.

> Who would call ZOrder.sort? Does this need to be in the API at all? Could this
> be replaced with a static Comparator<HighlightsLayer> (or make HL impl
> Comparable<HL>)?

It could probably be removed completely. I think it will only be called by the
infrastructure.

Thanks Jesse!

Re. HighlightsLayer.layerTypeId: makes sense when you read ZOrder docs, so
should just link to that.


Re. HL.Context.getFileObject() - if you don't mind an API dep on org.openide.fs
this would be fine. (Just one FO should suffice - you can't edit >1 file in one
document, I think!) Although (apparently?) undocumented, it seems
document.getProperty(StreamDescriptionProperty) will return the DataObject, e.g.
with JavaScript running in NB:

org.openide.windows.TopComponent.registry.activated.editorPane.document.getProperty("stream")
org.netbeans.modules.java.JavaDataObject@11f2b31[AbstractFileObject@1848ddf[javax/swing/text/Document.java]]


Re. OB and PB: I guess I would have to see an example of what they are for. Docs
should give more background here.

HL.Context.getFileObject():
I would rather have a utility method like
NbEditorUtilities.getFileObject(Document) somewhere else than in the HL api.

A few comments:

The delegating in HighlightsLayer seems a bit strange to me:
-it seems that if a HL delegating to a HC is to be created, it is necessary to
create a class, even if the class does not actually do anything. Eg.:
public final class MyHighlightLayer extends HighlightLayer {
    public MyHighlightLayer(HighlightContainer delegate) {
        super(<layerID>, <zorder>, false, false, delegate);
    }
}
and I think this will be quite common usecase for delegating HL.
-the relationship between getHighlights and getHighlightsSequence methods seems
quite confusing to me.

I think that something like this would work better:
public abstract class HighlightLayer implements HighlightContainer {
    //the two non-delegating constructors
    protected abstract HighlightSequence getHighlights(int start, int end);
    public static HighlightLayer createDelegatingLayer(/*same arguments as the
delegating constructor*/) {...}
}

But, maybe this would work even better:
public final class HighlightLayer {
    //no public constructor
    public static HighlightLayer create(String layerId, ZOrder order,
HighlightContainer delegateTo);
    public static HighlightLayer create(String layerId, ZOrder order, boolean ,
boolean , HighlightContainer delegateTo);
    //no public instance methods
}

The client would then create own instance of HighlightContainer (maybe using
PositionsBag), and the layer would be created using the factory methods.


The javadoc of PositionsBag says:
"The PositionsBag is designed to work over a single document, which is passed in
to the constructor."
although PositionsBag does not have any constructor which would take a document.
BTW: From API point of view, the PositionsBag and OffsetsBag seem quite similar.
Maybe these two classes could be merged somehow?


I have tried to re-write the Java "semantic" coloring to HighlightLayer. Is
there any way for me to try it?

Re. HL delegating to HC - The idea was that HL will usually delegate to
OffsetsBag or PositionsBag (they are both HC). If you write your own
implementation of HC which is actually meant to be a layer then you should
subclass HL and write that implementation as HL (i.e. no delegating). When using
HL that delegates to OffsetsBag/PositionsBag you have to write the code that
manipulates the contents of the bags somewhere and logical place seems to be
your HL I think.

I admit that it is a little bit confusing, but I don't see any other better
solution (which is not to say that there isn't any). I believe that most of the
HL implementations won't need delegating at all (i.e. certainly not those based
on lexer) and making HL final would unneccessarily force all HL to delegate.
Also, public static HL create(...) would not work well for
OffsetsBag/PositionsBag, where would you write the code updating the bags?


> The javadoc of PositionsBag says:
> "The PositionsBag is designed to work over a single document, which is
> passed in to the constructor." although PositionsBag does not have any
> constructor which would take a document.

That's clearly a mistake. I'll fix it.


> BTW: From API point of view, the PositionsBag and OffsetsBag seem quite
> similar. Maybe these two classes could be merged somehow?

You're right, they are pretty much the same. I was thinking that maybe we could
use generics and merge them somehow. However, there is a few subtle differences
that complicates the thing (e.g. PB.addAllHighlights and PB.setAllHighlights are
more restrictive). Also I am not sure if java generics can be used with
primitive types (i.e. 'int' in this case) and if not then auto-boxing ints to
Integeres might cause a significant perfomance degradation. Any ideas or help is
more than welcome.


> I have tried to re-write the Java "semantic" coloring to HighlightLayer. Is
> there any way for me to try it?

Yes, there is, please go to the opinions document and follow the instructions
for building and running the prototype. The prototype uses a special draw layer
to draw highlights contributed by all the registered HLs. It also disables all
other existing draw layers so any colors on the screen are those coming from
HLs. Look at o/n/m/editor/lib2/highlighting/Factory.java for an example of HLF
and o/n/m/editor/lib2/resources/layer.xml for how to register it in MimeLookup.

Thanks.

Created attachment 36107 [details]
The example of highlighting the first sentence of a javadoc comment.

I've just added the example module, which implements HL for highlighting the
first sentence of every javadoc comment in a document. The implementation is
based on java/lexer. It can be tested with a custom IDE build from trunk +
editor_api branch as described in the opinions document. You will probably have
to fix the link to NB platform. Later on it can either be moved to contrib or
the layer can be moved to the java module itself.

I've updated the code and javadoc according to the feedback. Please see the
attached diff and new javadoc zip.

Created attachment 36130 [details]
Commit log with the feedback changes

Created attachment 36131 [details]
Updated javadoc

-J-Dorg-netbeans-spi-editor-highlighting=true: please use standard
java.util.logging. (e.g. -J-Dorg.netbeans.spi.editor.highlighting.level=0)


Why is the highlighter in the example registered as
Editors/org-netbeans-modules-firstsentence-Factory.instance? Shouldn't a more
specific path be used?


Well, example looks OK I guess, though I don't think I would be able to write it
myself with copying an example! Certainly fails the "Emacs test":

(eval-after-load "cc-mode" '(progn (setq java-font-lock-keywords-3 (append
java-font-lock-keywords-3 '(("\\(^\\|[^/]\\)/\\*\\*+[
\t\n*]*\\(\\([^.*@]\\|\\.[^ \t\n]\\|\\*[^/]\\|@link[
\t\n]\\)+\\)\\([^*]\\|\\*[^/]\\)*\\*/\\([
\t\n]\\|\\<\\(abstract\\|final\\|native\\|synchronized\\|static\\|transient\\|volatile\\)\\>\\)*\\<\\(public\\|protected\\)\\>"
(2 'bold prepend)))))))

"...without copying..." I meant of course.

Re. -J-D... : Do you mean even for switching on/off the new SPI? Because that's
what the property does at the moment, it has nothing to do with logging.

Re. registration : It could probably be registered as text/x-javadoc. However,
the infrastructure curently doesn't use any defaults for embedded languages. For
example, the highlighter would not work for javadoc in java in JSP at the moment
unless it is registered in the Editors/. It is probably reasonable that the
infra loads highlighters from the full mimepath (e.g.
text/x-jsp/text/x-java/text/x-javadoc) and also highlighters from just the last
mime type (e.g. text/x-javadoc). If there are highlighters of the same type the
most specific one will be used (i.e. the one registered for the longest mimepath).

Re. complexity : I think that part of the complexity is caused by Lexer API. We
were discussing with mmetelka that maybe the Lexer API should provide an easy
way for creating 'exploded' TokenSequences (i.e. TokenSequence which iterates
inside embedded tokens and provides). At first it didn't seem to be an obvious
usecase, but it probably is. In the example I need to walk through all the
embedded languages and find all javadoc tokens no matter how deep in the tokens
hierarchy they are. Currently there is no easy way of doing that. Having the
'exploded' TokenSequence would simplify HSImpl a lot I think.

Re. emacs : I am sorry, but how does the 'emacs test' work?

Thanks!

Re. -J-D: never mind, I thought that was a logging flag.


Re. registration - yes, such a search order would make sense I think.


The "Emacs test" is trying to write very roughly equivalent functionality in #1
your .emacs file vs. #2 a NB module, and measuring

  log_2(sizeof(#1)) - log_2(sizeof(#2))

I get roughly -5.6 in this case, which isn't too great. :-/ Of course the
behaviors are not identical so it's not quite fair, but you get the idea.

Y01 I guess org.netbeans.api.editor.EditorUtilities can be deleted.

Y02 I am not really fond of having implementation part of an API - e.g: 
org.netbeans.api.editor.EditorDocument

Y03 Delete DialogFactory from SPI, use dependency on org.openide.dialogs

Y04 Javadoc in EditorImplementationProvider talks about org.netbeans.editor 
package, anyway you do not want this class in API, imho.

Y05 the need for classes in SPI package. I guess I can understand HLFactory 
and I really like that it is an interface and that it takes final class. 
However then I am lead to HighlightsLayer and I am stucked what is this a bit. 
Imho this is a pure mix of API and SPI - maybe it is not a problem, but 
still - who is supposed to call addHighlightListener? Only infrastructure? If 
so, make the method package private, maybe also make the listener hidden...
And yet another "This method is only called when this layer was not created 
passing in another HighlightsContainer" - is that a joke on API design or 
what? Why not you have two interfaces one for those who wish to pass in This 
method is only called when this layer was not created passing in another 
HighlightsContainer

We had the inception review, please see more details in the opinions document.
http://openide.netbeans.org/tutorial/reviews/opinions_88471.html

I've done most of the changes suggested by the reviewers. Here are the details:

#1 - HighlightsLayer - It's now final with the only public method HL.create(),
which takes all the parameters necessary for creating a layer such as ZOrder,
layer type id, HighlightsContainer, etc.
#2 - Separating support classes - There is a new package called
o.n.spi.editor.highlighting.support, which now contains the OffsetsBag and
PositionsBag classes.
#3 - Getting rid of 'events' classes - I tried to implement this change, but
things turned out really unnatural and ugly from both the SPI and the
infrastructure's implementation point of view. So, I relaxed this suggestion and
left the HighlightsChangeListener and HCEvent untouched. We can discuss this
again, but I don't thing these classes cause any real maintenance problem for
the SPI. I've also added AbstractHighlightsContainer to the support package to
make implementation easier.
#4 - Clarify use of AttributeSets - There is a new chapter in the package
summary, which discusses the use of AS in highlighting layers.
#5 - Delegating/decorating/proxying use case - Ok no support for that.

I'll attach the latest javadoc to this issue. Thanks!

Created attachment 36303 [details]
Latest javadoc (with changes from the inception review).

... and the jar: link for those who can use it.
jar:http://www.netbeans.org/nonav/issues/showattachment.cgi/36303/org-netbeans-modules-editor-lib2.zip!/index.html

It's not important for the review, but there are already lexers for java, html
and xml derived mime types and they are used by the new SytaxHighlighting layer
for coloring these documents. You can see the results when following the
instructions for running the prototype in the summary section of the opinions
document.

Looks good. As soon as I started at HLFactory I've been navigated to HL and then to ZOrder 
and HighlightsContainer. Both HLFactory and HC are interface so I expect I am supposed to 
implement them. I could find support subclasses in the javadoc of HC. I consider the API 
clean, readable and understandable.

Also I see no major evolution troubles - except maybe the HCEvent shall be final.

Re. #3 - Getting rid of 'events' classes - I still do not think that the JavaBeans listener pattern 
is the most appropriate here - it is too verbose and implementing the correct listener pattern is 
too much work for the thing one really wants to do. Please note that you have found that 
yourself: AbstractHighlightsContainer is the sign of this. If you choosen more simple pattern - 
e.g. callback - the implementators would have no need for AHC. However I am known to hate 
deep (even three) inheritance hierarchies in APIs[1] so I am biased and take this just as an 
advice.

Looks good. I am ready to vote yes: integrate to trunk.

[1] I hate deep hierarchies in spite of, due to and since I wrote CloneableEditorSupport and 
friends

I also think that the notification through the context would be simpler (minus 3
classes) but it's a detail.
I've made the event class final.
I agree with the integration to the trunk.

I agree with the integration to the trunk.

Created attachment 36720 [details]
The latest javadoc

I've added one more usecase to the list. The insfrastructure now allows modules
to filter layers used for rendering text in a particular component. Please read
more in the usecase description at the end of the SPI's package description.

jar:http://www.netbeans.org/nonav/issues/showattachment.cgi/36720/org-netbeans-modules-editor-lib2.zip!/index.html

Just a heads-up, I'm going to integrate this to trunk sometime this week.

The Highlighting SPI is now in trunk. I'll add commit logs as attachments.

Created attachment 37514 [details]
Commit log from the CVS server

Y11 Finish usecases on the first page of javadoc - e.g. at least remove TBD
Y12 Change stability to official
Y13 Document property APIs using <api/> 

Hi, I'd like to ask for the final review of the Highlighting SPI. So far Mila
(mmetelka) and Yarda (jtulach) have expressed their interest in attending the
review. Preliminarily the review should take place on Tue 22nd May 2007 at
10:00am (Prague's time) and we are going to organize a confcall. Is anybody else
interested? If so, please drop me an email at vstejskal@netbeans.org and I'll
send you the details or talk to Mila if you're in Prague.

The current status of the SPI can be found on:
http://openide.netbeans.org/tutorial/reviews/opinions_88471.html

Thank you.

Y11: I updated the link and removed TBD. I'd prefer leaving the use cases in the
SPI package summary, because there is likely to be more stuff added to the
library in the future with their own use cases that should not be mixed up.

Y12: done
Y13: done

The updated javadoc should be on the website soon. If not, I can attach it here
or you can obviously generate it for yourself.
http://www.netbeans.org/download/dev/javadoc/

I have just a minor comment - whether we want to retain the ZOrder or whether
use a numeric sort instead. Vita pointed me to Jesse's doc
http://wiki.netbeans.org/wiki/view/FolderOrdering103187 which shows that the
numeric ordering has benefits in environments where the items can come from
various modules. Anyway I have no strong opinion on this, we can discuss it
tomorrow.

Regarding ZOrder versus simple numeric sort - I have no strong opinion either;
what is right for folder ordering generally might not apply here, so just
consider how it will be used, and what would be most intuitive for API users.

I think that one of the problems that the simple numerical ordering is trying to
solve for folders holds true here as well and that is to make the layers
hierarchy stable enough to survive when a module is disabled. That sounds to me
like a good reason for using the numerical ordering instead of relative
references between layers. On the other hand I'd like to keep the predefined
racks in some form, because they define the basic structure where layers can be
plugged in according to the meaning of highlighings they provide. We could
perhaps use the numerical ordering to specify an exact position within a rack
and get rid of the existing relative references.

It terms of the API that would mean to remove all the existing methods in ZOrder
and add: public ZOrder forPosition(int position) {...}. Clients would always
have to pick one of the predefined rack ZOrders (the same as now) and then they
could fine tune the position by passing in a number.

The SPI was approved in today's final review. We also discussed the ZOrder class
and agreed that it should be changed to use the numerical ordering within racks
as suggested before. I'll update the opinion document and track the requested
change as TCR in this issue. Thanks.

The ZOrder was changed to use a numerical position.

Checking in lib2/src/org/netbeans/spi/editor/highlighting/package.html;
/cvs/editor/lib2/src/org/netbeans/spi/editor/highlighting/package.html,v  <-- 
package.html
new revision: 1.5; previous revision: 1.4
done
Checking in lib2/src/org/netbeans/spi/editor/highlighting/ZOrder.java;
/cvs/editor/lib2/src/org/netbeans/spi/editor/highlighting/ZOrder.java,v  <-- 
ZOrder.java
new revision: 1.3; previous revision: 1.2
done
Checking in src/org/netbeans/modules/editor/impl/highlighting/HLFactory.java;
/cvs/editor/src/org/netbeans/modules/editor/impl/highlighting/HLFactory.java,v 
<--  HLFactory.java
new revision: 1.3; previous revision: 1.2
done
Checking in
lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/MemoryMimeDataProvider.java;
/cvs/editor/lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/MemoryMimeDataProvider.java,v
 <--  MemoryMimeDataProvider.java
new revision: 1.3; previous revision: 1.2
done
Checking in
lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/SyntaxHighlightingTest.java;
/cvs/editor/lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/SyntaxHighlightingTest.java,v
 <--  SyntaxHighlightingTest.java
new revision: 1.5; previous revision: 1.4
done
Checking in
lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/HighlightingManagerTest.java;
/cvs/editor/lib2/test/unit/src/org/netbeans/modules/editor/lib2/highlighting/HighlightingManagerTest.java,v
 <--  HighlightingManagerTest.java
new revision: 1.3; previous revision: 1.2
done
Checking in lib2/src/org/netbeans/modules/editor/lib2/highlighting/Factory.java;
/cvs/editor/lib2/src/org/netbeans/modules/editor/lib2/highlighting/Factory.java,v
 <--  Factory.java
new revision: 1.4; previous revision: 1.3
done
Checking in lib2/test/unit/src/org/netbeans/spi/editor/highlighting/ZOrderTest.java;
/cvs/editor/lib2/test/unit/src/org/netbeans/spi/editor/highlighting/ZOrderTest.java,v
 <--  ZOrderTest.java
new revision: 1.3; previous revision: 1.2
done

Thanks for the reviews.

Done.