Y01 Missing annotation processor
Y02 Missing test
Y03 I'd be delighted if people could just annotate (constructor of) subclass of DataObject without the need to write loader or factory at all.

Thanks Jaroslav

I am a new comer so I was only on the Annotation Type discussion before implementing and provide test. Thats why Y01 and Y02 are not done.


For point 3 I

Thanks Jaroslav

I am a new comer so I was only asking for review on the Annotation Type
discussion before implementing and provide test. Thats why Y01 and Y02 are not
done.


For point 3 I would say that I have nothing to argue against. I will add Jesse
to the cc list is also the author or the wiki page.

As far I as understand Y03:

@DataObject.Factory.Registration should be restricted to DataObject constructor
subclass.


PS: sorry for partial posting happening while adding Jesse to cc list
PS2: what is the meaning of Y0.. ?

Created attachment 115053 [details]
Annotation type for dataloader with processor and unit test

Annotation default is usage of DataLoaderPool factory

possibility to register a subclass of dataloader via loader().
(if registration via dataloader subclassing is obsolete may not be a good idea to keep)


I am in trouble with on test line 172 of DataObjectFactoryProcessorTest.


I hope this diff is more reviewable.

Re. "Meaning of Y00" - we try to number comments to make them easy to be referenced. Usually we prefix them with name initials. For historical reasons I am using "Y".

As part of the review you are supposed to address these comments or state that you ignore some of them. Reviewers can then insist or give up.

The new patch looks very rich, thanks, we have to drive this to conclusion and integration for 7.2.

Y04 I prefer @DataObject.Registration - it is shorter and if the most common usage is annotating direct subclass of DataObject, it is natural.

Y05 The mimeTypes attribute of the annotation could rather be mimeType. We are usually using singular if it is expected that majority of usages will provide just a single string. Which I think can be expected in this case.

Y06 Would not it be better allow annotating directly any DataObject.Factory subclass (including DataLoader subclass)?.That would remove the need for loader attribute. Possibly the target of the annotation could only be TYPE. In case of DataObject.Factory subclass the type would have to have public default constructor. In case of DataObject subclass it would have to have constructor with FileObject and MultiFileLoader parameters (as required by http://bits.netbeans.org/dev/javadoc/org-openide-loaders/org/openide/loaders/DataLoaderPool.html#factory(java.lang.Class,%20java.lang.String,%20java.awt.Image)

Y07 Versioning: Change spec version of openide.loaders, write apichanges.xml and use @since tag on the annotation.

Y08 FileType wizard in apisupport.project should be rewritten to (conditionally - if new enough spec version of openide.loaders is present) use the new annotation.

Otherwise I think your work is perfect. If you polish the remaining few things, I'll be happy to integrate your change.

Created attachment 115166 [details]
Rewrite according review.

Thanks for the explanation.
I did a mistake in my local mercurial by commiting my change. I do hg diff -g -r 211239 -r 211240 (my localcommit). I hope its fine. 

Y04 
Done and more natural to use.

Y05
Done.

Y06
Done but with maybe overcautious check in the processor.

Y07
Changes loaders spec to 7.36 according to the revision I have.

Y08
I prefer doing this in a other attachement after being sure of versioning.

Regards
Eric

Re. Y05: Current state is OK. But I was not clear enough, the name should be "mimeType", but the type could stay String[] if you think it is useful.

Y09 Use f.position(dfr.position()) instead of 
+  if (dfr.position() != Integer.MAX_VALUE) {
+    f.intvalue("position", dfr.position());
+  }
See http://bits.netbeans.org/dev/javadoc/org-openide-filesystems/org/openide/filesystems/annotations/LayerBuilder.File.html#position(int)

Y10 Modify ide.branding/release-toplevel/CREDITS.html and I think I can integrate on Tue, Jan 31, 2012.

Re. Y08, please try to prepare the patch by Tue

Y05. I will redo the multiple mimeType with the typo fix as it seems used by some module (JSFLoader for example). 
However I see a drawback because in this case position will be set for all mimetype the same.


Y08. Started working on that with 7.36 as spec version number. I'll try to get it done as one patch (dataobject processor + wizard).

Y09. Thanks for the info.

Y10. Will go into this. It cost a bit of time to recompile the IDE for testing this :) .


