Bug 192595

Created attachment 103416 [details]
Proposed patch, minus apichanges

Please review.

Y01 Stuffing this into openide.filesystems seems wrong to me. I'd not be afraid to start new module org.netbeans.api.dtd or even reuse existing org.netbeans.api.xml (and move it to platform).

Such module can easilly have compile time dependency on filesystems and host any XML related annotations while having low closure of runtime dependencies. Existing utilities like org.openide.xml could be slowly migrated in there.

Potentially the api.xml current content (probably requires dependency on openide.nodes) could be moved away and the org.netbeans.api.xml code name used solely for purposes of new, modern APIs for dealing with XML.

(In reply to comment #3)
> Existing utilities like org.openide.xml could be slowly migrated in there.

I don't see any way of migrating org.openide.xml upwards. Even if e.g. XMLUtil were copied into some new module, the old version could not delegate to the new version, so what would be the point?

(Module auto deps could be used to redirect existing clients, but this would still be an incompatible change at the source level, which is a major disturbance for such a widely used module as openide.util. Anyway XMLUtil is used in a number of places in openide.filesystems.)

> Potentially the api.xml current content (probably requires dependency on
> openide.nodes)

And openide.loaders, and openide.text; and these dependencies are mentioned in public signatures. So there is no good way to minimize its dependencies. Anyway api.xml is used by IDE modules which work with XML document editors, which does not really belong in the platform and is a much more specialized task than registering entities in the system catalog to avoid network connections from XMLUtil.parse.

If openide.filesystems is unacceptable (even under a different package name), then I would rather put the annotation where the runtime currently resides, in openide.loaders. It would be possible to move this "down" into openide.filesystems or some new module TBD later.

(In reply to comment #4)
> I don't see any way of migrating org.openide.xml upwards. Even if e.g. XMLUtil
> were copied into some new module, the old version could not delegate to the 
> new
> version, so what would be the point?

The point if that people would start to use the new API migrating way from the original (misplaced) one. Creating the new API methods is the visible part of the change. How do we make the old XMLUtil and new API behave the same is implementation issue. 

> but this would
> still be an incompatible change at the source level, 

Option1: XMLUtil finds its own implementation in lookup (provided by the xml module)

Option2: We duplicate the code. Share the tests to ensure the code behaves the same.

Option3: New API delegates to old XMLUtils. In future we switch to option 1 or option 2.

> which is a major
> disturbance for such a widely used module as openide.util. Anyway XMLUtil is
> used in a number of places in openide.filesystems.)

That is indeed annoying. Option 3 would however work OK for this case.

> > Potentially the api.xml current content (probably requires dependency on
> > openide.nodes)
> 
> And openide.loaders, and openide.text; and these dependencies are mentioned in
> public signatures. So there is no good way to minimize its dependencies. Anyway
> api.xml is used by IDE modules which work with XML document editors, which does
> not really belong in the platform and is a much more specialized task than
> registering entities in the system catalog to avoid network connections from
> XMLUtil.parse.

OK. I just liked the codenamebase. Call the new module org.netbeans.api.xmlutil if you want.

> If openide.filesystems is unacceptable (even under a different package name),
> then I would rather put the annotation where the runtime currently resides, in
> openide.filesystems or some new module TBD later.

OK. Different package name (using the new org.netbeans.api style) is imho a TCR. New module like the above mentioned api.xmlutil is a TCA. 


> openide.loaders. It would be possible to move this "down" into

No! To not encourage new dependencies on openide.loaders is a TCR, imho.

(In reply to comment #5)
> Option3: New API delegates to old XMLUtils. In future we switch to option 1 or
> option 2.

This seems most attractive in general, but the devil is in the details.

For the case of XMLUtil, the problem is that there are a number of usages of XMLUtil in openide.filesystems and core.startup. Either we:

3a. Make openide.filesystems and core.startup depend on the new module, which must then be in core/*.jar. May not even work, since the new module must depend on openide.filesystems to get LayerGeneratingProcessor.

3b. Copy parts of XMLUtil into a private class in openide.filesystems, and copy other parts into a private class in core.startup. (Some aspects, esp. namespace support, can probably be omitted.)

For the case of EntityCatalog, the problem is that this is defined as an SPI, i.e. you could in principle register a subtype into global lookup. Either we:

3c. Make the new EntityCatalog be a subtype of the old one, which prevents eventual abandonment of the old one and looks ugly since it will inherit from a deprecated type.

3d. Search lookup for instances of either kind of EntityCatalog, which will not work for the case of an instance of the new EC when the old EC.getDefault() is called.

Compared to these obstacles, plus the overhead of a new module and the labor involved in getting hundreds of client classes to switch to a new import, the current patch seems much more straightforward.

(In reply to comment #6)
> 3b. Copy parts of XMLUtil into a private class

To clarify: this would be needed when finally deprecating XMLUtil, not when introducing a delegating copy (can use @SuppressWarnings("deprecation") in the meantime).

> For the case of EntityCatalog, the problem is that this is defined as an SPI

or

3e. Do not support custom instances of the new EC in the global lookup at all; continue to support instances of the old EC until this is deleted. (The support for layer registration can be hardcoded, and the other instance in current global lookup is from openide.loaders and only deals with long-deprecated APIs.)

Since the original patch has been sidetracked by a request to move even the unrelated XMLUtil into a new module, which is a much more disruptive change than just replacing a layer registration with an annotation, I will try a different approach: @EntityCatalog.Registration in openide.util, with processor and runtime impl there as well, using e.g.

---%<--- .../org-openide-loaders.jar!/META-INF/entities
-//NetBeans//Entity Mapping Registration 1.0//EN
org/netbeans/modules/openide/loaders/EntityCatalog.dtd
-//NetBeans IDE//DTD xmlinfo//EN
org/netbeans/modules/openide/loaders/xmlinfo.dtd
---%<---

and a new public method Set<String> publicIDs() in EntityCatalog. Should be straightforward and permit the annotation to live in the most natural place. The current xml/entities/ registration can be deprecated and the runtime impl can continue to live in openide.loaders.

Don't give up! I did not want to block the original attempt, just make it better. I understand the problems deprecating XMLUtil, so I am going to give up on that.

Still I believe new module is better than openide.filesystems or openide.loaders. 

I prefer use of LayerGeneratingProcessor over other configuration files.

So just create xml.catalog (I know it exists) or xml.catalog.api module and put your @EntityCatalogRegistration and its processor there. Make this module depend on openide.filesystems and let everyone who wants to register the catalog use this new module.

My primary vision is to move (most of) the XML related stuff into autoload modules and eliminate that from the fixed ones.

After some offline discussion: xml.catalog.api is reasonable but only if a cleaned-up version of what is now in the SPI of xml.catalog gets moved there, so it has some interesting content beyond what EntityCatalog does now. We probably want three annotations (plus plural variants):

1. @Catalog(id="projects", displayName="#...", shortDescription="#...", icon="...") to create a read-only XML catalog (displayed in Tools > DTDs and XML Schemas). For example, project.ant would do this instead of the current ProjectXMLCatalogReader.

2. @Entity(publicID="...", entity="something.dtd") for registering DTDs for internal use (such as for settings), similar to former proposal. Optional attr catalog="maven" to create an alternate entity catalog useful for user documents.

3. @Schema(uri="...", schema="something.xsd", catalog="projects") for registering schemas by namespace URI to a catalog of your choice. For example, various project type modules would use this to register schemas into project.ant's catalog.

EntityCatalog.getDefault could probably stay where it is (the new module would register the preferred EntityCatalog impl), and XMLUtil need not be touched, but there could be a new API such as:

public abstract class Catalog {
    public static final String ID_INTERNAL; // used for EC.default
    public static @CheckForNull Catalog forID(String id);
    public static Set<Catalog> allCatalogs(); // in Lookup
    public abstract String getID();
    public abstract String getDisplayName();
    public abstract String getShortDescription();
    public abstract Icon getIcon();
    public abstract EntityResolver createEntityResolver(); // cf. XMLUtil.parse
    public abstract Schema createSchema(); // cf. XMLUtil.validate
}

where the impl of createSchema in the standard catalog impl (created using annotations above) would either eagerly combine all schemas as ProjectXMLCatalogReader does today, or use LSResourceResolver to load a schema on demand if that proves to work. (MavenCatalog also uses some trick with "SCHEMA:" prefix that needs to be investigated.)

Some sort of compatibility layer for users of xml.catalog's SPI would be desirable; TBD what we can do.

Created attachment 105213 [details]
Slightly updated patch, in case this is still useful

*** Bug 35880 has been marked as a duplicate of this bug. ***

Too much for 7.2.

Any interest in pushing this forward?