Bugzilla – Full Text Bug Listing
| Summary: | WSDLElementImpl leaves spaces in source afrer forceNiceRemoveChild - EMF command undo/redo broken | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Webservices | Reporter: | Dimitar Tenev <dimitar.tenev> | ||||||||||||||||
| Component: | wst.wsdl | Assignee: | Project Inbox <wst.wsdl-triaged> | ||||||||||||||||
| Status: | ASSIGNED --- | QA Contact: | Keith Chong <keith.chong.ca> | ||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||
| Priority: | P2 | CC: | ccc, emil.simeonov, kaloyan, keith.chong.ca | ||||||||||||||||
| Version: | 3.1.2 | ||||||||||||||||||
| Target Milestone: | Future | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows Vista | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Dimitar Tenev
Created attachment 171249 [details]
Step 1 to the final problem
Created attachment 171250 [details]
Step 2 to the final problem
Created attachment 171251 [details]
Step 3 to the final problem
Created attachment 171252 [details]
Final problem - the wsdl is broken.
Dimitar, this appears to have been opened against Eclipse's Galileo SR2. That corresponds to WTP 3.1.2. If that is not correct, please let us know. Hi Dimitar, I read through the bug and at a first reading this does seem to fit the "major" importance criteria (major loss of function). Any chance you could write a small JUnit to demonstrate the issue? Created attachment 171517 [details]
Junit reproducing the bug.
Hi Valentin, I have created a Junit (junit.TestBug315951.testBug315951()), and attached it in the bugzilla. It is a plugin project which I have exported as archive. It has dependency to EMF Transaction API, so please make sure that you have it: http://download.eclipse.org/modeling/emf/updates/releases/ EMF TRANSACTION SDK 1.3.1 In case you need more information, please do contact me. Best regards, Dimitar Hi Colleagues, I 1.Affiliation - SAP 2. Be clear on which release you want this bug to be fixed in, or "last workable" date, if requesting a patch. "last workable" 3. Justify why this is a hotbug. Note that "our users don't like the bug" is not the type of reason that gets much attention. The motivation we are looking for to justify a P1-like priority is, basically, "this bug blocks our adoption of WTP" (explaining why, of course). Use case: We have a WSDL editor based on WTP WSDL EMF model. The editor represents WSDL file as UI tree and text source. The editor does use EMF commands to make undo/redo over WSDL EMF model. This functionality is broken by this bug. The problem: Making Undo of EMF command does leave formatting characters into WSDL source text. These characters make impossible the next undo command, because it is a text one. Text commands do work on characters ranges, and after undo of EMF command, these ranges are not valid anymore. The problem is allocated in org.eclipse.wst.wsdl.internal.impl . WSDLElementImpl.forceNiceInsertBefore(Node parent, Node newChild, Node referenceChild), which adds formatting whitespaces. Possible solution: Do not add formatting spaces into forceNiceInsertBefore(…) method. Hi Colleagues, The bug impacts WSDL editor, based on WSDL EMF model. The editor undo/redo is broken because of this issue. This means loss of data as well beacuse undo breaks the source of the opened wsdl, and can not be redo. Here is the info requested for Hotbug request. 1.Affiliation - SAP 2.Be clear on which release you want this bug to be fixed in, or "last workable" date, if requesting a patch. - "last workable" 3.Justify why this is a hotbug. Note that "our users don't like the bug" is not the type of reason that gets much attention. The motivation we are looking for to justify a P1-like priority is, basically, "this bug blocks our adoption of WTP" (explaining why, of course). Use case: - We have a WSDL editor based on WTP WSDL EMF model. The editor represents WSDL file as UI tree and text source. The editor does use EMF commands to make undo/redo over WSDL EMF model. This functionality is broken by this bug. The problem: - Making Undo of EMF command does leave formatting characters into WSDL source text. These characters make impossible the next undo command, because it is a text one. Text commands do work on characters ranges, and after undo of EMF command, these ranges are not valid anymore. The problem is allocated in org.eclipse.wst.wsdl.internal.impl . WSDLElementImpl.forceNiceInsertBefore(Node parent, Node newChild, Node referenceChild), which adds formatting whitespaces. Possible solution: - Do not add formatting spaces into forceNiceInsertBefore(…) method. Best regards, Dimitar Thank you for the JUnit. I've been able to re-create the JUnit failure. I'm looking into the code to understand what's going on. Could you please clarify when would you need a fix? Hi Valentin, We need the fix in WTP 3.2.1. Best regards, Dimitar Tentatively targetting to 3.2.1. Dimitar, here are my thoughts so far on this bug. I wondered why we did not see this problem before? It turns out that while the WSDL model is also used in by the WTP WSDL editor, the WTP editor does not mix EMF and text commands on the command stack. Instead, whether the user edits in source mode or in graphical mode, the editor consistently records all changes as text changes which then go on the undo stack, . When editing in source mode the changes are the result of manipulating the content in the text editor. In graphical mode text changes are a side effect of manipulating the WSDL model which automatically reconciles (updates) the underlying DOM which in turns updates the content in the text editor. I suspect that changing your editor to work like the WTP editor is not a viable solution for you, right? I'm exploring a few possible solutions and hope to reach a conclusion soon. I looked at the "nice insert" code closely and tried to understand what it does. If I read it right, the code is doing this:
1) looks for a text node before the insertion point
2) if it finds a text node and the node has a new line character, it removes all character data after the new line character and replaces it with a computed number of spaces based on the indent
3) it then adds a new text node after the one found in the previous step, using the same calculated indent
4) it then inserts the new element between the two text nodes
The "nice remove" code tries to find the text node prior to the element being removed, clears its content, then removes the element. There's more code that gets invoked only when there's no text node before.
Executing the insert followed by the remove - as exercised by your do/undo JUnit - leaves behind extra character data due to steps 2 and 3 above, and this seems to be causing the JUnit to fail.
About the possible solutions now...
One experimental patch I came up with tries to avoid touching the original text node and indent. It adds a new text node before the existing one and inserts the new element between the text nodes. The nice remove code then removes this extra text node on undo. This approach seems to get the JUnit to run green. It seems to make some sense but I'm not sure it covers all scenarios. While it doesn't affect the logical structure of the WSDL document, it has the potential to affect other model adopters if they expect certain white space formatting. I'll attach the patch shortly for you to give it a try with your editor.
An alternate approach would be to provide WSDL model adopters with an option to disable the formatting code path if needed. I've been considering a few alternatives on how to pass such an option down to the WSDLElementImpl class:
a) a static field in WSDLElementImpl - ugly global flag, would affect all WSDL models
b) a resource set level load option, that can be queried using eResource().getResourceSet().getLoadOptions().get("...")
c) a custom eFlags bit at the Definition level - requires quite a few code changes
d) an Eclipse preference - you'd need a product level preference
e) a Java System property - similar to d, but would require a JVM parameter
I'll attach a sample patch for option b) for now.
Any of these alternatives (a - e) will require you to format the new markup to keep the text representation readable and will have some sort of non-zero performance impact by checking the flag. Some are more involved than others, adding new methods or fields, which is typically not accepted in maintenance releases. Whatever we do come up with for the maintenance stream would have to be very safe to be accepted by the PMC.
Have a look at the patches, give them a try and let me know what you think.
Created attachment 173824 [details]
Alternative 1
Modifies the nice insert code to avoid touching the existing text node.
Created attachment 173825 [details]
Alternative 2b
Here's a patch that uses a resource set load option to disable the indent code path entirely.
You'll need to add this line of code when you set up your resource set:
resourceSet.getLoadOptions().put("FORMAT_ON_RECONCILE", Boolean.FALSE);
Hi Valentin, Thanks for the fixes. I have tested both of them, and they work fine, but only if I apply them also in org.eclipse.xsd.impl.XSDConcreteComponentImpl. Unfortunately I faced that absolutely the same issue is introduced in XSD API in the class org.eclipse.xsd.impl.XSDConcreteComponentImpl. The issue with this class is absolutely the same as the one in WSDLElementImpl. Your fixes work perfectly for XSDConcreteComponentImpl as well. Can you apply the fixes also in XSDConcreteComponentImpl? Some feedback for the fixes: 1. Its OK to apply any of them on your opinion. 2. If you decide to apply 2b, can you extract the constant "FORMAT_ON_RECONCILE" as a field in any class? Otherwise, my concern is that in the future releases it could dissapear somehow. Best regards, Dimitar Hi Dimitar, thank you for testing the patches. I do not own the XML Schema EMF model. If you need a fix for it please open a bug here https://bugs.eclipse.org/bugs/enter_bug.cgi?product=MDT, selecting the XSD component. As for the hardcoded option name, I cannot add a constant in the maintenance stream. I have a comment in the patch to remind me to do that in the head stream (// TODO Create a public constant in WSDLResourceImpl in the HEAD stream.) Since you need both the WSDL and XSD EMF models to work for your editor to work, I recommend postponing the fix until Helios SR1 / WTP 3.2.2. This would give us more time to test and perhaps come up with better solutions. Hi Valentin, I have created bug 319816 for the same issue in XSD API. Best regards, Dimitar I see that Ed looked at this problem too in bug 319816, and that there may be more to this issue than meets the eye. In particular, the scenario he explains in comment 5 - white space changes inside elements tags and perhaps the attributes order changing when removing XML elements through the model and then undo-ing the changes. You may want to review comment 14 which explains how the WTP WSDL and XSD have solved this problem by only using the text editor's undo/redo stack. Given that the WSDL and XSD models are very similar in design and code, I am going to match Ed's conclusion here and convert this to an enhancement. |