Bug 319855

Comment 1 Ian Tewksbury 2010-07-14 10:02:24 EDT

First a correction, in my description I said create a SWP and an HTML page, I totally meant create a DWP and a JSP.

------------

Right now when formatting the order of operations is to:

1. translate the HTML doc into a JS CompilationUnit
2. create the formatting edits needed on the CU
3. get the JS text from the translator
4. apply the edits to the JS text
5. replace the script region in the HTML document with the JS text that had the formatting edits applied

The issue (as mentioned in the description) is that the JS text retrieved from the JSTranslation contains this generated text and thus when it is injected back into the HTML document the generated text goes with it.

I have two different approaches to fix this issue, one dealing with it right in the FormattingStrategyJSDT#format method with no need for new API (other then making a method go from package protected to public) or what I think is the better solution to update IJsTranslator and IJsTranslation with a method to allow the user to ask for the JS text either with or without these generated place holders.

In either case the actual fix is the same, its just where the work takes place that is different.  In both cases the approach is to use the already recorded JsTranslator#fGeneratedRanges to go through and re-replace the generated text with the original text using the source HTML document.

In the case of the solution done in FormattingStrategyJSDT#format, JsTranslator#getGeneratedRanges needs to be made public and then in the #format method a bunch of casting needs to take place to get access to that method, then iterate over the regions replacing the text so the steps become:

1. translate the HTML doc into a JS CompilationUnit
2. create the formatting edits needed on the CU
3. get the JS text from the translator
4. replace the generated text with the original text
5. apply the edits to the JS text
6. replace the script region in the HTML document with the JS text that had the formatting edits applied

In the case of adding new API, to both IJsTranslation and IJsTranslator I would like to add the following method signature:

public String getJsText(boolean pureJS);

and mark the old method as depreciated:

public String getJsText();

The point of this new method signature would be that a caller can specify whether they want the JS text with these generated place holders making the returned text "pure JS" or if they want any text in the JS regions of the HTML as it was, so without these generated regions making the returned text not necessarily "pure JS".  With this new api all the #format method has to do is call this new API making the steps:

1. translate the HTML doc into a JS CompilationUnit
2. create the formatting edits needed on the CU
3. get the JS text from the translator using new API requesting the JS text without replacments
4. apply the edits to the JS text
5. replace the script region in the HTML document with the JS text that had the formatting edits applied

I have created both solutions because this maybe something that should go into 3.2.1 and in that case I assume we could not go adding the new API, but we could "patch" it for now and then put in the new API in a later release and revert the "patching" done in #format.

-----

In either solution another minor fix is needed to JsTranslator#translateJSNode.

Currently the following:
<script type="text/javascript">
var i = <jsp:output></jsp:output>;
</script>

Will result in an internal translation of
var i = _$tag_______</jsp:output>;

When it should be:
var i = _$tag________$tag________;

The side affect being when attempting to format:
<script type="text/javascript">
var i = <jsp:output></jsp:output>;
</script>

Nothing happens because the translation is invalid because it still contains the server side tag.

The problem is an off by one error in JsTranslator#translateJSNode.  There is a +1 when there should be one when calculating where to start looking for the next match when looking for tags in the JS region.

I guess this problem COULD be moved off into a separate bug, but without both these fixes neither one has any use to the user so I am currently supplying this minor fix in with both of my suggested patches for this problem.

------

Patches to follow.

Comment 2 Ian Tewksbury 2010-07-14 10:03:52 EDT

Created attachment 174293 [details]
Fix Patch - No New API

Fix patch without any new API

Comment 3 Ian Tewksbury 2010-07-14 10:04:21 EDT

Created attachment 174294 [details]
Fix Path - With New API

Fix patch with new API

Comment 4 Ian Tewksbury 2010-07-14 13:08:42 EDT

Created attachment 174318 [details]
New JUnit

Attaching a new JUnit to test the case of:

<script type="text/javascript">
var j = "<div>";
</script>

Testing the JSP server side tag case is blocked by Bug 313764 I will add a note there about adding in a test for this when that is complete.

Also it turns out the FormattingTests class was a bit broken because without a viewer the extended formatters are not loaded, and in this case the FormattingStrategyJSDT is added by extension point.  Therefor most of the changes in FormattingTests is to set up the tests so they have a viewer to use when requesting the formatter.

Comment 5 Chris Jaun 2010-08-11 14:51:35 EDT

This is what I see after a couple formats, undo's, and re-formats...

<script type="text/javascript">
	var i = p:output> </
	sp:output>;
;
	var j = "v>";
"
</script>

Comment 6 Ian Tewksbury 2010-08-19 11:08:08 EDT

(In reply to comment #5)
> This is what I see after a couple formats, undo's, and re-formats...
> 
> <script type="text/javascript">
>     var i = p:output> </
>     sp:output>;
> ;
>     var j = "v>";
> "
> </script>

I can not reproduce this.  I tried formating, then undo-redo-undo-redo, formating-undo-formating-redo-undo-formating, etc.

Also I could not reproduce it when combining it with the patch for Bug 321507 which both effect the same area of code.

Comment 7 Ian Tewksbury 2010-08-19 11:21:01 EDT

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

Comment 8 Ian Tewksbury 2010-08-19 11:22:34 EDT

Created attachment 177012 [details]
Fix Patch - Update 1

This patch is the same as the "no new api" patch plus the fix for Bug 321507 because they both touch the same bit of code.

Comment 9 Ian Tewksbury 2010-08-19 11:23:48 EDT

Created attachment 177013 [details]
New JUnits - Update 1

Here is the combined JUnits of this bug and Bug 321507

Comment 10 Ian Tewksbury 2010-08-31 08:33:06 EDT

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

Comment 11 Chris Jaun 2010-09-01 11:38:26 EDT

Checked into HEAD and 3.2.2

Was reviewed and approved as part of https://bugs.eclipse.org/bugs/show_bug.cgi?id=209738

Comment 12 Ian Tewksbury 2010-09-13 13:39:57 EDT

Verified with latest 3.2.2 driver.