Bugzilla – Full Text Bug Listing
| Summary: | Need to update JS include path after module core nature installed | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] JSDT | Reporter: | Ian Tewksbury <itewksbu> | ||||||||||||||||||
| Component: | Web | Assignee: | Nitin Dahyabhai <thatnitind> | ||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Nitin Dahyabhai <thatnitind> | ||||||||||||||||||
| Severity: | major | ||||||||||||||||||||
| Priority: | P1 | CC: | cmjaun, david_williams, kaloyan, neil.hauge, raghunathan.srinivasan | ||||||||||||||||||
| Version: | 3.2.3 | Flags: | david_williams:
pmc_approved+
thatnitind: pmc_approved? (raghunathan.srinivasan) thatnitind: pmc_approved? (naci.dai) thatnitind: pmc_approved? (deboer) neil.hauge: pmc_approved+ kaloyan: pmc_approved+ thatnitind: review+ cmjaun: review+ |
||||||||||||||||||
| Target Milestone: | 3.2.3 | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | PMC_approved WI59838 WI64107 WI68511 | ||||||||||||||||||||
| Bug Depends on: | 334172, 334922 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Ian Tewksbury
I have created Bug 333903 to track the needed changes in 3.3. Patch will be different there because it will add more descriptive strings. Created attachment 186419 [details]
Fix Patch
This patch uses an implementation AbstractIndexManager to be sure no resource change events are missed looking for a change to the .project file. In the case that a .project file changes the patch checks if the module core nature is there, if so it updates the class path if needed. The "if needed" is determined by adding a property to any default class path entries that are added due to module core originally not being present, these default entires will be removed if module core is added and the class path updated.
Patch passes all existing JUnits.
I don't love the placement of the class path attribute name and value in ModuleSourcePathProvider or the placement of the logic for updating the class path in the new JSWebResourceEventManager so feel free to move these to any place that seems better.
Created attachment 187226 [details]
Fix Patch - Update 1
This patch updates the last patch such that any exclude or include paths on existing class path entries that will be removed by this action are brought forward to the new ones where appropriate.
Because the logic for moving exclude and include paths forward is a bit complex I have added a suite of JUnits to test that part of the patch.
Created attachment 187231 [details] Fix Patch - Update 2 I accidentally included the patch for Bug 334922 in my last patch for this bug. Approved, although an additional change is needed so that jsdt.web.core requires at least the version of sse.core in 3.2.3 (due to its use of the AbstractIndexer from there). * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. Installation of the JS facet attempts to set the project's web-app root as a source folder so that we're aware of the user's JavaScript files. On a web project, this is done by adding a source folder to the initial JS Include Path for the project for each root returned to use by the module core APIs. This is preferred as it keeps us from indexing and including copied JS files (in the bin or classes folders) in our model. If the order in which the JS facet and ModuleCore nature are installed is reversed, though, that doesn't happen correctly--the entire project becomes the initial source folder and any further plug-in-driven changes that expect to manipulate inclusion or exclusion paths may not do so correctly. * Is there a work-around? If so, why do you believe the work-around is insufficient? The user could fix this by hand, but as the initial source folder already includes the entire project, nothing will seem out of place unless they know what to look for. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Tested by myself and Ian, plus a new test suite is included. * Give a brief technical overview. Who has reviewed this fix? Reviewed by Chris and myself. This patch has us watch for the nature's installation into projects (or when our web core plug-in is eventually started) and adjust the source folder entries *we added before* to use the module core information. As this is only done on entries we've marked as generated by our facet installation, it doesn't affect anything else the user has done to the include path or unrelated projects. * What is the risk associated with this fix? Fairly low. Created attachment 188099 [details]
supplemental patch for manifest.mf change
This looks like a significant change being made late in the release cycle. That said, I approve based on the comment 2 that all junits pass Released. Patch overwrites all existing entries other than the default one being updated. Created attachment 188994 [details]
Fix for Fix
Chris found a logic error in how I was trying to prevent over-writing any entries that were not our default entries. This is my suggestion for fixing that error.
Created attachment 188995 [details]
patch to fix original patch
Created attachment 188997 [details]
patch update
Created attachment 189031 [details]
larger patch, but maintains include path order
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. It corrupts the .jsdtscope file within a user's flexible JavaScript project, meaning all web projects when their .project files are touched. The original processing of Include Path Entries stored the updated entries in a new list, but due to incorrect logic, only the entries it was looking for and entries with attributes on them were added to the new list. It also set an incorrect value on the output location entry (there should be an empty value) and always moved the entries it was meant to update to the end of the list. * Is there a work-around? If so, why do you believe the work-around is insufficient? There is no workaround that the user can take. Attempting to circumvent the problem can start the plug-in that registers the listener, and as a save participant it will process any files modifies since the last time it was active. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Manually tested repeatedly by myself and Chris, with different Facets and JS containers. * Give a brief technical overview. Who has reviewed this fix? The loop is fixed, the order of all entries is preserved, and the output location is kept to its previous, empty value. Chris has reviewed the changes. * What is the risk associated with this fix? Low. Chris and I both looked at the problem independently, came up with similar solutions, and agreed to use this one. I'm not extremely familiar with this area, but with stated risk being low, and this being seen as a P1 issue by project lead, I approve. Verified with 3.2.3-20110216062908 candidate . |