[Freeciv-Dev] Re: new_city_dialog ver 12

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Oct 21, 2001 at 07:44:21PM -0500, Mike Kaufman wrote:
> there's a new version in incoming/ on the ftp site, Daniel will mirror
> it soon at
> 
>         http://www.acc.umu.se/~deepone/freeciv/
> 
> changes:
> 
> Another map sensitivity patch by Christian
> worklist commit problem fixed (I hope)
> a couple of city investigation bugs fixed.
> 
> Unless Christian comes up with any pressing issues, (since he seems to
> be the only one looking at it) I would suggest this be applied to CVS.
> 
> Yes, there are sure to still be bugs, but they're not going to get found
> at this point until more people start looking at it.
> 
> Maintainers: please start taking a good hard look.

Note the using of may and should.

In the number of turn in () in the Granaray line: there should be a
"-" if the city shrinks and 999 should be replaced by "never".

The "City Info" and the "City Map" frame shouldn't enlarge if the
window is enlarged.

"Unit list" should be "Present unit list".

"Sentry units" should be "Sentry all units".

If there are two items in the current worklist and the bottom is
deleted the "Up" button is active.

The wrapping of the happiness texts are still wrapped at the wrong
width. You may want to use a fixed layout.

"No trade routes exists" isn't centered.

I'm I correct that the tooltips are used in only one place? For
example you may provide tooltips for the units, which show the type,
hp, homecity, movements left.

In which order are the cities sorted for the next/prev button?

In the worklist tab: global worklists should be called "Worklist"
instead of "worklist".

It may be good if the two boxes "Current worklist" and "Available
Items" are swapped. This would follow the usual left-to-right flow.

The "Do you really want to sell xyz for jkashd" dialog may be
discarded since the player now sees the information.

You may add the buy information I still would found nice.

If the Granary is "130/130" the number of turns is 0. It should be 1.

If the city dialog is already open for a city and the user press
"popup" the second time in the city list dialog the existing dialog
should come to the front.

The text in the "city info" frame should be padded with whitespace to
have always the same fixed width. This would reduce the resize
flickering.

The "Trade" pane should be called "Trade routes".

In the "Next time open" choice: replace "City page" with "City
Overview Page".

If the misc. settings pane: if the window is enlarged the tooltip and
next-time-open frame should enlarge.

Ok enough with the comments from the user pov.

Technically this patch is not only large but huge: exactly 9000
lines. There is no way I can go over this all. So I can only comment
on the changes which aren't in the citydialog or worklist files:

       cost = get_unit_type(pcity->currently_building)->build_cost;
-      turns = city_turns_to_build(pcity, pcity->currently_building, TRUE);
+      turns = city_turns_to_build(pcity, pcity->currently_building, TRUE, 
TRUE);
     } else {

       if (!pcity->is_building_unit && is_wonder(pcity->currently_building)
-         && (city_turns_to_build(pcity, pcity->currently_building, 0) <= 1)
+         && (city_turns_to_build(pcity, pcity->currently_building, 0, 1) <= 1)
          && can_player_build_improvement(city_owner(pcity), 
pcity->currently_building)) {

+       dest_wids[wids_used] = wid_encode(0, 1, i);

0-vs-FALSE and 1-vs-TRUE. This isn't your fault but it something which
I have noticed.

Can you tell what kind of change you have done to gtk_accelbutton_new?

The new files happiness.[ch] should get an updated copyright header.

+#define WORKLIST_ADVANCED_TARGETS      1
+#define WORKLIST_CURRENT_TARGETS       0
+#define COLUMNS                                4
+#define BUFFER_SIZE                    100
+#define NUM_TARGET_TYPES               3

Please adjust the COLUMNS line. You can run indent over
{happiness,citydlg,wldlg}.[ch].

 #define HELP_CHATLINE_ITEM "Chatline"
+#define HELP_WORKLIST_EDITOR_ITEM "Worklist editor"
 #define HELP_CONTROLS_ITEM "Controls"

AFAIK all words are capitalized: "Worklist Editor"

Can you move supported_units_in_city and present_units_in_city to
climisc? These aren't used in the server code. You may also prefix
them with "num_" or "count_".

-int city_turns_to_build(struct city *pcity, int id, int id_is_unit)
+int city_turns_to_build(struct city *pcity, int id, int id_is_unit,
+        int include_shield_stock )
 {

The second line doesn't look rightly indented.

Ok finished.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  One nuclear bomb can ruin your whole day.