Martin writes back and the plot thickens...

Right after I sent in the patch, I got this back from Martin.

- - - - - - - - - -
Thanks a lot John, the patch looks good. I still think strongly that we
shouldn't introduce a new variable, as this just adds completixy for the user.
In getTypeContent,
context.setVariable(CodeTemplateContextType.TYPENAME,
Signature.getQualifier(cu.getElementName()));
only works for top level types. I think an extra parameter is needed for the
enclosing type name.
I was wondering, would you also be happy having simply a type _body_ template?
That would by symetrical to the method body template and save us the new
variable for the header declaration. By deault such a variable would be empty.
Sorry that I realized this quite late. But if there arent strong reasons against
this I would favour such a template.

- - - - - - - - - -

..And I thought I was done. The part about the "body" template was the biggest challenge. Why had we worked our way to the design of a type template with a "header" variable that the user could put anywhere in the class? It seemed like such a great idea at the time when the first design I thought of was to embed the class definition straight into the new file template. Was Martin just trying to make things as similar as possible to the old way eclipse used to work? What about the incredible power and flexibility users will get from having control over where the header and brackets go in the class definitions?

Well, now that somebody challenged it, I couldn't really think of a good use case that couldn't be solved just as well as Martin suggested. If you want to put some standard comments or embedded metadata in comments before every class or after, why not put it in the new file template? The only thing you couldn't do is put something between the class header and the opening bracket. But why would anyone want to do that?

Pondering these issues, I explained the situation to some of the developers at work, and they totally sided with Martin. "That's the way I would have done it..." So I'm starting to feel like a real idiot for ever thinking the way I just coded this up was a good idea. Of course nurturing my fragile ego has never been my co-workers' strong suit.

On top of all this, the failure cases with all the solutions I thought of up until now always bothered me. What if users forgot to put in the ${type_header} variable or forgot one of the brackets? Martin's idea solves that problem by having eclipse generate the header and brackets, and still giving users a template variable for the contents they can fill in. So I caved again, and wrote back...

- - - - - - - - - -

Martin,
I thought about your suggestion to cut down the typecontent variable to just
the typebodycontent and have eclipse generate the rest automatically. I can
easily make this change. Your comment about it being "quite late" - do you
mean I need to make this change quickly to make a deadline, or just that I
have done the work already?
I can think of two reasons to keep it the way it is, but neither is very
strong. The first reason is that at some point, somebody may want to take
advantage of the flexibility to put a special comment or something around the
header. I can't think of a good use case for this, though, so the simplicity
of your suggestion wins out, in my mind.
The second reason is that it may be *less* intuitive, rather than more to edit
a blank template rather than one with a ${type_header} variable and two
brackets - at least that looks like a class declaration rather than just a
blank space - what do I put here? I don't think this is a very strong argument
either, as it may just be more intutive to me because I wrote it that way.
Also, the typebodycontent cuts out the failure cases in the typecontent of
leaving out the header or one or the other of the brackets - I discussed that
problem earlier in the thread.
OK - two things.
First, you mention TYPENAME only works for top-level types. Do you want me to
detect top-level or not and switch which variable I include based on that? Can
I get to the type name in any way for an inner type?
Second, I think the default template will have to be one newline character
rather than blank to match the current behavior, but not hard-code the newline
so that the user can never get rid of it. I'm assuming this is a small issue,
but the default template won't exactly be blank.
Thanks,
- John
- - - - - - - - - -

The next day I had the new patch ready.

- - - - - - - - - -

Here is the new version with just a blank type body as the new template.
The variables are the same as the first patch (just the type name added to all
the default variables). Type header is now gone and the default template is one
blank line as mentioned in the comment above.

Finally coding...

..And done. Blink and you miss it. After all the hunting for what to change and negotiating how to do this, the code for the part we all agreed to came together in a surprisingly short time.

I refactored some of the code in NewTemplateWizardPage. I replaced the code inside the constructTypeStub method, which used to hard code the type header and the brackets. Out of that, I broke out the type header construction part into a new routine I called "constructTypeHeader". I also created a new routine in StubUtility, exposed through CodeGeneration (like all the rest of its classes) called getTypeContent which is patterned after the current getTypeComment, though most of the template reading methods follow the same pattern. Users pass the header data into this routine, and it inserts it into the template. Then the rest of the routines already pass the constructed type content on into the new file template routine, so no need for changes there.

So I ran all the current tests, and the part that took the longest was debugging and getting the current tests to run. If you add a code template into Eclipse, and you add a new variable into the text of the default template without putting in all the supporting code for the variable in the Eclipse classes, Eclipse will internally throw and log an error, and blank out your whole code template as invalid. To me, since I wasn't checking the logs when I ran it, it looked like my new template dissapeared. I thought I had erased one of my changes to implement the original template in one of my manual sync operations described below. I checked the code over and over and could not find what I had left out. After a few traces of the initialization code, I finally found the error. When I changed the ${type_header} text in the code template to "type_header" so the system wouldn't try to process it as a template variable, the template re-appeared. After that, it took only a few minutes to actually implement the code for the ${type_header} variable.

