The thrilling conclusion...

After all that, I easily finished the changes to complete the 4 new code templates. I added 4 new tests to NewTypeWizardTest to make sure each one works.

I added one test body that is called 4 times with different parameters to cover the 4 different templates. This isn't done in any of the other tests in NewTypeWizardTest, which rely on a fair amount of repeated code. It's the one decent bit of software design I think I've introduced into the eclipse code, and it feels marginally satisfying considering I've had to hold my nose and work my code into designs I never would have used or allowed to perpetuate if I had ownership.

To make sure all was well, I fired up the new Eclipse, and tried each of the templates. I tried adding some of the template variables into the classes, and found you could do some neat code generation things, like embed the class name into a static variable name, or into a string you could use for default functions like logging or security. I added some test cases for things like this into the automated tests, and introduced a bug into the test because I was using the same string for result comparison both pre- and post-processing I had to add a "resultBody" string with the expected substitutions to get the tests to pass.

One last issue - I ran the tests, and both the ui automated suite and "AllAll" had a handful of errors each. None of them looked related to my code, so I figured I would go ahead and submit the patch anyway with some "cover my butt" verbage in the check-in comment.

- - - - - - - - - -

Created an attachment (id=27746) [edit]
Adds 4 new code templates for user-definable type bodies

Here is a new patch that adds 4 code templates users can use to put different
standard default bodies into newly created classes, interfaces, enums, and
annotations. This patch fixes the problem with earlier attempts with the type
name template variable not resolving correctly for inner classes. This patch
also does not introduce any new ${} template variables that are exposed to
users.

The regression tests do not run entirely clean as of this patch, but since I
sync'ed to HEAD rather than an integ, I do not know if the tests would have
been clean before this change. The failed tests are: 5 failures in
RenameTypeTest from refactoring/AllAllTests, one failure in
ModifierCorrectionsQuickFixTest and 5 failures in ReorgQuickFixTest from
ui/AutomatedSuite. I don't think any of my changes caused these failures, and
don't know how to resolve them, but am willing to fix them if somebody can give
me a clue what I did.

Thanks,
- John

- - - - -

Created an attachment (id=27747) [edit]
Tests for user template patch

This patch introduces 4 new tests into NewTypeWizardTest to insure each typekind picks up its body template correctly. It also fixes up the code templatestore tests to include the new templates.

- - - - - - - - - - -

So here it is, the moment of truth. But as usual, I don't have to wait long for a reponse.

- - - - - - - - - - -

I would never copy an installation over an old one, just extract to a new
folder. I have a batch file where I simply have to update the path of the
installation directory.

I released your patch with the following changes:
- Introduced template id constants in CodeGeneration (CodeTemplateContextType is
not API)
- label description changes
- used lineDelimiter instead on '/n', added NPE check after getting the template
(as spec'ed)

Thanks a lot for your help!

Marking this bug as fixed, but feel free to add further improvements.

- - - - - - - - - -

This is looking encouraging. I was wondering if he'd catch the template ID constant thing after his earlier comments, so I wasn't surprised he changed that part. Seems like one more note of appreciation is appropriate...

- - - - - - - - - -

Great - Thanks! It was fun working with you on this.
I'll look forward to picking up the new feature in the next integ.
- John

- - - - - - - - - -

To which Martin wrote back:

- - - - - - - - - -

Thanks a lot for your time and endurance :-)! I think we have a good result!

- - - - - - - - - -

So ends the story of bug 108071 and my first eclipse contribution. I picked up the next integ, and indeed, the new code templates are in there working just like I've always wanted. So one developer can make a difference, and the open source develoment model looks like it's working as advertised.

I could go back to that code template default editing idea, or perhaps some changes to the Team Synchronization functions to add syncing to a label, but I have some other things in life I've been neglecting. So it's bye for now to eclipse contributions and see 'ya next time.

The syncing crisis - going down for the third time!

So, with a clear path ahead of me, I start what I hope is the final work on implementing the 4 new code templates for eclipse.

But I run into enough trouble syncing that I'm dead in the water, and have to start coming up with as many puns as I can about "syncing" as opposed to "swimming..." (Did you get the "dead in the water" bit? Just checking...)

