[Freeciv-Dev] Re: new_city_dialog ver 12

[Daniel Sjölie <deepone@xxxxxxxxxx>]

On 2001-10-23 12:49:32, Raimar Falke wrote:
> 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".

I like your idea in the later mail better...

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

I'm not sure about this... I think it looks very ok...

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

Ack.

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

Hmm... Or really "Sentry all present units"...
Hmm... Lets be consistent and make change all the buttons to
"Activate present units", "Sentry present units" and "List present
units" - ok?

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

Not if you delete it with double-click... :)
I'll fix that...

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

Perhaps, but maybe that can wait? I think it can be added as is then
someone who thinks that's really ugly (I don't really) can fix it
later...

> "No trade routes exists" isn't centered.

Hmm... It seems centered to me...
Screenshot?

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

Yes, and in tons of other places in the game... :)
Actually, with your suggestion of growth info on a seperate row I don't
think we need any tooltips at all... I'd rather consider covering the
game in tooltips a seperate project from this city dialog... :)

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

None... It's taken straigt from city_list_*
I think more advanced ordering here is also a patch that would stand
well on it's own after this one has been applied...

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

Ack.

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

Hmm... Interesting thought...
I think the layout on that page as a whole would suffer though...
I'll leave it for now - other opinions? Consider how the buttons would
be moved along...

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

Hmm... Yes...
Confirmation might be good but I'm with you... Others?

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

Yes, it might take some effort to make right though...
I probably won't have the time to do this soon and I'd like to see the
patch finished soon... It's a feature that could be added later of
course...

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

Ack.

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

Mmm... Perhaps... What should happen in the dialog you were in?
If it always should do that you would be forced to close the other
dialog to step on... Also I'm not sure there is a really good and
standard way to bring a window to the front...

I don't think it's worth the hassel... Not now anyway...

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

Ok...

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

Ok for now... I think "Trade" is enough if we get cramped for space in
the "tab row" but that doesn't seem to be the case just yet... :)

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

Ack.

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

Umm... It does for me...
What gtk version are you using?

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

Ok... Perhaps that can be changed later if desired? To not confuse this
patch any more with unrelated changes... :)

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

It didn't work before... The one in cvs still doesn't work for me...
Perhaps it's a gtk version problem but this implementation should be
safer...

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

Ok...

> +#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].

With what arguments?

>  #define HELP_CHATLINE_ITEM "Chatline"
> +#define HELP_WORKLIST_EDITOR_ITEM "Worklist editor"
>  #define HELP_CONTROLS_ITEM "Controls"
> 
> AFAIK all words are capitalized: "Worklist Editor"

Ok...

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

> Ok finished.

Good... :)

/Daniel

-- 
Now take a deep breath, smile and don't take life so seriously... :)