Community
Participate
Working Groups
If the new java class wizard is executed against a project and the project does not have any packages yet then the package field should be prefilled with the named of the project. Reproduce. Create an empty project go to the source folder select new class the package field is empty and it requires a lot of work to fill it in with the name of the project, which should, by convention, be the name of the top level package.
Good idea, we should do that if the project name is a valid package name.
This should be done for all type and package creation dialogs (and only if there's no better package context available, of course).
Created attachment 225201 [details] Patch
The patch has the following behavior: - if the source folder contains no package (excluding the default package) then we use the project name (only if the project name is valid i.e. has no errors or warnings, otherwise the package field is left blank). - if the source folder contains exactly one package then we use that package name. - if the source folder contains more than one packages then we leave the package field blank for the user to choose the required package.
(In reply to comment #4) > - if the source folder contains more than one packages then we leave the > package field blank for the user to choose the required package. If you already have all the packages in hand, maybe it is feasible to provide a drop-down dialog that contains all the packages?
(In reply to comment #5) > (In reply to comment #4) > > > - if the source folder contains more than one packages then we leave the > > package field blank for the user to choose the required package. > > If you already have all the packages in hand, maybe it is feasible to > provide a drop-down dialog that contains all the packages? We already have content assist for the packages that exist and the patch covers the case where the project name is pre-filled. I don't plan to invest more here.
(In reply to comment #6) > covers the case where the project name is pre-filled. I don't plan to invest > more here. Fair enough. Trying to get another slice of salami. Thanks for adding this guys.
Patch is almost good. Some issues: - I don't like that the code which guesses the name already messes with the status and sets internal state. Instead, simply validate the name and if it is valid, set the package. The code will later update the status automatically. With your patch the same update happens twice. - The check whether the source folder contains exactly one package should ignore empty parent packages. Test with "foo.bar" as project name.
Created attachment 225368 [details] Patch Thanks Dani. I have made the changes. Please review.
The Package Wizard now suggests the already existing package name even if the project or source folder is selected or there are other packages already. The behavior of the Type Wizard now looks OK but there's some polish required: - getPackage(...) - says 'elem' can be 'null' but that would cause an NPE - "elem.getJavaProject()" is there three times and 'elem' not used otherwise ==> change the parameter to be the Java project (don't allow 'null') - "if (packages != null)" is not needed - 'froot' is a bad name. Sorry that slipped through the first review. We use camel-case (e.g. 'packageFragmentRoot' or 'pkgFragmentRoot'). Also note that 'fRoot' would also be bad for a local variable since 'f' denotes a filed according to our naming guidelines. - if noOfPackages > 1 the loop can be exited - "length is 1 -> " I would remove that since it's clear from the 'if' already - The comment "// evaluate the enclosing type" should be moved down a bit. Otherwise the patch is OK.
Created attachment 225410 [details] Patch Polished the things suggested by Dani.
Comment on attachment 225410 [details] Patch (In reply to comment #11) The first paragraph of comment 10 is still not fixed :-( getPackages(...) - "cannot be <code>null</code>" is not needed as this is the default. We only have to document if 'null' is allowed - if noOfPackages > 1 the loop can be exited: better now, but even better than breaking out of the loop we can directly return. This makes the code easier to read (one can ignore the code that follows)
Created attachment 225551 [details] Patch
Fixed the first paragraph of comment 10. Now the Package Wizard does not suggest the already existing package name on selecting the project or source folder and no suggestion is made if the source folder already contains other packages. Also, polished the getPackages(...) based on the comments. Dani, please review and let me know the comments.
Comment on attachment 225551 [details] Patch (In reply to comment #14) > Fixed the first paragraph of comment 10. > Now the Package Wizard does not suggest the already existing package name on > selecting the project or source folder and no suggestion is made if the > source folder already contains other packages. > > Also, polished the getPackages(...) based on the comments. > > Dani, please review and let me know the comments. Thanks the behavior is now fine. What I now don't like is that we have two methods which almost do the same thing but have different names: #packageChanged() and #getPackageStatus(packName). I would either remove the former, or also move the following line into #packageChanged: fPackageStatus= packageChanged() so that it is indeed different. The @return description in is not well understandable: the package fragment to be pre-filled in this page or null if it is not applicable If one just read that sentence, then one does not know what "if it is not applicable" means. Also note, that we always put keywords like 'null', 'true', 'false' into <code>...</code>.
(In reply to comment #15) > The @return description in is not well understandable: ... in #getPackage
Created attachment 225558 [details] Patch Thanks for the review comments Dani. I have removed the method packageChanged(). Also, updated the @return statement and added @since in getPackage(...) as suggested by you.
Comment on attachment 225558 [details] Patch Thanks for the final patch Noopur. Committed your patch with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d473e3cfdf27b69a0689587fb0054f40a2643ea7
.