--
Appendix: (Y02 related)

The following unit test fail to get the correct loader for a DataObject.Factory subclass.

public void testDataLoad() of DataObjectFactoryProcessorTest.java
 {
 FileObject fo = createXmlFile("sdfsdf", ".tt3");
 assertEquals("text/test3", fo.getMIMEType());
 // XXX DoFPCustomLoader not loaded cannot assert for loader
 }

I was writting the following but not success
assertEquals("DataLoader type", DoFPCustomLoader.class, find.getClass());

Created attachment 115201 [details]
Annotation type for dataloader and wizard

First attempt for FileType wizard [1]
DataObject.Registration done according to comment #8


[1]
Usability: 
Wizard is not showing that multiview need icon.

[JG01] You need to check for null after calling Element.getAnnotation(Class), even if RoundEnvironment.getElementsAnnotatedWith(Class) listed the Element, due to (what I consider) a compiler bug.


[JG02] e.asType().toString() might not work for nested classes. You need to make sure to get a binary name.


[JG03] Check for public no-arg ctors for a DataObject.Factory is too much work. Rather use instanceFile or instanceAttribute, which is easier and does more precise checks than you have here; if that fails with LGE, go back and check for assignability to DataObject and its idiosyncratic ctor pattern.


[JG04] We need some clarification on the 'label' and 'icon' attributes. Are they used by anything in the UI? If so, they should probably be mandatory; if not, they should be dropped.


[JG05] Typo: DATAOBJET_REGISTRATION


[JG06] Recommend using FreeMarker's power more effectively. Pass basic data like mimeType, icon, label directly to the template if annotations are in use, simply leaving these undefined otherwise; FM has conditional expressions which can let the actual generated source code be visible in the template. The result is generally more readable and flexible.


[JG07] A hint in apisupport.refactoring to translate current registrations (using DataLoader.factory, or on a custom DataLoader) to the new syntax would be welcome.


[JG08] Several modules should be converted to use the new annotation to demonstrate the appearance (and help test JG07). See [1] for list.


[JG09] A major annoyance with the current registrations is not addressed at all by the current annotation: how to declare the context menu for the data loader. ([2] mentions this.) We would be eliminating the five or so lines in the XML layer for registering the factory, while leaving the sixty-plus lines used for its action list - the lines most likely to contain typos, in fact.

While this feature could probably compatibly be added later, it is worth considering now. One goal would be to be able to define the category and id of "standard" actions as Java constants, so that

<file name="PropertiesAction.shadow">
    <attr name="originalFile" stringvalue="Actions/System/org-openide-actions-PropertiesAction.instance"/>
    <attr name="position" intvalue="3000"/>
</file>

could be replaced with an entry in an annotation array such as

...
@Action(category=PropertiesAction.CATEGORY, id=PropertiesAction.ID, position=3000),
...

where

@ActionRegistration(category=PropertiesAction.CATEGORY, id=PropertiesAction.ID)
...
public class PropertiesAction ... {
  public static final String CATEGORY = "System";
  public static final String ID = "org.openide.actions.PropertiesAction";
  ...
}

when the action itself was for historical reasons in the API, though in modern cases only the constants would be exposed publicly.

Dealing with separators is another concern; annotation arrays may not be polymorphic, so something like

<file name="Separator1.instance">
    <attr name="instanceClass" stringvalue="javax.swing.JSeparator"/>
    <attr name="position" intvalue="300"/>
</file>

would need to be represented as e.g.

...
@Action(position=300),
...

where the processor would require either category and id (an action) or neither (a separator).

The existing @ActionReference would be preferred for actions defined in the same module (and mandatory for actions defined in a module this one cannot depend upon); the actions array would be needed for actions defined in a module this one depends on.

Generating 'position' attributes according to list position (100, 200, ...) would be nice but probably cannot work for compatibility reasons, especially given @ActionReference's which would insert at other positions; but the processor could at least enforce that the positions are in strictly increasing order, to prevent you from accidentally writing an action list which is visually misleading.


