Created attachment 112108 [details]
A set of XML schemas that demonstrate the issue, and an XML document indicating the code completion context (Parental.xml).

Uploaded set of schemas demonstrating the issue. To replicate the issue, extract the schemas to a directory, open the XML document in the editor, and invoke code completion (Ctrl-Space).

Created attachment 112109 [details]
Tests for the issue

Correct, completion support should expand the substitution groups whenever it encounters a completion item, which is a head of a substitution group. Might be even non-abstract element.

Support for tracking subst groups is not present at all at the moment, because schemas may import each other namespaces, we cannot make backward references and model the substitution in the schema model - the expansion must probably happen in the completion code.

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

Created attachment 112220 [details]
Excludes abstract elements from code completion suggestions, and adds all subtitutions for elements valid in completion context.

Svata, please review.

Created attachment 112461 [details]
Aggregated diff

The previously attached bundle cannot be used since some csets use a username @gmail.com which is forbidden and cannot be pushed. Anyway a number of changes remove largish files added in earlier changes, etc. For API reviews, or other cases where you reasonably expect to be heavily revising your changes and want to see what the final effect would be, http://wiki.netbeans.org/HgHowTos#Develop_API_review_patches_using_MQ is recommended.

Please consider whether xml.schema.completion/test/unit/src/org/netbeans/modules/xml/schema/completion/resources/XMLSchema.xsd really needs to be there; three copies are already present in the source tree, including one in xml.schema.completion/src!

Also avoid adding *.zip files to sources. A unit test needing test data should ideally minimize it to the point that it can be comfortably committed directly (or even generated from the JUnit-based source code), which also compresses better in the Hg repo and permits subsequent test updates to be reviewable as patches.

Note that since this applies to a public package, it should be API_REVIEW_FAST [1] (assuming it can be fixed to be compatible [2]) and should include changes to OpenIDE-Module-Specification-Version (or spec.version.base [3]), apichanges.xml, @since.

[1] http://wiki.netbeans.org/Review_Steps
[2] http://wiki.netbeans.org/API_Design
[3] http://wiki.netbeans.org/DevFaqImplementationDependency

Created attachment 112507 [details]
Diff with review suggestions addressed

Addressed the following suggestions:
sdedic:
1) Removed method renaming from AXIComponentFactory
2) Removed method addition from SchemaModel/SchemaModelImpl
3) Renamed lookup() method to avoid confusion

jglick:
1) Switched to aggregated diff
2) Removed excess XMLSchema.xsd instances
3) Removed zip from test resources
4) Bumped spec.version.base (and dependency from xml.schema.completion > xml.schema.model)
5) Updated JavaDocs with @since
6) Created apichanges.xml and documented API change

Please review

I do not know much about the domain so I hope sdedic can comment on the meat of the change. As to form:


Diff to SchemaModelImpl.java does not appear to do anything at all, should probably be reverted.


FindSubstitutionsVisitor does not look to be used except for its static methods, meaning that to minimize API surface the actual visitor should be hidden:

public class FindSubstitutions {
  public static Set<GlobalElement> resolveSubstitutions(SchemaModel model, String namespaceUri, String localName) {...}
  public static Set<GlobalElement> resolveSubstitutions(SchemaModel model, GlobalElement substitutionGroupHead) {...}
  private FindSubstitutions() {/* unused */}
  private static class FSVisitor extends DeepSchemaVisitor {...}
}

For that matter, only the rS method taking two String's seems to be used externally; the one taking GlobalElement could be left package private for the unit test (could always be made public later if it is needed).

