Bug 155962

Created attachment 75267 [details]
New annotation to add to objects willing to convert themselves to and from Properties

1. if anyone comes better name for the annotation, perfect
2. the change is useful of its own
3. it would be nice if TopComponent's template used the annotation instead of "Resolver"

During testing I found bug 156158. It needs to be fixed before the convertors get widely used in TopComponent wizard.

Created attachment 75360 [details]
Improved patch with ability to do "readResolve" and its usage in TC wizard

I looked at it and haven't found anything wrong with it. Just make it buildable with JDK1.5.

I would prefer that this effort be spent on creating a better way to persist TC state in general (using NbPreferences),
so we can stop using the settings module generally, but this at least seems better than what we have now.


[JG01] Import the annotation, rather than using FQN, in the template.


[JG02] Specifying a public ID for a nonexistent DTD in an annotation is bizarre. Can we fix settings to not require a
"DTD" at all? Failing that, could the annotation at least generate a "public ID" based on the FQN of the class?


[JG03] The patch in NbInstaller line 598 is unnecessary.


[JG04] I'm not really clear on why NbInstaller is being patched at all. Who is creating duplicate entries and why? How
is this related to the rest of the patch?


[JG05] The annotation belongs in the spi package, not api. Any API would be for requesting that objects be read or
stored, whereas the SPI is for defining how storage should work for your objects.


[JG06] apichanges fails to mention the change to readProperties semantics.


[JG07] For ignoreChanges, "all" (or the array {"all"}) should be a constant defined in the annotation.


[JG08] The AP should probably be checking roundEnv.processingOver().


[JG09] The AP should verify that the class contains valid read/writeProperties methods, or whatever it is the runtime
will need, in addition to the constructor.


[JG10] instantiableClassOrMethod seems to have a type arg which is always null. Delete.


[JG11] convElem[1] seems to be ignored. Is it even possible to use a methodvalue convertor? AFAIK it is not, and you do
not declare ElementType.METHOD anyway. So simplify this code.


[JG12] Patch to XMLPropertiesConvertorTest.java is unnecessary, do it separately.

JG01: OK.
JG02: I leave it as it is. The Public ID encode FQN: "-//org.myorg.pkg//ClassName//EN"
JG03,04: Duplicate entries are created for modules on classpath. Which happens especially in unit tests. It makes 
debugging hard and I think we want to eliminate the duplicates as soon as possible. Anyway, it is unrelated change to 
this issue, but I guess we want to do it.
JG05: The spi is for those who write own convertors. api is for those who use them. This annotation is use of existing 
convertor. That is how I convinced myself to put it into api package. Also it belongs next to already existing 
api.Factory (layer API).
JG06: OK.
JG07: OK. 

Code cleaned as suggested. Thanks for the review.

Created attachment 75527 [details]
Patch to integrate on Saturday

compiles OK in
core-main#2c97115e422d

I still think JG02 is weird. The format of the file being written is in fact predetermined; you can put whatever value
you like as the attribute here, because there is no such DTD anywhere in existence. It is an unfortunate relic of the
Convertors architecture that a "DTD public ID" is required as a unique identifier for the convertor, but we can and IMHO
should hide this fact from annotation users, who already have a unique ID: the class being converted. (The settings
module should in this case call Utilities.translateNames to allow for the usual refactorings.)