Community
Participate
Working Groups
Created attachment 245553 [details] Adding external annotations to JDK jars Bug 331651 describes the general requirement for having external annotations so that nullity and other annotations for .jar files can be specified "outside" source code. Bug 440477 describes the compiler centered code needed; this bug aims to describe and track the IDE user interface changes that are needed to implement this. External annotations are directly associated with any library .jar that contributes to the classpath. These can be: 1. jars in the "project" classpaths of projects. 2. jars provided by any JDK. External annotations will be presented as a single file (...) in a to-be-determined format (bug 440474). Each jar can have 0..1 external annotation files associated with it. The external annotation file for that jar only takes effect for the classes found (and used) from that jar. The UI for external annotation files closely mirrors the UI made for connecting Javadoc or sopurce attachments to jars on the classpath: a single "external annotations" file, in the Eclipse-defined format, can be connected to each jar on the classpath. The locations in the UI where this is exposed are: 1. Window -> Preferences -> Java -> Installed JRE's. On a JRE's edit page we see the jar files, and each of them can be associated with a "Source Attachment" and a "Javadoc Location". There will be added an "External Annotations" entry per jar, with the selected file. 2. Project Properties -> Java Build Path -> Libraries. This shows again a list of jars for the project, with Source Attachment, Javadoc Location and some other options. Here too we'll add an "External Annotations" node. For both screens adding an external annotations file works the same: 1. Selecting the "External Annotations" node in the jar list enables the edit/remove buttons. 2. Clicking edit shows the same UI as the current edit button for "Source Attachment". It allows selection of the file either from workspace or from "external location". The selection made will be saved in either .classpath (for a project) or in the JDK's properties stored in preferences. I would like to contribute the UI implementation of this functionality, in cooperation with Stephan Hermann. An initial implementation can be seen on Github: https://github.com/fjalvingh/eclipse.jdt.core/tree/externals (use the externals branch for this one) https://github.com/fjalvingh/eclipse.jdt.ui https://github.com/fjalvingh/eclipse.jdt.debug If accepted, I will rework this implementation and synchronize with Stephan's work. See the attached screenshots for the proposed UI (as implemented by the Github code). Once this basic implementation is done extensions can be defined that allow for: - allowing conversion of "foreign" annotation formats to Eclipse's defined one - allowing automatic downloading of annotations made for well-known libraries.
Created attachment 245554 [details] Adding external annotations to JDK jars
Created attachment 245555 [details] Open location dialog for external annotations file.
I support this request and will indeed cooperate with Frits where needed. A few comments: (In reply to Frits Jalvingh from comment #0) > External annotations will be presented as a single file (...) in a > to-be-determined format (bug 440474). While interactively adding more annotations I believe an "exploded" layout will be more convenient: one text file per class in a directory structure matching the packages. That's also the layout which the current strawman implementation in JDT/Core recognizes. Folks that only consume - not modify - external annotations will certainly find a single-file approach easier. For this I see two options: merge multiple classes into a single text file, or: zip a directory structure with multiple files into a single archive. I think the decision between these two options should be based primarily on performance measurements, see bug 440629. For the UI this simply means: I think the attachment should accept a directory or a file (of to be determined format and extension). > Each jar can have 0..1 external > annotation files associated with it. The external annotation file for that > jar only takes effect for the classes found (and used) from that jar. I'm not sure if this is meant to relate to the "exploded" layout mentioned above. Other than that I see no need to provide multiple sets of external annotations for one library. If using annotations from different sources it should be (made) easy to merge them before consumption. Am I missing anything? One question not mentioned in comment 0: I think it would be good if the operation that creates this attachment also validates that the given file or directory actually contains legal external annotations. Bonus validation: do the external annotations match the classes in the library? (a) Do the annotated classes exist and (b) do the mentioned method signatures match? JDT/Core can provide basic functionality for this validation. Are similar validations implemented for javadoc or source attachments?
> While interactively adding more annotations I believe an "exploded" layout > will be more convenient: one text file per class in a directory structure > matching the packages.... The JDK's "rt.jar" contains 19036 .class files in my 1.7 jdk, so this would create 19000+ small files in a directory structure... I think that is a /lot/ of stuff just for this, and performance especially on Windows would definitely suffer greatly: Windows opens small files at least 20 to 40 times(!) slower than Linux... I think we should require zipped files; with a bit more work we can implement UI assisted manipulation of annotations by either lazily re-zipping the file on changes, by keeping a delta file associated with the zip, or by making use of the fact that a zip file can actually very easily be edited because it is indexed. Be that as it may; my implementation now indeed accepts both file and directory as a valid annotation source. > I'm not sure if this is meant to relate to the "exploded" layout mentioned above. I meant two things: * a single jar can have only ONE extra annotations source, and it's optional. This means that if users for instance use multiple tools to scan for nullability that the results of these tools must be merged outside the scope of Eclipse. * The annotations source for a jar is /only/ ever used for a class that really comes from that jar. So if the classpath contains two jars X and Y containing A.class, with Y.jar having an external annotations source then /only/ if the A.class is loaded from Y.jar will that source be used for A.class. > I think it would be good if the operation that creates this attachment also validates...... I can do that, of course, but depending on the "depth" of the check this might be quite slow. A simple check could see if at least the constituent files are parseable, and if the classes mapped at least can be found in the .jar. I think mapping methods too is not that interesting because the only thing we might find out is that a method is present in the external annotations while it is not there in the jar.. That seems like a lot of work for little gain. Perhaps a better check would be to specify that the format contains a hash signature of the .jar it was made against; this would at least ensure that jar and annotations really belong together? I would propose to do that check, but to leave it at the parsing-and-class-mapping stage. > Are similar validations implemented for javadoc or source attachments? I don't think so; I studied those implementations (and actually stole from them mostly) and except specifying a fixed file extension I saw no check at all.
Regarding individual files vs. zip vs. one big text file for an entire library (proposed by Frits via email): IMHO we should definitely support on-demand loading, which in the compiler typically happens for class. I don't see how this fits to the idea of having one monster text file, which doesn't lend itself to indexing, does it? I _could_ see a compromise: one file per package, but that does definitely look like - a compromise :) I think *conceptually* the 1-file-1-class approach is cleanest. For reasons of packaging / distribution and optimized file access on windows a zip file is a good technical wrapper. Normally, you wouldn't store a zip in VCS, use the plain individual files for that and only use zip for distribution, right? This situation is only skewed by the desire to combine editability with optimized packaging, right? I.e.: a project consuming an eea-zip (perhaps produced by s.o. else) may still want to *add* more annotations. Two quick ideas on this: 1. Updated zip-file needs to be merged back into the source VCS: Unpack any modified entries and merge them into VCS, then recreate the zip Doable but sounds a bit awkward. 2. S.t. like "Melting Ice" technology (as used by Eiffel some 25 years ago :) Combine the frozen (zipped) parts with melted overlays of recently modified elements. I.e. zip is read only, editable text files can be overlayed. At intervals you will want to re-freeze the melted parts, i.e., re-create the zip (which may require submitting your melted parts to the maintainer of the zip). To illustrate the melting process: as soon as you add one annotation to a class (e.g., via quick assist), the corresponding external annotations found in the zip are exploded and stored in a plain file. This would ensure that for each class all information is either found in a text file or in the zip, merging both sources never has to look *into* a class. I kind of like (2), but that would require the option to make two attachments: zip + directory. Would that be two classpath attributes? By comparison, a 9MB text file (as would represent the entire JRE) sounds broken to me - simply do the scrollbar test: if the scrollbar approaches a scale where a single pixel move jumps more than one screen, the scrollbar is fubar - clear indication that the file is too big for editing, IMHO. Another danger: if you have (many) merge conflicts, a file of that size runs danger to never resynchronize after the conflict location, i.e., the entire tail after a bad conflict will appear as one big conflict. Individual files ensure that no merge conflict will ever spill over from class to another.
Frits, do you want to update your contribution to my changes from bug 440477 comment 9? You'll find the changes as commit d251082014c79ac58d46043261a61c9e07ce547e in my feature branch. From a quick glance I guess that CPListElement.create(Object, IClasspathEntry, boolean, IJavaProject) could be your friend here.
(In reply to Stephan Herrmann from comment #6) > Frits, do you want to update your contribution to my changes from bug 440477 > comment 9? I will try to doso this weekend.
I hope I did what was needed; the code in eclipse.jdt.core now uses IClasspathAttribute, and the UI code "translates" that to it's own concept of a classpath. Since manipulating a IClasspathAttribute[] was being done more than once, and since I could not find any reasonable api for it I added some stuff to JavaCore; it already contains newClasspathAttribute so I added findClasspathAttribute and setClasspathAttribute. I can easily remove that/move it somewhere else once we are sure we are getting more stable. I noticed something else though, a bit late perhaps. I have concentrated on the UI parts that let you add the annotations to the JDK, as you might have seen from the screenshots. Meaning I currently add those annotations via: Preferences > Java > Installed JRE's; edit JRE; select jar etc. This means that the attibutes here are part of the JRE's "classpath", not a project's classpath. This is probably the reason why configuring them does not cause them to be used by your code, sadly enough ;-) I will investigate that a bit more but it'll take time because this classpath code moves quite a lot of stuff in many places. I noticed in your test code that apparently you expect the attribute on each project's JDK container? Does that mean that you think it should be configured like that per project? I have not yet created the UI for the Java build path, and I might wait a bit doing that until there is some kind of OK. But this would probably mean that both container elements and library elements need to be able to accept external annotations. Problem I see with this is that it is (1) lots of work to add the same file to all projects and (2) error prone.. Anyway, the code is at github .org/fjalvingh again; to prevent mistakes I removed all other branches ;) On the file issue: > I _could_ see a compromise: one file per package, but that does definitely > look like - a compromise :) Compromise is what distinguishes an engineer from a scientist ;-) One file per package amounts to 1217 files in our example dataset; the biggest ones are 30..90K but these are few; most are around 1.5K or smaller. This might be at least workable for Windows. It also depends on how reading is done; it might be wise to cache the read result for whenever another class from the same package is needed. I am quite loath to add 20.000 files to my repo. But 1.2K might be reasonable. We can go all ways with this depending on how expensive parse is: if parse+cache is fast it might not matter if we load a bigger file if we do it once? For me it seems crucial that the external annotations can be version controlled well. My preferred way of working with any IDE is to just get everything needed from the VCS and to depend as little as possible on the workstation's configuration and environment. This makes it /easy/ to work with multiple people on the same project with the same configuration and settings. For these externals that is important because if they are outside version control then team members with outdated/missing annotations can easily commit code that does not compile at others, and it makes configuring workstations that much harder because everything must be installed "just so". This is also why I hate library variables: they point to the wrong location most of the time. Any binary format like zip causes problems with version control. Even though the whole set of annotations I gave you compresses to only about 400K of binary this means that every 50 character change will be a new 400K binary. What's worse is that since these are zipped already they do not compress at all, so while changing the 9MB file 10 times in Git will also create 10 copies of it - since they are mostly the same the result after compression is that very little extra space is needed. For our .zip however every change is another 400K gain in repository size. For me this completely rules out a zipfile as format, especially because we now also want an UI to easily modify this file, and we have for now no tool to generate it. Having a tool might mean we only changed that file very little. But having no tool but an UI means we'll change it a lot for it to become useful. I thought of your option 2, "baseline plus delta", too. I'm not a big fan because it requires all kinds of extra infrastructure to merge the changes into the zip every now and then. I think I'd rather have the separate files then... And we would still have the problem of how to store the delta. (btw: we would not need two attributes imo for that; just one that contains the "base name", you get two files by adding two different extensions - safer?). Ah well, let's think a bit more, and see how expensive parsing a full file is anyway.
(In reply to Frits Jalvingh from comment #8) > I hope I did what was needed; the code in eclipse.jdt.core now uses > IClasspathAttribute, and the UI code "translates" that to it's own concept > of a classpath. Since manipulating a IClasspathAttribute[] was being done > more than once, and since I could not find any reasonable api for it I added > some stuff to JavaCore; it already contains newClasspathAttribute so I added > findClasspathAttribute and setClasspathAttribute. I can easily remove > that/move it somewhere else once we are sure we are getting more stable. > > I noticed something else though, a bit late perhaps. I have concentrated on > the UI parts that let you add the annotations to the JDK, as you might have > seen from the screenshots. Meaning I currently add those annotations via: > > Preferences > Java > Installed JRE's; edit JRE; select jar etc. > > This means that the attibutes here are part of the JRE's "classpath", not a > project's classpath. This is probably the reason why configuring them does > not cause them to be used by your code, sadly enough ;-) I will investigate > that a bit more but it'll take time because this classpath code moves quite > a lot of stuff in many places. > > I noticed in your test code that apparently you expect the attribute on each > project's JDK container? Does that mean that you think it should be > configured like that per project? I have not yet created the UI for the Java > build path, and I might wait a bit doing that until there is some kind of > OK. But this would probably mean that both container elements and library > elements need to be able to accept external annotations. Problem I see with > this is that it is (1) lots of work to add the same file to all projects and > (2) error prone.. I hope we can get the effective settings as the sum of paths configured per JRE and paths configured in the projects .classpath. The merging would happen in ClasspathEntry.combineWith(ClasspathEntry) as invoked from JavaProject.resolveClasspath(). Question remains: how does the path configured with the container (org.eclipse.jdt.launching.JRE_CONTAINER) reach the point of resolving? From playing with your code in the debugger it seems like the following might be a (the?) missing link: org.eclipse.jdt.internal.launching.JREContainer.buildClasspathAttributes(IVMInstall, LibraryLocation, boolean). I tried to validate my theory by looking at the resulting configuration in the project's build path, but then I observed that org.eclipse.jdt.internal.launching.JREContainerInitializer.getAttributeStatus(IPath, IJavaProject, String) answered NOT_SUPPORTED for "annotationpath". HTH Goal should of course be: for JRE it is enough to configure external annotations per workspace. For other libraries we want to configure them per the project's classpath (since there's no per-workspace configuration for them). LMK if the logic in ClasspathEntry.combineWith() needs more sophistication (like: what happens if workspace and project have different settings for the same extraAttribute?).
Small observation: In the "External Annotation Attachment Configuration" it is possible to enter a "Path" under "External Location", while in the radio "Workspace location" is still selected. The result of such editing is: nothing! :) In comparable dialogs (e.g., Source attachment), detail enablement follows the parent selection.
(In reply to Stephan Herrmann from comment #10) > Small observation: In the "External Annotation Attachment Configuration" it > is possible to enter a "Path" under "External Location", while in the radio > "Workspace location" is still selected. The result of such editing is: > nothing! :) Thanks; it is fixed ;-)
> Question remains: how does the path configured with the container > (org.eclipse.jdt.launching.JRE_CONTAINER) reach the point of resolving? Well, I walked through the code and fixed what seemed missing. I now definitely see the annotion walker being used and configured on resources from the jdk. But I see no extra errors, even if I do: HashMap<String, String> hm = new HashMap<>(); hm.putAll(null); I see no error. I did some debugging and it appears that all annotations die a soft death inside TypeSystem (through LookupEnvironment.createAnnotatedType -> typeSyste,getAnnotatedType) which is a no-op... Which seems to be a consequence of LookupEnvironment's constructor: this.typeSystem = this.globalOptions.sourceLevel >= ClassFileConstants.JDK1_8 && this.globalOptions.storeAnnotations ? new AnnotatableTypeSystem(this) : new TypeSystem(this) So for now support is only there for Java 8... Which makes it hard to test because I still cannot use 8 annotations due to Juno having those problems with'm, and Mars hanging all the time ;-) I will try to create a small test thing on 8 to verify, but at least the annotation walkers are called- which supposedly is something.
(In reply to Frits Jalvingh from comment #12) > > Question remains: how does the path configured with the container > > (org.eclipse.jdt.launching.JRE_CONTAINER) reach the point of resolving? > Well, I walked through the code and fixed what seemed missing. I now > definitely see the annotion walker being used and configured on resources > from the jdk. nice! > I did some debugging and it appears that all annotations die > a soft death inside TypeSystem (through > LookupEnvironment.createAnnotatedType -> typeSyste,getAnnotatedType) which > is a no-op... You're right, the approach using a ITypeAnnotationWalker still needs some retrofitting for declaration annotations. This is not TypeSystem's fault, but due to the fact that this approach creates *type annotations*, which for 1.7 need to be "switched" to declaration annotations. This needs to be done in coordination between - the walker (which should sanity-check that declaration annotations are not found in TYPE_USE locations) and - BinaryTypeBinding & LookupEnvironment, which create annotated types for 1.8, and instead need to create annotated parameters / methods / fields for declaration annotations. I was intending to first "finish" the 1.8 implementation (the harder part) and then do the necessary adjustments for 1.7-. > [...] Mars hanging all the time ;-) Well, Mars should stop that behavior before too long ... so continued testing is always highly welcome ...
nitpick regarding (from latest in github/fjalvingh): //--- URL indexLocation = lib.getIndexLocation(); if(indexLocation != null) { IClasspathAttribute indexCPLocation = JavaCore.newClasspathAttribute(IClasspathAttribute.INDEX_LOCATION_ATTRIBUTE_NAME, indexLocation.toExternalForm()); classpathAttributes.add(indexCPLocation); } IPath annotationsPath = lib.getExternalAnnotationsPath(); if (null != annotationsPath && !annotationsPath.isEmpty()) { IClasspathAttribute xAnnLocation = JavaCore.newClasspathAttribute(IClasspathAttribute.EXTERNAL_ANNOTATION_PATH, annotationsPath.toPortableString()); classpathAttributes.add(xAnnLocation); } //--- how come some locations are "null-or-valid" and annotationPath can also be empty? Shouldn't we use the same strategy (null-or-valid) in all these cases?
(In reply to Frits Jalvingh from comment #8) To answer your thoughts on packaging (to zip or not to zip), I'd like to step back and draft the typical usage scenarios we envision, and see to what degree these scenarios can be served by existing strategies vs. what's new for external annotations and needs a new strategy: == Lazy Consumers == Ideally, for every library a project is using, somewhere on the net (a central repo?) a corresponding package of external annotations can be found. We envision that these annotations will reach a level of completeness such that client projects don't typically need to edit/amend them. In this scenario, external annotations are just another kind of project dependency, and hopefully existing mechanisms for handling dependencies can be leveraged (which is kind of hard to predict as we don't want to restrict this approach to, say, maven, or osgi/p2 ...). In this scenario no-one wants to store the external annotations in their own VCS, they shall be consumed from standard channels. == Busy Maintainers == Each package of external annotations must be managed by a maintainer, for whom a convenient source format / layout is key. S/he will use the build technology of her/his choice to create distributable packages from those sources. This scenario is exactly the same as for managing regular source trees. == Very Early Adopters == Before a huge repo of external annotations exists, projects may choose to not wait for s.o. else to create annotations for a required library, but do this as part of the project's work. To reuse existing strategies, such a project could simply create a sub-project where they play the role of an annotation maintainer: maintain textual annotations in VCS and publish the result to their main project using an established build technology and packaging format. == Early Adopters == A project might find a (possibly big, but) incomplete existing package of external annotations. This package could be the result of an automated tool (which probably doesn't find all relevant annotations), or s.t. distributed by s.o. else. This, I believe, is the only genuinely new scenario. For this scenario I see the following ingredients: - consume existing annotations - amend existing annotations with your own - re-synchronize the new annotations with the (3rd party?) package I would describe this scenario as a mixture of the above scenarii. I estimate that typically own annotations will be a very small fraction compared to the existing annotations. What's different from just playing several roles: we'd probably want the own annotations "closer" to the main project. E.g.: we may want a quick fix to edit a file "right here" not in another project. And before it has been sync'ed with upstream, we may want to store the new annotations in the current project's VCS. Before continuing a discussion about solutions, do we agree about these scenarios?
x-ref to: "State of the Union": bug 331651 comment 56 and: https://wiki.eclipse.org/JDT_Core/Null_Analysis/External_Annotations
I'm preparing to release the JDT/Core part into master before next week. As a next step I've filed bug 459831 against JDT/Launching. @Frits, please upload to that bug the changes you have in the o.e.j.launching plugin. Please produce a patch without pure whitespace changes. Subsequent polish I'm happy to discuss in the bug after the patch is available.
Here's a bug: ExternalAnnotationsAttachmentBlock can be created with a null fEntry. In that case chooseExtJarFile() and chooseExtFolder() will throw NPE. Perhaps org.eclipse.core.runtime.Path.EMPTY will be your friend?
The overall effort includes changes in jdt.core, jdt.launching, jdt.debug.ui and jdt.ui. Dependencies are as follows: Required changes in jdt.core have already been released to master (via bug 440477). Two additional proposed new API in JavaCore can be avoided, see bug 459831 comment 4 item (1.c). Changes in jdt.launching and jdt.debug.ui are mutually dependent due to a new interface method (interface marked @noimplement, @noextend, exception documented for one class from jdt.debug.ui, see bug 459831 comment 4 item (0)). Changes in jdt.ui do *not* depend on changes in jdt.launching. Changes in jdt.debug.ui, however, depend on one new method in jdt.ui: BuildPathDialogAccess.configureExternalAnnotationsAttachment() I will now start the detailed review of changes for jdt.ui. Should this review appear to block bug 459831 we could easily start by releasing the new method in jdt.ui as an empty stub.
Review comments based on latest from github (Feb 15): (1) Handling of variable entries is unclear: (1.a) NewWizardMessages has unused entries _varlabel, _varlabel_button, error_varnotexists and more. (1.b) AnnotationsAttachmentBlock_filename_description describes a connection, which doesn't seem to be implemented, nor would it be useful, I think. (1.c) CPListElement has a FIXME in a CPE_VARIABLE block, looks stale to me. (1.d) Commented method isVariableEntry in ExternalAnnotationsAttachmentBlock. -> JDT/Core supports variable-based paths, and I believe it should be possible to define the external annotation path relative to a variable regardless of the kind of the classpath entry. This may require changes in the dialog, not only in label texts. Feel free to ask if the general concept of classpath variables is unclear. (2) ExternalAnnotationsAttachmentBlock.canBrowseFileName() { // FIXME remove What's this about? (3) Warnings in ExternalAnnotationsAttachmentBlock (unused private method, local variable, unused field, javadoc inconsistency at ctor) - some may be resolved by implementing variable-based paths. (4) ExternalAnnotationsAttachmentBlock.updateFileNameStatus copy-pasted code comment needs adjustment (is it valid, indeed?). (5) ExternalAnnotationsAttachmentDialog contains a stale @since javadoc tag. Please remove. Only public (non-internal) API needs @since tags. (6) We need an icon. It's unfortunate, that javadoc already uses the natural choice, the '@' icon. Interestingly, there is a tiny difference between annotation_obj.gif and jdoc_tag_obj.gif, but I'm afraid this doesn't really help us here. Perhaps just a different color helps us here? Or maybe instead of file+'@' we should combine jar+'@'? (7) BuildPathDialogAccess.configureExternalAnnotationsAttachment: @return javadoc needs adjustment, we are not returning a classpath entry. For remaining polish you already know the trick: - avoid whitespace-only changes - update copyright to 2015 - add a contribution entry into header of existing files - for new files *you* are actually the copyright holder (or your company) Please attach as a patch or push as a single commit to gerrit (as you prefer). And: please don't feel discourage by the above list. I highly appreciate the efforts invested. Other follow-up issues have been filed as separate bugs: bug 461301 (property page) and bug 461300 (documentation). @Frits, please let me know to what degree you can / want to participate also in those.
(8) I'm confused about how the following combine: (a) define annotationpath for a JRE via preferences > installed JREs (b) modify annotationpath for a JRE via build path > libraries It seems that by (b) I am *deleting* all entries from (a). This is not an issue for regular libraries. Just for a JRE modification via the build path seems broken. There's a slight chance that this got broken during updating of my local patch. So once you have a stable state, sharing a patch here would help synchronize.
Created attachment 251562 [details] Proposal for the "External Attachments" icon Me doing graphical work is feared throughout my company; let's scare some more people. This is what I could do with the "external annotations" icon so far; tips are welcome 8-/ I however have no idea how to add this binary file as part of the patch diff; I will attach the binary as an attachment to this bug?
(In reply to Frits Jalvingh from comment #22) > Created attachment 251562 [details] > Proposal for the "External Attachments" icon > > Me doing graphical work is feared throughout my company; let's scare some > more people. This is what I could do with the "external annotations" icon so > far; tips are welcome 8-/ My first thought: the icon looks very "male" ;P The icon can be clearly distinguished from any existing Eclipse icons, good. Have you considered the @+jar idea mentioned above? Technically, we would want to avoid some of those white background pixels around the edge (problem only shows on dark background ...). All these are not blockers, IMHO. We can use this icon as a placeholder for today and see if during the next days others want to chime in. > I however have no idea how to add this binary file as part of the patch > diff; I will attach the binary as an attachment to this bug? yes, separate attachment is fine (and tell us the path where it should be stored).
Ok, let's answer a lot of the comments in one set to prevent spamming everyone even more 8-/ Comment# 21: (In reply to Stephan Herrmann from comment #21) > (8) I'm confused about how the following combine: > (a) define annotationpath for a JRE via preferences > installed JREs > (b) modify annotationpath for a JRE via build path > libraries > It seems that by (b) I am *deleting* all entries from (a). Yes, this is the same behavior as for the other entries there. Try deleting for instance the source attachment - it also percolates to the JDK's settings. Comment# 23: > My first thought: the icon looks very "male" ;P Well, yes, now you mention it 8-/ It's not my need to compensate for anything but I tried to capture the idea of "external" with that arrow outside. I know those white pixels are bad, but I really do not know how to handle those tools to create something reasonable here especially with .gif which has nothing reasonable for transparency. Actually anything .gif like will only show "reasonably" on both white and black if the icon itself is filling a lot of area. I will apply what passes for my graphical skills to your proposal of jar + @ as soon as at least the code works ;) BTW: I also needed to fix the JDT.Debug contribution to use that icon. The bug reported in bug 459831 where clearing all fields in the annotations dialog did not clear the annotations is fixed. Review comments from comment# 20: > (1.a) NewWizardMessages has unused entries _varlabel, _varlabel_button, > error_varnotexists and more. All checked and obsoletes removed. > (1.b) AnnotationsAttachmentBlock_filename_description.... Removed. > (1.c) CPListElement has a FIXME in a CPE_VARIABLE block, looks stale to me. Yes, it was a remnant of the earlier solution refused by Marcus. Removed. > (1.d) Commented method isVariableEntry in ExternalAnnotationsAttachmentBlock. Removed > -> JDT/Core supports variable-based paths, and I believe it should be > possible to define the external annotation path relative to a variable > regardless of the kind of the classpath entry. Will do as another addition to this change, so that at least core functionality is present and working. > (2) ExternalAnnotationsAttachmentBlock.canBrowseFileName() { // FIXME remove > What's this about? Hehehe, it is from the variable support I removed. I will keep it for now as your earlier requirement probably re-requires it ;-) > (3) Warnings in ExternalAnnotationsAttachmentBlock (unused private method, > local variable, unused field, javadoc inconsistency at ctor) - some may be > resolved by implementing variable-based paths. Yes, I removed some of the unused code for now but will refit as soon as I understand what's needed. > (4) ExternalAnnotationsAttachmentBlock.updateFileNameStatus copy-pasted code > comment needs adjustment (is it valid, indeed?). Done. > (5) ExternalAnnotationsAttachmentDialog contains a stale @since javadoc tag. > Please remove. Only public (non-internal) API needs @since tags. Done. > (6) We need an icon. See discussion above, but another icon is there. > (7) BuildPathDialogAccess.configureExternalAnnotationsAttachment: @return > javadoc needs adjustment, we are not returning a classpath entry. All of that javadoc fixed. > For remaining polish you already know the trick: Should all be done. > And: please don't feel discourage by the above list. I highly appreciate the > efforts invested. No problem ;-) I can take it as well as I hand it out myself ;-)
Created attachment 251563 [details] Contribution for adding external annotations to build path and VM path
(In reply to Frits Jalvingh from comment #25) > Created attachment 251563 [details] > Contribution for adding external annotations to build path and VM path Great, I'll double check that all is well. Does the other patch (JDT/Debug) need an update, too, or are all changes contained here? I assume you are still preparing two amendments: icon & stating compliance with the CoO.
Created attachment 251564 [details] Proposal: less "male" icon combining jar + @
(In reply to Frits Jalvingh from comment #27) > Created attachment 251564 [details] > Proposal: less "male" icon combining jar + @ Can you please attach just the icon, not as part of a screenshot? TIA
Created attachment 251566 [details] Icon resource to be copied Copy into: org.eclipse.jdt.ui/icons/full/obj16/external_annotation_location_attrib.gif
I have authored 100% of the contribution. I have the necessary rights to submit this contribution, including any necessary permissions from my employer. I am providing this contribution under the license(s) associated with the Eclipse Foundation project I am contributing to. I understand and agree that Eclipse projects and my contributions are public, and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with the license(s) involved.
(In reply to Frits Jalvingh from comment #24) > Ok, let's answer a lot of the comments in one set to prevent spamming > everyone even more 8-/ > > Comment# 21: > (In reply to Stephan Herrmann from comment #21) > > (8) I'm confused about how the following combine: > > (a) define annotationpath for a JRE via preferences > installed JREs > > (b) modify annotationpath for a JRE via build path > libraries > > It seems that by (b) I am *deleting* all entries from (a). > Yes, this is the same behavior as for the other entries there. Try deleting > for instance the source attachment - it also percolates to the JDK's > settings "percolates to the JDK's settings" is not what I'm seeing. (1) Use the JRE preferences to assign for all jars of the JRE the annotation path /Prj1/annots1 (2) Use a project's build path to change annotation path for resource.jar to /Prj1/annots2 Result: none of the JRE jars has an annotation path any longer. Can you reproduce this?
New Gerrit change created: https://git.eclipse.org/r/43887
> Can you reproduce this? I can. I'm trying to hunt it down. So far it looks like IClasspathAttribute is not generic at all, so code is explicitly allowing/handling specific instances only like the javadoc parts, sigh.
Final polish to be done before committing (I can do these if necessary): - update copyright year in all changed text files - remove unused //$NON-NLS-1$ in JavaPluginImages - fix two more @since tags: Constant in ISharedImages needs @since 3.11 (not 3.8) New method in BuildPassDialogAccess needs @since 3.11 (missing) - remove stale @param tag on ExternalAnnotationsAttachmentBlock(..) - warnings re unused variables/method in ExternalAnnotationsAttachmentBlock: if these are going to remain unused for M6 please remove for now. (JDT wants to remain warning-free :) ) - in the javadoc for configureExternalAnnotationsAttachment() "possibly modified" doesn't sound right, considering that Path instances are immutable. (If you're wondering about the @since tags: we use "API-tools" to validate the workspace against a defined "baseline", LMK if you're interested in how this works).
(In reply to Frits Jalvingh from bug 459831 comment #36) > Sorry; the bug was in the UI code, so this change indeed only fixes the > icon. The updated UI code fixes the bug, it was in > ExternalAttributeConfiguration line 89 (old) there. So let's discuss this here ... I still see the buggy behavior: ExternalAnnotationsAttachmentBlock.getAnnotationsPath() refuses to return an empty path but returns null instead. Inside VMLibraryBlock.edit(IStructuredSelection, int) you only invoke fLibraryContentProvider.setAnnotationsPath for nonnull returns Hence, the null return (after emptying the input field) has no effect, right?
(In reply to Stephan Herrmann from comment #34) > Final polish to be done before committing (I can do these if necessary): > - update copyright year in all changed text files Hope I caught all 8-/ > - remove unused //$NON-NLS-1$ in JavaPluginImages Fixed > - fix two more @since tags: > Constant in ISharedImages needs @since 3.11 (not 3.8) > New method in BuildPassDialogAccess needs @since 3.11 (missing) > - remove stale @param tag on ExternalAnnotationsAttachmentBlock(..) Done > - warnings re unused variables/method in ExternalAnnotationsAttachmentBlock: > if these are going to remain unused for M6 please remove for now. > (JDT wants to remain warning-free :) ) Done > - in the javadoc for configureExternalAnnotationsAttachment() > "possibly modified" doesn't sound right, considering that Path instances > are immutable. Fixed. > (If you're wondering about the @since tags: we use "API-tools" to validate > the workspace against a defined "baseline", LMK if you're interested in how > this works). I know I have a lot of warnings about not having an API baseline ;) I will try to find out more... But if you want to help me 1st thing is really getting that workspace stable 8-/ I'll mail you oob about that. On comment# 35: My bad 8-( This code is used by both the project classpath and the VM classpath code. I fixed it in the project part only because at that time I did not have the VM change current 8-/ It should be fixed now.
Created attachment 251567 [details] New version with review comments handled.
(In reply to Frits Jalvingh from comment #33) > > Can you reproduce this? > I can. I'm trying to hunt it down. So far it looks like IClasspathAttribute > is not generic at all, so code is explicitly allowing/handling specific > instances only like the javadoc parts, sigh. handshake: I can no longer reproduce, but I can't find a code change the seems to correspond to this issue, either. For my curiosity / education: where is that fix ? :) (In reply to Frits Jalvingh from comment #37) > Created attachment 251567 [details] > New version with review comments handled. I've received the updates in both bugs, patches apply cleanly. Icon added. Smoke tests passed. Thanks. Unless you have more fixes in the pipeline for today, I will: * try to convince Hudson that all is well (first attempt had a seemingly unrelated tests failure) * push the JDT/UI part in its current state, to unblock JDT/Debug. Alongside I will file a new bug for follow-up issues. First thing that will go into that bucket: support for classpath variables.
(In reply to Stephan Herrmann from comment #38) > (In reply to Frits Jalvingh from comment #33) > > > Can you reproduce this? > > I can. I'm trying to hunt it down. So far it looks like IClasspathAttribute > > is not generic at all, so code is explicitly allowing/handling specific > > instances only like the javadoc parts, sigh. > > handshake: I can no longer reproduce, but I can't find a code change the > seems to correspond to this issue, either. For my curiosity / education: > where is that fix ? :) You can always see the detailed log @ github; the problem was in JREContainerInitializer (jdt.debug). The code to update the vm classpath was explicitly only checking for the javadoc classpath attribute instead of persisting them generally. I added the external classpath there. So much for generic code ;-) > (In reply to Frits Jalvingh from comment #37) > > Created attachment 251567 [details] > > New version with review comments handled. > > I've received the updates in both bugs, patches apply cleanly. Icon added. > Smoke tests passed. Thanks. Phfew ;-) Thanks for your help and sorry it took so long 8-/ > Unless you have more fixes in the pipeline for today, I will: > * try to convince Hudson that all is well > (first attempt had a seemingly unrelated tests failure) > * push the JDT/UI part in its current state, to unblock JDT/Debug. > > Alongside I will file a new bug for follow-up issues. First thing that will > go into that bucket: support for classpath variables. Deal. I'm done for this part for now. Again, thanks for your help!
Gerrit change https://git.eclipse.org/r/43887 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3478a850278f2be0b18137ec3eb5eeab119a73db
Sources look good, manual testing was successful, and we have a successful Hudson build. Released for 4.5 M6. Thanks, Frits! Further polish and enhancements to be done in separate bugs.
- fixed the layout (only 3 columns were used; off-by-one error in spacers) - fixed unused code warnings http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3864184b4aa1d8395e2b30b597428338f7633215 :
(In reply to Markus Keller from comment #42) > - fixed the layout (only 3 columns were used; off-by-one error in spacers) > - fixed unused code warnings Thanks, I should've caught these. The unused column and code will be revisited when we address variable based paths during M7.
