Created attachment 22365 [details]
The zip containing javadoc documentation including arch document

I would like to ask the DevRev team for review. The currently assigned reviewers
are Jan Lahoda, Petr Pisl, Yarda Tulach and Tomas Pavek. The review is planned
to be done on June 06.

TDB1: Not sure I love AbstractPaintComponent/DefaultPaintComponent for a few reasons:

1.  I have a feeling a lot of folks will construct 1 component for each item to be rendered.  I don't see 
any other way to set the icon except in the constructor.  This might not scale terribly well.

2.  Component subclasses are inherently dangerous (can you guarantee it will always be constructed in 
the event thread, etc.?)

3.  It means there is a lot of surface area to its API.  

4.  Do we really want to have variable height completion items?  Unless we really think we will need 
that, we could probably do without a component subclass entirely - the code for the popup will be 
more efficient (if not variable height, every list item is not needed to calculate the preferred size).

5.  Consider using org.openide.awt.HtmlRenderer for left/right text and having items simply provide an 
HTML string instead of providing painting logic.

What I could imagine this being is something like:

public interface ItemUI {
   public String getLeftText(); //document that HTML subset is OK
   public String getRightText();
   public Icon getIcon();
}

(the above methods could be grafted onto CompletionItem).  All the painting/dealing with graphics 
objects can then be private, and it will be easier for people to use.  If you feel strongly that custom 
painting should be allowed, add
public void paint (Graphics g, Dimension d)
to be called only if getLeftText() returns null.

TDB2:  CompletionItem and CompletionDocItem sound as if they're going to be very similar, but 
actually a CompletionItem is one item in a list, and CompletionDocItem is an unrelated interface 
representing the documentation for one item.  Would prefer a class name like ItemDocumentation.

TDB3: AbstractPaintComponent.resetCachedRightTextWidth() is an implementation detail and really 
shouldn't be in the API.

TDB4:  Some class and method names can be shorter (that it's about completion can be inferred from 
the package name):
CompletionItem > Item
(if kept) AbstractPaintComponent.resetCachedRightTextWidth() -> reset()

