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.
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.
Created attachment 74022 [details] Annotation, processor and test, no api changes yet
[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.
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
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.
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.
"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.
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.
'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.
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.
"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.
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
Created attachment 76150 [details] Another approach to make (ant project) registrations more declarative
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
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?
[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.
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..
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.
Created attachment 76473 [details] API fixes and cleanup of modules in standard distribution
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.
Created attachment 76480 [details] API fixes and cleanup of modules in standard distribution 2 (sorry for previous patch)
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.
We will integrate on Monday: http://hg.netbeans.org/ergonomics/rev/2a6a3bb9a45e Just press "the button" during Monday, Pavel. Thanks.
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
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.
As far as I know this is already fixed.
(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.