[Freeciv-Dev] Re: (PR#4729) cleanup to canvas<->city pos functions

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> Worthwhile, but fix the one inconsistency which is actually I think a
> future raft of bugs in hiding.

>>(Note city_to_canvas_pos could return a boolean, for consistency, but it 
>>should always be TRUE.  We should never have to deal with off-canvas 
>>tiles at all.)
> 
> 
> bad (for three reasons besides the final foolish wishlist sentence)
> 
> 1)  It is not always TRUE, and/or forcing coders to insure this condition
> leads to all sorts of ad hoc ways to check or miss the checks.
> 
> 2)  All the coordinate functions should return a standard boolean that
> confirms that the transformation was successful (even if it seems like it
> cannot fail). Thus one never has to worry about which does and doesn't.
> and never has to update code when a future update adds something that
> changes this. Think about interfaces, that means thinking about the future
> and not just the present or past.
> 
> 3)  It would not hurt to return the condition of tile_visible_map_canvas()
> for city_to_canvas_pos(), or the equivalent clip part of map_to_canvas_pos()
> since it is perfectly reasonable for one to iterate over city coordinates in
> a canvas setting without doing either a checked iteration (to guarantee any
> city tile is on the map, and a city can overlap the map edge), or that it
> lies within the current gui window. One can always ignore the boolean, but
> it would seem like it is more often useful than not and should be a quick
> test.
>      In fact, I would probably return the clip() condition which is valid in
> all contexts of this transformation, and make a second
> city_to_canvas_pos_checked() that calls this and adds the map condition.
> This is much the same as the city iterators that can run in full city or
> map constrained modes.

So you're saying that city_to_canvas_pos should return the boolean?  I 
can't quite understand the syntax of your response (is it the note or 
the patch that is "bad")?

jason