This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
|
Description
Jesse Glick
2008-06-05 20:19:55 UTC
Cool! Thanks. I did not forget about this. I'm happy to help by updating java/web/j2ee/import modules to this new API. Since I have the list of usages, it would be easiest for me to replace all of them in one sweep, I think. Will probably file API review first with XMLUtil.java patch (coming soon), and if it seems acceptable to people, file a proposed patch of other modules a couple days later for additional review. AuxiliaryConfigImpl now in projectapi. validate to be implemented separately in issue #42686. (also check: ProjectXMLCatalogReader) findElement should throw IAE, not return null, in case >1 element of the given name can be found among children of the specified parent. See issue #141277 for background. Created attachment 94822 [details]
Changes to XMLUtil.java
Here are the proposed changes to XMLUtil. I added 6 methods and 6 unit tests. These methods were extracted from 21 module and 95 files. I can added the diffs to specific modules if required. The module list is: ant.freeform apisupport.project j2ee.archive j2ee.clientproject j2ee.earproject j2ee.ejbjarproject java.freeform java.j2seproject maven o.n.bluej openide.loaders profiler.nbmodule project.ant projectapi projectimport.eclipse.core projectimport.eclipse.web projectimport.jbuilder ruby.project ruby.rakeproject web.freeform web.project Created attachment 94823 [details]
Unit test for new methods on XMLUtil.java
> I can added the diffs to specific modules if required.
Yes, this would be great. Thanks!
Created attachment 94844 [details]
ant.freeform refactor diff
Created attachment 94845 [details]
apisupport.project refactor diff
Created attachment 94846 [details]
j2ee.archive refactor diff
Created attachment 94847 [details]
j2ee.earproject refactor diff
Created attachment 94848 [details]
j2ee.ejbjarproject refactor diff
Created attachment 94849 [details]
java.freeform refactor diff
Created attachment 94850 [details]
java.j2seproject refactor diff
Created attachment 94851 [details]
maven refactor diff
Created attachment 94852 [details]
o.n.bluj refactor diff
Created attachment 94853 [details]
openide.loaders refactor diff
Created attachment 94854 [details]
profiler.nbmodule refactor diff
Created attachment 94855 [details]
project.ant refactor diff
Created attachment 94856 [details]
projectapi refactor diff
Created attachment 94857 [details]
projectimport.eclipse.core refactor diff
Created attachment 94858 [details]
projectimport.eclipse.web refactor diff
Created attachment 94859 [details]
projectimport.jbuilder refactor diff
Created attachment 94860 [details]
ruby.project refactor diff
Created attachment 94861 [details]
ruby.rakeproject refactor diff
Created attachment 94862 [details]
web.freeform refactor diff
Created attachment 94863 [details]
web.project refactor diff
Created attachment 94864 [details]
j2ee.clientproject refactor diff
I am not sure I can see the whole picture in such amount of diffs. I'd suggest to create a named branch and integrate all these changesets into it. Now you shall be able to do it in your own team repository too. See http://wiki.netbeans.org/PrototypesRepo for details. Y01 Versioning information is missing. Increment API module spec version, add @since to new methods, write an apichange.xml entry. Y02 All modules that start to use the new API need to depend on new spec version of the API module. Otherwise I like the work and the amount of duplicated code it eliminates. (In reply to comment #32) > I am not sure I can see the whole picture in such amount of diffs. I'd suggest > to create a named branch and integrate all these changesets into it. Now you > shall be able to do it in your own team repository too. Probably mvfranz has no push access, so this is not an option. Attaching diffs is fine, but it would be better to attach one diff covering the whole repository - people can always ignore parts they are not interested in. I can upload one patch for all changes (trust me, adding 21 attachments was not fun). I can do this before or after I make the two changes for the API bump. Address the advices and then attach new patch. (In reply to comment #34) > I can do this before or after I make the two changes for the API bump. After, so we can see a final diff. Other than lack of @since, apichanges.xml, and versioning updates, everything looks good, I think - thanks for the hard work! You seem to be listed under https://sca.dev.java.net/CA_signatories.htm#m so that's no problem. To the code in XMLUtil.java: [JG01] Use generics (List<String>), and for-loops in preference to Iterator. Easier to read. E.g.: for (Element e : findSubElements(parent)) { ... } [JG02] Interpretation of 'order' param in appendChildElement is not obvious and should be clarified in Javadoc: that it is a list of local names; that it may have no duplicates; what happens if the given element is not listed at all; what happens if the order cannot be satisfied for this or that reason; etc. [JG03] Use IllegalArgumentException rather than asserts for inappropriate input, such as index == -1 in appendChildElement. [JG04] Clarify that findElement(parent, name, null) matches _any_ namespace, rather than only elements with no namespace. [JG05] Javadoc phrases such as "DOM provides a similar method" can be replaced with an appropriate {@link ...}. [JG06] translateXml missing Javadoc fields. [JG07] 'doc' parameter to copyDocument could probably be omitted from signature; just use to.getOwnerDocument(). [JG08] translateXml and copyDocument could probably be merged as they do much the same thing, though with different implementations - translateXml's use of Node.cloneNode(true) is more general than the limited cases in copyDocument. Since it may be useful to have both methods for different caller use cases, consider making one be implemented in terms of the other. Yes, I have already signed the license stuff for the OpenJDK (BSD/Haiku). I can make all the suggested changes, not a problem. Just to be clear, I did not write these new methods, I extracted them from the code that is being removed. The only thing I did write were the unit tests. (In reply to comment #37) > I did not write these new methods, I extracted them from the code > that is being removed. That's fine, but we have higher standards for API code - if the interface is wrong, we cannot change it later. (In reply to comment #36) > [JG02] Interpretation of 'order' param in appendChildElement is not obvious and > should be clarified in Javadoc: that it is a list of local names; that it may > have no duplicates; what happens if the given element is not listed at all; > what happens if the order cannot be satisfied for this or that reason; etc. It is confusing, I think I understand it, but explaining it might be hard. Append a child element to the parent at the specified location. The <code>desiredAppendOrder<code> is an unique list of local names of existing child elements and the new element to be appended. <em>All</em> existing child elements must be specified. Children are not re-ordered @param parent parent to which the child will be appended @param el element to be added @param desiredAppendOrder order of the elements which must be followed @throws IllegalArgumentException if the order cannot be followed, either a missing existing child or missing new child entry from the desiredAppendOrder What is hard to explain, is that the 'order' does two things. 1. ensure that all existing and the new child elements are specified, and 2. the element that we append before (insert before). The order does not really matter other then what follows the new child. > [JG04] Clarify that findElement(parent, name, null) matches _any_ namespace, > rather than only elements with no namespace. This is not how the implementation I picked works. Since it is using 'tagName' when the namespace is null and not 'localName' <h:table> is not the same as <table>. If I change it to use localName then <f:table> and <h:table> are the same, and there are now multiple elements with the same name These are the two implementations I picked from: if ((namespace == null && name.equals(el.getTagName())) || (namespace != null && name.equals(el.getLocalName()) && namespace.equals(el.getNamespaceURI()))) if (name.equals(el.getLocalName()) && ((namespace != null && namespace.equals(el.getNamespaceURI())) || namespace == null)) I don't care which implementation is used. I just picked the first one, and that it how I wrote my tests. (In reply to comment #39) > What is hard to explain, is that the 'order' does two things. 1. ensure that > all existing and the new child elements are specified, and 2. the element that > we append before (insert before). The order does not really matter other then > what follows the new child. Something like this. #1 is a precondition; #2 is the useful part. The intent is to make it easy to insert new child elements when an XML schema (such as for project.xml#//data) specifies a particular order but some items are optional or can be repeated. The order here should match the <xsd:sequence> in the schema. It is assumed you are starting with a valid document, and you want to ensure that it is still valid after the insertion. > I don't care which implementation is used. I just picked the first one, and > that it how I wrote my tests. But you need to pick the behavior that matches what the existing clients rely on, or the IDE will be quite broken. (*) Setting namespace=null is used when searching for a similar child element using any of a number of historical schema versions. For example, http://www.netbeans.org/ns/freeform-project-java/1.xsd defines a <built-to> which is comparable to the <built-to> in .../2.xsd; if java.freeform were calling this method on a parent <compilation-unit>, and it could be starting with a project.xml defined with either schema version, it would pass null for the namespace to match only by local name. (*) Be sure to run unit tests of modules you are editing (ant test-unit -Dtest-unit-sys-prop.ignore.random.failures=true). If the tests passed before - which they should at least for those listed in nbbuild/hudson/core-main - they ought to pass after the refactoring too. (In reply to comment #40) > But you need to pick the behavior that matches what the existing clients rely > on, or the IDE will be quite broken. (*) Setting namespace=null is used when > searching for a similar child element using any of a number of historical > schema versions. For example, > http://www.netbeans.org/ns/freeform-project-java/1.xsd defines a <built-to> > which is comparable to the <built-to> in .../2.xsd; if java.freeform were > calling this method on a parent <compilation-unit>, and it could be starting > with a project.xml defined with either schema version, it would pass null for > the namespace to match only by local name. > > (*) Be sure to run unit tests of modules you are editing (ant test-unit > -Dtest-unit-sys-prop.ignore.random.failures=true). If the tests passed before - > which they should at least for those listed in nbbuild/hudson/core-main - they > ought to pass after the refactoring too. I have run the unit tests on all the projects using both implementations and all projects give me the same errors as without my changes. There seem to be more failures on OS X since Javac and rt.jar are different. At this point, it seems that the unit tests do not test this functionality. If null matching is the desired logic, I can change the new code and tests to match that. The usages that I found of passing null for a namespace are: compares tagName and null namespace - apisupport.project (3) - projectimport.jbuilder (1) comares localname and null namespace - projectimport.eclipse.core (7) - projectimport.eclipse.web (2) All other projects specify the namespace. (In reply to comment #41) > If null matching is the desired logic Yes, passing null for namespace should mean "match any element with the given tag name, regardless of what namespace - if any - it is in". This ought to work whether the document was parsed with namespace support on or off, or created in memory using DOM 1 or DOM 2+ methods. Created attachment 96080 [details]
XMLUtil patches for refactored methods
Here are my latest changes for XMLUtil, sorry it took so long, I had some issue after I did a pull -u and redid the changes.
I made the changes mentioned, all but the combing copyDocument and translateXML. I looked at it, and made them even more similar, but when I tried to combine/re-use the logic something when horribly wrong.
The only additional change I did was to use localName and nodeName for findElement. It seems that the parser using by projectimport.jbuilder does not set localName on nodes. Not sure if this is the DOM1 vs DOM2 logic that was mentioned.
This should apply to main as of today. I will upload the rest of the patch once requested.
Created attachment 96166 [details]
Patch for modules to use new XMLUtil methods
This is the patch that removes the refactored methods and changes the usage to XMLUtil version. It should also contain the bumping of the required openide.util module to 8.3.
(Best to attach a single patch at a time, with all API & usage together.) JG02 - appendChildElement Javadoc is still a bit mysterious; the summary in this issue was more helpful than the Javadoc I see in the patch. "the existing item" is unspecified, and it is not mentioned that elements of 'order' are element local names. But I guess I can fix this kind of thing up when applying the patch. JG06 still open. Again not a blocker. [JG09] Any reason you skipped defaultErrorHandler (e.g. from org.netbeans.modules.projectimport.eclipse.core.Util)? Something like this is used quite widely. Does not have to be done as part of this issue if you are out of time, of course, but seems to fall into the same category as the other methods. [JG10] Junk in ant.freeform.Actions. [JG11] Dependencies in client modules are not consistently updated, e.g. in apisupport.project and j2ee.archive (maybe others). In other cases (e.g. j2ee.clientproject) test dependencies are changed but not main module dependencies. All client modules should have one dependency change in project.xml: to use the new rev of org.openide.util. (In reply to comment #46) > (Best to attach a single patch at a time, with all API & usage together.) > > > JG02 - appendChildElement Javadoc is still a bit mysterious; the summary in > this issue was more helpful than the Javadoc I see in the patch. "the existing > item" is unspecified, and it is not mentioned that elements of 'order' are > element local names. But I guess I can fix this kind of thing up when applying > the patch. > > > JG06 still open. Again not a blocker. > I will take another look at these. > > [JG09] Any reason you skipped defaultErrorHandler (e.g. from > org.netbeans.modules.projectimport.eclipse.core.Util)? Something like this is > used quite widely. Does not have to be done as part of this issue if you are > out of time, of course, but seems to fall into the same category as the other > methods. > There are a few defaultErrorHandler methods spread throughout the different projects. I did not touch any of them. I can look at including them as well. > > [JG10] Junk in ant.freeform.Actions. > > > [JG11] Dependencies in client modules are not consistently updated, e.g. in > apisupport.project and j2ee.archive (maybe others). In other cases (e.g. > j2ee.clientproject) test dependencies are changed but not main module > dependencies. > When changing some of the dependencies I got errors, here is one bug #178584, guess I will have to change the files manually. > All client modules should have one dependency change in project.xml: to use the > new rev of org.openide.util. (In reply to comment #46) > (Best to attach a single patch at a time, with all API & usage together.) > JG06 still open. Again not a blocker. Added some descriptions to the parameters. > > > [JG09] Any reason you skipped defaultErrorHandler (e.g. from > org.netbeans.modules.projectimport.eclipse.core.Util)? Something like this is > used quite widely. Does not have to be done as part of this issue if you are > out of time, of course, but seems to fall into the same category as the other > methods. > Looked at this. questions are: Exceptions.attachMessage vs ErrorManager.getDefault().annotate Logger.getLogger(Util.class.getName()).log vs ErrorManager.getDefault().notify These are from diff -u projectimport.eclipse.web/src/org/netbeans/modules/projectimport/eclipse/web/Util.java ruby.rakeproject/src/org/netbeans/modules/ruby/modules/project/rake/Util.java Ruby Rake is different from the other three instances I found in the way exceptions are logged. Is one method preferred over another? > > [JG10] Junk in ant.freeform.Actions. Removed (In reply to comment #48) >> [JG09] defaultErrorHandler > Exceptions.attachMessage vs ErrorManager.getDefault().annotate > Logger.getLogger(Util.class.getName()).log vs ErrorManager.getDefault().notify ErrorManager is effectively deprecated; do not use it for new code. Use Logger.getLogger(Whatever.class.getName()) for logging, as in projectimport.eclipse.*. Created attachment 96651 [details]
All changes for XMLUtil
This has all the changes needed to refactor the XML methods into XMLUtil. The only item that needs work is the appendChildElement documentation (AFAIK).
I manually changed the j2ee.archive dependency as the UI does not allow it. I tried to recreate the dependencies, but there seem to be circular dependency with the j2ee modules.
JG11 not fully addressed; see j2ee.clientproject/nbproject/project.xml. j2ee.earproject also looks incorrect, maven and profiler.nbmodule are not updated at all, and there may be others wrong. Please review the patch yourself and double-check all the dep changes. Should be possible to get this patch applied this week in time for 6.9 Beta cutoff on Friday. Created attachment 96724 [details]
Refactored code for XMLUtil
I rechecked the project dependencies. It seems that some hg pull -u I did undid my dependency changes.
(In reply to comment #52) > Created an attachment (id=96724) [details] > Refactored code for XMLUtil This is essentially the same patch as before, with the exception of line numbers and +import java.util.logging.LogRecord; Please, read your own diff before attaching. 'diffstat -p1 whatever.diff' is also helpful to get an overview of what files are modified. Created attachment 96832 [details]
XMLUtil Refactoring patch
I have rechecked the changes. It seems that the UI cannot change the dependencies (project.xml) so I had to edit them manually. I ran diffstat and scrolled through the patch. Everything looks good (but I could be wrong). I applied to an updated pull (different repo) and only the openide.util manifest has a conflict - version already been bump to 8.3.
(In reply to comment #54) > It seems that the UI cannot change the dependencies Probably in a new dev build it can; you're on CC for the bug so you should see notification when the fix makes it into a nightly. > applied to an updated pull (different repo) and only the openide.util manifest > has a conflict - version already been bump to 8.3. Which means that this patch needs to be updated to change 8.3 to 8.4, not just in openide.util/manifest.mf, but in every nbproject/project.xml which updates the dep on org.openide.util, and in @since tags and apichanges.xml. Sorry, but there is no clean way to merge API version increments, and someone else happened to be doing a change in org.openide.util recently (CharSequences). Created attachment 96877 [details]
XMLUtil Refactoring patch
Changed the version to 8.4, API doc to say seven instead of six new methods, updated date to today.
Minor patch problem: openide.util 8.2 -> 8.4 was rejected (should have been 8.3 -> 8.4). Thank you for your hard work! core-main #44728f9b507e Integrated into 'main-golden', will be available in build *201004090201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/44728f9b507e User: mvfranz@netbeans.org Log: #136595: DOM utility methods to add to org.openide.xml.XMLUtil. |