TDB5:  A bit unclear what the InputMap/ActionMap on CompletionItems are for - seems a bit 
underdocumented.  If you keep AbstractPaintComponent (hoping you don't), that component has 
Action/Input maps - maybe use those?

TDB6:  (I'm assuming the InputMap/ActionMap for items is used when the item is selected and that we 
are offering some way to handle keybindings for individual items other than VK_ENTER) - if we have 
Input/Action maps, what is DefaultCompletionItem.enterAction() for?

TDB7:  Unclear what DefaultCompletionItem.getValue() is for - if users of this class are expected to 
subclass it anyway, it doesn't seem like it adds much value to have this constructor argument - what is 
it for?

TDB8:  DefaultCompletionItem.getPaintComponent() should probably return AbstractPaintComponent, 
not DefaultPaintComponent, as it is documented that the user should override it.  

TDB9:  Not at all clear what is the difference between DefaultCompletionItem.getPaintComponent() and 
DefaultCompletionItem.getPaintComponent(boolean selected), or when one versus the other should be 
called.

TDB10:  Most methods are pretty thin on documentation - for example, CompletionItem.getInputMap() 
is documented as "Returns the item's input map" but gives no clue why CompletionItems have 
InputMaps.

TDB11:  EventObject seems like a strange choice for argument and return type from several methods in 
CompletionTaskProvider - unless it really is legal to use things like WindowEvent or ComponentEvent or 
FocusEvent here, it seems like some less abstract parent class would feel less strange.

TDB12:  CompletionItem.SortKey.getSortPriority() - "Returns the item's priority. A lower value means a 
lower index of the item in the completion result list."  Lower than what?  I assume this is used to group 
different types of completions together (so, i.e. all abbreviations will be grouped together, before or 
after class name completions, etc.).  Seems like it might make more sense to delete this class and move 
getSortPriority() to CompletionItem (documenting the bounds for return values) and have 
CompletionItem extend Comparable (possibly documenting that CompletionItem.toString() should 
return the text value if we will have types that don't know about each other sorting against each other).

> TDB1: Not sure I love AbstractPaintComponent/DefaultPaintComponent for a few
reasons:
> 
> 1.  I have a feeling a lot of folks will construct 1 component for each item
to be rendered.  I don't see 
> any other way to set the icon except in the constructor.  This might not scale
terribly well.

No, they should be singletons. This works like a cell renderer's component -
since it's only used in AWT thread it's enough to have just one instance reused
for rendering of any item of the same "type". Maybe I should rename
PaintComponent to e.g. RenderComponent and improve javadocs.

> 
> 2.  Component subclasses are inherently dangerous (can you guarantee it will
always be constructed in 
> the event thread, etc.?)

I remember the thread on nbdev regarding component creation outside AWT thread
but honestly the same pattern (singleton static paint components)
was used in the original code completion and in the current completion as well
with no apparent problems. If necessary we may use and recommend
a pattern with lazy creation and static getter
which will ensure safe creation in AWT thread.

Anyway I guess we need support class for the rendering component to simplify the
process
of completion providers for developers. Also we need to give the items the same
rendering look
(icon, left text, right text).

> 
> 3.  It means there is a lot of surface area to its API.  
> 
> 4.  Do we really want to have variable height completion items?
> Unless we really think we will need 
> that, we could probably do without a component subclass entirely - the code
for the popup will be 
> more efficient (if not variable height, every list item is not needed to
calculate the preferred size).

Personally I do not think that writing of rendering component is so difficult.
Developers
should already be used to it from list cell renderers.
Anyway as this is a SPI support class I assume slightly more experienced developers
than for an API usage.
I do not know whether there will be a usecase for a variable heihgt completion item.
I see one somewhat crazy :) usecase:
As there is no horizontal scrollbar in the completion popup
(ellipsis will be displayed if item's left text is too wide e.g. method with too
many parameters)
we might implement item's left text line wrapping and display the remaining text
on an extra line.

> 
> 5.  Consider using org.openide.awt.HtmlRenderer for left/right text and having
items simply provide an 
> HTML string instead of providing painting logic.

Yes, that would be convenient. What about the performance of such setup?
There may be hundreds of the items in the completion window.
In fact the completion item (e.g. a java method) will first have to compose
the html string from its data i.e. concatenate the method's name
plus its arguments intermixed with all the color information specification
in html format. That string (that one should probably be cached in the
completion item)
will then be decoded in the html renderer.
I think that we could give it a try in this early stage
hopefully the performance will be satisfying and even if not we could still use
it for some of the items (e.g. methods) and the performance-critical items (e.g.
classes) could be done in other way. 

BTW once the item gets selected we override the item's colors
and only use the selection background/foreground colors
(e.g. arguments names (in magenta) become black when the item is selected).
Does the HtmlRenderer allow for such functionality?

> 
> What I could imagine this being is something like:
> 
> public interface ItemUI {
   > public String getLeftText(); //document that HTML subset is OK
   > public String getRightText();
   > public Icon getIcon();
> }
> 
> (the above methods could be grafted onto CompletionItem).  All the
painting/dealing with graphics 
> objects can then be private, and it will be easier for people to use.  If you
feel strongly that custom 
> painting should be allowed, add
> public void paint (Graphics g, Dimension d)
> to be called only if getLeftText() returns null.

I still don't think that the cell rendering is so difficult pattern.
Having only the three mentioned methods might IMHO be too limiting
for some uses though the present usecases would be satisfied.
What about putting the three methods into the support classes
only but leave the cell renderer component pattern in the CompletionItem?

> 
> TDB2:  CompletionItem and CompletionDocItem sound as if they're going to be
very similar, but 
> actually a CompletionItem is one item in a list, and CompletionDocItem is an
unrelated interface 
> representing the documentation for one item.

Almost true but the CompletionDocItem gets returned
also when asking for documentation window only with Ctrl+Shift+SPACE
(not displaying completion window).

> Would prefer a class name like ItemDocumentation.
> 
> TDB3: AbstractPaintComponent.resetCachedRightTextWidth() is an implementation
detail and really 
> shouldn't be in the API.

Well it's protected final method which can't do any much harm. But I'll probably
remove it
as typically the right text isn't too complex anyway so the effect on
performance isn't big.

> 
> TDB4:  Some class and method names can be shorter (that it's about completion
can be inferred from 
> the package name):
> CompletionItem > Item
> (if kept) AbstractPaintComponent.resetCachedRightTextWidth() -> reset()

I do not like such names simplifying much because then when you see a code
with just 'Item' you don't know what does the class mean unless you scroll
to import section or use fully qualified names. Of course there are helper tools
in the IDEs like hyperlinking etc. but still I'm not convinced.

> 
> TDB5:  A bit unclear what the InputMap/ActionMap on CompletionItems are for -
seems a bit 
> underdocumented.  If you keep AbstractPaintComponent (hoping you don't), that
component has 
> Action/Input maps - maybe use those?

It's for handling the keybindings pressed when the completion item is active.
One particular problem with rendering component is that it's java.awt.Component
(similar to cell rendering components are) while InputMap and ActionMap
is Swing-specific. But we can require JComponent if necessary.

But as I'm now thinking about it the input map could be used without
item's rendering (e.g. for instant substitution) but the rendering
can be simulated to obtain the maps so it's a bit strange but possible.

BTW I will probably keep just one support class for the component not the
present two.

> 
> TDB6:  (I'm assuming the InputMap/ActionMap for items is used when the item is
selected and that we 
> are offering some way to handle keybindings for individual items other than
VK_ENTER) - if we have 
> Input/Action maps, what is DefaultCompletionItem.enterAction() for?

It's for overriding of the behavior of what happens when VK_ENTER key gets pressed.
The InputMap of the DefaultCompletionItem is only sensitive to VK_ENTER.

> 
> TDB7:  Unclear what DefaultCompletionItem.getValue() is for - if users of this
class are expected to 
> subclass it anyway, it doesn't seem like it adds much value to have this
constructor argument - what is 
> it for?

The completion items are usually wrappers around certain data value which they
encapsulate (e.g. the JMI elements).
It was called getAssociatedObject() in the original code completion.
The supporting classes e.g. rendering component
can get the data value which it then manipulates (instead of the item itself).
But it seems that for simplicity it will be better to eliminate this abstraction
and just use the item and leave this for implementors.


> 
> TDB8:  DefaultCompletionItem.getPaintComponent() should probably return
AbstractPaintComponent, 
> not DefaultPaintComponent, as it is documented that the user should override it.  

Could not be due to use of the setValue() but if we eliminate it then it could be.

> 
> TDB9:  Not at all clear what is the difference between
DefaultCompletionItem.getPaintComponent() and 
> DefaultCompletionItem.getPaintComponent(boolean selected), or when one versus
the other should be 
> called.
> 

DefaultCompletionItem.getPaintComponent(boolean selected) implements the
CompletinoItem interface.
DefaultCompletionItem.getPaintComponent() is protected and intended to possibly
be overriden
by implementors if the DefaultPaintComponent does not suffice. Maybe
getPaintComponentInstance()
name would be better?


> TDB10:  Most methods are pretty thin on documentation - for example,
CompletionItem.getInputMap() 
> is documented as "Returns the item's input map" but gives no clue why
CompletionItems have 
> InputMaps.

I'll fix that.

> 
> TDB11:  EventObject seems like a strange choice for argument and return type
from several methods in 
> CompletionTaskProvider - unless it really is legal to use things like
WindowEvent or ComponentEvent or 
> FocusEvent here, it seems like some less abstract parent class would feel less
strange.

Since you are the second person that complains I will create a common superclass
for the three possible events.

> 
> TDB12:  CompletionItem.SortKey.getSortPriority() - "Returns the item's
priority. A lower value means a 
> lower index of the item in the completion result list."  Lower than what?

If item1 has sp=2 and item2 has sp=5 then item1 will be located before item2 in
the completion popup window.

> I assume this is used to group 
> different types of completions together (so, i.e. all abbreviations will be
grouped together, before or 
> after class name completions, etc.).  Seems like it might make more sense to
delete this class and move 
> getSortPriority() to CompletionItem

This was the original state (the two methods were part of the CompletionItem
directly).
In fact all present CompletionItem's implementations have the two methods in the
completion item
and the CompletionItem's impl implements the SortKey.
The SortKey presence just emphasizes that the two methods are intended for sorting.
Anyway I have no strong opinion regarding that - we may remove the SortKey
if it's seems like a complicating factor.

> (documenting the bounds for return values) and have 
> CompletionItem extend Comparable (possibly documenting that
CompletionItem.toString() should 
> return the text value if we will have types that don't know about each other
sorting against each other).

I would definitely *not* reuse toString() for anything like this. There should
be an extra method.
I would not implement Comparable because there is a usecase for alphabetical
ordering
(competitive products have it as well). Therefore there are two comparator
implementations
- one that prefers priority and has the sort text is a less significant criterion
and the other one that prefers sort text and priority is less important
(see CompletionImpl for implementations of the two comparators).

Tim, thanks a lot for the comments.

>No, they should be singletons. This works like a cell renderer's component - since it's only used in 
>AWT thread it's enough to have just one instance reused for rendering of any item of the same "type".

I understand that that is the intent of the API, but there's nothing that enforces it.  Singletons as in per-
provider singletons, or global singletons?  With an interface to supply text/icon, you can have one 
global renderer component that acts as the list cell renderer, and users of the API won't have to worry 
about painting logic at all unless they really want to...*and* you can add the methods directly to 
CompletionItem and have one less class in the API.  I assumed that it was intended to be a singleton 
like a list cell renderer, but I'm not sure everybody will have the sense to implement it that way, so 
better to just not make them need to worry about it.

>Personally I do not think that writing of rendering component is so difficult.  Developers should 
>already be used to it from list cell renderers.

I was having an interesting conversation with Scott Violet about this - that basically there needs to be 
some "convert to text" object that one can use in place of a cell renderer for most cases - people tend 
to do a lot of things wrong when writing cell renderers (validation & property change support should be 
overridden to be no-ops, etc.).  Writing a cell renderer is easy for you and me, because we both have 
tons of experience doing that sort of thing.  It's torture for folks who grew up on server side java.

> As there is no horizontal scrollbar in the completion popup (ellipsis will be displayed if item's left text 
> is too wide e.g. method with too many parameters) we might implement item's left text line wrapping 
> and display the remaining text on an extra line.

All the more reason to have the items only supply text - so your code can handle any kind of wrapping 
you need to do, and the provider of the hint doesn't need to worry about it.  If you do a custom 
implementation of getPreferredSize(), it can be very efficient.

> Yes, that would be convenient. What about the performance of such setup? There may be hundreds of 
> the items in the completion window. In fact the completion item (e.g. a java method) will first have to 
> compose the html string from its data i.e. concatenate the method's name plus its arguments 
> intermixed with all the color information specification in html format.

The performance of org.openide.awt.HtmlRenderer is 10x that of Swing HTML rendering, and it works 
fine for Explorer, property sheet, etc.;  it's as fast an HTML renderer as it is possible to write in Java, I 
believe.  BTW, if you want to for some reason use UIManager keys for the colors and define them in 
UIDefaults (add as appropriate to core/swing/plaf), you can do font color="!controlShadow", etc.  I 
suspect the performance issues will be in gathering the items, not in calculating the preferred size.  If 
you're really worried about it, you have the option of calculating the size based on character count 
(works better with a fixed width font, or you can base it on the width of a wide character from a 
proportional font;  anyway, you don't need me telling you all of this).

> BTW once the item gets selected we override the item's colors and only use the selection 
> background/foreground colors (e.g. arguments names (in magenta) become black when the item is
>selected).  Does the HtmlRenderer allow for such functionality?

I'd suggest that you document that item providers should only influence font style, not color, and let 
your code wrap the left/right text in color tags, since your code will have more knowledge of the 
background color of the list, etc.  Or you could strip the color tags;  or you could pass a boolean 
argument to the getLeftText/getRightText methods to determine whether color info should be included 
(though this doesn't seem so nice).

> I still don't think that the cell rendering is so difficult pattern. 

Well, I think of Jesse saying he gets indigestion whenever he has to import java.awt or javax.swing 
anything;  lots of programmers today are used to doing business logic, not painting.  We might as well 
make it easy for them.

> Having only the three mentioned 
> methods might IMHO be too limiting for some uses though the present usecases would be satisfied.
> What about putting the three methods into the support classes only but leave the cell renderer 
> component pattern in the CompletionItem?

I like this idea;  provide an already-implemented getRenderer() method that returns an optimized list 
cell renderer, which will render the result of getLeft/RightText & getIcon, and documents that normally 
this is not overridden.

> Almost true but the CompletionDocItem gets returned also when asking for documentation window 
> only with Ctrl+Shift+SPACE (not displaying completion window).

Okay;  could we rename CompletionDocItem to, say DocumentationItem, or Documentation?

> It's for overriding of the behavior of what happens when VK_ENTER key gets pressed.
> The InputMap of the DefaultCompletionItem is only sensitive to VK_ENTER.

InputMap/ActionMap seem like overkill for this situation;  nobody is really likely to have a vast number 
of keys defined for one item.  What about just having a method CompletionItem.handleKey (KeyEvent 
ke), and check if the event was consumed?  It would get rid of all the issues of having a Component but 
needing a JComponent, etc.

I'm not sure if 

Filled a TCR (issue #59667) and two TCAs (issue #59668 and issue #59669).

Opinions document (for the inception rewiev) has been created:
http://openide.netbeans.org/tutorial/reviews/opinions_59388.html

Hi,

     Sub: What I have to do in my application if I want to use editor code 
completion API

 

            As in my application, the user can enter java code in simple text 
editor, so if I want provide features like code completion. What I suppose to 
do. If u can mail me sample code of it, that will really help me.

 

Thanks and Regards,

Kotesh Munugoti.

Currently the code completion only works for the NB based editors. The
infrastructure could work with just little changes for any editor but the CC
providers (e.g. the JavaCompletionProvider) are tied to NB lexical and parsing
analysis.

The requested TCRs and TCAs were resolved. Closing the issue.