Created attachment 87135 [details]
Diff with proposed change, updated "apichanges.xml" and increased module API version

Created attachment 87136 [details]
Example: An implementation of the ProjectAnnotator for Kenai module

Y01 Don't mention "ProvidedExtensions.RemoteLocation" attribute in javadoc of ProjectAnnotator. That is completely 
different API provided by some other module, unrelated to ProjectUI

Y02 I don't understand why there needs to be two methods. Imho one annotateIcon(...) is enough. Especially if it is 
enough for the Kenai sample and also in javadoc both bodies of the method are same.

Y03 Maybe rename the interface to ProjectIconAnnotator

Y04 Write a unit test.

02) If it was just about Kenai, it is correct. But as projectui (ProjectsRootNode.java) distinguishes between
opened/collapsed project nodes and their icons, I believe it is reasonable to allow the distinction from this new API,
so that it does not have to be extended later for the purpose. This is a way to provide a completely new icons for the
project nodes - they should have collapsed/expanded variants... Interface and the javadocs of the projectannotator are
clear about this. If code doubling was a problem, you can write your new method annotateIconImpl() and call it from both
methods... But if this is a big problem, I will do this...

I will deal with the rest of the points soon...

Re. Y02: I also believe, but as we are engineers let's stop believing and let's base the decision on facts: Please 
provide a usecase that could not be realized with one method and needs the two.

Fictive use case: A project type use an icon that looks like a book (it is a module that supports a book typesetting).
Icon has 2 states, opened book and closed book (depends on a state of a node). I would like to write an additional
support for the project type (for example I write a module that publishes the book) and I decide to annotate the icon by
drawing a support logo on the book. Apparently, I want to write the picture there when the book is closed and write a
text there when it is opened...

> but as we are engineers let's stop believing and let's base the decision on facts

This would start a philosophical debate, I guess. As I would like to achieve some future usefulness with this API, I
would prefer to stop basing decisions on facts known at this very moment (facts that might be - and probably are -
incomplete) and use my MatFyz rational abilities to believe that there will be at least one user who would say: "Why the
heck they don't support it when it is so easy...". I would suggest to do a traditional, boring and conservative PL
analysis of implementing one method instead of both methods - what exactly is the problem with having 2 methods in a
class with self explanatory names that are well documented?

OK, what about this:

- ProjectIconAnnotator will not be an interface but a class
- there will be one abstract method "annotateIcon"
- there will be a method "annotateOpenedIcon" with default implementation "return annotateIcon();"

User will by default implement only one method and he/she will have the opportunity to override the other one...

Re. "fictive usecase" - OK, that is enough. Feel free to leave two methods in an interface, just modify the javadoc to 
include this kind of usecase. Test it.

[JG01] Service name should mention the word "icon" if that is all it does. Like Y03.


[JG02] Rather than modifying ProjectsRootNode, consider modifying ProjectUtils.getInfo to create a proxy ProjectInfo
which would apply badges. This would ensure that the badged node appears elsewhere, e.g. Recent Projects list, which I
think is what you would want.


[JG03] Continuing Y02, if opened vs. closed state is relevant, please just pass a boolean param to a single method.
Simpler to implement, and the clumsy example impl in Javadoc would be shorter.


[JG04] Use imports, not FQNs, in Javadoc example (can omit import list from displayed example), and avoid blank lines in
sample code (whole class should be short anyway). Do not use <br> for anything.

Thanks Jesse, I will have a look at your suggestions (mainly JG02).

Also, what do you think about the API variant I mentioned in the comment from "Tue Sep 8 14:39:01 +0000 2009"? One
method to implement + one that can be overriden? I like it slightly better than the idea with boolean parameter that
will be potentially ignored... Basically, Jarda is right that the more often use case is the same annotation for opened
and closed node icon.

Prefer the boolean param. It will normally be ignored but it is easy enough to pay attention to it if you want to.

ok

Created attachment 87511 [details]
Fixed impl patch