Once I started running the tests, all the current tests in NewTypeWizardTest failed because of the default for my "New Java types" typecontent code template didn't match the original behavior. The problem was in the blank lines, and after some tracing (since I couldn't seem to interpret the output of the test failure correctly) I found the formula was two newlines between the class braces, and one newline after the last class brace. Then it passed all the tests. I ran an "AllAllTests" the way Dirk told me to, and it passed all 3179 tests.

I made patches in Eclipse's CVS view for my changes to jdt.ui and jdt.ui.tests, and sent it in.

- - - - - - - - - -
Here is a patch that adds a "typecontent" template that the system uses to
process user-definable content that will be substituted for the
${type_definition} variable in the "newtype" code template. This patch also
adds a ${type_header} variable for users to insert into the "typecontent"
template.
The default text for the new template matches the original version, and so
passes all current tests with one modification which adds the new template
variable so the test variable counts match what is now in the template store.
No new automated tests are added to insure different user edits of the new
template behave as expected (though I tested some manually). Let me know if
you have any changes in the above patch you want or any other work you want me
to do on this.
Thanks,
- John
- - - - - - - - - - - - -

All in all, it was not as bad as I thought it would be as far as design. Since we simplified the problem substantially by not allowing users to direct where Eclipse inserts the default method stubs, I didn't have to change any of the current APIs, and could simply add one that fit in the style of the others.

There are plenty of things in this code that are not how I would have done it. I still don't like the mixing of model and view in the NewTypeWizardPage, and the splitting of responsibilities so that some code is generated in NewTypeWizardPage, and some is generated in StubUtility. I still believe this code is more difficult than it should be for current clients to use, and it is difficult to test. Like many software developers, I have strong, individualistic opinions about the way things should be. But, it was easy enough to make this change keeping in the style of the current code, and that is part of the deal, especially for us non-committers. This was a good exercise for me as I am used to having more control over the details of how my code is written, and fitting into someone else's style was interesting.

I think we will have to confront some of these issues more directly if we decide to allow the users to direct where the system inserts new method stubs, but that is a more complex question for another day.

One more comment on this exercise, I never did find a good solution for syncing the Eclipse source tree and keeping my changes up with the new integration releases from Eclipse. I tried several things and none of them worked, and I ended up saving off my changes, pulling down new builds from Eclipse, and merging my changes back in using file merge utilities. I did this twice, and it was an awkward pain in the butt. There must be a formula for syncing to Eclipse integration labels, and I'm going to ask the experts and report back.

Martin responded right away - so here's more bugzilla correspondence.
Note I cave right away on design issues. It is their code, after all, and Martin's suggestions make it easier for me to get something done and into the code base quicker.

- - - - - - - - - - - -

------- Additional Comment #9 From Martin Aeschlimann 2005-09-21 05:49

[reply] ------- Hi John, thanks for you comment! Here are my responses:
I don't see where we would break any clients or tests when using
${type_declaration}, So I would head for that, it's the simplest solution for
the user (no changes).
The only thing that changes is that everybody that used the new file pattern
should know that we now have an extra template for the type declaration. As
said, that will take its time.
The idea of using variables like ${superclass_constructors} doesn't work, in my
opinion. First, NewTypeWizardPage.constructTypeStub is an API method and we
currently give all clients the full freedom to create whatever they want in the
type body. Second, there many ways to categorizing methods, static methods,
public/private methods, getter/setters, fields.. Some of the categories overlap,
so it will be hard to write a template that is still correct. Then, when
creating methods you also have to create imports. Imports depend on the context
where the type is used. Textual templates are not powerful enough here.
So if you are fine with 'I'll copy/paste my methods in the catrogies later' then
we should go with that. From comment 7 I see that there are other use cases for
this template, so it would be great to have the new template.
If you have a more convenient API for getCompilationUnitContent API, please make
a suggestion. We just have to keep the old API areound and compatible. But no
problem to add an extra, more convenient variant. To be honest, I don't really
see how things can be made much simpler. There's a certain number of parameters
that have to be passed, and I found it simpler to more API with less parameters,
having smaller blocks of funtionality, increasing the flexibility how you can
use the various parts. E.g. we have functionality that just needs a type comment
(Add Javadoc action), some functionality doesn't want comments (pass a single
'null').

