Community
Participate
Working Groups
There are several tests missing in the NewJavaClassConstructor#getStatus() method. What do we report if the project is not a Java Project? Is this okay? Should we just turn it into Java Project (I'm leaning toward "no") Need to check that the packageName is valid. getStatus() might also report warnings (or info) along the lines of "The project does not exist; it will be created." I think this could be a handy addition. If we do this, we'll have to change the users of this method. All senders of getStatus() currently check for isOk(); these senders will have to instead check to see if the response represents an ERROR status.
I'll work on these things.
It looks like all this status stuff has been solved by the NewElementWizardPage. So I'm going to refactor NewLiteClassCreationWizard and break out the WizardPage class so that it extends NewElementWizardPage.
I'd rather you don't. NewJavaClassConstructor should do the validation work, not the UI. The NewJavaClassConstructor was created to provide a separation between the UI and the code that does the actual construction work. This will allow us to use the NewJavaClassConstructor in other contexts; e.g. with other UIs. If you put the validation logic in the UI, then that reuse becomes impossible.
I agree with you. The validation work will stay in the NewJavaClassConstructor. Just the "dealing with the status object" in a good way (i.e. putting the message up in the header and deactivating the Finish button) is in the UI obviously and that has already been handled by the NewElementWizardPage. It seems like we should reuse it rather than recreate this functionality since this class is part of the API and is designed to be subclassed. I'll see how well it works and submit a patch if it works well.
Created attachment 157730 [details] Improved status checks and displaying of status There are a couple more status checks in the NewJavaClassConstructor. One checks that the project that the new class is to be created in is a Java project. The other checks that the project exists and if not throws an INFO status with some useful information for the user. Also, I had to make some changes to the NewLiteClassCreationWizard so that it displays INFO status messages properly.
Created attachment 157731 [details] Tests for the improved status checks Just some more unit tests to test the new status checks.
I'm going to add some checks for packageName.
Created attachment 157739 [details] Added validation checks to NewJavaClassConstructor In addition to changes in previous patch, I added status checks for duplicate class name and checks for validity of packageName. This patch can be used instead of the previous one. Also, needed to make a small change to the Wizard so that the message didn't get stuck on an error message.
Created attachment 157740 [details] Tests for added validation checks on NewJavaClassConstructor Just a couple of small changes in addition to the new tests for the last version.
I think these patches leave the constructor in pretty good shape for our simplified purposes. There are a bunch of other checks that we could add later from the NewContainerWizardPage.containerChanged() method that deal with all kinds of unlikely scenarios. It would be nice to override the error messages that are returned from JavaConventions.validateJavaTypeName(). They all refer to problems with the 'type' name rather than the 'class' name which is kind of confusing. But I don't think this is easy to do since it uses so much internal code. We'd have to copy over a whole bunch of code to fix this. Or we could hack it for the English language messages by intercepting them, interpreting which message got triggered and changing them. The main thing to address perhaps is the package creation and verification.
I've applied the latest patches and will mark them as iplog+. But we're not 100% there yet... Need to add a check for the package name as well. I'll leave the bug open until this is addressed.
Created attachment 159367 [details] mylyn/context/zip
(In reply to comment #11) > I've applied the latest patches and will mark them as iplog+. But we're not > 100% there yet... Need to add a check for the package name as well. I'll leave > the bug open until this is addressed. If the patch for Bug 303426 does get applied, it fixes this problem.
