Patches: how to distribute? scope?

The RPTools applications are written in Java. If you're interested in contributing to any project here by submitting patches to the source code, this is the forum to ask questions about how to do so. Please put the two-letter tool name abbreviation in your thread Title. To enter this group, go to the Usergroups page of your User Control Panel and join the Java Developer group.

Moderators: dorpond, trevor, Azhrei

Locked
User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Patches: how to distribute? scope?

Post by Azhrei »

Hey, Trevor.

I thought about making this a PM but figured it would be good for others to be aware of as well.

Q1: How do you want the scope of patches contained?

I have been doing a lot of cleanup. Thanks to some testing by Phergus, the Load/Save Map feature has had a fixed implemented and seems to work. Obviously, I had to add entries to the properties file. Since I was in there, I figured I'd also correct some semantic issues with the menu selections (like adding ellipses to the ends of some menu text).

Then I started work on the menu accelerator shortcut. I have that code mostly working (the first few items on the Edit menu are added dynamically, so those still need to be done).

I've been adding javadoc to the code as I go along as well. I've fully documented the I18N class (which was easy -- it was small!) and branched out to a couple of others.

In addition, I added isDevelopment() to the MapTool class to allow for a "cleaner" end-user environment while still being able to see debugging output during testing. (The method simply checks the version string and does the obvious.)

How do you want such a conglomeration of patches submitted? I would like to be able to submit individual patch sets for each separate area, but I don't see any easy way to do that. I'm fairly new to SVN but it appears that the branch functionality is supposed to do that... Can the SVN repository have access opened up for each developer under the branches/ location? I'm thinking branches/craig, branches/azhrei, and so on. Or if there's a better way, I'm all ears. :) (I've read chapters 4 and 5 of the SVN book, but only once. I'll read it again soon.)

And is there any benefit (I think there is) for developers to highlight the entire file and choose Source -> Format on the Eclipse menu? That would keep the formatting the same across all patch submissions.

Q2: How should patches be sent?

For example, Phergus has tested my load/save map changes because I uploaded the diffs to my web site. Is sending a patch set like that an acceptable approach in your point of view? Is there a better technique, or one you'd prefer above a patch set?

Again, if the branches work the way I expect them to (!), that would probably be a better solution. You could merge from a branch at any point.

Q3: Do we need a WIP list? (There were only supposed to be two questions!)

I'm thinking that with so many people working on the code at one time, should we have a work-in-progress thread somewhere? Maybe the wiki is the best place for it? Something that a developer can edit to indicate what projects they're working on so that other folks don't duplicate their work.

==========================

Okay, let the discussion begin! I've got to get to bed 'cuz I'm up in a few hours to hit Busch Gardens and I'm not going to last the whole day unless I crash now. :)

Phergus
Deity
Posts: 7132
Joined: Fri May 12, 2006 8:56 pm
Location: Middle of Nowhere, NM
Contact:

Re: Patches: how to distribute? scope?

Post by Phergus »

Branches would probably be a good route and would allow for more testing prior to actual releases.

The current method of sending Trevor patch files seems to work okay and there really isn't a lot of difference between getting two different patch sets to merge in and merging off of two different branches back into the trunk. In the case of the branches it is easier to look and see what the intent was in the case of conflicts and get a working fix before doing the merge.

I would like to see some guidelines defined for working on the code. I like the idea of requiring the use of Source -> Format before checking source back in. Along with Ctrl-Shift-O to clean up the Imports.

Some kind of coordination mechanism like a page on a Wiki would be good. Especially in the case where someone is working on something that impacts a large number of files or goes for several days.

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

Thanks, Phergus. Anyone else? Craig? Trevor?

I'm inclined to send Trevor what I have now, even though the string externalization isn't complete. I find things like javadoc to be very useful and I'm thinking other developers may too. (But man, is that a large job!)