------- Additional Comment #10 From John Kaplan 2005-09-21 15:26
[reply] ------- Martin - OK - good enough. I can move ahead based on your suggestions. If we
don't provide any new template variables to direct eclipse where to put
methods, it simplifies the problem quite a bit. To my mind, it removes the
need to change the existing APIs.
I am still uneasy about introducing more model functionality (especially
template reading and resolving code) into NewTypeWizardPage, but will proceed
with the target of changing as little of the existing code as possible. I'll
try first to create a StubUtility/CodeGeneration routine to read the new user
template for the type content, then call that and evaluate the template in a
new body for NewTypeWizardPage.constructTypeStub.
I'll let you guys know how that works one way or the other.
- John

------- Additional Comment #11 From John Kaplan 2005-09-22 00:20
[reply] ------- (In reply to comment #7)
> The discussed changes will also fix the common template complaint of wanting
> to add default members for logging and versioning.
Matt - thanks for the comment, and the vote. I'll put in a test case that has
a default member variable to be sure this works.

- - - - - - - - -

So next post, I will get right to coding and testing...

A couple of replys to my last bugzilla comment came after the weekend. The first one is the first I've heard from Martin, whom Dirk introduced in his first reply as the code template expert.

- - - - - - - - - -

Hi John,
I would favor a new code template 'type body' that will be used for the variable
${type_declaration} in the new file template (I don't see why a new variable
would have to be introduced, Dirk?).
CodeGeneration.getCompilationUnitContent can stay the same. The type content
that is passed to it should be generated using the new type body template. A new
API getTypeBodyContent() would be added for that, and it should get all
information that it requires (type delaration header, type name, cu name...)
When creating a new type, the type wizard would first construct the new cu using
these templates and on that insert the all methods to be created. I don't see
how you could solve that with variables for all created methods (e.g.
${main_method}}. We can't know what kind of methods will be created by clients.
One problem is that all client of
CodeGeneration.getCompilationUnitContent will have to be updated, there will
always be clients that aren't.
An other probelm is that i'm not sure if you really will get what you want. New
methods will probably be entered at the very beginning, and can't understand
your comments in the body. This will be also the case for all other new members
that will be added programatically.

------- Additional Comment #7 From Matt Sanford 2005-09-19 10:18
[reply] ------- The discussed changes will also fix the common template complaint of wanting
to add default members for logging and versioning.

- - - - - - - - - -
My answer to Martin:

Martin,

Thanks for your ideas. Here are my responses.

I don't want to put words in Dirk's mouth, so jump Dirk in if I get this wrong, but I think he wanted me to be conservative about changing the current behavior of the ${type_declaration} variable so it wouldn't break any current tests or invalidate any current client uses.

I was planning to default the new template so it implemented the current hard-coded behavior out of the box, and so would pass all the tests. Since I haven't yet proven I can do that successfully, I can understand Dirk wanting to be cautious about it. I have always hoped I could prove it though, so I don't have to enter a new template variable, because it would mean two template variables that do the same thing, one hard-coded, one user-configurable, and I believe that would be confusing to users.

I had first envisioned, as you imply, that eclipse would generate all new methods at the top of the class, and users would have to cut-and-paste them down into their class where they wanted them to go. If you look at Dirk's first response, however, you can see he suggests creating template variables that direct eclipse to place generated methods at user-specified points within the class. So, you could have methods corresponding to the different options in the new class dialog go different places in the newly generated class. For example:

${type_header} {
// constants

// instance variables

// constructors
${superclass_constructors}

// inherited methods
${inherited_abstract_methods)

// main method
${main_method}
}

I agree with you this isn't a perfect solution, as we can't anticipate all the kinds of methods different clients can generate, and hard-code template variables for them. However, I agree with Dirk this is a better user experience than putting all the generated methods at the top of the class, and we could provide template variables for all the generated methods we currently know about, if we have a version of a template evaluating method that takes a collection of structures that include template variable name, optional content for the name and some flags to indicate how to process (supress as in filecomment if user deselects "Generate comments", supercede passed content with user template variable if present, or always use passed content). (I suggest this as the workable alternative to hard-coding each template variable as a parameter in the signature of the call that evaluates the template. It could be a new version of getCompilationUnitContent if you want to evaluate the whole template at once, or a routine that would replace the guts of NewTypeWizardPage.constructTypeStub if that's what you guys want me to do. Speaking of which...)

The last point to discuss is changing the CodeGeneration.getCompilationUnitContent signature. I see your point that I could accomplish this by pre-evaluating the templates to generate the typeContent, (however complex that becomes with these added method generation variables or not), then passing the resulting typeContent into the current API. I will do it that way (or at least attempt it) if you really want me to. But I think that would be perpetuating (and making worse) the bad coupling and weak cohesion that already exist between and among NewTypeWizardPage and StubUtility/CodeGeneration.

I would go back to the arguments I made in my last comment. And I'll add my opinion that StubUtility should have the responsibility of evaluating templates and reading variables and the NewTypeWizardPage should simply direct StubUtility as to what to do for particular cases. I think this would be a big improvement over the current state, where clients have to make several calls to StubUtility to gather data, pass the data back as parameters to StubUtility in getCompilationUnitContent, and then check for success and fix up a hard-coded default in case of failure. Every client currently requires a paragraph-length interaction with StubUtility to make a getCompilationUnitContent call, and there is a lot of repeated code between clients. I would like to try to reduce that to a few lines.

If it were my code base, that's what I would do. I think it fits the spirit of open-source, where we try to constantly refactor and improve the software to help it reach it's best design. On the other hand, I'm not a committer, and I'm not so totally wedded to this idea (i.e. changing the getCompilationUnitContent API) that I won't do it the other way (keeping it the same) if you guys insist.

Let me know about changing the API and about the new template variables (${superclass_constructors}, ${inherited_abstract_methods}, and ${main_method}) and I'll forge ahead with whatever you think is best.

Thanks,
- John

Posted on the bug (Eclipse Bugzilla Bug 108071)

I have studied the code in NewTypeWizardPage, CodeGeneration, StubUtility, and the 2 other references to the CodeGeneration.getCompilationUnitContent API in the jdt package (in AccessorClassCreator and MoveInnerToTopRefactoring).

The bad news is I can't see any way to do this enhancement without changing the getCompilationUnitContent API.

The biggest reason is that StubUtility.getCompilationUnitContent is responsible for evaluating templates and resolving the template variables. With this change, I will have to shift the responsibility for inserting the content of a new type and its stub methods from their current hard-coded treatment in NewTypeWizardPage down into StubUtility, so I will have to pass more information (for new template variables) into StubUtility.getCompilationUnitContent to allow it to resolve all the template variables at once.

A secondary reason is that the client interaction with StubUtility.getCompilationUnitContent is currently tightly coupled with its clients. All clients currently are calling getLineDelimiterUsed from StubUtility directly, and usually making separate calls to getTypeComment and getFileComment and passing the results back to the call to CodeGeneration.getCompilationUnitContent. In addition, two of the clients have essentially repeated fail-safe code after the call to getCompilationUnitContent returns. (These are NewTypeWizard.constructCUContent and MoveInnerToTopRefactoring.createSourceForNewCu.) Basically, the interaction between the clients and the getCompilationUnitContent method could be made a lot cleaner and cries out to be refactored.

I doubt if it is even possible to keep the current version of the API around as a facade that calls the new routine in the background, because I predict I will run into some unreconcilable failure cases and strange user experience when the user uses new templates and the old code doesn't allow the data to be passed in to fill them. The same goes for creating new parallel code and leaving the old code intact, which has the added problem of creating a duel maintenance problem. I would much rather clean up the current code for all concerned and for any future enhancements as well.

So, what I'm willing to do to fix all of this is:
Add additional tests to cover the new functionality and combinations, alter any existing tests as needed to pass with the new API.
Fix the getCompilationUnitContent API to be more flexible and to require less coupling between its clients and itself.
Refactor the current clients to use the new API and clean up repeated code made unnecessary by the change.

As far as the exact form the new API will take, let me prototype a few possibilities, and then come back with a suggestion or some choices.
So let me know what you think. I know this isn't what you wanted to hear, but if you approve me doing this, I have a way forward and can get this done without too much trouble. (In other words, it's a tractable/straighforward problem as long as you accept the constraint that the API has to change.)

AccessorClassCreator.getUnformattedSource(IProgressMonitor)
MoveInnerToTopRefactoring.createSourceForNewCu(ICompilationUnit, IProgressMonitor)

Running unit tests and Syncing (or not) with CVS...

The tests finally run!

Dirk came through with the magic answer in an e-mail right away after my last request.'

- - - - - - -

John -you need to have org.eclipse.test.performance in your workspace. You can find the project in the CVS repository.
I tested the following setup:
- fresh workspace
- checked out org.eclipse.test.performance and org.eclipse.jdt.ui.tests
- executed org.eclipse.jdt.ui.tests.wizardapi.NewTypeWizardTest
Dirk

- - - - - - - - - -

So I tried it, and by golly it worked like a charm. I experimented with running the tests some, and e-mailed Dirk back:

- - - - - - - - - -

Dirk - Hey it's working! Thanks for the tip.

BTW - I got the first little part of the change working - a new ${type_content} code template that appears on the Window->Preferences.. dialog. Next I need to do some refactoring of the NewTypeWizardPage internals to get the ${type_declaration_header} variable to work. But now I can do regression tests, so this really unblocks me.

I may have a question about CVS syncing if I can't figure it out on my own. Otherwise, I'll keep you informed of progress as I go.

- John

- - - - - - - - - - - -

There is a suite of tests in the org.eclipse.jdt.ui.tests package called "AutomatedSuite.java". This suite has 952 junit tests in it, and takes a little under 15 minutes to run on my Dell laptop. The tests all run green (no errors) with the latest Eclipse integ build.

If you try to run every test in the larger "ui" package within the jdt tests directory, there are 3170, it takes about 3 hours, and there are around 80 errors and failures. I wrote 2 e-mails to Dirk to clear up expectations of how I am supposed to work with these things.

Dirk, I'm on to my next round of questions. I have figured out how to sync back with CVS, so I don't think that will be a problem, but I've run the test suite a few times in different combinations, and I want to make sure I'm running it right.

There's a test class in the org.eclipse.jdt.ui.tests package called "AutomatedSuite" that has 952 junit tests in it. It is running green (all tests pass) in the latest integ build (20050906-800). I just wanted to confirm this is the right test suite to run to check for regressions. I noticed there are other tests that are included, but are not in this suite, and they do not run green. If I run everything (just select the "ui" package and run it all as a plug in test) there are over 3100 tests, and around 80 failures and errors.

I just wanted to double-check it was the expectation that people should run the "AutomatedSuite" tests and that these tests are expected to run with no errors for each integ. If that is the case, then development for folks like me is much simpler. If there's another suite I'm supposed to run, let me know. Running everything and keeping track of which failures were there and which failures I caused is quite painful, and I just want to make sure that's not your expectation.
Thanks,
- John

- - - - - - - - - -

John,

running AutomatedSuite is the correct thing to do. It is the root suite forall ui related JDt tests. There is another one for refactoring tests. It iscalled AllAllTests. However for your work on the New wizard it is enoughthe run the AutomatedSuite regulary. Before providing the patch you can runthe AllAllTests once to do a last check. However the refactoring tests are> 3000 and take some time ;-).