Attaching patch fixed to reflect comments. Jesse, if you have time, please have a look at ProjectUtils.getInformation
(it is your code and I hope I didn't rape it too much...).

Also, I wrote some basic testcases to test the registered annotator works...

Y11 The method in javadoc needs to be public: Image annotateIcon(Project p, Image original, boolean openedNode) {
Y12 Need to increment spec version of the projectapi module
Y13 Missing @since in ProjectIconAnnotator
Y14 Missing change in apichanges.xml

OK, I will fix those points - reassigning to myself for implementation.

Sorry, did not get a chance to look at last patch before; hope these comments do not come too late.


[JG05] ProjectIconAnnotator Javadoc mentions "ProjectAnnotator" still.


[JG06] ProjectIconAnnotator should be in org.netbeans.spi.project, not org.netbeans.api.project.


[JG07] Weird indentation in AnnotateIconProxyProjectInformation.java. (Maybe just an artifact of JG11.)


[JG08] The code

public void addPropertyChangeListener(PropertyChangeListener listener) {
    pinfo.addPropertyChangeListener(listener);
}

is incorrect since the PropertyChangeEvent will then be fired from pinfo, not the AnnotateIconProxyProjectInformation
which ProjectUtils actually returns and thus what clients actually see. This can mess up WeakListeners and so on. You
need to make your own PropertyChangeSupport based on the AIPPI instance and refire events from the original.


[JG09] ProjectInformation.getIcon is not permitted to return null, so there should not be code handling the case that it
does (other than optionally to log diagnostic errors to console).


[JG10] Avoid using JUnit 4 idioms; NbTestCase does not support them well anyway. Also please omit empty methods to
reduce noise. (I do not recommend using the IDE's test case template as it produces a lot of junk.)


[JG11] Do not use the IDE's diff facility to generate patches. Use command-line Mercurial and be sure to specify

[diff]
git=1

in your ~/.hgrc or otherwise pass --git.


[JG12] To be more useful, ProjectIconAnnotatorTest should use the ProjectUtils to invoke the annotators, thus using
AIPPI, rather than searching for them in lookup directly. In fact the current test really doesn't test anything; so long
as the interface remained unchanged, the impl code in the main module could do anything at all and the test would still
pass.


[JG13] The change to RecentProjectsTest.java is unnecessary and irrelevant to this patch; revert it.

Created attachment 100331 [details]
Another version of proposed change.

I tried to implement all your suggestions.

JG11 would be useful for making diffs that could actually be applied and tested; the IDE's diffs cannot create the test icons. Not important now but a good idea for the future.


[JG14] Is there any potential use case for an annotator to change its badge (e.g. hide/show) dynamically? If so, ProjectIconAnnotator needs a ChangeListener event set, and AnnotateIconProxyProjectInformation needs to refire events from it (and redo annotation).

Even as it is, AIPPI should check for changes in PROP_ICON from pinfo and recalculate its icon field - the current code will have a stale cache, which will be a problem if the project dynamically changes its icon in response to various changes in metadata.


[JG15] getIcon is not allowed to be null, so your code paths dealing with it as null are wrong; delete, or change to assertions.


[JG16] "Return original if you do not intend to change the original icon (i.e. you just add a tooltip to the image)" is contradictory; adding a tooltip *is* a change (and you are returning a different object).


[JG17] A TestCase subclass's constructor should take a String name arg and pass it to the super constructor. There is no need to hardcode a test name.

Also do not use @org.junit.Test in conjunction with a JUnit 3.x test case, since the tests are identified by name convention in that case.

Also please omit setUp or tearDown methods which are empty or just call super; clutters source code.

Created attachment 100375 [details]
Jesse's suggestions implemented

OK. Check it out. Thanks

JG14 cont'd - OK; defining a single PROP_ICON which does not correspond to any actual property is a little weird though. Generally PropertyChangeListener should be used only on classes which actually look like a bean, with a meaningful list of properties with changeable values. Usually ChangeListener is used for non-bean-like classes which perform some calculation that can somehow change over time. Not a blocking issue, just a preference.

Also ProjectsRootNode ought to listen to changes. If anything, dynamic changes will be much more visible here than in ProjectInformation; PI is normally called only on occasion by e.g. the Open Project dialog, and is rarely listened to. By contrast, a PRN is displayed permanently in the Projects tab, so you would expect ongoing changes to project state to be reflected immediately here.

Another oversight related to dynamic changes: resultChanged could mean that the icon has to change, too; so updateIcon would need to be called and PROP_ICON fired. (Probably rare for this to happen, but if you are going to listen to lookup changes you might as well do it correctly.)

Created attachment 100408 [details]
Another version of proposed change.

JG14 cont'd §1 - Implemented

JG14 cont'd §2 - volunteer needed

JG14 cont'd §3 - Implemented

Applied with full listening and various other implementation and stylistic changes: core-main #8f8f422f35ce

Another fix caught by apisupport.project tests: core-main #3268f5768fac

Thank you, Jesse.

Integrated into 'main-golden', will be available in build *201006260001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/8f8f422f35ce
User: Jesse Glick <jglick@netbeans.org>
Log: Issue #171516: SPI to badge project icons.

Integrated into 'main-golden', will be available in build *201007130001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/f78bd9dd32e4
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #171516: Now the property change listeners on ProjectInformation are attached too soon, which breaks ergonomics tests. Relaxing them, just checking that some listener is added.