This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 153655 - Project type registration shall be done with @annotation
Summary: Project type registration shall be done with @annotation
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Ant Project (show other bugs)
Version: 6.x
Hardware: All All
: P3 blocker (vote)
Assignee: Pavel Flaska
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 160265 194703
Blocks:
  Show dependency tree
 
Reported: 2008-11-21 17:08 UTC by Jaroslav Tulach
Modified: 2011-01-25 18:00 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Annotation, processor and test, no api changes yet (33.28 KB, patch)
2008-11-21 17:10 UTC, Jaroslav Tulach
Details | Diff
Another approach to make (ant project) registrations more declarative (47.44 KB, patch)
2009-01-22 19:06 UTC, Jaroslav Tulach
Details | Diff
API fixes and cleanup of modules in standard distribution (88.14 KB, patch)
2009-02-02 15:49 UTC, Jaroslav Tulach
Details | Diff
API fixes and cleanup of modules in standard distribution 2 (sorry for previous patch) (118.75 KB, patch)
2009-02-02 16:58 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2008-11-21 17:08:12 UTC
In order to implement Open Project dialog in
http://wiki.netbeans.org/FitnessForever
I need list of project triggers: I need a kind of indication to find out that a module can potentially load a project. 
I have two choices: either I hardcode this information into my module, or I create standard way of providing this 
information. Here is my attempt to do the latter option. This is just a sketch, I'll improve the docs and also design 
similar annotation for AntBasedProjectType.

In case, this is an acceptable path, I'll polish the patch during next week. Otherwise I hard code the info to my 
module and everyone adding new project type will have to adjust it.
Comment 1 Jaroslav Tulach 2008-11-21 17:10:14 UTC
Created attachment 74022 [details]
Annotation, processor and test, no api changes yet
Comment 2 Jesse Glick 2008-11-21 17:43:20 UTC
[JG01] Please do not copy all of FileOwnerQueryTest (including bogus @author!) for no good reason. Surely whatever it is
you are adding could be tested in a few lines of code. I'm not even sure what FileOwnerQuery has to do with project
loading exactly. Make a fresh, small, simple test that other people can actually read and maintain.


[JG02] From what I can infer (there is no useful Javadoc), the new @ProjectFactory.Registration is useful only in cases
where the ProjectFactory impl requires the presence of some specific filename. This is a more specific case than the
general PF contract, so I think the name .Registration should not be used, as that would imply that this annotation was
the canonical way to register any PF. Not sure exactly what class name to suggest, but perhaps RequiredFileRegistration.


[JG03] I presume that the new registration would be used for Maven project types, perhaps Rails? Maybe
AntBasedProjectFactorySingleton (but not concrete Ant-based project types)? Would need to see some example usages in
client modules.


[JG04] Why does requiredFiles() specify default {}? Seems like this attribute should be mandatory; {} would seem to
indicate that the factory would never be loaded. (No Javadoc, so unclear if this is AND semantics or OR semantics, but
impl looks like AND.)


[JG05] Is there any concrete use case for more than one required file? I can't think of any. Anyway, if this does need
to be supported, just create one file attribute, e.g.

<attr name="requiredFiles" stringvalue="nbproject/project.xml:something/weird.xml"/>

This would be simpler.


[JG06] Suggest .instanceAttribute("delegate", ProjectFactory.class) rather than .newvalue("delegate", binName). This
would (a) save you from needing to check for a public constructor, (b) check that clazz is really a ProjectFactory
(which you do not do now), (c) permit the annotation to be applied to factory methods too (if permitted in @Target).


[JG07] To be clear, the proposed system would reduce startup time but not implement Feature on Demand - right? I.e. you
still need to actually have the module installed which provides the project type in order for it to be recognized at all.
Comment 3 Milos Kleint 2008-11-21 20:47:17 UTC
MK1: there are currently 42 known project types.
You wrote, "In case, this is an acceptable path, I'll polish the patch during next week. Otherwise I hard code the info
to my module and everyone adding new project type will have to adjust it."
Can you elaborate what are the consequences for the project owners that you decide to ignore in case the api change
doesn't pass?