[1] http://deadlock.netbeans.org/hudson/job/nbms-and-javadoc/lastStableBuild/artifact/nbbuild/build/generated/layers.txt
[2] http://wiki.netbeans.org/DeclarativeRegistrationUsingAnnotations#Problems_with_actions

JG04: 3a5a5252aa72
JG05: OK.
JG01: 7e601cd4970b
JG02: OK. 
JG03: Leaving as it is.
JG07: Up to Eric or Jesse.
JG06: I've created completely new template for annotation based registation.
JG08: Probably should follow JG07.
JG09: Indeed, the wizard should generate no layer XML. Fixed.

Integrated
http://hg.netbeans.org/ergonomics/rev/dc417bdb3643
now let's see what gets broken.

I'am late sorry. Jaroslav all ready patch everything.

JG04:
I think iconebase was used in the IDE (File | New File... wizard) to show the icon in the list. But it seems also that the new multiview is requesting a icon too.

JG07,JG08:
I would be happy to take a look a this task after I can resynchronize my mercurial. 
I must wait for main silver to be merge or clone ergonomics.


-----


If I can do a remark on the New File Type Wizard wizard.

[EB01]
  On the third step Name,Icon and Location:
1) If a icon is set ok no pb;
2) If no icon use the empty file icon(for example) and publish it with a prefix. 

This may allow dev to take care of icon later with only the icon to modify.

EB01 - possible, though for other cases we either make the icon optional in the annotation and ensure that the runtime behaves reasonably with no icon set; or make the icon mandatory in the wizard.

For this case you would need to specify an icon for use in the data node anyway, I think.


For JG09 it looks like you adopted the syntax of @ActionReference(s) as a separate top-level annotation:

@DataObject.Registration(mimeType="text/whatever", iconBase=..., displayName=...)
@ActionReferences({
    @ActionReference(
        path="Loaders/text/whatever/Actions",
        id=@ActionID(
            category="System",
            id="org.openide.actions.OpenAction"
        ),
        position=100, 
        separatorAfter=200
    ), ...
})

This seems rather ugly - you need to specify the path redundantly for each action, and need to use a nested @ActionID. Would be rather less verbose if a special-purpose targetless annotation were introduced for this use case:

@DataObject.Registration(mimeType="text/whatever", iconBase=..., displayName=..., actions={
    @Action(
        category="System",
        id="org.openide.actions.OpenAction",
        position=100,
        separatorAfter=200
    ), ...
})

Such an annotation might be best defined in org.openide.awt since it may well be necessary to reuse it in various other modules; @Target({}) ensures that it may only be used in nested mode. Some friend API in org.openide.awt to process an array of these things (given a String path) would be helpful, reusing ActionProcessor code.


[JG10] 'position' is usually unnecessary on @DataObject.Registration since there is likely to be only one loader for a given MIME type, so apisupport.wizards should not generate it.

[JG11] This code

@TemplateRegistration(folder="Other", content="${PREFIX}${TEMPLATE_NAME}")
public static WizardDescriptor.InstantiatingIterator templateIterator() {
    return null;
}

cannot be right; you may not return null from a registered factory method (*). Did you mean to place @TR on package-info.java instead (CMF.packageInfo)?


(*) If JSR 305 were finalized then

@javax.annotation.Nonnull(when=javax.annotation.meta.When.ALWAYS)
@javax.annotation.meta.TypeQualifierNickname
@interface TemplateRegistration {...}

would be useful on annotations registering Java objects, though this would also require @Retention(CLASS) for use from FindBugs.

Re. JG11 - not a nice workaround, but it works[1]. Requiring templates to be registered on package-info.java is very restrictive and not nice to the File Type wizard at all[2]. Some easy to use support should be created first.

[1] What would be the difference between missing attribute and attribute returning null from filesystem perspective anyway?
[2] I don't want to write code that checks package-info.java, verifies no @TemplateRegistration is there. If it is there it removes it, replaces it with @TemplateRegistrations...

