Complete.Org: Mailing Lists: Archives: freeciv-dev: September 2003:
[Freeciv-Dev] Re: resync'ed and stylified (PR#5378) Diplomacy informatio
Home

[Freeciv-Dev] Re: resync'ed and stylified (PR#5378) Diplomacy informatio

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: dspeyer@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: resync'ed and stylified (PR#5378) Diplomacy information in map middle-click
From: "andrearo@xxxxxxxxxxxx" <andrearo@xxxxxxxxxxxx>
Date: Mon, 15 Sep 2003 06:46:37 -0700
Reply-to: rt@xxxxxxxxxxxxxx

On Sun, 14 Sep 2003, Daniel L Speyer wrote:
> Here's the diplomacy information in middle-click patch, resycnhronized,
> stylified and commented.
>
> Enjoy!
> --Daniel Speyer

Here's the second review of the patch. (recheck codingstyle)

Andreas Røsdal
I. Testing

1. Was the patch tested (yes / no-elaborate)?

Yes

2. Which client was used to testing the patch (N/A if it's a
server-only patch)?

gtk-1.2, gtk-2.0, Xaw, Xaw3d

3. Does the patch work / do what it promises to (yes / no)?

Yes, patch works.

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

II. Code correctness

1. Does the patch adhere to the style guide (yes / no)?

There's still some Codingstyle problems. 
Here's some examples:

+    if(punit && (punit->activity == ACTIVITY_GOTO || punit->connecting))  {

+    if(punit && (punit->activity == ACTIVITY_GOTO || punit->connecting)) {

+  if(tile_get_known(xtile, ytile)>=TILE_KNOWN_FOGGED) {

+      if (end) {
+       content = end+1;

+  char *diplo_adjectives[DS_LAST] = {_("Neutral"), _("Hostile"), 
_("Ceasefire"),

+      }else{

+    if(punit->owner == game.player_idx) {
+       struct city *pcity;
+       pcity=player_find_city_by_id(game.player_ptr, punit->homecity);
+       if(pcity)
+         my_snprintf(tmp, sizeof(tmp), "/%s", pcity->name);
+      }

2. Is the patch well commented / easy to read (yes / no-elaborate)?

Yes

3. Are there coding errors in the patch (list)?

No

4. What can be improved / simplified / done differently (list)?

Would it be possible to add win32 support also?

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

III. Verdict

1. Should the patch be
  a) rejected completely
  b) split - elaborate
  c) revised and resubmitted for review
  d) revised and then committed
  e) committed as it is

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

  Verdict: c) revised and resubmitted for review. 

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


[Prev in Thread] Current Thread [Next in Thread]