Where to start? I reviewed earlier episodes of this epic to see if I've described how I was working. Didn't find much useful. (I'll have to complain to the author...) The issue I've had is that I have never gotten the nightly builds to build successfully, so I've been taking downloads of the latest integs. Then, I'll go to the CVS Repository Perspective and download the source for the two projects I've needed so far (jdt.ui and jdt.ui.tests) and get the source that corresponds to the integ I've downloaded. (Note the org.eclipse.test.performance that Dirk admonished me I needed back in mid-September hasn't changed yet, so I don't need to update it.) You have to navigate these huge lists of versions for each eclipse package, and not all the versions avaiable on the website seem to be available in the CVS perspective. The CVS perspective is a few versions behind, especially if they are cranking out integs on the eclipse website. But, despite this, these versions always build just right, and the tests are green. (For those of you who aren't JUnit fanatics, this means all the automated tests pass.)

Life isn't exactly rosy working this way, because there is no function in eclipse to update from an integ version.

OK - one more digression just to make sure we understand each other... Working with CVS in the "normal" way, you don't check out files and lock them so other developers can't change 'till you're done. That wouldn't begin to work in the wild world of open source. You check out unlocked files, make changes, and then just before you integrate (which you do every time you run JUnit tests unless you're a degenerate loser) you "update" which is a one-way sync with the source repository. This way, if somebody has checked in new changes since you last checked out, you get them, merge them in with your changes, and then you can rerun your tests secure in the knowledge there aren't any nasty integration surprises lurking. If your tests run green, you check in, and let somebody else worry about the pain of merging.

OK OK, so back to why life ain't rosy. Since I haven't figured out how to successfully merge in the normal CVS manner with a new integ when I take one, how do I get the changes I've already made in the source merged into the new integ's version? The first thing I tried a long time ago was to get out of eclipse and use CVS directly to update to a label. This isn't as easy as it sounds, and involves delving into options having to do with the "sticky tag" that I don't think I got right. In any case, it resulted in one of those now familiar 200 compilation error states, so I wrote the whole thing off as a disaster and never tried again.

Next fallback? Brute force hacking. I got a new integ load of eclipse with source for my files and copied the changes in the 5 files I've made so far over using a file merge utility. As I mentioned in my 9/25 post, I've done this twice already at this point, and it is profoundly unsatisfying. As a matter of fact, it makes me feel like technological pond scum.

So the urge was strong to find the "real" way to do this. Eclipse committers can't be working this way - this is ridiculous. Thus my earlier questions to Martin, and his response about using the HEAD revision of the files I was changing plus a bunch of others. (This is presumably to catch all the changes from the likely places people are actively working, so all the changes stay in sync.) This seemed to make sense, and I was in a good enough state that my tests were all green, so I boldly used the "sync to repository" command in elcipse even though I wasn't entirely sure what it synced to (assuming HEAD) and did the merges of my changes in using functions in the "Team Synchronizing" section in eclipse.

BTW - Eclipse did a very good job with the synchronizing functions for file merging. These are better than any of the file merge utilities I was using. The usability of selecting each change and deciding whether to take the change or skip it, or which version should be the source for any particular change, or to just copy all the changes from one version to another are all great and less error-prone.

What I ended up with when I first did this had no compliation errors. I forgot to run the tests, but we'll cross that bridge shortly. So I figured there would be no problem with making some changes, syncing again and checking in, right? Slam dunk.

Imagine my chagrin when I made a bunch of changes and got the new 4 template version almost done, synced again, and a bunch of random compliation errors show up again. Now this is a problem. These errors look like half-implemented features or misaligned versions and they're not in the area I've been working in. Looks like I shouldn't try fixing them. So I can't get eclipse to build in this current state. Unless there's some other magic I am overlooking, my next instinct is to back up to my previous successful method, to sync up to an integ label.

Trouble is that before, I was always syncing from an earlier version to a later, but this time, I am going to want to extract my changes from a later version back into an earler version (HEAD to a last good integ). I can't imagine this is the way to go. I go back to earlier statements that this is ridiculous and something must be wrong here. There comes a time in every red-blooded male's life when he has to admit it's time to ask directions.

- - - - - - - - - -

Martin & Dirk,

Ran into some trouble synchronizing so I didn't get it done yet.
Will do it over the weekend.

Do you guys really sync to HEAD? Whenever I try there are always compilation
errors, so I can't get a clean build or test run.

I've been trying to sync to the last integ label, but the functions to do that
easily are missing from eclipse CVS support, and I haven't found the forumula
yet. (The "Team Synchronization" functions are cool, but I have only figured
out how to synchronize to HEAD.) I have been syncing using external tools, and
it's a pain.

So I can't use HEAD and I can't sync to an integ label. What am I missing?
Thanks,

- John

- - - - - - - - - -

As usual, Martin responded right away...

- - - - - - - - - -

John, no problem, there's no rush.
We work from HEAD with the plugins I listed, and -I forgot to say- always use
the latest integration build (I20050927-1200 currently) for the rest of the
plugins as binary (you automatically get these when you use this build to
develop with. If you are not, go to Preferences - Plug-in Dev. - Platform Target
to set the location to the latest I-build).
What are the errors you get?

- - - - - - - - - -

Hmm - Could this be the missing magic piece? When I did my last sync, I didn't update the underlying integ. If it changed out from under me between the two times I sync'ed to HEAD, that would explain why it worked the first time and not the second.

So I downloaded yet another Eclipse integ, and point my current mess over to it as Martin recommended above. There are still compliation errors. (Dang!) But not as many (Well, maybe...) Understanding that coding requires infinite patience, I look into these errors, and find they refer to files in the folders I'm synchronizing. Since I had to hand-select the files to synchronize, there's a chance I may have overlooked these. So I go back into CVS perspective, and make sure that I select every folder or file except the ones I changed and copy the latest version from HEAD over what's there.

Low and behold, the errors dissapear. I'm golden.

Next - the home stretch.

Chapter X, where I discover some nasty code, and get on my OO soapbox....

My idea was to have each subclass of NewTypeWizardPage pass in a template ID on construction, and then NewTypeWizardPage would pass the ID along to my new StubUtility.getTypeBody in a parameter.

So with new enthusiasm, I got the latest integ, hoping Martin would have committed my changes, and the source for the HEAD revisions of the packages Martin recommended above, and used the Synchronize function in the Team Synchronize view in eclipse.

When I surfed in the code however, not only were my changes not in there, but I noticed that somebody had introduced a member in NewTypeWizardPage called "fTypeKind" that identified which subclass was realizing the abstract NewTypeWizardPage. This member was accessed all over the place in NewTypeWizardPage, and was passed into the constructor.

This type of design is very frowned upon among software developers who understand OO technology. It causes a lot of maintenance headache when you want to introduce a new subclass. (You have to edit the superclass, and possibly some of the other subclasses. The whole idea is to rarely have to edit the superclass, or to even understand much of it beyond its interface.) I couldn't keep quiet about this. Not only did it make my job harder in getting the changes in, but it meant if I was to fit in with the existing design, I would have to add more of the same kind of badness, and contribute to the deterioration of the code. There was no way I was going to do this unless the committers told me explicitly that's what they wanted. So here I go on my soapbox...

- - - - - - - - - -

Martin,
Took in the latest integ, then the head revisions of the parts you listed. The
changes aren't in the head revision yet, so I re-applied my patch to my local
copy and started making the new changes. I have the new code templates done,
but haven't finished the changes to StubUtility and NewTypeWizardPage yet
because I didn't want to have merge problems with your changes.
It turns out there already are constants that identify the type in
NewTypeWizardPage, and they are placed in a member fTypeKind. It is referenced
17 times throughout the class in if branches and switch statments. This is
kind of shocking to me - I'm an OO purist and would always do overridden
methods in subclasses rather than subclass-aware logic in the parent class.
But, that would take quite a bit of cleaning up, so if you'd rather, I could
keep changes to a minimum by piggybacking on the switch statement currently in
constructTypeHeader (which I'll refactor back into constructTypeStub, the way
it was before I started) and assign the right template ID that way.
I could go either way, it's either doing some extra work to clean things up or
holding my nose and slipping in some quick-and-dirty code.
Let me know your preference, and tell me when you check in the past changes,
and I'll update, finish the new changes, and resubmit another patch.
Thanks,
- John

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

Martin's reply came back the next day.

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

I see what you mean. It's all API, so changes are difficult and enums and
annotation are recent additions. I would introduce a protected method
'getTypeBodyContent', but make sure the default implemention (with the switch)
is in the NewTypeWizardPage so that no existing clients not knowing of the
change are broken.

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

Yuck. So far, it seems that whenever there are bad programming practices in the code, they are always protected by being "API." This is why when you create an API, you need to be cautious about it in the first place so you don't get locked into a code base of deteriorating quality. I think there are is an alternative to throwing more bad code after the worse. You could deprecate the old nasty API, replace it with something better, and clean up the uses of it that you can get to. Besides, the "API" we're talking about is the constructor to an abstract class (NewTypeWizardPage) which really should be protected, and since the class is subclass-aware, you couldn't add another subclass anyway without altering the base class. This is a pretty weak API. So I stay on my soapbox and try one more time....

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

Martin - to clarify, the "API" that relates to this is the NewTypeWizardPage
constructor - and any "clients" would be subclasses of NewTypeWizardPage. The
rest of it is hidden in NewTypeWizardPage implementation. Since the superclass
is aware of its subclasses, I don't see how you could have any client that
could be affected by this, except subclasses that inherit directly from
NewTypeWizardPage, but they would have to pass in one of the four valid "type
kind" parameters and pretend to be that type kind. I couldn't conceive of
there being other subclasses, but maybe that's just me.

My inclination would not be to create a new virtual method just to pass in a
string identifier, (though I would use virtual methods to clean a lot of the
other sub-type based logic out of NewTypeWizardPage, but that's a side issue).
I was thinking this is best solved by having the subclasses pass in the data
to a new version of the constructor, then the rest is encapsulated in
NewTypeWizardPage. The problem I have with that is the switch statement on the
types in the code that used to be in constructTypeStub that I broke out into
constructTypeHeader. I don't like that way of doing things, but I like less
doing what amounts to the exact same thing with two different designs in two
different places in the same class.

So, I believe my choices are to change the NewTypeWizardPage constructor and
the constructors in all the subclasses, and pass both the template ID and the
text that will be embedded in the code that way, (to my mind, this is the
clean solution) or to piggyback the template IDs into the same switch
statement that is currently there (to my mind, this is the quick-and-dirty
solution). If the constructor calling sequences can't change because they are
API, then the quick-and-dirty solution would be the one to go with.

- John

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

Martin wrote back right away as usual, but unfortunately, I didn't win this argument either.

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

The code template framework is currently JDT.UI internal. No other plugin can
contribute a code template and also can't access code templates by an ID.
That means using the code template ID in the constructor makes no sense for
other non-JDT-UI plugin.

What I would suggest is a protected method
getTypeBodyContent(ICmplilationUnit cu, String newTypeName, String linDelim)
that by default returns the type body template according to the current
fTypeKind, but could be overridden by clients like the NewTestSuitePage (in the
junit plugin) to return a different type body.

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

Oh well. It is his code after all and he has to live with it. No matter what, I can't see the new protected method. The code already there has switch statements in it, and I don't see putting in a virtual method that switches on type right next to a switch statement that switches on an "fTypeKind" member. I'm just going to assign my template ID variable in that switch statement, and be done with it. Kind of like that part in "Alice's Resturaunt" where instead of picking up the one ton of garbage, they threw theirs down so there was two tons of garbage at the foot of the hill...

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

OK - I'll leave the constructor alone and make the changes in the class as you
suggest. I haven't seen the last patch checked in when I synchronized. I can
accumulate all the changes (including the parameter to pass the type name as
you suggested earlier) and send a new patch either tonight or tomorrow. Since
I'm removing the original "newType" code template and replacing it with the 4
subclass templates, it'll save me some change merging.

Thanks,
- John

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

Next - syncing, as in the Titanic, or, that syncing feeling....

Martin responds again with more work for me...

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

Hi John,
with 'late' I meant it was late after having all the discussion already and you
already having a patch. Time wise we are fine M3 is not so soon.
I think I would favour that approach, mostly for the consistency reason.
Thanks for the patch!
Having the CU, you can't conclude the type name. The user might create an inner
type. So I think, method getTypeBody needs an additional parameter String typeName.
I'm not sure if the new line in the template would do a difference as the body
will be reformatted and the settings in the formatter will be used. I think that
the best solution anyway.
I'll apply the patch and fix the remaining issues (type name). Thanks a lot!

- - - - - - - - - -

John, another idea: I think it would be good to have different templates for
class, interface, enum and annotation. Do you want to add that? Otherwise I can
add the change.
From the API standpoint I would add an extra 'int kind' flag, using new constants.

- - - - - - - - - -

This opened up a couple of cans of worms. First of all, there was the additional work of adding the 4 new templates. Second, doing any more work meant I had to face the fact that I hadn't figured out how to sync to Eclipse. I definitely wanted to make the change, though, it was a good idea that I had considered myself, and I wanted to do as much as I could to finish the contribution. Without thinking too much about it, I wrote back with what I thought I would have to do to make the change, and asked for advice as to how to sync.

- - - - - - - - - -

Martin - I would be glad to do the change adding different 'kinds' of templates. I was thinking of that myself, but thought it was too big a change to suggest before you guys had accepted my first change. Just to get it straight - this means 4 code templates in the Window->Preferences->(etc) dialog, and the 'int kind' enum that NewTypeWizardPage passes through to StubUtility will select the proper template?
New Java class body
New Java interface body
New Java enumeration body
New Java annotation body

How about instead of an enum, the "NewxxxWizard" classes pass the ID of the template down to StubUtility directly instead of requiring a lookup based on an enum? This way the Wizard classes don't have to refer to an enum defined in StubUtility, and it saves us the lookup.

One more thing - what's the best way to pick up your Eclipse changes and the checked in version? Should I wait for the next integ or is it safe to just pick up the head versions of the affected few files? I just haven't figured out what to sync to in CVS, so I'm using the last integ build and wiping it out and starting over each version. I haven't done any really iterative development yet.

Thanks,- John

- - - - - - - - - - -

Martin wrote right back.

- - - - - - - - - - -

You can take HEAD, but sometimes also you need some referenced plugins in HEAD.
Follow the jdt-ui dev mailing list for messages about what other plugins are
needed in HEAD until the next I-build. It happens from time to time that we
forget to write this on the mailing list, but here are the candidates:

org.eclipse.core.filebuffers
org.eclipse.jdt.core
org.eclipse.jface.text
org.eclipse.ltk.core.refactoring
org.eclipse.ltk.ui.refactoring
org.eclipse.text
org.eclipse.ui.editors
org.eclipse.ui.workbench.texteditor

Sure the kind constants can be the template keys (good idea), but you have to
check them before using them (throw an illegal argument exception otherwise)!

- - - - - - - - - - -

OK - so far so good, a little encouragement and a new thing to try with the syncing. So next time I delve into the code and try to see how to fit my new changes in....

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.

Setting up a CVS download of Eclipse so I can get to development....

CVS is available for windows at:
http://www.wincvs.org/download.html

I downloaded it today, and tried to get into the eclipse CVS site using WinCVS, but wasn't successful. It's easier just to use Eclipse directly.

Instructions for setting up CVS for eclipse, for both committers, and common slobs like me:
http://dev.eclipse.org/cvshowto.html.

Still haven't gotten all the source downloaded yet, my connection is slow for big bulk downloads. I think it's my home DSL ISP, not the eclipse servers, because it runs fast when I download from the network at work.

Dirk replied quickly to my bug comment, as he did to the original submission.

------- Additional Comment #3 From Dirk Baeumer 2005-08-29 04:24
[reply] ------- John,
something we have to keep in mind is the fact that the current "New Java File"
template is API. Clients can create a new CU using the CodeGeneration API class
which basically forwards to StubUtility. We have to make sure that current
clients will not get broken by "reusing" the current ${type_declaration}
variable. So before going down that path we need to investigate what your
approach means regarding API and backwards compatibility.
I still tend to introduce a new variable (it avoids all the API and
compatibility issues). What we can do is adding a new varaible ${type_content}
which is a sub variable like you suggested.

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

Dirk -
OK, I can set it up as a new variable: ${type_content}.
I am still concerned about the user experience with both the ${type_declaration} and the ${type_content} variables available. It seems it is asking for confusion along the lines of "I changed the definition in ${type_content} but the new generated class still didn't change..." because they were using ${type_declaration} and didn't notice the difference. I assume it will be easy enough to change if I get it working this way, and it looks like the API and regression tests can still work with the re-implementation. It seems to me it will be dependent on how we integrate the new changes, but since I haven't done it yet, I can't prove it one way or the other.
Got CVS set up, and I'll try to run the original tests next.

That's all for tonight. Next comes downloading the source and running the current test suite....

The eclipse committers responded to my bug right away, early in the morning after I submitted it. Here's the response:

------- Additional Comment #1 From Dirk Baeumer 2005-08-26 06:31
[reply] ------- John,
I really encourage you to investigate further in this topic and try to provide a
patch for it that can be included into Eclipse.
I favour solution one. The only porblem I see with it is that
${type_declaration} can represent a type containing more than just an empty
body. For example if you check any of the "Generate method stubs" options on the
wizard how will this go together with the type body you are providing. So you
need more than just one variable. The templete has to look more like this:
{$ype_declaration_header} {
// A comment
{$main_method}
}
So we need variable for the main method, constructor and abstract inherited
methods as well.
The "procedure" for getting code into Eclipse is as follows:
- the code should be provided as a patch against a reasonable "latest" version
of the Eclipse code (e.g. against the latest integration build). The code
can be found in the Eclipse CVS repository. The project you are looking for
is org.eclipse.jdt.ui (some basic steps how to get the source can be found at
the end of
http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/jdt-ui-home/r3_2/main.html#Overall_Planning)

- you have to provide test cases as well. The tests has to go into
org.eclipse.jdt.ui.tests
- all code patches have to be provided as attachments to bug reports. We can
use this bug report for this
I am not the expert for code templates and Martin (the expert) is currently out
of office. But as a starting point you might want to have a look at the class
StubUtility which is the facade for all template based code generation.

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

I read it later that day, and was excited they were so positive about the idea. Especially interesting was Dirk's idea about template variables that show where to put the generated methods inside the class. I had also spent some time considering how to place generated methods, but hadn't considered using template variables to indicate their location.

In considering how this would work, I started getting uneasy about the user experience with mutually exclusive template variables and with the template variables for the positions of methods inside the class being coupled with one declaration template variable and not another. This implies some complex and confusing user experience, so I chewed on it for a while and came up with another idea, expressed in my next comment to the bug, composed here.

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

Dirk,

Thanks for your reply, and I appreciate the encouragement.

I studied the source for a few sessions and thought about your ideas of other template variables to show the location of generated methods. That makes things harder to implement, but it will definitely make a better user experience. The way I had originally thought of it (and the way I still want to implement it as a first iteration before I take on the additional complexity) would have the comments generated in the right place inside the class, but all the stub methods would just be generated at their default location, which I'm assuming (hoping) is at the top of the method. Then, for a second iteration, I'd add the template variables to indicate where the different types of generated stub methods should go, and allow them to be interspersed with the comments.

After considering your suggestions and my original ideas for a while, I became more uneasy about the user experience if we allow alternate versions of the template variables (${type_declaration} and ${type_declaration_header}) and then offer other template variables that the user has to be aware are coupled with only the ${type_declaration_header} version, and further require the user to understand they have to manually place their class brackets. It just sounds like there are a lot of failure cases and a lot of potential user confusion.

What I came up with after some headscratching is a new suggestion. Instead of offering an alternate ${type_declaration} version, why not have ${type_declaration} insert a user-definable "sub"-template like ${filecomment} and ${typecomment} currently do? That way, the template variables available for showing where the generated method stubs go in the class can be offered only in the ${type_declaration} editing context, and we no longer have to worry about the users understanding how they are coupled. Along with that, the code behind the type stub generation can put in some protective default behavior if the user forgets to add one or both brackets (default placement is right after the class declaration header for the open bracket, and at the bottom off the template for the close bracket). Finally, the default definition of the "sub"-template can be set to the current behavior, taking care of any backward-compatibility user expectations.

In addition, this leaves open the future possibility of allowing users to define different templates for the different types: interfaces, classes, annotations, enums.

Let me know if this sounds like a plan, or if anyone over there has any further suggestions, and I'll get started.

As far as process, I will get a CVS download set up. Up until now, I have been downloading the source from your download site, but that was just to do some preliminary surfing. I like to do test-driven development, so I'll try to get some bare-bones test cases written first. That way, I can work through the logistics of building and running the tests before I get very far. Or perhaps I'll just try to mimic the current behavior with the new user-definable class template variable, so the current test cases will run.

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

That's all for tonight - next, as promised, I will attempt to get a CVS build up and running, and to run the current JDT tests...

Deeper into the source...

I'm a child of the Mac UI revolution of the mid 80's. My first IDE was Lightspeed Paschal, and I still think the far superior way to "know" code is to watch it do it's thing directly in a source-level debugger. I still catch myself making hidden derisive snorts when I hear excellent and talented developers extolling the virtues of printf statements for debugging. Printf statements have their place, but when you have a debugger available, they are like trying to explore your backyard through laboriously drilled holes in a neighbors fence from two blocks away.

Especially to get to know new, complex code quickly, I like to trace it a few times in a debugger and familiarize myself with the runtime flow of the design, as well as the developer's language and style.

My wife and girls went shopping today and gardening and chores were done, leaving me with some precious weekend afternoon spare time. I put Miles Davis Get Up With It on the stereo and started tracing through NewTypeWizardPage.createType() on my PDE set-up of a recent integration build of Eclipse 3.2. (While eating some fresh cantaloupe melon and a cup of Horizon blueberry yogurt. I agree with Kent Beck that "there must always be food" but further than that, development is a complete experience. Just the right mood, tunes, and food make it go so much better....)

// Even more boring part starts here....
// You may skip to where the marginally less boring part starts up again, I won't be offended...

NewTypeWizardPage.createType()
calls
StubUtility.getCompilationUnitContent()
retrieves the user defined template by calling StubUtility.getCodeTemplate() with ID of CodeTemplateContextType.NEWTYPE_ID
Despite the similar but more limited text that has already been generated and put into "content" in NewTypeWizardPage.createType(), the template text is here, a seeming duplication with some additional content and some (like the package name and imports) missing content.
calls
StubUtility.evaluateTemplate()
that immediately calls
CodeTemplateContext.evalulate(), passing the template read in above
calls
TemplateTranslator.translate(), passing the text pattern (the user entered template) from the pattern
calls
TemplateTranslator.findVariables() after "translate" strips out the dollars and braces.
this marks the positions and lengths of the identifying text of the template variables that are embedded in the translated string.
pop back up to CodeTemplateContext.evalulate(), now that all the variables have been parsed and translated, then it calls CodeTemplateContextType.resolve(), which calls TemplateVariableResolver.resolve() on each template variable to expand the text of each variable and process them into the translated string.
It builds up an ArrayList of ReplaceEdit objects, each object representing the replacement of the template variable identifying text with the actual text value of the template variable that has been processed earlier in this operation.
Then TemplateVariableResolver.resolve() creates a Document object and initializes it with the translated text pattern that was processed above (now in the "buffer" variable).
Then it creates a MultiTextEdit object, passes it the variable positions and list of ReplaceEdit objects processed above, and calls "apply" on it (which resolves to TextEdit.apply) to call a big stack of subroutines that eventually sort everything out.
TextEditProcessor.performEdits()
MultiTextEdit(TextEdit).dispatchPerformEdits(TextEditProcessor)
TextEditProcessor.executeDo()
MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, IDocument)
ReplaceEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, IDocument)
ReplaceEdit.performDocumentUpdating(IDocument)
Document(AbstractDocument).replace(int, int, String)
Document(AbstractDocument).replace(int, int, String, long)
GapTextStore.replace(int, int, String) - this finally replaces the text.

Now pop back up to NewClassWizardPage(NewTypeWizardPage).constructCUContent(ICompilationUnit, String, String) and this newly created string is put into a new local string called "content". Went back and reviewed the relation between the calling createType "content" variable I mentioned early in this stack. It looks like that is just used to help initially create the "type stub" text that will eventually be inserted into this new "content" variable. It is not used later in createType after constructCUContent returns.
Back in constructCUContent, it creates a new ASTParser to process the newly processed source text. If all goes well, it returns the content to createType, which spreads it around a bit and then wraps up. There is a case, however, when it creates a new string buffer, and reconstructs the content from the package and type information one more time and returns that. I haven't hit that case yet, but will probably need to know about it at some point.

// Even more boring part ends here, marginally less boring part starts up again....

I know seems like a lot of useless rigamarole for those not surfing right with me. That's perfectly right - it is a lot of useless rigamarole. But it may be helpful for reference later and save me a few round trips searching in the source.

The upshot is that the action in creating a source template is all in the template variable processing. Up in createType(), the constructTypeStub() routine creates the class declaration that I'm interested in, and initializes some variables for subroutines, then tosses everything down to constructCUContent() that grinds through the already created information to sort everything into its place. This suggests the way to get my changes to work are to create a sibling to constructTypeStub() inside createType() that constructs the stub for my new template variable, which is close to the same as the original, but is missing the brackets. This way, either the original template variable or the new template variable can be called, and the program will react accordingly. There are a couple of wrinkles to investigate, however. The first is what about user behaviors that violate the implied assumptions (using the original ${type_declaration} variable implies no brackets in the user part of the template, but the new ${type_declaration_no_brackets} implies there are. What if the user doesn't comply with the brackets? What if the user puts in both template variables? And, will this work at all in contexts where methods are initially inserted into the class source (like starting from an interface, or checking some of the "create method stub" options on the wizard.) Will have to get to those in a later installment...

Copy of the text of the bug I filed into eclipse this evening.
It is logged in bugzilla as #108071.
https://bugs.eclipse.org/bugs/show_bug.cgi?id=108071


In the "New Java Files" code template, can't add comments inside the newly created class.


Problem:
Users can't put comments in a Java class template that are to be embedded inside the class (between the class brackets), so every new class starts with the comments in the right place without requiring the user to edit each class by hand after it's generated.

e.g. I want something like this:
public class Blee {
// constants

// member variables

// constructors

// etc...
}

...but I get this:
public class Blee {

}
// constants

// member variables

// constructors

// etc...

..or if I put in my own brackets, I get this:
public class Blee {
}
{
// constants

// member variables

// constructors

// etc...
}

Steps to reproduce:
Open Window->Preferences->Java->Code Style->Code Templates->Code->New Java Files
Edit the "Pattern:" window to include something like this:

${type_declaration}
// comment

..to get the first result above, or this:

${type_declaration}
{
// comment
}

..to get the second result above.

The Pitch
This is a convenience I've been using for 10 years before I used eclipse. You start with the same file template and fill in what you need, so you always know the basic order of items in every class, and don't have to keep typing in the section headers yourself. Having all files organized the same way with section headers is a best practice that many well-respected developers recommend. Here are some references. They all present their schemes for ordering functions within a file or within a class (as the language warrants) and their recommended commenting formats.

J2EE Design and Development - Rod Johnson p.144-5 "Delimit sections of code."
Code Complete - Steve McConnell p.449 "..order the source file carefully."
C Style: Standards and Guildelines - David Straker p.125 "Sort functions by a standard scheme..."

OK, so it's a relatively small thing, but it bugs me enough I'm willing to fix it myself and contribute the change. All I need is somebody to review the change and give me some advice on logistics (e.g. do you need a CVS changelist or can I e-mail source files, what tests should I run/fix up etc...) and to help choose a preferred solution from the alternatives.

Proposed Solution Alternatives:
1.) Create a new template variable that doesn't hard-insert brackets and allows users to insert their own brackets where they choose in their template. This is my preferred solution for a couple of reasons. First, it is an addition, so it doesn't affect the current user behavior by design, and so is safe from causing regressions based on assumptions of current usage. (Of course, I understand it may cause regressions based on implementation, but I pledge to handle those.) Second, it gives the user the ultimate flexibility in controlling exactly what appears after the type declaration before the opening class brackets, what appears between the class brackets, and what appears after the class brackets. It would go something like this:

