Re: [Maposmatic-dev] [PATCH] [ocitysmap] Rescale category header where needed

[Jeroen van Rijn]

On Tue, Apr 17, 2012 at 08:33, David MENTRE <address@hidden> wrote:
> Hello Jeroen,
>
> Just a few comments...
>
> 2012/4/17 Jeroen van Rijn <address@hidden>:
>> +        # Measure header text and rescale if needed
>
> I would put more hash marks (###) to separate more clearly this part
> of the code. In fact, maybe this should be put in a specific
> _rescale_something function. I don't know.

Certainly, it can be refactored out to a method of the category draw class.

>> +        if scale < 1.2:
>
> How has been determined this 1.2 value?

Not at all scientifically, i'm afraid. Initially I wanted to go with
reusing draw_utils.adjust_font_size. It had that same value. In
testing the patch and drawing 'Öfftenlichen Gebäuden', I noticed that
the scale factor as computed was sometimes too close to 1 for comfort
where it could really do with resizing. Scaling it to ~80% as I
remembered from that helper function seemed like a good idea, and it
looked good in the test case I was using #1

http://git.savannah.gnu.org/cgit/maposmatic/ocitysmap.git/tree/ocitysmap2/draw_utils.py#n165


>> +        # Restore target width
>> +        layout.set_width(target_width)
>> +        layout.context_changed()
>> +        # Restore context
>> +        ctx.restore()
>
> Is it really necessary to call set_width(target_width) as the context
> is restored just afterwards?

I haven't tried with it commented out, to be honest. I considered it
good practice to explicitly set it as the counterpart to
set_width(-1), to keep it balanced like the ctx.save/restore calls.
Additionally it was my understanding those were to save the
transformation matrix, so it didn't occur to me it might also save
which wrapping mode and width had been set initially. To be fair, this
is the first time I'm getting all up in Pango and Cairo's grill.

But I can certainly comment out the line and see if it makes a difference :)

> Overall, very cleanly commented code, nice to read!
Thanks!

#1 A Dutch city, but with -L de_DE.UTF8. I only have the Dutch OSM
data installed at the moment on my dev system. I'm waiting for the
license change to have rolled over before importing the entire planet,
so I can have rolling updates.

Best regards,
Jeroen.

-- 
↑↑↓↓←→←→BA[Start]