Community
Participate
Working Groups
Created attachment 176186 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java, and JavaFacetInstallConfig.properties Investigation: There is no validation preformed on the Default output folder String. Proposed patch: Adds a validateDefaultOutputFolder() method to the installconfig file. Calls the new validation as the user enters input. Prohibits finish and displays error status if the provided path is invalid against IWorkspace.validatePath. Prohibits finish and displays a new NLS message from the installconfig.properties file if the provided path is empty. To reproduce: -Open any EE wizard (in this case, dynamic web project), -Go to the java facet page, -Delete the default output folder -Finish the wizard. The following exception is shown and the projects are note created correctly... Failed while installing Java 1.6. java.lang.ClassCastException: org.eclipse.core.internal.resources.Project incompatible with org.eclipse.core.resources.IFolder
The case is valid, but the patch needs some more work... 1. The new validation logic should be inside the main validate method on JavaFacetInstallConfig, not in a separate method. 2. The approach used to show the error in response to changes in the text field is not correct (the reportError method). Take another look at that code and follow the practice used there already. I don't have the code in front of me, but there should be something like refreshValidation() that you can call from the event handler.
I am unclear on your comment about following practices of any existing refreshValidation type architecture, I was unable to locate such a method, I would appreciate it if you could point me in the right direction. Also, while investigating solutions to address your comments, i discovered another error case: -If the provided Default path is a path nested withing a source folder path an error is thrown: [Cannot nest output folder 'Ejb/source/out' inside 'Ejb/source'] As the code to validate this situation is so similar to some of the source folder validation, it would seem the validation code could be shared between them.
So you understood the bit about all validation being accessed via JavaFacetInstallConfig.validate() method? The only reason there is a public method factored out for validating source paths is that we wanted to access that logic specifically from the popup dialog used for adding new source paths. The key is that the wizard page banner is the place to display all of the page's validation. It shouldn't be accessed by individual fields directly. The information should flow like this: user change in ui -> change in model -> event -> listener -> call validate method and update the wizard banner Note that the text field change listener should just update the model. It shouldn't try to update the wizard banner. Typically, a wizard page would already have (change in model -> event -> listener -> call validate method and update the wizard banner) part written. Oddly enough, this doesn't seem to be the case for JavaFacetInstallPage. Fortunately, it is easy enough to write. If you take a look at JavaFacetInstallConfig, you will see a listener infrastructure. Then look in JavaFacetInstallPage for bindUiToModel method. Add a model listener in this method that will refresh the validation result in response to change in any model property.
Created attachment 176469 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java, and JavaFacetInstallConfig.properties Konstantin, Thanks for the input and suggestions, I have addressed your previous points in a new proposed solution. New patch proposal: -Added an listener to the Wizard page, -Calls validate() when InstallConfig changes a property -Disallows finish if there is an error status -Changed Default output folder validation methods, -Accessed only by calls to validate() -Changed visibility to private -Added validation checks for an Output path nested into a Source path -Added another NLS message to report on the new nested path error Note: As the full validate() method is called by the new listener, this configuration calls all source folder validation as well as checking the output path against each existing source folder on each user keystroke as a new output path is entered. I have considered using a flag in InstallConfig to determine if the list of current source folders has been validated against one another, thereby preventing unnecessary existing source folder validation checks, but was unsure if this follows best practices.
The second versions looks much better. I will proceed with integration.
Created attachment 176744 [details] Patch v3 On a closer look, the previous version of the version failed to account for a number of scenarios. In particular, it only covered one of the two possible nesting relationships between source and output folders. Both are invalid. It also did not add the new validation logic in such a way that it would be accessible via early validation in "add source path" dialog. The attached patch resolves these issues and is ready to be released. Will raise for PMC approval due to changes to NLS strings.
Requesting PMC approval of this fix for 3.2.2 release. NLS strings have been added and some existing ones have been changed. This is a continuation of key validation work started in 3.2.1 release.
looks good
Released patch v3 to WTP 3.2.2, WTP 3.3.0 and fproj 2.0 code streams.