Okay, here are some guidelines. Each of these should be accomplished before generating patch sets:
  1. Thou shalt always highlight the content of any Java source code thee hath modified and select Source -> Format. Note: this requires that the user reset the line length in the Eclipse properties: Trevor wants the line length set to 200 characters.
  2. Thou shalt always select Source -> Reorganize Imports.
  3. Thou shalt always submit code that includes Javadoc comments for major classes and methods. (T'would be nice to require full Javadoc for everything, but alas, that is unlikely.)
  4. Thou shalt always use the /* */ style of comments in front of classes and methods and may use single-line comments in front of member variables and small snippets of code, but See Rule #3!
  5. Thou shalt always use parameter names different from member field names so that disambiguation using this is not necessary.
  6. Thou shalt always clearly delineate private constructors with comments so that those who come after thee may retain thy sanity.
  7. Thou shalt never use hard-coded strings in code when a property from an external file can be used. (In MapTool's case, this means calling I18N.getText(propertyKey) and adding a definition for the propertyKey to i18n.properties. Also, all of the show*() methods in MapTool, such as showError() and showWarning(), take propertyKeys as well as strings -- only use propertyKeys!)
  8. Thou shalt always use static final String's when hard-coded strings are appropriate. Examples include resources that are embedded inside the MapTool JAR, such as unknown.png -- the question mark image. Other images that may be considered part of the "theme", such as toolbar images, button images, and so forth, should be retrieved from an images property file; I propose images.properties since we already have sounds.properties. (A string in the code would reference a pathname in the property file.)
  9. Thou shalt report all exceptions that are true errors. (InterruptedException while waiting for a timer can be ignored, for example. But all other errors should be handled by calling MapTool.showError(propertyKey) or similar and passing both a propertyKey and the Throwable object representing the exception.)
  10. Thou shalt always use defined properties instead of hard-coded strings when possible. (Such as File.separator instead of "/" and now AppActions.menuShortcut instead of "ctrl".)
There are surely others that you want added, or you probably don't like a few of these and want to take them out. Please speak up. :)

Edit: I've attached an exported set of Eclipse Preferences as a ZIP file. Unpack the ZIP and use File > Import... to read them. I've commented out some lines in the file that I thought might confuse someone else's setup such as the default Workspace location. If it won't load for some reason, you might try uncommenting those lines. There are 35 such lines commented out.
Attachments
eclipse-preferences.zip
Unpack and use File > Import... to read these Preferences while in Eclipse.
(24.23 KiB) Downloaded 180 times

Craig
Great Wyrm
Posts: 2107
Joined: Sun Jun 22, 2008 7:53 pm
Location: Melbourne, Australia

Re: Patches: how to distribute? scope?

Post by Craig »

Azhrei wrote:Thanks, Phergus. Anyone else? Craig? Trevor?

I'm inclined to send Trevor what I have now, even though the string externalization isn't complete. I find things like javadoc to be very useful and I'm thinking other developers may too. (But man, is that a large job!)
Both the javadoc and string externalization would be big jobs :) Which reminds me since I have just started cleaning up a lot of null pointer exceptions and fixing some of the error messages in the macro functions I should probably externalize those strings. Since you are in the I18n class would you be able to add 2 methods
getString(key, ...)
getText(key, ...)
That format the arguments with MessageFormat. I can supply the whole of the methods if you like. That would make it a lot easier to externalize a lot of the strings in the macro area.


I don't have any huge problems with the list you posted. Although its all up to Trevor what he wants ;).

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

Craig wrote:Both the javadoc and string externalization would be big jobs :)
Tell me about it. :( One of the things I decided to do what to create a structure for the keys I was going to be adding; something that fit well with the existing ones. Currently, all Actions use "action." as the prefix for the key. All messages use "msg." with some being "msg.info." and others being "msg.error." I added "file.ext." and (another one that I don't recall right now).

(edited the previous post to include another item on the list.)
Which reminds me since I have just started cleaning up a lot of null pointer exceptions and fixing some of the error messages in the macro functions I should probably externalize those strings. Since you are in the I18n class would you be able to add 2 methods
getString(key, ...)
getText(key, ...)
That format the arguments with MessageFormat. I can supply the whole of the methods if you like. That would make it a lot easier to externalize a lot of the strings in the macro area.
Okay, no problem. Makes sense actually, since I had to do maybe 5 or 6 of those while externalizing the other strings.

There's also a decision to be made: should error messages use the same string from the properties file over and over or should there be a separate one for each error? The first is obviously more efficient, but if errors are occurring so often that efficiency is a concern then there's a much larger problem! ;)

I may look into using reflection on error messages so that the class and method can be included at the end of the error message.
I don't have any huge problems with the list you posted. Although its all up to Trevor what he wants ;).
Okay, cool.

User avatar
gioppo
Giant
Posts: 145
Joined: Tue Dec 16, 2008 9:09 am
Location: Torino (Italy)
Contact:

Re: Patches: how to distribute? scope?

Post by gioppo »

I'm also interested in this topic.
I was looking for developers instruction, but have not found anything (I'm lazy so I can have missed it).
If there are some coding style to observe I'd like to know.
Also for patches which is the rule?
Is there a dev list to discuss code matter that could be boring for the forum and people interested could miss?

I do not pretend to be a full time developer, but since that if I'm able I like to give something back to open source projects that help me having a good product and I know that each project have it's own rules I'd like how to collaborate with the best gain of everybody (me that do not waste time [happened a few time and it's a pity], the project that can get a hand), I do not like to make personal hacks and if I manage to add something usually there is little difficulty in doing in a way that can be included in the baseline.
Luca

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

I was really hoping Trevor would chime in on this one and give us some direction as to how he saw things proceeding.