MK2: It's unclear to me what are the usecases for using the registered information? are you going to use it in Project
open chooser? In that case Icon property for the project type is missing. That's a rather big performance bottleneck
right now as the project needs to be loaded (not only recognized) to get the icon from ProjectInformation in project's
lookup.

MK3: to achieve the fast browsing of projects (without loading them), a simpler solution is to add icon to the project
factory registration somehow. The only troublemaker there is the AntBaseProjectsSingleton which unfortunately represents
a multitude of project types. Maybe a separate API could be created there?

+1 on JG07

Comment 4 Milos Kleint 2008-11-21 21:03:40 UTC
addition to MK1: The Lazy loading registration shall be tailored for project types that are less likely to be used. Eg.
there's no point in optimizing for j2se project, web or maven projects as these are likely to be used (if installed).
It's the long tail where some potential gains are to be made. Therefore there's no point in hardcoding stuff.
Comment 5 Jesse Glick 2008-11-21 22:17:39 UTC
MK2/MK3 is potentially important but somewhat orthogonal. As currently planned, just looking at a project in Open
Project would cause classes from that module to be loaded. In general, we would prefer to avoid creating the Project
object just to load icons. This can only be solved by introducing a ProjectFactory2 (or whatever) with a method to get
an icon (or null) for a potential project dir, with the idea that ProjectChooser's badger would call this new method
after isProject, and call loadProject only if the user actually clicks on the dir (in which case you need the name
anyway, and should probably check that project.xml is valid etc.). With such an SPI, the declarative registration could
easily support an optional icon attr. This would speed up the project chooser even when all modules were fully loaded.

Ant-based projects are not a big issue because the PF2 method would be given the dir. The only problem I can think of is
that we would not want to open & parse project.xml twice - once to satisfy isProject, once to get the icon. Possible
solutions:

(1) Keep a WeakHashMap<FileObject,String> in ABPFS to remember the project type for dirs. Would work, but ugly.

(2) Have PC call the icon method only (skipping isProject for a PF2), though this gets tricky because you really need
three kinds of return values (which does not fit nicely into Icon|null): yes a project, with the following icon; not a
project; and yes a project, but I don't know what icon yet. Perhaps the third case could be handled by the PF2 impl
(such as PFF) calling loadProject and using ProjectInformation, like PC does today and would need to continue to do when
encountering an old PF.
Comment 6 Jesse Glick 2008-11-21 22:25:57 UTC
"these are likely to be used (if installed)" - well, I have Maven support installed, but I don't use it most of the time
I am working in the IDE. Of course lazy-loading this by itself would only avoid loading one or two classes, barely
significant. More useful if most of those 42 known project types are installed and converted to use lazy registration.
With luck you might be able to avoid loading 20 or so classes in a heavy IDE installation, which might make a measurable
difference.
Comment 7 Milos Kleint 2008-11-24 07:24:17 UTC
jglick: ProjectManager2 with the new method to get the icon makes sense. I like your 2nd option better. Possibly we could
have a signature like "Result isProject2(File)" where the Result would be a final class with getIcon() getter. The
isProject2() method would return null if not applicable.

Comment 8 Jesse Glick 2008-11-24 14:53:45 UTC
'Result' could work, and would be extensible if in the future we wanted PF2 to be able to report e.g. project display
name. Alternately, could stay with PF2.getIcon (returning null if !isProject()) but have a special constant 'Icon
UNKNOWN_ICON' which would signal ProjectChooser to load the icon from ProjectInformation if it needs it.
Comment 9 Jaroslav Tulach 2008-11-25 09:10:25 UTC
Re. logic of "requiresFiles". In case the list of these files is empty, then the project factory is always being 
delegated to. In case it is non-empty, than the project factory is activated if at least one of the files exists. This 
imho qualifies the annotation to called ProjectFactory.Registration (as with defaults it behaves as it 
META-INF/services used to). Btw. grails project factory checks two files for both being present, but I guess even with 
one of them being around we can risk false positives and assume it is grails project.

