Bugzilla – Full Text Bug Listing
| Summary: | Formatting a JSP file, containing Java snippets, leads to misformatted or even lost code | ||
|---|---|---|---|
| Product: | [WebTools] WTP Source Editing | Reporter: | Nicholas Wright <cs94njw> |
| Component: | jst.jsp | Assignee: | Nitin Dahyabhai <thatnitind> |
| Status: | VERIFIED FIXED | QA Contact: | Nick Sandonato <nsand.dev> |
| Severity: | critical | ||
| Priority: | P3 | CC: | cbridgha, neil.hauge, raghunathan.srinivasan, thatnitind |
| Version: | 3.3.1 | Flags: | thatnitind:
pmc_approved?
(david_williams) raghunathan.srinivasan: pmc_approved+ thatnitind: pmc_approved? (naci.dai) thatnitind: pmc_approved? (deboer) neil.hauge: pmc_approved+ thatnitind: pmc_approved? (kaloyan) cbridgha: pmc_approved+ nsand.dev: review+ |
| Target Milestone: | 3.2.5 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | PMC_approved, WI81015 | ||
| Bug Depends on: | 361606 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Nicholas Wright
Created attachment 203826 [details]
When Format is run, code is lost and badly formatted to cause errors.
Created attachment 203827 [details]
When Formatted, a small piece of code is formatted so it errors, but no code is actually removed.
FormattingStrategyJSDT appears to be the strategy applying the problem changes. Created attachment 205375 [details]
smaller test file
Created attachment 205376 [details]
didn't generate properly
Running either the JSP-Java or Client-side JavaScript formatting alone doesn't cause an error, but running them together when there's overlap causes some contents to be lost. The simplest fix I can see is to make sure the JSP-Java formatting constrains itself to only create TextEdits for the areas contained in the partition it's asked to format. The Client-side JavaScript formatting already does this, and while it's a mess, a rewrite I tried doesn't change the behavior in a way that fixes this bug.
Unit tests attached to check for data loss after formatting, but some tests in the org.eclipse.jst.jsp.ui.tests.format.TestContentFormatter suite still need updates for the indentation changes that were made to formatting during 3.3, and it needs to be added back into the main JSP UI suite.
Many thanks for the original file,...cs94njw.
Opened bug 361195 for a lingering indentation issue. Created attachment 205454 [details]
patch 2
The SSE side of things looks good, though I noticed a weird exception. It only ever happened once, though. org.eclipse.wst.sse.core.internal.NotImplementedException: intentionally not implemented at org.eclipse.wst.sse.core.internal.text.MinimalDocument.getLineOfOffset(MinimalDocument.java:216) at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.translateJSNode(JsTranslator.java:604) at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.translate(JsTranslator.java:321) at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.reset(JsTranslator.java:275) at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.run(JsTranslator.java:736) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) Yeah, that is weird. Adding the explicit cancel() call during release() removed most of those reported traces, but I see that one occurrence pop up irregularly, as well. Retargetting to 3.2.5, as it happens there as well.
*Explain why you believe this is a stop-ship defect.
Cuases loss of data.
*Is there a work-around? If so, why do you believe the work-around is insufficient?
No workaround other than to not have scriptlets in client-side scripting areas.
*How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
Manual, plus added and updated JUnits.
*Give a brief technical overview. Who has reviewed this fix?
When formatting a JSP, the Platform API invokes a corresponding slave formatter for each partition in the text document. Each formatter applies its changes right then, but the formatter for JSP Java scriptlets was not constraining itself to only making changes in the partition it was given during that call. The Client-side JavaScript formatter was also not reliably getting its translation in JSP files, leading to those sections not being formatted properly.
*What is the risk associated with this fix?
Low. The JSP-Java formatter is now using the same range limiting options when calling the JDT formatter as the Client-side JavaScript formatter has been using when calling the JSDT formatter (which itself is a fork from the JDT one). Also, the unit tests specifically look for lost content when formatting the reporter-supplied attachment 203826 [details], as well as with the prior formatting unit tests.
approve - sounds like a bad problem, and has good test coverage Released. Opened bug 361553 for 3.3.x/3.4. Verified with M-3.2.5-20111021173028 |