Bugzilla – Full Text Bug Listing
| Summary: | ClasspathDependencyWizardFragment.ClasspathPushupLabelProvider.getText() doesn't handle non-container classpath entries, wizard enablement error | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Larry Isaacs <larryisaacs> | ||||||
| Component: | jst.j2ee | Assignee: | Rob Stryker <stryker> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | ccc, david_williams, neil.hauge, raghunathan.srinivasan | ||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
raghunathan.srinivasan: pmc_approved+ cbridgha: pmc_approved? (naci.dai) cbridgha: pmc_approved? (deboer) neil.hauge: pmc_approved+ cbridgha: pmc_approved? (kaloyan) cbridgha: review+ |
||||||
| Target Milestone: | 3.2 RC4 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | PMC_approved | ||||||||
| Attachments: |
|
||||||||
Assigning to Rob for initial investigation. Please retarget as appropriate. Moving this out to WTP 3.2.1, since WTP 3.2 is shutting down. Interestingly, this class was only intended for EAR projects to use, not Web projects. I really like the patch, but unfortunately it won't actually make the web project 'consume' these selected jars. ugh, so many intersecting and criss-crossing use cases make this quite difficult. Currently, this reference container is nothing more than a UI stub which toggles the .classpath attribute that says "push me up into a parent". However currently only EAR projects check their children to do this. I really like this patch. I would like to nominate this for approval and then PMC approval. I'll answer all questions just to make sure I didn't leave anything out ;) I've modified the patch to fix another finish enablement issue I discovered which is also critical.
a) Be clear on which release you want this bug to be fixed in, or "last workable" date, if requesting a patch. .
I would like to see this in this week's build, and at the very least 3.2 final.
* 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.
A user using the page to modify which classpath entries get pushed into an EAR project (not the use-case above, but one very affected by this patch) may see that some or all of the classpath entries may not have descriptions other than a toString(). (specifically, non-container entries will behave this way). If the user desires to select just one or two of them, they have no way to know which classpath entry they are actually selecting. Also it makes our new page look unprofessional with uninformative toStrings() everywhere.
Furthermore, in the EAR case, assuming the user has one entry "checked" before entering (editing) the entries, and the user unchecks that box, "finish" is not enabled. The code was looking at whether any new entry was checked, when it should have been looking at whether any check state has CHANGED.
* Is there a work-around? If so, why do you believe the work-around is insufficient?
There is no real work-around to knowing what the details for the classpath entry are when provided only a toString(), however if you play around with the tooling long enough, some of the entries will "resolve" and a string will show. There is no workaround to the check / uncheck situation other than hand-modifying the .classpath file.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
This is manually tested by Larry and also verified by myself.
* Give a brief technical overview. Who has reviewed this fix?
I've reviewed this, Larry has proposed it, and I think it's good. I believe Chuck will also review this. The technical overview is that in the case of a non-container entry, a better string is returned. The wizard finish state is now fixed due to looking for changes instead of 'checks'
* What is the risk associated with this fix?
I see minimal room for risk here in this patch.
Created attachment 170767 [details]
Larry's patch with also the 'finish' enablement corrected
Hate to obsolete your patch, Larry, but I've added the enablement stuff =/
clearly mistargeted forgot to add the review. doh! I approve... this is relatively isolated, and tested from a few adopters.. Adding PMC flags.... Rob - please answer any questions from the PMC folks.... clearly missed the target twice! Setting it to 3.2 RC4 I think I recall a similar problem in WTP 1.5! And it does look pretty bad ... and can "prevent use" so close to "major" (loss of function). I can't follow the patch ... didn't apply and look at full code ... but looks pretty complicated logic. So, good when you say "tested by several adopters". Thanks for catching, fixing, and testing. We have 3 +1's and a component lead +1. Is it suitable to commit this now? I know there are deadlines ;) (In reply to comment #12) > We have 3 +1's and a component lead +1. Is it suitable to commit this now? I > know there are deadlines ;) Yes, you can commit. I did the commit right after Raghu's comment but forgot to close the issue. Closing it now. |
Created attachment 167028 [details] Possible patch to display non-container classpath entries more reliably The ClasspathDependencyWizardFragment.ClasspathPushupLabelProvider.getText() assumes all classpath entries are "containers", which can cause somewhat quirky behavior because only actual classpath containers will trigger appropriate classpath container initialization. The following scenario shows this for me: 1. In a new workspace, create a Dynamic Web Project and a Utility Project. 2. In the Utility Project, go to the Libraries tab of Java Build Path page and use the "Add Variable..." button to add JUNIT_HOME/junit.jar to the build path. 3. In the Dynamic Web Project, go to the Deployment Assembly page and click Add. 3. Select "Referenced Projects Classpath Entries" and click Next. In the wizard page that appears, the "jar" displayed as the child of the Utility Project will appear as something like "org.eclipse.jst.j2ee.internal.ui.preferences.ClasspathDependencyWizardFragment$WrappedClasspathEntry@1d2b2f9". This results because no classpath containers have been initialized, and the addition of a "variable" classpath entry does nothing to kick the initialization. Eventually, possibly to getText()'s failed attempt to get a classpath container, initialization does take place and JavaCore.getClasspathContainer() conveniently returns a non-null container with a reasonable description (I believe for error handling purposes). For non-container classpath entries, using the entry's path seems a better alternative. The patch currently uses entry.getPath().toOSString(). I think this makes the most sense for entries for external jars or folders, and offers consistency with respect to entries which are not external. Feel free to update or replace the patch if a different string for the non-container entries is deemed more suitable.