[PATCH weston GSoC v2] desktop-shell: make panel clock configurable

[Armin Krezović]

On 08.03.2016 11:01, Pekka Paalanen wrote:
> On Mon, 7 Mar 2016 15:42:45 +0100
> Armin Krezović <armin.krezovic at fet.ba> wrote:
> 
>> On 07.03.2016 13:37, Pekka Paalanen wrote:
>>> On Thu, 3 Mar 2016 11:39:13 -0800
>>> Bryce Harrington <bryce at osg.samsung.com> wrote:
>>>   
>>>> On Thu, Mar 03, 2016 at 03:48:03PM +0100, Armin Krezović wrote:  
>>>>> This patch enhances the panel clock by adding a config file
>>>>> option which can be used to either disable the clock or make
>>>>> it also show seconds in the current clock format.
>>>>>
>>>>> v2: Implement suggestions from Pekka's review.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=57583
>>>>> Signed-off-by: Armin Krezović <armin.krezovic at fet.ba>    
>>>>
>>>> I might name the enum elements CLOCK_FORMAT_* but as long as it's kept
>>>> local to the desktop-shell.c client it probably doesn't matter.
>>>>
>>>> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
>>>>
>>>> (I just glossed over the adjustments to the cairo position mathematics.
>>>> I assume it'll be pretty obvious visually if there's an error with
>>>> that.)  
>>>
>>> Hi Armin,
>>>
>>> there is some funny logic that I'd like to see fixed, details inline.
> 
>>>>> @@ -368,12 +373,21 @@ panel_clock_redraw_handler(struct widget *widget, void *data)
>>>>>  			       CAIRO_FONT_WEIGHT_NORMAL);
>>>>>  	cairo_set_font_size(cr, 14);
>>>>>  	cairo_text_extents(cr, string, &extents);
>>>>> -	cairo_font_extents (cr, &font_extents);
>>>>> -	cairo_move_to(cr, allocation.x + 5,
>>>>> +	cairo_font_extents(cr, &font_extents);
>>>>> +
>>>>> +	if (!clock->size_set) {
>>>>> +		allocation.x = allocation.x + allocation.width - extents.width;
>>>>> +		allocation.width = extents.width + 1;
>>>>> +		widget_set_allocation(widget, allocation.x, allocation.y,
>>>>> +				      allocation.width, allocation.height);  
>>>
>>> The call to widget_set_allocation() belongs in the resize function, not
>>> here in the redraw function. Did you have problems putting it there?
>>>   
>>
>> When we had a conversation on IRC about this one, you said that the
>> proper way to fix this is to get the length from text extents, which
>> I did.
> 
> Hi Armin
> 
> Yeah, sorry, I tend to talk about things unrelated to the immediate
> topic in question, to give a more broader view and background, and then
> just leave it to the listener to figure out what's actually in scope
> for the immediate task. :-)
> 
> Using extents of the longest possible time string for a format would be
> the proper way to allocate the widget size, but the topic here is just
> about adding new formats rather than fixing everything.
> 
>> I didn't find a way to get access to text extents from within panel
>> resize function, which is ran before cairo_text_extents is in this
>> function.
> 
> Getting the extents in the resize function is indeed the key to the
> proper sizing solution, which also makes it a little harder to do.
> That's what my last comment in the bugzilla about extents refers to.
> 
> It's also a thing one might not bother to do without a reason, like a
> configurable font or some user setup where the constants are wrong - or
> another GSoC applicant wanting to prove his skills.
> 
> 
> Thanks,
> pq
> 

Hi,

I've modified the patch to use fixed width for the clock widget.
190 for clock format with seconds, 170 (same as current) for the
one without seconds. But in doing so, I had to somehow forward
the clock format value to the panel resize handler, which is why
new panel structure member was introduced.

Thanks for all the feedback so far.

Armin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160308/822813a9/attachment.sig>