Dirk

- - - - - - - - - - -

Oh - one more question - does the passing of junit tests have anything to do with whether the integ is marked with a green check mark or a red "x" in the download site? I noticed that sometimes different platform builds have either checks or x's. What's the code for that?

Thanks,
- John

- - - - - - - - - -

Yes, if the build has a red x at least one test has failed. You can clickon the red X which will take you to a list of test results you can furtherinspect to see what test has failed. Currently we are running the tests onethree different platform and it may be they fail only on one.
Dirk

- - - - - - - - - -

CVS and not Syncing

Well, I was wrong about syncing up with CVS - I couldn't find the formula to do it after all. There are commands in Eclipse to sync to head, but since the head revisions don't build (at least I don't think they do...) I didn't try that. I've been working with an integ build, and wanted to update my local build to the next build while retaining my changes. That's a pretty normal thing to do with CVS, but I couldn't find the formula in the Eclipse integration. In CVS, you do an "Update" command, and choose the label you want to update to. In Eclipse, the "Update" menu command (and the "Synchronize with repository" for that matter) don't allow you to specify the label you want to sync to (or update from).

So I went to WinCVS on my machine, and tried to update from there. The "Update" command was there as I expected, with options. I tried it, and it seemed to work. It gave it's usual reams of output where it copied files from the server to my box, and it seemed to do the merges right for the files I had changed. That's when I wrote the e-mail above that syncing would be "no problem."

But then, back in Eclipse, I tried to rebuild, and it thrashed around in pitiful death throes for a few seconds before puking out a gazillion errors and saying "rosebud...." So, with no other bright ideas to fall back on, I got a new integ, hand-checked the diffs to my files, and copied them over. (I was at my daugther's soccer practice with no good diff-merge utility on my laptop, so not only did I have to do this tedious work with MS-Word, I looked like a real nerd to the other parents sitting in the shade tapping on my Dell D-800 brick and muttering to myself about sub-standard tools.) Hope I get this fixed (the syncing, that is - there's no hope for me to stop looking like a nerd. Hell, I love being a nerd - that's why I do this.)

Got it all going and played around with some more coding and prototyping.

My addition of the ${type_content} variable broke two of the 952 jdt junit tests, both in TemplateStoreTest.java. Adding a new code template variable changes the count of variables, and you have to inform the tests of your new variable by including it in a "ALL_CODE_TEMPLATES" member array that's defined near the top of the class. (But not at the top of the class! Somebody's got to review the Sun Java coding standards....)

I tried to work a new variable through the calling sequence of the existing classes, and that was enough of a prototype for me to smell trouble. Next up, I have to write to the Eclipse committers to convince them the current class factoring is too hosed up to make this change, and needs refactoring and deprecating the current API.

That done, I reverted my changes to the "NewClassWizard" and "StubUtility" classes. It's tricky to find how to revert if you want to cancel changes you've made. In the CVS Repository perspective, you can't browse and select the file you want in the view on the left and find a menu item to revert it. But you can search for the file using the "Search->file" menu, then right click on it in the bottom "search" results view. If you right-click the file in the window, the pop-up menu has a "Replace with..." sub-menu, and one of the menu items allows you to replace the file with the version you last got from CVS.

Something on both the test front and the development front this time...

On the test front, still can't get tests to execute, but had e-mail exchange with Dirk:

Hi John,
the trick is to launch the tests using JUnit Plug-in Test launcher. The launcher makes sure that the Eclipse runtime is setup correctly. The launcher works the same way as the normal JUnit launcher. Simply pick a TestCase or a suite and run it.
HTH
Dirk

Dirk - When I try to run any JUnit test in the jdt test directory as a plug-in test, I get a "no classloader" error.
Could you give me some further advice how to get around this?

Thanks,
- John


For example, I select the NewTypeWizardTest.java file in the Package Explorer, right click, and select Run As... -> JUnit Plug-In Test from the pop-up menu.

I get the following stack trace:

"Error: " java.lang.IllegalArgumentException: No Classloader found for plug-in org.eclipse.jdt.ui.testsjava.lang.IllegalArgumentException: No Classloader found for plug-in org.eclipse.jdt.ui.tests
.runtime.RemotePluginTestRunner.getClassLoader\n(RemotePluginTestRunner.java:67)at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.loadSuiteClass(RemoteTestRunner.java:428)at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.getTest(RemoteTestRunner.java\n:380)at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:445)at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344)at (Bunch more of stack trace omitted)....