(In reply to comment #16)
> I don't want to write code that checks package-info.java, verifies no
> @TemplateRegistration is there. If it is there it removes it, replaces it with
> @TemplateRegistrations...

Currently it will just add another of the same annotation which (pending JEP 120) the user has to wrap in the plural annotation. It is true that CMF.packageInfo does need to be extended to include logic as proposed in bug #207577 JG11, in case you happen to run the wizard twice in the same package; but this is already true for several other wizards, so it would just all be fixed at once.

JG07,JG08:

I start working on this point. Is it fine to keep this task in the current issue?

[EB02].
After trying to do refactory on image module and seeing other dataloader instance it may be more usefull for openide loaders users to have

 plural Registrations possibility to make a per mimeType (display, positioning and icon)

of this form

@DataObject.Registrations({
@DataObject.Registration(mimeType="image/png", displayName="#TODO"),
@DataObject.Registration(mimeType="image/gif", displayName="#TODO")
})

[EB03].
I'm not a refactory specialist but it seems impossible in the current state to have annotation declared as inner type to have a nicer import

current:
import org.openide.loaders.DataObject.Registration;
@Registration(...) 

more readable (personnal point of view)
import org.openide.loaders.DataObject;
@DataObject.Registration(...)

(In reply to comment #19)
> 
> [EB02].
> After trying to do refactory on image module and seeing other dataloader
> instance it may be more usefull for openide loaders users to have
> 
>  plural Registrations possibility to make a per mimeType (display, positioning
> and icon)
> 
> of this form
> 
> @DataObject.Registrations({
> @DataObject.Registration(mimeType="image/png", displayName="#TODO"),
> @DataObject.Registration(mimeType="image/gif", displayName="#TODO")
> })

If
@DataObject.Registration(mimeType={"image/png","image/gif"}, displayName"#TODO)
is not enough. Consider creation of the patch that adds Registrations.

Created attachment 115551 [details]
DataObject Registrations and Hint

This patch is based on a new clone of main-silver (so it contains all what Jaroslav do during previous discussion)

DataSystem.
-Add Registrations for multiple DataObject Registration
-New UnitTest for Registration and also MimeResolver Registration usage (my  usage may be a bit ugly )

ApiSupport Hint
-Hint for refactory DataObject
   Only tried with Image
 

Hint "works" but. If I take the place of the user of the hint. 
[EB03] give a not coherent import with the wizard generated Loader.
and I add the following:

[EB04]. what is the status of Bundle for the display name?
Should it be a generated as annotation ?
Should it stay as bundle ?
In the ideal world what should be done?

EB04 and EB03 are for me more meta level thoughts on consistency in netbeans.

I miss an point in my previous comment

[EB05]
Should be a hinter for the actions in the Loaders/mime/type/Action



To handle [JG08]. 

How deep I can go ?
Use of Message annotation? or keep Bundle? 
Use generated layer of an action if I see a easy one ?

Are "module dev" happy with refactoring of their module ? 
Should it be asked ?

(In reply to comment #18)
> JG07,JG08:
> 
> I start working on this point. Is it fine to keep this task in the current
> issue?

Better file a separate issue for it so progress can be tracked and comments kept separate from comments about the API.

(In reply to comment #21)
> This patch is based on a new clone of main-silver

I hope you did not actually rerun 'hg clone http://hg.netbeans.org/main-silver/' when all you need do is 'hg pull -u' in an existing clone.

> [EB04]. what is the status of Bundle for the display name?
> Should it be a generated as annotation ?

Yes please use @Messages in newly generated code.

(In reply to comment #22)
> Use of Message annotation? or keep Bundle? 

For updating existing code with Bundle.properties entries just refer to the existing key. A separate hint exists to move the key into @Messages if desired.

Created attachment 115751 [details]
@DataObject.Registrations api, processor, test

Allows multiple DataObject.Registration when needed.

api version still 7.36 because is important (for me) to keep the both annotation (registration and registrations) together.

Hint is no more there.

"DataObject#Registration" will not hyperlink in Javadoc. Need to use @link somehow. (Can never remember the right syntax for nested classes; what works for javac does not always work for javadoc.)

Created attachment 115770 [details]
@DataObject.Registrations api, processor, test

Javadoc fix