Re. icon: Imho, the current APIs already provide a way for open dialog to obtain icon without loading the actual 
project class/module. One just needs a Project decorator. I've done that in:
http://hg.netbeans.org/ergonomics/rev/736a0f3cc0bf
http://hg.netbeans.org/ergonomics/rev/9a378df3bf79
So for purposes of this API Review, I would suggest to extend the ProjectFactory.Registration with "String iconBase()" 
attribute. The ProjectFactoryFactory in
http://www.netbeans.org/nonav/issues/showattachment.cgi/74022/X.diff
would then instantiate "class DecoratingProject implements Project, ProjectInformation".

Re. relation to FoD: The FoD build needs to understand which modules provide support for what project type. Currently 
I can only get info about all ProjectFactory implementations and AntBasedProjectType impls, but I have no clue what 
they do. With declarative ProjectFactory.Registration I can find that out by reading project's layer file.
Comment 10 Jesse Glick 2008-11-25 14:56:52 UTC
"In case the list of these files is empty, then the project factory is always being delegated to." - in such a case the
delegating registration is just overhead. You should simply continue to use @ServiceProvider as you today. The proposed
registration annotation is only useful insofar as there is some lazy aspect to the registration; we might in the future
wish to introduce one or more alternative lazy registration annotations for ProjectFactory. So I stand by JG02/04.


Using "a Project decorator" to implement lazy icon loading sounds like a bad idea to me. This is unnecessary and could
introduce all kinds of complex bugs. Delaying loading of the real PF is OK, but once loaded, use the real classes.
Comment 11 Milos Kleint 2008-11-26 10:32:44 UTC
I've created a separate issue for discussion of delaying loading of project instances in order to obtain icon in project
chooser dialog
http://www.netbeans.org/issues/show_bug.cgi?id=153923
Comment 12 Jaroslav Tulach 2009-01-22 19:06:31 UTC
Created attachment 76150 [details]
Another approach to make (ant project) registrations more declarative
Comment 13 Jaroslav Tulach 2009-01-22 19:17:37 UTC
Here is another patch to make AntBasedProjectType registrations declarative. It introduces new 
@AntBasedProjectRegistration annotation and moves into it all the declarative information from AntBasedProjectType2. 
The annotation can be applied to any Project subclass directly which renders AntBasedProjectType obsolete (as such I 
have deleted the ABPT2[1]). 

The patch is in raw form, versioning, documentation, etc. are missing. Guys look at it and tell me how you'd like the 
API to look like. I do not care much about the form, just the ergonomics effort[2]'s static analysis need all the 
registration to be in layer and fully describe itself. The patch satisfies this and seems to work.

I plan to address your comments and then rewrite all the AntBasedProjectType registrations to this new style, 
integrate and then improve the static analysis to use content of Services/AntBasedProjectTypes/

[1] ABPT2 may but need not be deleted. I prefer to remove it as it was not part of the release yet.
[2] http://wiki.netbeans.org/FitnessForever
Comment 14 Jaroslav Tulach 2009-01-26 12:24:57 UTC
Is the annotation looking OK? Shall there be the nested annotation or it is better to have
sharedName() default "data";
privateName() default "data";
sharedNamespace();
privateNamespace();
directly in the annotation? Is removal of ABPT2 OK?
Comment 15 Jesse Glick 2009-02-01 17:51:46 UTC
[JG01] I would vote for the String-valued attributes directly in the annotation over the nested annotation.


[JG02] I think removal of ABPT2 is OK, assuming that with this patch no one will still be using it. If for some reason
we needed to reintroduce it later, we could.


[JG03] Inconsistent to use getConstructor but getDeclaredMethod. I would recommend just requiring factory methods to be
public.