We'll see what he says.

On the development front, I got a new code template for Dirk's proposed new ${type_content} template to appear on the Window->Preferences...->Java->Code Style->Code Templates->Code display.

It took a lot more fiddling than I expected. I had to make changes to a number of files in the org.eclipse.jdt.ui package, and the change kind of mysteriously appeared after not appearing for a while and I'm not sure what was the last change I made to make it work.

There is a file in the templates subdirectory of the package called default-codetemplates.xml. In that file, the text for all the familiar code templates is listed, like $(filecomment) and ${type_declaration) and the like. My first step was to copy and paste ${type_declaration} and edit it to make a new version for the new ${type_content} my first prototype looked something like this:



Eager for results, I fired up Eclipse (select the newly edited package, select Run As...->Eclipse Application, and off it goes - you gotta admit that's pretty slick...) but as foreshadowed, my new code template did not appear in the list.

So, I went searching for references to the "newtype" template (called "New Java files" in the window preferences list, but internally it's "newtype.") I found a bunch of hits, and added what I thought would be the proper corresponding reference to my new template everywhere the "newtype" template was referenced.

The list of files I changes was:
CodeTemplateBlock.java - added a new branch to a big stack of if-else's in getText().
PreferencesMessages.java - added a static string variable for the template's label.
PreferencesMessages.properties - added a property with the actual text of the template's label.
CodeTemplateContextType.java - added several strings for context and ID, and added another branch in a stack of if-else's in the constructor that list the "sub-variables" allowed to appear in the text of my new one. Note I'll likely have to go back and edit this as I make it more real. Also, there's a TODO further down in validateVariables() I'll have to fill in when it's real. The newtype variable requires a package and a type declaration to be valid, and mine will require a type declaration header.
Then there's our old friend default-codetemplates.xml as described above, and that's it.

Huh - only 5 files? It seemed like a lot more when I was doing it. I got most of them on the first try, but my new template still wouldn't appear in the list. After some hair-pulling and tracing of references to defined variables (not just the "newtype" identifier") I think I found the one that was blocking it from appearing. It did seem to unexpectedly appear, so I either lost track of which change I'd made or it needs to be started up twice for some reason for the change to take effect. I couldn't swear to it either way.

...And speaking of unexpected things, remember my pitifully slow DSL connection? After I called and denied any culpability to Qwest, I tried as a lark to swap out the phone chord between the wall and the DSL modem. What would happen but all of a sudden the connection came up to full speed as advertised. 150k-per-second downloads, meaning I can now download a full Eclipse distro in about 8 minutes. Boy, was I wrong about that. It got me thinking that development projects are full of these small assertions we make - many right, but many wrong, and it's always a matter of proving somehow which of your theories are wrong or right until your project starts to work and your tools start acting to your expectations (or your expectations start coming more in line with your tools....)

Next up for this project, I worry about how do I sync/merge with CVS now that I'm making changes and I don't have any CVS privileges? Will Dirk come through and let me know how to run junit tests since I don't want to make any more changes without running regression tests? And will I get to the fun part of making my new template come alive by refactoring some of the code I mentioned in earlier posts?

Could be coding, but still can't get the tests to run. It's more fun to code, but I have to get the tests running or I can't do test-driven development. Even if I was coding first, I'd have to regression test it before I could contribute a solution in good conscience.

As I mentioned before, the main dilemma I’m struggling with has to do with whether to separate or combine the concepts of Eclipse the IDE I’m using and Eclipse the target program I’m updating. The “test.xml” file seems to want to point to a single installation with a single “eclipse-home” directory that includes all other parts as sub-directories. This doesn’t make much sense to me, as in the middle of iterative development, you’d be breaking your IDE all the time. Seeming to reinforce this, both the PDE and the CVS integration put the source and target of the Eclipse you are actively developing into a separate workspace. My assumption is that there would be two main elements in the classpath, the workspace files you are editing, and then the rest of an Eclipse installation for the other stuff.

Another possibility is that the test.xml file is targeted only for testing of a complete build, so the expectation is that you would build a complete Eclipse install with the tests included and run the test.xml from there. So the test.xml wouldn’t be useful for an iterative development experience (or at least you would have to be used to a lot of repeated builds in your iterative dev cycle.)

The gory details: When I try to run the "test.xml" file included directly in the jdt tests directory, it fails immediately, looking for a non-existent "org.eclipse.jdt.ui.tests_3.1.0" directory. The jdt tests directory I downloaded from CVS into my workspace directory does not include the embedded version string. (Though there are similar directories in the Eclipse install that do have the version string.) I start forcing the ant file to match my configuration, which means changing “eclipse-home” to my workshop directory, not my eclipse install directory, and taking the “_3.1.0” suffix away from the "org.eclipse.jdt.ui.tests” property. Then the script fails trying to find org/eclipse/core/launcher/Main. This is back in the eclipse install directory, so changing “eclipse-home” caused this problem. Hmm – this reinforces my “complete build” theory. I have two problems with that: I don’t know how to do a complete build that includes the eclipse install and all the tests I want, and even if I did, I’m more interested in iterative development testing than complete platform testing, given that the area I’m going to change is pretty small and specialized. This is worth a question to my new friends at Eclipse.

So let’s look at running jdt junit tests in isolation – the normal way to do this in Eclipse is to select the test class itself and select “run as junit test” or select a plug-in and select “run as junit plug-in test.” First, there is a project in org.eclipse.jdt.ui.tests called “test plugin.” Looks worth a try, so I select it and then select “run as junit plug-in test” and it fails almost immediately with an error: “java.lang.IllegalArgumentException: No Classloader found for plug-in org.eclipse.jdt.ui.tests.” BTW, it seems to fail this way whenever you select something that isn’t really a plug-in with this command. So much for that. OK so let’s look for a specific test to run in isolation. The test I’m most interested in is in the package “org.eclipse.jdt.ui.tests.wizardapi” and it’s called “NewTypeWizardTest.” It fails almost immediately as well, but at least gets into the junit test class and starts executing code. This is the farthest I’ve gotten, as the other two things I’ve tried get derailed by weird configuration errors before they even start (as described above). The NewTypeWizardTest routine calls something that calls the routine ResourcesPlugin.getWorkspace(), and it fails because the workspace is null, displaying the message "workspace is closed." It seems the ResourcesPlugin.startup() method needs to be called first, but it is not called. (I set a breakpoint inside it and the error hit but not the breakpoint.) By the way, this happens with all the test classes included in the “org.eclipse.jdt.ui.tests” package, including the “HelloWorld” test in the “org.eclipse.jdt.testplugin.test” package.

Is there some protocol I am not following that causes the right plug-in(s) to be initialized and started up to do these tests? I don’t see any good way to fake setting up the ResourcesPlugIn – there are warnings all over to not try to instantiate it yourself, but it obviously isn’t being instantiated.

Wrote an e-mail to Dirk and cc’ed the jdt developer list….

Dirk et al,

I’m trying to run the jdt junit tests so I can test my changes to the NewTypeWizardPage and related classes as described in bugzilla bug 108071. I’m running into roadblocks and wonder if you can give me any advice.

First, can the tests be run individually from Eclipse? Whenever I try the tests fail immediately and the cause seems to be that the “ResourcesPlugin” plug-in isn’t being initialized. (The symptom is calls to ResourcesPlugin.getWorkspace() throw a “workspace is closed” exception because the internal “workspace” variable is null, and it gets initialized in the “ResourcesPlugin.startup()” method.) Is there a formula for getting plug-in dependencies to be initialized to run a junit test? I have also tried running plug-in tests from the jdt tests directory, but these all fail even sooner with the error: “java.lang.IllegalArgumentException: No Classloader found for plug-in org.eclipse.jdt.ui.tests.”

I would much prefer to run the tests this way (meaning singly and directly from Eclipse) if possible. It is more convenient for me for iterative development.

I also tried running the “test.xml” ant script that is included in the jdt tests package, but that is failing because of path problems. It seems to want to have all the classes and tests in the same hierarchy under a single “eclipse-home” directory, and not to have a default Eclipse install with separate packages in an Eclipse development workspace. The latter is the way I have it set up, and the way the PDE and CVS integrations to Eclipse seem to steer you. Is it that you’re not supposed to use Eclipse as an IDE when you’re developing Eclipse? Or is there some step in the process I’m missing?
Please advise. Also, if this is better to be attached to the bug rather than this e-mail list, let me know. I assume I’m not the only one in the world who ever had these problems…

Thanks,
- John

Fingernails getting dirty…

This project has split into two tasks: figure out the design for the changes I’ll have to make, and get the darned jdt tests to run. I’ve made good progress on the first task. I have been working through roadblocks on the second with no results yet.

Designing the Solution
After staring down the code and running a source-level debug a few times, a good solution is starting to come into focus. The issue is that, as tediously belabored in an earlier post, one routine (NewTypeWizardPage.createType) reads in all the “sub-templates” or fragments of different, half-processed information needed to piece together the new class, then passes each piece as parameters to another routine (StubUtility.getCompilationUnitContent) that evaluates the template. In subsequent runs, I found that the addition of the methods is later, back in NewTypeWizardPage.createType, and it is done through the AST (IType) representation of the newly created class.

Considering that Dirk admonished me I have to preserve both the API of the StubUtility.getCompilationUnitContent routine and the original behavior of the whole shebang, I’m left with a fairly clear choice. Either I keep the original routines completely in tact and write a parallel code path to process my new template variables that has a bunch of repeated code from the original (boo hiss – it should be obvious to any decent developer this is a very bad idea.) Or, I refactor the current routines to allow the new functionality of user definable comments and class structure while having it default to the original behavior (yay, woo hoo.)

A couple of things make this interesting. There’s no way I can get away with keeping the API the same (at least not one that I would ever consider…) for StubUtility.getCompilationUnitContent. The reason is that if we are to give the user control of the internal class structure through new template variables, we have to resolve those template variables, and I believe we have to resolve them all in one place. So, we have to pass new data (i.e. the generated method stubs for inherited methods, main method, etc…) into this routine as parameters in addition to its current parameters. So how do I keep the API the same and change it at the same time? That part’s easy, I simply wrap a call to the new API in the old API and default the new variables to safe blanks. Then how do I have it mimic the behavior of the original by default? I can’t claim that part is going to be easy, especially considering I’m changing the mechanism of inserting the generated method stubs and doing it at a different place in the code. I’ll have to experiment to see how to mimic the old behavior.

Trying to Run the jdt Tests
Since this is a blog, it’s tempting to indulge in a huge diatribe about how hard it is to get the jdt junit tests to run. It’s even more tempting to mention complex test infrastructure and setup is a barrier to entry that keeps independent developers from making more contributions to Eclipse, and slows down those of us who try, making us more frustrated and less productive. But far be it from me to give in to such temptation. Just the facts…

The jdt junit test ant scripts fail right away with “cannot locate file” errors, and the junit tests don’t run in isolation. The farthest I got with one of them was a “workspace is locked” error, which implies the junit test got as far as actually executing. That was a fleeting victory, however, and soon had degenerated into “could not find classloader” issues which indicate I screwed up the configuration badly enough that junit tests couldn’t even begin. On top of that the files from the CVS HEAD revision aren’t free of error – there are a lot of compiler defects in a couple of the classes in the “refactoring” directory. This may be unrelated, but it doesn’t give you much confidence in building and running tests successfully.

I’m slowly working through the jdt test ant scripts, fixing paths that point to the wrong directories, and filling in the missing parts of the tree. Though the PDE seems pretty cool at first, it looks like it put the eclipse source in a place outside the tree where the ant scripts don’t expect it, and the directory names are different too. This is probably too screwed up for a single mortal to fix in his spare time. Next step is to erase the whole tree and start over with a blank directory and a fresh CVS sync. I have found in the past that I usually hose things up so badly at first it’s best to start over after some initial exploratory blundering. Most often, I find the blundering is a valuable learning experience and it gives me some knowledge I’ll use later in the project. My suspicion is that I’m mixing Eclipse the running IDE and Eclipse the target tree I’m building and have to keep the two separate.

I have spent some time looking for web resources that explain how to run jdt tests, or any eclipse tests, for that matter. There are a few old complaints floating around of broken tests ala 2003 and 2004, but no instructions. I will write them out as soon as I figure out the magic formula for myself, but at this point, I’m sadly under-qualified.

One silver lining in all this, I finally called up Qwest and complained about my 10kbps download rates that are taking 3 hours to download an Eclipse tree. Needless to say, this is severely stepping on my buzz. They confirmed that with this pricey DSL I’m signed up for, my downloads should be 15 times faster. After trying to blame it on spyware (same speed on two computers, both have anti-spyware software cleaning them regularly), me using a telephone filter on my DSL line (nice try, but NOT), and a bad cable on my end (that seems to throttle the download rates to exactly 10.0 kbps every time but cause no errors? Hmmm…), they agreed to check out their equipment at the central office and run some tests.