Bugzilla – Full Text Bug Listing
| Summary: | [explorer] Expanding a JS file in a Library in the Project explorer results in no children displayed under node | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] JSDT | Reporter: | Ian Tewksbury <itewksbu> | ||||||||||
| Component: | General | Assignee: | Ian Tewksbury <itewksbu> | ||||||||||
| Status: | CLOSED FIXED | QA Contact: | Nitin Dahyabhai <thatnitind> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | cmjaun | ||||||||||
| Version: | 3.2 | Flags: | cmjaun:
review-
thatnitind: review- thatnitind: review+ |
||||||||||
| Target Milestone: | 3.2.2 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 174100 [details]
Sample Project
Created attachment 174102 [details]
Fix Patch
Fix patch as described in the description. Passes smoke test of project explorer.
Rejecting this first patch; the blind casting to IJavaScriptUnit after also testing if "next" were an IClassFile is a problem waiting to happen. (In reply to comment #3) > Rejecting this first patch; the blind casting to IJavaScriptUnit after also > testing if "next" were an IClassFile is a problem waiting to happen. Opps...I read right passed that. Good catch Nitin. (In reply to comment #4) > (In reply to comment #3) > > Rejecting this first patch; the blind casting to IJavaScriptUnit after also > > testing if "next" were an IClassFile is a problem waiting to happen. > > Opps...I read right passed that. Good catch Nitin. Ditto. And in my attempt to fix the problem I seem to have found another. Which is weird because I could have sworn this was working perfectly yesterday. But some how by creating and releasing the working copy I am getting into some infinite loop that I can't figure out. Okay then....can we do something to just make sure we aren't getting any log errors and we can try to fix the node expansion in a future release. (In reply to comment #6) > Okay then....can we do something to just make sure we aren't getting any log > errors and we can try to fix the node expansion in a future release. Unless we would want to just totally ignore that exception we would have to do a bunch of casting and instance of checking to then determine the package root and see if its source or not and if its not then ignore the error. The real problem here is there is no such thing as "binary JavaScript" its an idea that was copied over from JDT when JSDT was first created and unfortunately it still sitting around. In this case the specific issue is in CompilationUnit#validateCompilationUnit. For a file in a library the returned status is fail because the package root is not source its "binary". Amazingly enough if you just get rid of this check everything just works. But I am not sure of the far reaching implications of doing that. As for my original fix, at this point I don't know why it appeared to work for me yesterday but today there is an issue. I must have had a patching issue when testing. Anyways, the problem with the original patch was that every time you create a working copy an ElementChangedEvent gets sent out that the PackageExplorerContentProvider then hears and decides it needs to refresh, in the course of refreshing it creates a working copy, which then sends out an event which the PackageExplorereContentProvider hears....I think you get the picture. So In an effort to get the patch working I updated the PackageExplorereContentProvider#processDelta to return false for IJavaScriptElementDelta#F_PRIMARY_WORKING_COPY events. But alas I hit another problem. The label provider started erroring for the same reason as the PackageExplorerContentProvider, a file in a JS library is neither a working copy nor in a source package root so the CU wont build the structure. It is at this point that I realized I could probably run around patching up all of these wholes trying to work around this requirement of CompliationUnit#buildStructure that it must be a working copy or in a source package root, or I could just get rid of the source package root requirement. This brings me back to the beginning of the discussion. This requirement for the CU to be in a source package root has been there since JSDT was committed to CVS. My question is, is there a reason. More precisely, is there any reason we can't remove this requirement? Is there any reason the CU should care if it is in a source or "binary" package root? It still has all of its other checks to be sure the file is openable and such. In the broader scheme of things the whole idea of "binary" JS needs to be removed, but that is definitely not possible for the 3.2.1 time frame. Any thoughts on removing this package root check for 3.2.1? I have created Bug 319763 to silence the unnecessary logged error message. This bug will remain open to track actually fixing it so that files in JS libraries can be expanded to show their packages and classes. Created attachment 174538 [details]
Possible Fix Patch
My suggestion for a fix is to remove the requirement that a resource be in a source package root for the CU to be valid. This fixes the problem and I can think of no reason that a JS resource in a "binary" root can't be a valid CU.
Nitin thinks it maybe a sound idea but would like to look into the idea more himself first.
Will have to allow it unless the package fragment roots are made to check with each other's exclusion patterns since overlapping library and source folders are now explicitly allowed. Committed to 3.2.2 and HEAD. Thanks, Ian. Verified in latest 3.2.2 builds. |
Created attachment 174099 [details] Console Log STEPS: 0. Import attached zip and skip to step 6 1. create a static web project 2. create a folder 3. mark that folder as a JavaScript library folder 4. create a file in that folder with some classes in packages in it 5. copy the file into the WebContent folder 6. attempt to expand both the file in the library directory and in the web content directory in the project explorer RESULTS: 1. the file in the web content directory expands fine showing all the packages and classes in that file 2. the same file expanded when it is in a library folder has nothing listed under it and an error is logged to the console (attached). CAUSE: CompolationUnits for files in libraries will not create a working copy of themselves unless explicitly told to. This is a problem because the CU has to be a working copy in order to get its children to build the results to show under the expanded file. THE FIX: Check to see if the requested IClassFile or IJavaScriptUnit has a working copy before attempting to get its children, if it doesn't then explicitly create a working copy, get the children, and then dispose of the working copy.