Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 316384

Summary:

[formatter] FormattingStrategyJSDT should be added by provisionalConfiguration

Product:

[WebTools] JSDT

Reporter:

Ian Tewksbury <itewksbu>

Component:

Web

Assignee:

Ian Tewksbury <itewksbu>

Status:

RESOLVED FIXED

QA Contact:

Nitin Dahyabhai <thatnitind>

Severity:

normal

Priority:

CC:

cmjaun, nsand.dev

Version:

3.2

Flags:

cmjaun: review+
nsand.dev: review+
thatnitind: review+

Target Milestone:

3.2.1

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Fix Patch	none

Description Ian Tewksbury

2010-06-09 17:16:28 EDT

Currently the FormattingStrategyJSDT is added by  StructuredTextViewerConfigurationJSDT#getContentFormatter the problem with this method is that it is none deterministic whether the StructuredTextViewerConfigurationJSDT or StructuredTextViewerConfigurationHTML will be loaded.  (This is a bit of a lie because with full knowledge of the code StructuredTextViewerConfigurationJSDT will currently always be loaded because it is associated with the editor id rather then the content type ID, but this means the JSDT formatting does not get associated correctly with JS in other editors but with the correct partition type)

The solution is to use the already existing (but not well known) provisionalConfiguration slaveformattingstrategy.  Providing the formatter through this means that whenever the script partition type shows up the formatter will be correctly applied.

Patch to follow.

Comment 1 Ian Tewksbury

2010-06-09 17:19:16 EDT

Created attachment 171579 [details]
Fix Patch

Fix patch to use provisionalConfiguration slaveformattingstrategy for both HTML and JSP documents and remove the overriding of getContentFormatter now that it is being done by extension point.

All existing JUnits pass and no new ones need be added because there is no user end functional change here just back end.  But even though this is a back end only change this is important so that if other contributors are using script regions in their documents they correctly pick up JS formatting for those regions.

Comment 2 Nick Sandonato

2010-06-09 18:31:05 EDT

I think you also need to add org.eclipse.wst.html.SCRIPT as a target for the JSDT JSP support. But other than that, I approve.

Comment 3 Ian Tewksbury

2010-06-10 07:52:20 EDT

(In reply to comment #2)
> I think you also need to add org.eclipse.wst.html.SCRIPT as a target for the
> JSDT JSP support. But other than that, I approve.

Its the same formatter class (FormattingStrategyJSDT) that gets registered for both the org.eclipse.wst.html.SCRIPT and org.eclipse.jst.jsp.SCRIPT.JAVASCRIPT region and because the provisionalConfigurations are not content type aware org.eclipse.wst.html.SCRIPT will be formatted in JSP documents due to the provisionalConfiguration in the HTML plugin.  If you notice all of the other provisionalConfigurations  in the JSP plugin all only register themselves on org.eclipse.jst.jsp.SCRIPT.JAVASCRIPT because HTML does the registering for org.eclipse.wst.html.SCRIPT.

Comment 4 Chris Jaun

2010-06-10 10:30:10 EDT

Well...I don't know much about how this stuff works, so if Nick says its okay, and you have done good testing, its fine with me.

Comment 5 Chris Jaun

2010-06-22 15:30:50 EDT

Fix checked into 3.2.1

Comment 6 Chris Jaun

2010-06-22 15:51:56 EDT

Fixed