matthiasblaesing commented on code in PR #6096:
URL: https://github.com/apache/netbeans/pull/6096#discussion_r1245694220
##########
enterprise/web.jsfapi/nbproject/org-netbeans-modules-web-jsfapi.sig:
##########
@@ -1,5 +1,5 @@
#Signature file v4.1
-#Version 1.54
+#Version 1.55
Review Comment:
Incompatible API/SPI changes normally require a bump of the major version of
the spec. The enterprise modules were always lenitent with this, so I'm
inclined to agree to keep it, but the right way would be:
- bump major release version to 2
- bump specification version to 2.0
- update users to the new major release version and bump their specification
version (compatible change)
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/completion/JsfAttributesCompletionHelper.java:
##########
@@ -246,15 +247,15 @@ private static void
addTypesFromPackages(CompletionContext context, CompilationC
}
}
- public static void completeSectionsOfTemplate(final CompletionContext
context, final List<CompletionItem> items, String ns, OpenTag openTag) {
+ public static void completeSectionsOfTemplate(final CompletionContext
context, final List<CompletionItem> items, String ns, OpenTag openTag,
JsfVersion jsfVersion) {
Review Comment:
See my comment in `JsfUtils.getRoot`: I think the final parameter is not
necessary.
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/actions/NamespaceProcessor.java:
##########
@@ -131,9 +120,8 @@ private List<VariantItem> getSortedVariants(String prefix) {
String ns = it.next();
Library library = supportedLibraries.get(ns);
if (prefix.equals(library.getDefaultPrefix())) {
- if (jsfSupport != null && jsfSupport.isJsf22Plus()) {
- ns = library.getNamespace();
- }
+ ns = jsfVersion.getNamespaceUri(prefix);
Review Comment:
The original code used the namespace from the library - should this not be
possible anymore?
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/JsfSupportImpl.java:
##########
@@ -58,6 +61,21 @@ public class JsfSupportImpl implements JsfSupport {
private static final Logger LOG =
Logger.getLogger(JsfSupportImpl.class.getSimpleName());
+ private static final Map<JSFVersion, JsfVersion> JSF_VERSION_MAPPING;
+ static {
+ Map<JSFVersion, JsfVersion> map = new HashMap<>();
+ map.put(JSFVersion.JSF_1_0, JsfVersion.JSF_1_0);
+ map.put(JSFVersion.JSF_1_1, JsfVersion.JSF_1_1);
+ map.put(JSFVersion.JSF_1_2, JsfVersion.JSF_1_2);
+ map.put(JSFVersion.JSF_2_0, JsfVersion.JSF_2_0);
+ map.put(JSFVersion.JSF_2_1, JsfVersion.JSF_2_1);
+ map.put(JSFVersion.JSF_2_2, JsfVersion.JSF_2_2);
+ map.put(JSFVersion.JSF_2_3, JsfVersion.JSF_2_3);
+ map.put(JSFVersion.JSF_3_0, JsfVersion.JSF_3_0);
+ map.put(JSFVersion.JSF_4_0, JsfVersion.JSF_4_0);
+ JSF_VERSION_MAPPING = Collections.unmodifiableMap(map);
Review Comment:
Is there a reason for the two version implementations?
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/completion/JsfAttributesCompletionHelper.java:
##########
@@ -461,7 +462,7 @@ public static void
completeFaceletsFromProject(CompletionContext context, List<C
public static void completeXMLNSAttribute(CompletionContext context,
List<CompletionItem> items, JsfSupportImpl jsfs) {
if
(context.getAttributeName().toLowerCase(Locale.ENGLISH).startsWith("xmlns")) {
//NOI18N
//xml namespace completion for facelets namespaces
- Set<String> nss =
NamespaceUtils.getAvailableNss(jsfs.getLibraries(), jsfs.isJsf22Plus());
+ Set<String> nss =
NamespaceUtils.getAvailableNss(jsfs.getLibraries(),
jsfs.getJsfVersion().isAtLeast(JsfVersion.JSF_2_2));
Review Comment:
I think `getAvailableNss` needs to be changed to take the version. The
boolean is used to determine if only the current namespace or also legacy ones
should be reported. With JSF4 this would add a third set:
- \>= JSF4 would get the new namespace + legacy jcp + legacy sun
- \>= JSF2.2 && < JSF4 would get legacy jcp + legacy sun
- < JSF2.2 would get legacy sun
At least that is my interpretation from looking at it.
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/actions/NamespaceProcessor.java:
##########
@@ -167,10 +153,9 @@ private ResultCollector(HtmlParserResult parserResult) {
this.nsCollector = new NamespaceCollector(parserResult);
this.compCollector = new ComponentCollector(parserResult,
prefixMap);
this.unresolvedCollector = new UnresolvedCollector(parserResult);
- initialize();
}
- private void initialize() {
+ private void initialize(JsfVersion jsfVersion) {
Review Comment:
See discussion in `JsfUtils.getRoot` I think this is not necessary.
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/facelets/CompositeComponentLibrary.java:
##########
@@ -89,7 +90,7 @@ public LibraryType getType() {
@Override
public String getDefaultNamespace() {
- return LibraryUtils.getCompositeLibraryURL(getLibraryName(),
support.getJsfSupport().isJsf22Plus());
+ return LibraryUtils.getCompositeLibraryURL(getLibraryName(),
support.getJsfSupport().getJsfVersion().isAtLeast(JsfVersion.JSF_2_2));
Review Comment:
I'm surprised. Did they really go to URNs and did not change the composite
component namespace?
##########
enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/JsfUtils.java:
##########
@@ -99,12 +100,9 @@ public static Result getEmbeddedParserResult(ResultIterator
resultIterator, Stri
return null;
}
- public static Node getRoot(HtmlParserResult parserResult, LibraryInfo
library) {
- Node rootNode = parserResult.root(library.getNamespace());
- if ((rootNode == null || rootNode.children().isEmpty()) &&
library.getLegacyNamespace() != null) {
- rootNode = parserResult.root(library.getLegacyNamespace());
- }
- return rootNode;
+ public static Node getRoot(HtmlParserResult parserResult, LibraryInfo
library, JsfVersion jsfVersion) {
+ String namespace =
jsfVersion.getNamespaceUri(library.getDefaultPrefix());
+ return parserResult.root(namespace);
Review Comment:
I think this changes the semantic. The way this is written allows using both
namespaces, while the new code only allows the "right" one. I would assume,
that JSF implementations will recognize also old namespaces and I think the old
behavior was sane.
You then can also keep the signature to its original form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
