Menu
▾
▴
rails-devel
|
From: Freek D. <sf_...@ma...> - 2008-05-24 18:35:52
Attachments:
defaultgame.diff
|
Hi, I recently downloaded 18xx rails, and like it (even though I only own 1856, which is not yet complete). I like to help, though I have only very limited time on my hands. However, I might be able to make minor adjustments for things I run into. For example in the UI, it is sometimes beneficial to see more information (e.g. how many trains the bank still has, the upgrade value of a city while laying a tile, stricter tile-laying rules -companies can now place unreachable tile-, saving of preferences between games, unlimited tiles -a variant I prefer to play-, perhaps even automatic route calculation) So two questions: what are the coding rules? In particular to spacing? I'm most customed to Python at the moment (not Java, sorry), and I'm getting nuts by the mix of spaces, 3-space tabs, 4-space tabs and 8-space tabs. Please pick one (I don't care which) and be consistent. Secondly, how should I submit these minor patches? Sending them to the list, uploading to sourceforge, ...? For starters, here is a 4-line patch so I can specify a preferred game type in my.properties. Regards, Freek Dijkstra PS: I'm using Mac OS X, and made a Xcode project (instead of using Eclipse). If anyone is interested, let me know. |
|
From: Erik V. <eri...@hc...> - 2008-05-25 20:42:11
|
Hi Freek, Welcome to the club! Your patch looks useful to me, but I'll leave it to Brett to incorporate it. Coding rules: I have written most of the code, and am using 4-space indents. Things go wrong when Brett reformats with his (I believe) 3-space indents ... ;-) Indeed we have never settled that and it sometimes looks ugly. I propose 4 spaces.... (Tabs or spaces: does it matter? Should I care? Not sure how to control it). Submissions: start with sending patches like these. In some stage Brett may give you commit access. For what it's worth, some initial comments about your further suggestions: > how many trains the bank still has, You can see that in the GameStatus window. The subject of duplicating more info from there into the OR window is coming up regularly. I'm not very much in favour of that, as the OR window is already overcrowded - at least at my resolution it often gets higher than my screen, so that I must scroll the map. In the past we had it split into two windows, and I'm not convinced that it was a good move to combine these. But perhaps you actually meant putting the number into the text of the Buy Train popup selction box. That's OK, but it's not as trivial a change as it seems. > the > upgrade value > of a city while laying a tile, E.g. as a tooltip for the upgrade tiles at the left-hand side of the map? Good idea. > stricter tile-laying rules > -companies can > now place unreachable tile-, ... and tokens, yes. The concept of 'routes' (or just 'reachability') has not been implemented yet. IMO it's not a 'minor' change. > saving of preferences between games, What preferences? I can imagine a UI to manage my.properties might come in handy, althoug I wouldn't consider it a priority. > unlimited tiles -a variant I prefer to play-, Currently we only have per-game variants/options, but this one would qualify as a useful generic option, perhaps to be put in my.properties. > perhaps even automatic > route calculation) ... and revenues, I suppose you mean... Sure we want that, but this is definitely not a minor update.... Best regards, Erik Vos |
|
From: John A. T. <ja...@ja...> - 2008-05-25 21:18:19
|
Erik Vos wrote:
> Coding rules: I have written most of the code, and am using 4-space indents.
> Things go wrong when Brett reformats with his (I believe) 3-space indents
> ... ;-)
> Indeed we have never settled that and it sometimes looks ugly. I propose 4
> spaces....
> (Tabs or spaces: does it matter? Should I care? Not sure how to control it).
>
FWIW, on Google Web Toolkit we use the Checkstyle plugin for Eclipse,
and define Eclipse format settings so everyone gets the same result when
they autoformat.
--
John A. Tamplin ja...@ja...
770/436-5387 HOME 4116 Manson Ave
Smyrna, GA 30082-3723
|
|
From: brett l. <wak...@gm...> - 2008-05-26 06:39:54
|
On Sun, May 25, 2008 at 1:42 PM, Erik Vos <eri...@hc...> wrote: > Hi Freek, > > Welcome to the club! Your patch looks useful to me, but I'll leave it to > Brett to incorporate it. It looks good to me, but I'm not going to have a chance to merge it until mid-week. Erik, if you want to merge it sooner, feel free. > Coding rules: I have written most of the code, and am using 4-space indents. > Things go wrong when Brett reformats with his (I believe) 3-space indents > ... ;-) > Indeed we have never settled that and it sometimes looks ugly. I propose 4 > spaces.... > (Tabs or spaces: does it matter? Should I care? Not sure how to control it). > Yes. 4 space indents, expand tab characters to spaces is fine by me. I must apologize for this, as its entirely my fault. I work from a couple different machines and haven't been consistent about my formatting settings. In general, please just follow typical Java conventions. > Submissions: start with sending patches like these. In some stage Brett may > give you commit access. > My rule of thumb is submit around 4-5 patches, and then I'm usually seen enough of your work to be comfortable allowing you to commit directly. > Erik Vos ---Brett. |
|
From: John A. T. <ja...@ja...> - 2008-05-26 07:10:21
|
brett lentz wrote:
> On Sun, May 25, 2008 at 1:42 PM, Erik Vos <eri...@hc...> wrote:
>
> Yes. 4 space indents, expand tab characters to spaces is fine by me.
>
> I must apologize for this, as its entirely my fault. I work from a
> couple different machines and haven't been consistent about my
> formatting settings.
>
> In general, please just follow typical Java conventions.
>
If everyone is using Eclipse, I strongly suggest setting formatting and
checkstyle rules and everyone following those. Otherwise, you wind up
with tons of whitespace changes in the diffs.
--
John A. Tamplin ja...@ja...
770/436-5387 HOME 4116 Manson Ave
Smyrna, GA 30082-3723
|
|
From: Erik V. <eri...@hc...> - 2008-05-26 21:17:00
|
Well, it ssems that not everyone is using Eclipse. But I take your point and will have a look at Codestyle. Erik. > -----Original Message----- > From: rai...@li... > [mailto:rai...@li...] On Behalf > Of John A. Tamplin > Sent: Monday 26 May 2008 08:10 > To: Dev...@sc...; > li...@sc...; > fo...@sc...; > "Ra...@sc...":an 18xx game > Subject: Re: [Rails-devel] Patchfiles > > brett lentz wrote: > > On Sun, May 25, 2008 at 1:42 PM, Erik Vos > <eri...@hc...> wrote: > > > > Yes. 4 space indents, expand tab characters to spaces is > fine by me. > > > > I must apologize for this, as its entirely my fault. I work from a > > couple different machines and haven't been consistent about my > > formatting settings. > > > > In general, please just follow typical Java conventions. > > > If everyone is using Eclipse, I strongly suggest setting > formatting and > checkstyle rules and everyone following those. Otherwise, > you wind up > with tons of whitespace changes in the diffs. > > -- > John A. Tamplin ja...@ja... > 770/436-5387 HOME 4116 Manson Ave > Smyrna, GA 30082-3723 > > > -------------------------------------------------------------- > ----------- > This SF.net email is sponsored by: Microsoft > Defy all challenges. Microsoft(R) Visual Studio 2008. > http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ > _______________________________________________ > Rails-devel mailing list > Rai...@li... > https://lists.sourceforge.net/lists/listinfo/rails-devel > |
|
From: Erik V. <eri...@hc...> - 2008-05-26 21:24:38
|
> On Sun, May 25, 2008 at 1:42 PM, Erik Vos <eri...@hc...> wrote: > > Hi Freek, > > > > Welcome to the club! Your patch looks useful to me, but > I'll leave it to > > Brett to incorporate it. > > It looks good to me, but I'm not going to have a chance to merge it > until mid-week. Erik, if you want to merge it sooner, feel free. Here I must confess, that I have never taken the trouble so far to sort out how to deal with such patches... :-( > > Coding rules: I have written most of the code, and am using > 4-space indents. > > Things go wrong when Brett reformats with his (I believe) > 3-space indents > > ... ;-) > > Indeed we have never settled that and it sometimes looks > ugly. I propose 4 > > spaces.... > > (Tabs or spaces: does it matter? Should I care? Not sure > how to control it). > > > > Yes. 4 space indents, expand tab characters to spaces is fine by me. My current setting is 4 with tabs, but in fact I almost never reformat, because the formatter tends to break lines in ways I don't like. I'm pretty good at formatting manually (he boasted), but no doubt tabs and spaces get mixed this way. But I agree that it's time to sort this out. Erik. |
|
From: brett l. <wak...@gm...> - 2008-05-27 02:01:42
|
On Mon, May 26, 2008 at 2:24 PM, Erik Vos <eri...@hc...> wrote: >> On Sun, May 25, 2008 at 1:42 PM, Erik Vos <eri...@hc...> wrote: >> > Hi Freek, >> > >> > Welcome to the club! Your patch looks useful to me, but >> I'll leave it to >> > Brett to incorporate it. >> >> It looks good to me, but I'm not going to have a chance to merge it >> until mid-week. Erik, if you want to merge it sooner, feel free. > > Here I must confess, that I have never taken the trouble so far to sort out > how to deal with such patches... :-( > Ok. No worries there. I'll take care of it as soon as I have time. ---Brett. |
|
From: Erik V. <eri...@hc...> - 2008-05-27 21:53:55
|
> > > Yes. 4 space indents, expand tab characters to spaces is > fine by me. > > My current setting is 4 with tabs, but in fact I almost never > reformat, > because the formatter tends to break lines in ways I don't like. > I'm pretty good at formatting manually (he boasted), but no doubt > tabs and spaces get mixed this way. > > But I agree that it's time to sort this out. I have committed an experimental formatting standard for Eclipse named Rails-format.xml (in CVS under 18xx). It is an export from a new standard named "Rails" that I created out of the Eclipse default. The main change is, that tabs are not used at all - I found that advice in several places. For the rest it is... well, the way I like it... To use it, I guess it must be imported in the same place from where I created it: Project|Properties|Java Code Style|Formatter. Somewhere I found a remark that in Eclipse 3.x it is possible to reformat all sources in one fell swoop, but I haven't yet discovered how to do that. I haven't reformatted anything yet. I also had a look at Codestyle, but from a superficial glance I concluded that it does only reporting, no automatic or on-demand formatting. I doubt if it will add much to what we can already do with Eclipse to find simple whitespace differences. Erik. |
|
From: John A. T. <ja...@ja...> - 2008-05-27 22:26:23
|
On Tue, 27 May 2008, Erik Vos wrote:
> I also had a look at Codestyle, but from a superficial glance I concluded
> that it does only reporting,
> no automatic or on-demand formatting. I doubt if it will add much to what we
> can already do with Eclipse
> to find simple whitespace differences.
I assume you mean Checkstyle? It doesn't correct errors, but the nice
thing is it flags them as errors/warnings (as configured), so once the
codebase is cleaned up it stays that way. Also, you can configure
additional code style guidelines such as variable naming conventions (ie,
all-caps constants, camel-case identifiers, etc).
--
John A. Tamplin ja...@ja...
770/436-5387 HOME 4116 Manson Ave
Smyrna, GA 30082-3723
|
|
From: Erik V. <eri...@hc...> - 2008-05-28 22:00:33
|
> Somewhere I found a remark that in Eclipse 3.x it is possible > to reformat > all sources in one fell swoop, > but I haven't yet discovered how to do that. I haven't > reformatted anything > yet. Found it here: http://www.artima.com/forums/flat.jsp?forum=121&thread=107852 Open Project Explorer view, right-click project, select Format. Not tried yet. Changed Rails format on one point: no blank lines between field declarations (too spacious IMO). If you're all agree, I'll reformat and commit the whole bunch next Sunday. Erik. |
|
From: Freek D. <sf_...@ma...> - 2008-06-02 22:56:37
Attachments:
tooltips.diff
|
Erik Vos wrote: >> the upgrade value of a city while laying a tile, > > E.g. as a tooltip for the upgrade tiles at the left-hand side of the map? > Good idea. Attached is a patch. Regards, Freek Dijkstra |
|
From: Brett L. <wak...@gm...> - 2008-06-03 21:19:11
|
On Tue, 2008-06-03 at 00:55 +0200, Freek Dijkstra wrote:
> Erik Vos wrote:
>
> >> the upgrade value of a city while laying a tile,
> >
> > E.g. as a tooltip for the upgrade tiles at the left-hand side of the map?
> > Good idea.
>
> Attached is a patch.
>
> Regards,
> Freek Dijkstra
> plain text document attachment (tooltips.diff)
> diff -waur -x Entries -x Entries.log 18xx repos/rails/ui/swing/UpgradesPanel.java 18xx/rails/ui/swing/UpgradesPanel.java
> --- 18xx/rails/ui/swing/UpgradesPanel.java 2008-02-19 21:31:58.000000000 +0100
> +++ 18xx/rails/ui/swing/UpgradesPanel.java 2008-06-03 00:30:02.000000000 +0200
> @@ -280,9 +280,32 @@
> }
>
> public void mouseEntered(MouseEvent e) {
> + Object source = e.getSource();
> + if (!(source instanceof JLabel)) return;
> +
> + if (tokenMode) {
> + // nothing to show
> + } else {
Is there a reason you didn't use "if (!tokenMode)" ?
> + protected void setToolTip()
> + {
> + TileI currentTile = TileManager.get().getTile(internalId);
> + StringBuffer tt = new StringBuffer("<html>");
> + tt.append("<b>Tile</b>: ").append(currentTile.getName()); // or getId()
Tooltip text should support localization. We can catch this with a
second patch.
I'll simplify the tokenMode bits, and commit this shortly.
---Brett.
|
|
From: Freek D. <sf_...@ma...> - 2008-05-25 22:08:43
|
Erik Vos wrote: > Coding rules: I have written most of the code, and am using 4-space indents. > Things go wrong when Brett reformats with his (I believe) 3-space indents > ... ;-) > Indeed we have never settled that and it sometimes looks ugly. I propose 4 > spaces.... > (Tabs or spaces: does it matter? Should I care? Not sure how to control it). I don't care, as long as it is consistent. Let's look at the statistics of *.jave files in the rails directory: 39194 lines of code 30748 lines with some sort of indentation 2542 lines combining tabs and 2 or more spaces (That should exclude lines with extra space to indent /* */ comments) 18245 lines with tabs only 9825 lines with spaces only 69 lines with 3 spaces 4008 line with 4 spaces 82 lines with 5 spaces (huh?) 53 lines with 6 spaces 68 lines with 7 spaces 2404 lines with 8 spaces 43 lines with 9 spaces 1 line with 10 spaces 54 lines with 11 spaces 1442 lines with 12 spaces etc So it seems most popular is tabs, then 4-spaces, then 3-spaces. >> saving of preferences between games, > > What preferences? I can imagine a UI to manage my.properties might come in > handy, althoug I wouldn't consider it a priority. Saving the my.properties based on the last settings. For example, if the last time I started a game of 18Kaas with John, Joe and Jane as players, and unlimited D-trains, it would be nice to see those options as the default the next time the program is run. Regards, Freek |
|
From: brett l. <wak...@gm...> - 2008-05-26 06:42:10
|
On Sun, May 25, 2008 at 3:08 PM, Freek Dijkstra <sf_...@ma...> wrote: > Erik Vos wrote: > >> Coding rules: I have written most of the code, and am using 4-space indents. >> Things go wrong when Brett reformats with his (I believe) 3-space indents >> ... ;-) >> Indeed we have never settled that and it sometimes looks ugly. I propose 4 >> spaces.... >> (Tabs or spaces: does it matter? Should I care? Not sure how to control it). > > I don't care, as long as it is consistent. > > Let's look at the statistics of *.jave files in the rails directory: > > 39194 lines of code > 30748 lines with some sort of indentation > > 2542 lines combining tabs and 2 or more spaces > (That should exclude lines with extra space to indent /* */ comments) > > 18245 lines with tabs only > 9825 lines with spaces only > > 69 lines with 3 spaces > 4008 line with 4 spaces > 82 lines with 5 spaces (huh?) > 53 lines with 6 spaces > 68 lines with 7 spaces > 2404 lines with 8 spaces > 43 lines with 9 spaces > 1 line with 10 spaces > 54 lines with 11 spaces > 1442 lines with 12 spaces > etc > > So it seems most popular is tabs, then 4-spaces, then 3-spaces. > Yep. It's pretty grotty. Feel free to send in a cleanup patch, if you like. ---Brett. |
|
From: Brett L. <wak...@gm...> - 2008-05-27 22:06:59
|
On Tue, 2008-05-27 at 23:53 +0200, Erik Vos wrote:
> >
> > > Yes. 4 space indents, expand tab characters to spaces is
> > fine by me.
> >
> > My current setting is 4 with tabs, but in fact I almost never
> > reformat,
> > because the formatter tends to break lines in ways I don't like.
> > I'm pretty good at formatting manually (he boasted), but no doubt
> > tabs and spaces get mixed this way.
> >
> > But I agree that it's time to sort this out.
>
> I have committed an experimental formatting standard for Eclipse named
> Rails-format.xml (in CVS under 18xx).
> It is an export from a new standard named "Rails" that I created out of the
> Eclipse default.
> The main change is, that tabs are not used at all - I found that advice in
> several places.
> For the rest it is... well, the way I like it...
>
> Erik.
Sounds good.
We should start building out some developer documentation on the
website/wiki, starting with a CodingStyle page. Perhaps Iain has time to
do this?
---Brett.
Only spammers and relatives send HTML mail.
-- Faux_Pseudo
|
|
From: Iain C. <ia...@ch...> - 2008-05-28 13:18:37
|
On 5/27/08, Brett Lentz <wak...@gm...> wrote: > We should start building out some developer documentation on the > website/wiki, starting with a CodingStyle page. Perhaps Iain has time to > do this? Sure. I'll get onto it over the next week or so. I am considering a wiki page for each implemented game and a separate page/area for developer documentation. -- Iain |
|
From: Brett L. <wak...@gm...> - 2008-05-27 22:57:47
|
On Sat, 2008-05-24 at 20:35 +0200, Freek Dijkstra wrote:
> plain text document attachment (defaultgame.diff)
> diff -waur 18xx repos/my.properties 18xx/my.properties
> --- 18xx/my.properties 2007-07-23 21:59:16.000000000 +0200
> +++ 18xx/my.properties 2008-05-23 00:05:48.000000000 +0200
> @@ -55,8 +55,12 @@
> ### Default players
> # Comma-separated list of player names.
> # Useful for game testing purposes.
> #default_players=Alice,Bob,Charlie
>
> +### Default game
> +# Name of game selected in the game selection window.
> +# Useful for game testing purposes.
> +#default_game=1830
>
> ####################### Log4J properties ##############################
> # For information on how to customise log4j logging, see for instance
> diff -waur 18xx repos/rails/ui/swing/GameSetupWindow.java 18xx/rails/ui/swing/GameSetupWindow.java
> --- 18xx/rails/ui/swing/GameSetupWindow.java 2008-01-28 00:27:54.000000000 +0100
> +++ 18xx/rails/ui/swing/GameSetupWindow.java 2008-05-24 16:55:37.000000000 +0200
> @@ -176,9 +175,13 @@
> }
>
> private void populateGameList(List<String> gameNames, JComboBox gameNameBox) {
> + String preferredgame = Config.get("default_game");
> for (String gameName : gameNames) {
> String gameText = gameName + " - " + GamesInfo.getNote(gameName);
> gameNameBox.addItem(gameText);
> + if (preferredgame.equals(gameName)) {
> + gameNameBox.setSelectedItem(gameText);
> + }
> }
> }
>
Patch applied.
Sorry for the wait on this one. The (American) holiday weekend slowed me
down a bit.
---Brett.
|
|
From: Freek D. <sf_...@ma...> - 2008-06-03 21:33:27
|
Brett Lentz wrote:
>> + if (tokenMode) {
>> + // nothing to show
>> + } else {
>
> Is there a reason you didn't use "if (!tokenMode)" ?
No. I copied it, and forgot to check later (I dislike the implicit
assumption that there are only two modes, but never felt it was
important enough to improve).
Regards,
Freek
|
|
From: Brett L. <wak...@gm...> - 2008-06-03 21:36:25
|
On Tue, 2008-06-03 at 23:32 +0200, Freek Dijkstra wrote:
> Brett Lentz wrote:
>
> >> + if (tokenMode) {
> >> + // nothing to show
> >> + } else {
> >
> > Is there a reason you didn't use "if (!tokenMode)" ?
>
> No. I copied it, and forgot to check later (I dislike the implicit
> assumption that there are only two modes, but never felt it was
> important enough to improve).
>
> Regards,
> Freek
>
Ok. I've committed your patch, and just simplified the statement.
---Brett.
|
|
From: Freek D. <sf_...@ma...> - 2008-06-03 22:22:21
|
Sorry to bother you, but it seems part of my patch went wrong. One of the modifications was made on the wrong line. http://rails.cvs.sourceforge.net/rails/18xx/rails/ui/swing/hexmap/GUIHex.java?revision=1.16&view=markup Now reads: 799 if (getHexModel().getDestinations() != null) { 800 tt.append("<br><b>Destination</b>:"); 801 for (PublicCompanyI dest : getHexModel().getDestinations()) { 802 tt.append("</html>"); 803 tt.append(dest.getName()); 804 } 805 } This is wrong: the tt.append("</html>"); should be outside the for loop. (and the remove tt.append(" "); should be restored) So it should read: 799 if (getHexModel().getDestinations() != null) { 800 tt.append("<br><b>Destination</b>:"); 801 for (PublicCompanyI dest : getHexModel().getDestinations()) { 802 tt.append (" "); 803 tt.append(dest.getName()); 804 } 805 } 806 tt.append("</html">); 807 toolTip = tt.toString(); 808 } 809 This was my fault: my diff file was in error; it seems I accidentally sent a diff between two working copies, instead of a diff between a working copy and the repository. I like to blame CVS this time (I'm used to svn). I just manually verified all other commits (including those from last weeks commit) and all other files are fine. Sorry, Freek |
|
From: Brett L. <wak...@gm...> - 2008-06-03 22:32:05
|
On Wed, 2008-06-04 at 00:20 +0200, Freek Dijkstra wrote: > Sorry to bother you, but it seems part of my patch went wrong. > One of the modifications was made on the wrong line. > > http://rails.cvs.sourceforge.net/rails/18xx/rails/ui/swing/hexmap/GUIHex.java?revision=1.16&view=markup > Now reads: > > 799 if (getHexModel().getDestinations() != null) { > 800 tt.append("<br><b>Destination</b>:"); > 801 for (PublicCompanyI dest : getHexModel().getDestinations()) { > 802 tt.append("</html>"); > 803 tt.append(dest.getName()); > 804 } > 805 } > > This is wrong: the tt.append("</html>"); should be outside the for loop. > (and the remove tt.append(" "); should be restored) > > So it should read: > > 799 if (getHexModel().getDestinations() != null) { > 800 tt.append("<br><b>Destination</b>:"); > 801 for (PublicCompanyI dest : > getHexModel().getDestinations()) { > 802 tt.append (" "); > 803 tt.append(dest.getName()); > 804 } > 805 } > 806 tt.append("</html">); > 807 toolTip = tt.toString(); > 808 } > 809 > > This was my fault: my diff file was in error; it seems I accidentally > sent a diff between two working copies, instead of a diff between a > working copy and the repository. > > I like to blame CVS this time (I'm used to svn). > > I just manually verified all other commits (including those from last > weeks commit) and all other files are fine. > > Sorry, > Freek > Fixed checked in. Thanks for double-checking. :-) ---Brett. |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X