Bugzilla – Full Text Bug Listing
| Summary: | Java Class Constructor class | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Wayne Beaton <wayne.beaton> | ||||||
| Component: | IDE4EDU | Assignee: | Project Inbox <ide4edu-inbox> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | milesbillsman | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 300672 | ||||||||
| Attachments: |
|
||||||||
I'll try working on this. Look at NewJavaProjectConstructor and the corresponding test case for ideas. Oh, and I've already created the NewJavaClassConstructor class. It's only a placeholder at this point. Cool. I've found the NewJavaClassConstructor and have started writing it using the NewJavaProjectConstructor as reference. I will take a look at the test cases as well. Thanks for the tips. Created attachment 157052 [details]
Version 01 - 2 ideas: constructor from scratch or wrapper for NewTypeWizardPage
So I've started on this and have made some progress. I've included 2 ideas I've partially explored and a couple simple JUnit tests for them.
The first idea is to follow Wayne's lead with NewJavaProjectConstructor and do everything from scratch. I started down this rabbit hole and this is a fairly large undertaking if we want it to do things like allow initial templates, extending superclasses, implementing interfaces, etc., basically a bunch of the features that are already implemented. These all seem like reasonable things for IDE4EDU to do since a first year Java course would cover object oriented programming and we want to allow templates to allow the student's to ramp up quickly. Furthermore if the idea is to create something that will work its way up into the main Eclipse code there's a lot of features that would need to be carefully transplanted from NewTypeWizardPage and added to this code or completely rewritten. Considering NewTypeWizardPage has 2500 lines of code this is a big job. On the other hand we could definitely do a cleaner job than NewTypeWizardPage does by weaving the wizard and class creation functionality together so tightly.
The second version creates a wrapper for NewTypeWizardPage by creating a private class that extends it and then internally using all of the public and protected methods to do our new class creation. This seems kind of promising but JUnit tests fail since it needs a Workbench up and running. It may be difficult to get NewTypeWizardPage to act solely as a non-GUI class. This is bad because it doesn't give us the nice separation that would be ideal.
Any thoughts would be welcome. I'm thinking that the first idea may be the way to go. Thanks.
Wrapping the NewTypeWizardPage is just wrong. This class should be completely non-visual. i.e. no GUI code at all. FWIW, I think the NewTypeWizardPage does it wrong: they mix up visual and non-visual code in such a way that makes reusing anything a PITA. I'd like to do it right with this implementation. Your first crack shows some promise. I'm getting some compile errors, however. I had set the Java version for the to J2SE-1.4; I assume you increased that to Java 5.0 in your workspace. It's reasonable to make this Java 5.0, so I'll fix that in the bundle manifest. Having said that, I think you may have Boolean and boolean mixed up. I think that you really mean boolean (small-b), the primitive type. At least that's how you're using it. Regarding your comment in the getJavaProject(...) method... you don't have to sort out which project is selected. This class should have no knowledge of any selection in the workbench, any view, any editor. It should know nothing of the user interface. It just needs to be told which project is selected. When you set the project name, that should be enough. Given the workspace root and the name of a project, we should be able to sort out the project. In fact, I have code that does just that in the NewJavaProjectConstructor. I need a little more time to review the patch. I'll look harder at it tonight. I feel like we can commit at least some part of it. Stay tuned. Sounds good Wayne. I didn't think it was ready for committing quite yet as I was just exploring the idea and how to roughly execute it. There's lots of TODO's and the code is a bit messy! : ) So would it be a good goal to make this constructor do everything that NewTypeWizardPage does or would you rather just make it do the subset that we need for IDE4EDU (i.e. no package stuff, etc.)? Also, I'll try to come up with some ideas for new class construction that would be extra helpful to a student. We can commit stuff that's only partially complete. We can also delete stuff that we later decide needs to be deleted. That's just part of doing business. BTW: if you run your JUnit tests using "Run As > JUnit Plug-in Test", it will create and launch an Eclipse environment (with its own workspace) to run your tests in. It does take a little longer than a straight "POJO" JUnit test, but it's effective. It's how the various projects test their plug-ins. The NewJavaClassConstructor is promising.
A couple of small nits:
if (!shouldCreateProjectIfNecessary && !project.exists()) {
return new Status(IStatus.ERROR, Activator.PLUGIN_ID,
"There is no project by the name " + projectName +
" to create a new class in.");
}
The logic in the test takes too many brain cells to understand. Is there a simpler question that we can ask, perhaps using a new method? Maybe something like:
if (!projectAlreadyExistsOrCanBeCreated()) {
return new Status(IStatus.ERROR, Activator.PLUGIN_ID,
"There is no project by the name " + projectName +
" to create a new class in.");
}
The message contained in the status should be contained in a single string. Take at the String#format() method (since we've decided to run with Java 5 syntax and libraries).
I'm not loving the shouldCreateProjectIfNecessary identifier. Too clunky. Maybe just "createProjectIfNecessary" or just "createProject" might be better. Thoughts?
It may also be the case that we don't have a valid project name. We can do two things here: (1) we can check to make sure that the project name is valid, or (2) we can document an assumption that the provided name is valid. I believe our current use case will support the second option (I believe that we will provide the project name programmatically rather than ask the user for it). For the first option, it'd be best if we can avoid duplicating code from NewJavaProjectConstructor.
IFile file = project.getFile(className);
if (file.exists()){
return new Status(IStatus.ERROR, Activator.PLUGIN_ID,
"A class with this name already exists.");
}
The className is different from the file name (the file name is the class name with ".java" appended). This is something that definitely belongs in a unit test.
This is a good first cut. A future version should make more interesting class files. Perhaps a setting in the constructor can be toggled to create a main(String[]) method. A second setting could automagically throw a "Hello World" in there for kicks. Another setting could optionally add some JavaDoc. Probably should run the whole thing through the formatter while we're at it...
I think that I'll wait for your next revision before I commit. Please attach the constructor and tests as separate attachments.
I've decided to move ahead with parts of your patch. I got it in my head that I wanted to get Bug 300672 going, and this is a necessary part of making that bug go. Another observation: if (mypackage.getKind() == IPackageFragmentRoot.K_SOURCE && mypackage.getElementName() == packageName) { javaPackage = mypackage; System.out.println("Package: '" + mypackage.getElementName() + "'"); break; } The == comparison between mypackage.getElementName() and packageName may work sometimes. In Java, == checks for identity. So as long as the two variables in the comparison point to the _exact same object_, this will answer true. Generally when you're comparing strings, you should use string1.equals(string2) which will check to see if the strings contain the same characters in the same order. Also,using "System.out" for testing is so 1980s (while music in the 80s totally rocked, debugging sucked). Use a breakpoint; let the debugger help you. I'm working on applying your patch; there'll be some refactoring involved. Comment on attachment 157052 [details]
Version 01 - 2 ideas: constructor from scratch or wrapper for NewTypeWizardPage
Only portions of this patch have been applied.
I have applied parts of the patch and have marked the patch iplog+. I am leaving this bug open since there's still lots of work to do. (In reply to comment #12) > I have applied parts of the patch and have marked the patch iplog+. I am leaving > this bug open since there's still lots of work to do. I've changed my mind. Since the major push of this bug is complete, I'm marking it as FIXED. I/we will open additional bugs to address deficiencies. Miles, I've taken the added step of actually committing the test class you provided. FWIW, when you create a new file, your name should appear in the copyright (unless you have to assign copyright to another entity). When you provide significant modifications, your name should appear in the copyright header. Created attachment 157675 [details]
mylyn/context/zip
|
Hacking the "New Java Class" wizard from JDT makes me itch. It's just not the right approach. I propose the creation of a constructor class whose instances are able to create a new Java class. The constructor can be helpful and relatively high-level. Use could be along the lines of: JavaClassConstructor constructor = new JavaClassConstructor(); constructor.setClassName("MyClass"); // constructor.setUseDefaultPackage(true); constructor.setPackage("my.package.name"); constructor.setProjectName("My Java Project"); constructor.setCreateProjectIfNecessary(true); constructor.construct(); The instance should be created with reasonable defaults, so that a user can provide minimal configuration and still get something useful. Taking a project name (rather than a project instance), makes it the constructor's responsibility to find the actual project instance when it gets the construct() directive. If the createProjectIfNecessary property is set, the project will be created. Note that we may need additional values to create the project (such as the Java version string, or something). It'd also be cool if the various properties are observable (check out ListenerList) so that a GUI can hook into them. But that's relatively low priority. Note that this class should not have any GUI elements to it. By detaching it completely from the GUI, we make it very reusable in other contexts. I may take a run at an initial implementation of this tonight. Feel free to beat me to it.