${type_declaration_no_brackets}
{
// the template class comments...
}

and could accomodate something as bizarre as this:
${type_declaration_no_brackets}
// some squirrelly thing here, perhaps a comment or future metadata tag
{
// the template class comments...
}// some other squirrelly thing, like a tool-specific embedded comment

2.) Change the behavior so the new type wizard detects if the user has inserted brackets in the template, and if so, uses these brackets instead of hard-inserting their own. This saves creating a new template variable, and seems like a sophisticated solution, but has a couple of strange corner cases. These involve things like putting brackets in a comment that is intended to go after the class, like an embedded comment that some tool would want in the code:

${type_declaration}

// xp3911{unintelligible-junk}

The code would have to be smart enough to parse comments and not include brackets in comments in its detection. Generally, this solution isn't as safe as the first because it changes the behavior of the current ${type_declaration} template variable and may violate some oddball user assumption somewhere.

3.) Change the behavior so the new type wizard considers everything below the ${type_declaration} template variable to be embedded in the class rather than below it. This was the first solution I thought of, but I like it the least because it changes the default behavior of the current ${type_declaration} template variable the most severely, and doesn't have the flexibility to accomodate the squirrelly comments the first solution can handle. Blech. Included here just for purposes of discussion.

So whaddya say guys? Will you accept one of these solutions (or a variation as we agree) if I implement it right? Isn't this what the spirit of eclipse is all about?