Created attachment 23412 [details]
Architecture Questions

Created attachment 23413 [details]
Overview of changes in the related modules.

The reviewers for this issue are pbuzek, jtulach, mroskanin and ppisl.

The work on the API already started. The attached documents are continuously
modified to track the planned state and might not exactly correspond to the
current implementation.

The implementation sits on the tag_editor_support branch and is spread among the
following modules:

xml/palettesupport
html
html/editor
html/editor/lib
web/core
web/jspsyntax

The following additional changes will be made according to the pbuzek's
suggestions if possible:

1. refactor the code from the html/editor/lib to html/editor
2. refactor the code from the web/jspsyntax to web/core

The following pbuzek's suggestions were already projected into the attached Arch
document:

1. merging of J2EEPaletteItemCustomizer interface into J2EEPaletteItemInsertable
interface
2. the J2EEPaletteItemContext and J2EEPaletteItemCookie classes are no longer needed
3. J2EEPaletteItemDataObject.getCookie() must work with
J2EEPaletteItemInsertable interface instead of J2EEPaletteItemCookie class
4. J2EEPaletteItemInsertable interface must extend Node.Cookie interface
5. the insertion logic implemented int the J2EEPaletteItemCookie class will be
shifted into the code handling the mouse actions executed on the palette
(palette actions, palette listener and drop target), perhaps via some utility method
6. J2EEPaletteItemInsertable.insert(...) method will have the following signature:
insert(JTextComponent, boolean): void;

Libor, is it possible to attach javadoc?

not yet written; it would not correspond with the changes because they are not
yet implemented.

basically, the only API for palette items will be:

/** Implementation of this interface must be returned as a cookie
 * from the node that represents J2EE palette items.
 */
J2EEPaletteItemInsertable extends Node.Cookie {

  /** Insert the component represented by the palette item 
   * into the text component. If the palette item has a customizer
   * and the showCustomizer is true then the cutomizer should be 
   * displayed prior to insertion.
   *
   * @param textComponent the TextComponent into which the item should be added
   * @param showCustomzier if false do not show customzier, if true show
   *  a customizer if the item has any
   */
  void insert(JTextComponent textComponent, boolean showCustomizer);
}

The editor will create the palette and in its PaletteActions it will lookup the
J2EEPaletteItemInsertable and call the insert method. The second parameter was
added to make it possible to suppress the customizer and just add the item in a
default way (e.g. when a modified key is pressed).

As I wrote in a private e-mail, I don't see a reason to have the word "J2EE" in
API class names, because this is a general interface between the palette and the
editor, not specific to J2EE. My preference would be "EditorPaletteItemInsertable".

MR01: xml/palettesupport module depends on editor module. Because of one usage
of  NbEditorUtilities.getDataObject(d) in DnDUtilities.getPath(). Please remove
this dependency and use:
      Object sdp = doc.getProperty(Document.StreamDescriptionProperty);
        if (sdp instanceof DataObject) {
            return (DataObject)sdp;
        }
instead.

MR02: please do not use editor's BaseDocument in your API. As in:
J2EEPaletteItemContext class: public BaseDocument getTargetDocument() or
setTargetDocument(BaseDocument targetDocument). Use Document instead.

MR03: setters in API class J2EEPaletteItemContext like: setTargetDocument or
setAttributeValue or Attribute.setValue look very suspicious to me.

MR04: editor/lib package is standalone library. It shoudn't depend on openide
(with exception of openide.util package) packages like:
org.openide.loaders
org.openide.dialogs
org.openide.nodes
org.netbeans.spi.palette
org.netbeans.modules.projectapi
org.netbeans.modules.editor
org.openide.explorer
Please, move your implementations from html/editor/lib to html/editor/src 

MR05: packages like:
org.netbeans.editor.ext.palette.api
org.netbeans.editor.ext.palette.spi
org.netbeans.editor.ext.palette.support
org.netbeans.editor.ext.dnd.api
org.netbeans.editor.ext.dnd.spi
are not very good idea, because we plan to deprecate org.netbeans.editor and
org.netbeans.editor.ext contents in the future completely. Maybe
org.netbeans.modules.editor.palette.api or org.netbeans.lib.editor.palette.api
would be better.


 

Created attachment 23633 [details]
diff of palette client support

Created attachment 23634 [details]
Arch doc diff

Created attachment 23635 [details]
Arch doc updated

To be short:  
1. I do not like the level of indirection needed to create ActiveEditorDrop 
instance. It would be simpler to have  
 
!ELEMENT active-editor-drop EMPTY> 
+<!ATTLIST active-editor-drop  
+    class CDATA #REQUIRED 
+> 
 
and just do Class.forName(className, true, 
Thread.getContextClassLoader()).newInstance() - imho this would be simpler for 
users. 
 
2. No tests - I'd like to see registration of XML file with body and 
ActiveEditorDrop into layer, creation of Palette over a directory and 
iteration over the nodes of the palette to find out the node is really there, 
the icons and names are ok and that the drag(), getLookup(), clipboardCopy 
contain what they should contain. Imho this is more important then any effort 
for good design. 

ad 1. there is no indirection, item implementor writes directly the class name
(it is already implemented, I forgot to document it in the arch doc)

ad 2. currently working on it

re.1: sorry, I overlooked that. re.2: ok. allandall: ok, from me. I guess. 

PJ01: The DTD should be documented (inline, not in the arch. document), so the
clients know what each element means. Currently, it's not easy to figure out.

Created attachment 23752 [details]
unit tests

Created attachment 23753 [details]
DTD

As far as I know we turned this into a fasttrack with a small addition to 
core/palette. I am adding API_REVIEW_FAST. 
 
Also I guess this is now fixed and integrated. So I guess this issue can be 
closed. Meanwhile it is a defect that it is open, is it not? 

Yes, you are right.

Closing DevRev issue.