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....