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

[Pekka Paalanen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160308/32b528ce/attachment.sig>