[JG04] The processor should verify that the class or return value type is assignable to Project.


[JG05] Consider creating the instance file name based on type(). This would help enforce that "no two registered
instances may share the same type".


[JG06] "It is forbidden for the result of this method to change from call to call." is nonsensical when applied to an
annotation attribute. Same for @param, @return.
Comment 16 Milos Kleint 2009-02-02 09:22:16 UTC
looks generally ok to me in purely technical terms. Unlike jglick, I would prefer nested annotations for readability's sake.

However I'm somewhat concerned about the complexity of the annotation. The major selling point of annotations (as I
understood it) was to clean up and simplify the APIs. I don't consider this one align with that goal. I understand why
ergonomics needs it, however at the same time ergonomics needs it in netbeans.org sourcebase only.. IMHO a more sensible
approach would be to just wire up the layer files for nb.org content and keep the api as it is..
Comment 17 Jesse Glick 2009-02-02 14:55:53 UTC
The annotation is no more complex than the ABPT interface. I would be concerned if it were common practice for an ABPT
impl to do subtle computations of whatever kind, and the annotation had to try to "keep up" with these subtleties
somehow. But in fact all ABPT impls that I know of simply return constant values for all methods except createProject,
which just calls the constructor of the Project impl, and it is hard to imagine what else they would want to do.
Comment 18 Jaroslav Tulach 2009-02-02 15:49:32 UTC
Created attachment 76473 [details]
API fixes and cleanup of modules in standard distribution
Comment 19 Jesse Glick 2009-02-02 16:17:06 UTC
Looks like for the newly converted modules, you define sharedNamespace & privateNamespace, but omitted sharedName &
privateName. This would be a compilation error, right?


BTW your last diff seems to be missing newly added files.
Comment 20 Jaroslav Tulach 2009-02-02 16:58:15 UTC
Created attachment 76480 [details]
API fixes and cleanup of modules in standard distribution 2 (sorry for previous patch)
Comment 21 Jesse Glick 2009-02-02 17:13:55 UTC
I see, you are intentionally defaulting to <data>. Still, where a named constant already exists for the config el name
(e.g. in XsltproProjectType), please either use it in the annotation, or delete the constant after verifying that it has
no other uses.
Comment 22 Jaroslav Tulach 2009-02-20 17:37:02 UTC
We will integrate on Monday:
http://hg.netbeans.org/ergonomics/rev/2a6a3bb9a45e
Just press "the button" during Monday, Pavel. Thanks.
Comment 23 Quality Engineering 2009-03-02 20:57:52 UTC
Integrated into 'main-golden', will be available in build *200903021401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/2a6a3bb9a45e
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #153655: Using annotation to register AntBasedProjectTypes
Comment 24 David Konecny 2009-03-09 04:22:02 UTC
I might be missing something but this issue seems to broke all project unit tests. Try web or j2se and most of the tests
fail because AntBasedProjectType cannot be found. There is issue 159622 tracking web project test failures which I
assigned to Jarda.

I struggle to figure out how to make the tests running (apart from directly calling
AntBasedProjectFactorySingleton.create). It should be very simple and the problem is most probably in my unit tests
setup but nothing I tried helped. What I find annoying is that previous relatively simple concept of placing instance
class to META-INF lookup was replaced by an annotation which has a processor which generates
META-INF/generated-layer.xml file which is then merged with module layer and which can be then looked up via default
(systemFS) lookup.
Comment 25 Pavel Flaska 2009-05-12 15:26:05 UTC
As far as I know this is already fixed.
Comment 26 Jesse Glick 2011-01-25 18:00:22 UTC
(In reply to comment #24)
> I struggle to figure out how to make the tests running (apart from directly
> calling
> AntBasedProjectFactorySingleton.create). It should be very simple and the
> problem is most probably in my unit tests
> setup but nothing I tried helped.

You might be seeing bug #194703.