Created attachment 113840 [details]
Is this what you wanted me to do?

Yes, looks right.

I would not bother testing the effect of validateResource because this is already covered in other tests, and anyway you probably need to xor with AnnotationProcessorTestUtils.searchClasspathBroken to pass on both JDK 6 and 7 (see 23b4f1e0a67b).

ergonomics#feeba36e7bd9

I've faced the problem with compatibility, as described in comment 8 in bug 196452. Originally people could specify icon with fullpath without initial /, now this would mean relative icon and it would not be found. The code to deal with this is a mess. Should have been the stringFromAResource there, it would be much simpler.

(In reply to comment #3)
> Originally people could specify icon with fullpath without initial /

Right, as specified.

> now this would mean relative icon

Because you introduced a call to LayerBuilder.absolutizeResource - but why? Existing registrations were already using absolute icon paths, and the originally attached patch would continue to handle these. Now you have committed something different, which is actually an undocumented API change out of scope for this issue, and which will produce incorrect results in (admittedly unusual) cases when using JDK 6.

...and is inconsistent with every other processor I know about which manages an attribute named "iconBase". If we want to permit relative paths for icons, it should be done across the board, in a separate issue under API review, and the relative syntax would need to be explicit (e.g. using "./" or "../" as a prefix).

Reopening so this does not get lost; need to just revert to original patch, at least for the validateResource section.

> originally attached patch would continue to handle these

No, it would not. It would fail as the originally attached tests demonstrate. Every absolute path would have to be prefixed with /

> need to just revert to original patch

I see no problem in supporting relative paths. Originally we handled just absolute, now we can deal also with relative. Or is the problem that the icon search works only on JDK7? I such case I can support relative paths only on JDK7 (and on JDK6 use only absolute ones).

If this is an compatible API change, then I want to do it. Support for relative icons is more flexible and natural.

Btw. the build failed as

compile:
    [mkdir] Created dir: /space/workspace/ergonomics/localhistory/build/classes
 [nb-javac] Compiling 30 source files to /space/workspace/ergonomics/localhistory/build/classes
   [repeat] 
   [repeat] 
   [repeat] An annotation processor threw an uncaught exception.
   [repeat] Consult the following stack trace for details.
   [repeat] java.lang.IllegalArgumentException: directories not supported
   [repeat] 	at com.sun.tools.javac.util.DefaultFileManager$RegularFileObject.<init>(DefaultFileManager.java:1271)
   [repeat] 	at com.sun.tools.javac.util.DefaultFileManager$RegularFileObject.<init>(DefaultFileManager.java:1266)
   [repeat] 	at com.sun.tools.javac.util.DefaultFileManager.getFileForOutput(DefaultFileManager.java:1085)
   [repeat] 	at com.sun.tools.javac.util.DefaultFileManager.getFileForOutput(DefaultFileManager.java:1054)
   [repeat] 	at com.sun.tools.javac.processing.JavacFiler.getResource(JavacFiler.java:434)
   [repeat] 	at org.openide.filesystems.annotations.LayerBuilder.validateResource(LayerBuilder.java:343)
   [repeat] 	at org.netbeans.core.multiview.MultiViewProcessor.handleProcess(MultiViewProcessor.java:115)
   [repeat] 	at org.openide.filesystems.annotations.LayerGeneratingProcessor.process(LayerGeneratingProcessor.java:124)
   [repeat] 	at com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:625)
   [repeat] 	at com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:554)
   [repeat] 	at com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:699)
   [repeat] 	at com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:981)
   [repeat] 	at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:727)
   [repeat] 	at com.sun.tools.javac.main.Main.compile(Main.java:353)
   [repeat] 	at com.sun.tools.javac.main.Main.compile(Main.java:279)
   [repeat] 	at com.sun.tools.javac.main.Main.compile(Main.java:270)

(In reply to comment #7)
> It would fail as the originally attached tests demonstrate.
> Every absolute path would have to be prefixed with /

Can you elaborate please? validateResource expects a path without initial slash, so this statement makes no sense to me.

> Or is the problem that the icon search works only on JDK7?

Yes, at least when searchClasspath=true, as it is here (and probably needs to be).

Another issue (even on JDK 7) is that if "p1/r.png" and "p2/p1/r.png" are both valid icons, iconBase="p1/r.png" would be ambiguous when used from a class in the p2 package if either absolute or relative paths were accepted.

> I such case I can support relative paths only on
> JDK7 (and on JDK6 use only absolute ones).

-1 on this; a given source base (with -source 6) should not be compilable using JDK 7 but not compilable using JDK 6! This would cause havoc for people running the IDE on JDK 7 but setting nbjdk.home to JDK 6 (as I do), since the CI build would break.

The only safe solution (for an attribute which has historically been treated as an absolute path with no leading slash) is to require relative paths to be syntactically differentiated from absolute paths. My preference is for "./" or "../" as prefixes to force interpretation as relative - it is unambiguous (no legitimate absolute path could use those prefixes), the intention is immediately clear to a reader, and the syntax is compatible with that used for attributes processed via absolutizeResource. This would be a new static method in LayerBuilder, e.g.:

public static String fixRelativeResource(Element originatingElement, String resource) throws LayerGenerationException {
    if (resource.startsWith("./") || resource.startsWith("../") {
        return absolutizeResource(originatingElement, resource);
    } else {
        return resource;
    }
}

> If this is an compatible API change, then I want to do it.

Only if supported for all annotations taking an "iconBase" attribute, not just @MultiViewElement.Registration. Doing it for just one would be too confusing.


(In reply to comment #8)
> java.lang.IllegalArgumentException: directories not supported
>     at com.sun.tools.javac.util.DefaultFileManager$RegularFileObject.<init>(DefaultFileManager.java:1271)

LocalHistoryTopComponent uses

  iconBase="", // no icon

which is not permitted by the API as far as I can see, and I am not sure what it is supposed to mean. (Should the window really have no icon when this tab is selected?) Probably validateResource should

if (resource.isEmpty()) throw new LayerGenerationException(...);

and if there is an intended use case for omitting an icon in @MVE.R, it should be documented and handled somehow in its processor.

(In reply to comment #9)
> (In reply to comment #7)
> > It would fail as the originally attached tests demonstrate.
> > Every absolute path would have to be prefixed with /
> LocalHistoryTopComponent uses
> 
>   iconBase="", // no icon
> 
> which is not permitted by the API as far as I can see, 
> and I am not sure what
> it is supposed to mean. (Should the window really have no icon when this tab is
> selected?) 
according to jrojcek the history tab shouldn't have it's own icon when selected but the "source" tabs icon should be used instead. I believe we discussed it some time ago with jarda and the result was that its ok to use "" at that place. Another thing is that issues with the missing icon in the history tab had to be solved as in #204072.

(In reply to comment #10)
> the result was that its ok to use "" at that place

In that case:

- Javadoc must specify that iconBase="" is legal and what it means (or this could be a default value for the attribute)
- processor must handle .iconBase().isEmpty(), perhaps by not generating any layer attribute for it
- runtime must handle empty or null icon attribute (according to what the processor generates)

I've dropped all attemps for relativeness (reported bug 206129 instead), allowed iconBase to be "" and make the attribute optional:
http://hg.netbeans.org/ergonomics/rev/95110cbf04cc

I had to integrate it to fix the ergonomics build. I hope the change reflects results of the review.

95110cbf04cc seems safe. Thanks for opening a separate issue for the relative paths - this should be more comfortable to track and review.

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/feeba36e7bd9
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #204174: validate the iconBase to point to real icon