Trevor?

User avatar
trevor
Codeum Arcanum (RPTools Founder)
Posts: 11311
Joined: Mon Jan 09, 2006 4:16 pm
Location: Austin, Tx
Contact:

Re: Patches: how to distribute? scope?

Post by trevor »

Heh, I think I break about half of those guidelines every time I sit down to code on MT :)

Generally speaking, you should send me a patch whenever you finish a feature or set of cleanup or whatever. Sending the patch to me via email seems to work fine.

I'm not too worried about the formatting because I can format just as easily.
Dreaming of a 1.3 release

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

I'm going to sticky this and put it in the Bug Reports forum. When we (meaning the currently active Java developers) have settled on what we like/don't like, I'll put everything into a single post at the beginning of this thread.
trevor wrote:Heh, I think I break about half of those guidelines every time I sit down to code on MT :)
I know. Me, too. But discipline is important when there are going to be many cooks in the kitchen. Granted, I'm only doing salads and not entrees, but still... ;)
Generally speaking, you should send me a patch whenever you finish a feature or set of cleanup or whatever. Sending the patch to me via email seems to work fine.
Okay.
I'm not too worried about the formatting because I can format just as easily.
So you don't mind receiving a huge patch full of lines that are mostly just formatting changes? That's a relief. I was worried that it would be a huge pain. :( I don't know if files can be highlighted in the Package Explorer and then Source -> Format can be used, but if so it might be a Good Thing to do that for b50. A lot less spacing changes after that.

A new patch from me will be in the mail.

Now: what do I do about other features? For example, I want to take a look into the disappearing table problem. If/when I fix it, the next patch generated will include all the old stuff again. Ugh! What do you think about opening up the permissions on the branch section of the SVN server?

User avatar
gioppo
Giant
Posts: 145
Joined: Tue Dec 16, 2008 9:09 am
Location: Torino (Italy)
Contact:

Re: Patches: how to distribute? scope?

Post by gioppo »

Having a branch on the SVN could be usefull.
On the formatting of the source code I find that it is a bit unconfortable.
I did as proposed, but since, obviously it takes time to Trevor to accept the patch I have my "home branch" quite reformatted and the merge tool gives me lots of differences and I run the risk of loosing some stuff (I have to say that I do not go with automated merge, but prefer to accept each mod, I've learned that the time so spent is regained with lesser merge problems).
I propose that at some time Trevor should issue a code freeze and who can help (unless Trevor want to do that job on his own) should just submit formatted code (we could divide packages).
Luca

User avatar
jfrazierjr
Deity
Posts: 5176
Joined: Tue Sep 11, 2007 7:31 pm

Re: Patches: how to distribute? scope?

Post by jfrazierjr »

Azhrei wrote: So you don't mind receiving a huge patch full of lines that are mostly just formatting changes? That's a relief. I was worried that it would be a huge pain. :( I don't know if files can be highlighted in the Package Explorer and then Source -> Format can be used, but if so it might be a Good Thing to do that for b50. A lot less spacing changes after that.

If you do formatting changes, we may want to talk Trevor into giving up his Eclipse format settings. They can be exported and imported per project, so everyone could be on the same page when using Source->Format, instead of everyone getting their own stuff that might then need to be reformatted again by Trevor....
I save all my Campaign Files to DropBox. Not only can I access a campaign file from pretty much any OS that will run Maptool(Win,OSX, linux), but each file is versioned, so if something goes crazy wild, I can always roll back to a previous version of the same file.

Get your Dropbox 2GB via my referral link, and as a bonus, I get an extra 250 MB of space. Even if you don't don't use my link, I still enthusiastically recommend Dropbox..

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

The only big issue he relayed to me was that he didn't like the way the formatter split the long lines into shorter ones. With which I (partially) agree.

I've been trying to get him to set his Eclipse Preferences to allow really long lines. (Under Preferences -> Editor there's a "formatting" section. And the line wrap there I set to 160 so that long lines won't wrap. Of course, any line that long probably should be wrapped somewhere, but presumably that's left to the programmer.)

But I'm betting he's got better things to do. ;)

User avatar
trevor
Codeum Arcanum (RPTools Founder)
Posts: 11311
Joined: Mon Jan 09, 2006 4:16 pm
Location: Austin, Tx
Contact:

Re: Patches: how to distribute? scope?

Post by trevor »

I've set my line wrap to 200, which seems like it's working mostly the way I like.
Dreaming of a 1.3 release

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: Patches: how to distribute? scope?

Post by Azhrei »

Can you spend the time to reformat some chunks of code before checking them in? The more stuff you check in with the formatting corrected, the easier it'll be to apply patches. And the best time to do that is right after a build, before individual contributors have built up huge patch sets!

Locked

Return to “Java Programming Info”