(In reply to comment #12)
Thanks for the feedback.

> Diff to SchemaModelImpl.java does not appear to do anything at all, should
> probably be reverted.
Agreed: it probably only has space added or something at the moment. I'll revert it properly.

> FindSubstitutionsVisitor does not look to be used except for its static
> methods, meaning that to minimize API surface the actual visitor should be
> hidden:
> 
> public class FindSubstitutions {
>   public static Set<GlobalElement> resolveSubstitutions(SchemaModel model,
> String namespaceUri, String localName) {...}
>   public static Set<GlobalElement> resolveSubstitutions(SchemaModel model,
> GlobalElement substitutionGroupHead) {...}
>   private FindSubstitutions() {/* unused */}
>   private static class FSVisitor extends DeepSchemaVisitor {...}
> }
This is essentially what I had at first, living in the resolvers (non-public) package. The reason I exposed the visitor was for consistency with the rest of the visitors package classes (it didn't seem to belong in the model package). Do you have a suggestion for where the above utility class should live? Actually, I just noticed that I forgot to hide the constructor and expose the substitution result set. Given that doing those things (hiding the constructor and adding an accessor for the result set) would reduce the API surface, does that sound like a reasonable compromise? 

> For that matter, only the rS method taking two String's seems to be used
> externally; the one taking GlobalElement could be left package private for the
> unit test (could always be made public later if it is needed).
That makes sense. Will do.

(In reply to comment #13)
> The reason I exposed the visitor was for consistency with the rest of
> the visitors package classes

Then there may have been a sloppy API to start with. Someone knowledgeable about the details of the module should comment.

Created attachment 112522 [details]
Diff with more feedback addressed

Feedback addressed:
jglick:
> Diff to SchemaModelImpl.java does not appear to do anything at all, should
> probably be reverted.
Done (also reverted SchemaModel.java for the same reason)

> FindSubstitutionsVisitor does not look to be used except for its static
> methods
Hid constructor, exposed result set. Awaiting feedback (perhaps from Svata?) on leaving visitor exposed to maintain consistency with existing visitors API.

> For that matter, only the rS method taking two String's seems to be used
> externally; the one taking GlobalElement could be left package private for the
> unit test (could always be made public later if it is needed).
Done

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

Svata, have you had a chance to look into this?

[SD4] getAllConnectedSchemas should use Set + check for duplicities, to avoid circular import references.

[SD5] resolveSubstitution should not log unresolved substitution at all, or with e.g. FINEST level - the situation is fairly common if the inspected schema is not interested in the nsUri:localName at all

[SD6] note that the client is responsible for traversing import/include schemas in the javadoc of forSubstitutionGroup(). I would even recommend to remove this method from public API, as a resolve() method exists already.

Sorry for not catching them during the 1st run

[SD6] apichanges.xml - at the end, please fill the placeholders in htmlcontents

re. visitors in general: I would actually prefer to hide the visitor classes behind one utility class; they are quite single-purpose to be classes.

Created attachment 112890 [details]
Diff with Svata's feedback addressed

(In reply to comment #18)
> [SD4] getAllConnectedSchemas should use Set + check for duplicities, to avoid
> circular import references.
done: recursed into referenced schemas and switched to use a Set

> [SD5] resolveSubstitution should not log unresolved substitution at all, or
> with e.g. FINEST level - the situation is fairly common if the inspected schema
> is not interested in the nsUri:localName at all
done: removed log.

> [SD6] note that the client is responsible for traversing import/include schemas
> in the javadoc of forSubstitutionGroup(). I would even recommend to remove this
> method from public API, as a resolve() method exists already.
I'm not sure which method you mean. I can't find one called forSubstitutionGroup(). If this method was present in an earlier patch, it might have been removed.

> Sorry for not catching them during the 1st run
No worries: thanks for looking into it.

> [SD6] apichanges.xml - at the end, please fill the placeholders in htmlcontents
done. 

> re. visitors in general: I would actually prefer to hide the visitor classes
> behind one utility class; they are quite single-purpose to be classes.
done: hid the visitor, but left the utility class in the visitors package. Is that ok?

(In reply to comment #20)
> (In reply to comment #18)
> > [SD4] getAllConnectedSchemas should use Set + check for duplicities, to avoid
> > circular import references.
> done: recursed into referenced schemas and switched to use a Set
ok.

> > [SD6] note that the client is responsible for traversing import/include > I'm not sure which method you mean. I can't find one called
> forSubstitutionGroup(). If this method was present in an earlier patch, it
> might have been removed.
> 
OK.

> done: hid the visitor, but left the utility class in the visitors package. Is
> that ok?
OK.

(In reply to comment #21)
Thanks Svata. Is this able to be pushed now?

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/4967cd723300
User: Svata Dedic <sdedic@netbeans.org>
Log: #203793: Adds substitution group members to code completion

Please do not forget the Target Milestone. Also check ide.branding/release-toplevel/CREDITS.html if you have not done so already.