[PATCH weston GSoC v2] desktop-shell: make panel clock configurable
Armin Krezović
armin.krezovic at fet.ba
Mon Mar 7 14:42:45 UTC 2016
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.
>
Hi,
>>> ---
>>> clients/desktop-shell.c | 85 ++++++++++++++++++++++++++++++++++++++++++-------
>>> man/weston.ini.man | 7 ++++
>>> weston.ini.in | 1 +
>>> 3 files changed, 81 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
>>> index 6ab76dc..d98f8b5 100644
>>> --- a/clients/desktop-shell.c
>>> +++ b/clients/desktop-shell.c
>>> @@ -49,6 +49,8 @@
>>>
>>> #include "weston-desktop-shell-client-protocol.h"
>>>
>>> +#define DEFAULT_CLOCK_FORMAT FORMAT_MINUTES
>>> +
>>> extern char **environ; /* defined by libc */
>>>
>>> struct desktop {
>>> @@ -122,6 +124,9 @@ struct panel_clock {
>>> struct panel *panel;
>>> struct task clock_task;
>>> int clock_fd;
>>> + int size_set;
>>> + char *format_string;
>>> + time_t refresh_timer;
>>> };
>>>
>>> struct unlock_dialog {
>>> @@ -356,7 +361,7 @@ panel_clock_redraw_handler(struct widget *widget, void *data)
>>>
>>> time(&rawtime);
>>> timeinfo = localtime(&rawtime);
>>> - strftime(string, sizeof string, "%a %b %d, %I:%M %p", timeinfo);
>>> + strftime(string, sizeof string, clock->format_string, timeinfo);
>>>
>>> widget_get_allocation(widget, &allocation);
>>> if (allocation.width == 0)
>>> @@ -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.
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.
>>> + clock->size_set = 1;
>
> Using this flag also has the problem, that if the output video mode
> changes permanently, we need to resize again, but you never reset the
> flag. We get resizes through desktop_shell_configure() which through a
> function pointer schedules a widget or window resize.
>
Yeah, I only thought about that after I hit sent (sometimes working too fast).
Still, it would be properly resized from panel resize handler (see (1) down below).
> A quick quiz(*): what is the value of 'allocation' when entering this
> if-branch? ;-)
>
According to printf-foo I've used, it's the value set in the panel resize function
(makes sense, since resize handler is ran before the widget is created).
>>> + }
>>> +
>>> + cairo_move_to(cr, allocation.x + 1,
>>> allocation.y + 3 * (allocation.height >> 2) + 1);
>>> cairo_set_source_rgb(cr, 0, 0, 0);
>>> cairo_show_text(cr, string);
>>> - cairo_move_to(cr, allocation.x + 4,
>>> + cairo_move_to(cr, allocation.x,
>>> allocation.y + 3 * (allocation.height >> 2));
>>> cairo_set_source_rgb(cr, 1, 1, 1);
>>> cairo_show_text(cr, string);
>>> @@ -385,9 +399,9 @@ clock_timer_reset(struct panel_clock *clock)
>>> {
>>> struct itimerspec its;
>>>
>>> - its.it_interval.tv_sec = 60;
>>> + its.it_interval.tv_sec = clock->refresh_timer;
>>> its.it_interval.tv_nsec = 0;
>>> - its.it_value.tv_sec = 60;
>>> + its.it_value.tv_sec = clock->refresh_timer;
>>> its.it_value.tv_nsec = 0;
>>> if (timerfd_settime(clock->clock_fd, 0, &its, NULL) < 0) {
>>> fprintf(stderr, "could not set timerfd\n: %m");
>>> @@ -407,9 +421,18 @@ panel_destroy_clock(struct panel_clock *clock)
>>> free(clock);
>>> }
>>>
>>> +enum {
>>> + FORMAT_MINUTES,
>>> + FORMAT_SECONDS,
>>> + FORMAT_NONE
>>> +};
>>> +
>>> static void
>>> -panel_add_clock(struct panel *panel)
>>> +panel_add_clock(struct panel *panel, int format)
>>> {
>>> + if (format == FORMAT_NONE)
>>> + return;
>>> +
>>> struct panel_clock *clock;
>>> int timerfd;
>>>
>>> @@ -424,6 +447,20 @@ panel_add_clock(struct panel *panel)
>>> panel->clock = clock;
>>> clock->clock_fd = timerfd;
>>>
>>> + switch (format) {
>>> + case FORMAT_MINUTES:
>>> + clock->format_string = "%a %b %d, %I:%M %p";
>>> + clock->refresh_timer = 60;
>>> + break;
>>> + case FORMAT_SECONDS:
>>> + clock->format_string = "%a %b %d, %I:%M:%S %p";
>>> + clock->refresh_timer = 1;
>>> + break;
>>> + case FORMAT_NONE:
>>> + /* Silence a compiler warning */
>>> + break;
>>> + }
>>> +
>>> clock->clock_task.run = clock_func;
>>> display_watch_fd(window_get_display(panel->window), clock->clock_fd,
>>> EPOLLIN, &clock->clock_task);
>>> @@ -439,6 +476,7 @@ panel_resize_handler(struct widget *widget,
>>> {
>>> struct panel_launcher *launcher;
>>> struct panel *panel = data;
>>> + struct rectangle allocation;
>>> int x, y, w, h;
>>>
>>> x = 10;
>>> @@ -450,12 +488,20 @@ panel_resize_handler(struct widget *widget,
>>> x, y - h / 2, w + 1, h + 1);
>>> x += w + 10;
>>> }
>>> - h=20;
>>> - w=170;
>>>
>>> - if (panel->clock)
>>> + h = 20;
>>> +
>>> + if (panel->clock) {
>>> + widget_get_allocation(panel->clock->widget, &allocation);
>
> A quick quiz(*): what is the value of 'allocation'? ;-)
>
>>> +
>>> + if (allocation.width)
>>> + w = allocation.width;
>>> + else
>>> + w = 170;
>>> +
>
> This looks very strange. Why is this?
>
> If FORMAT_MINUTES then w=170, otherwise w= whatever constant the
> seconds format needs. That should work well enough, we didn't use the
> real text extents before either. Using real extents from the font/text
> would be a topic for a different patch.
>
Oh well, I misunderstood you when we talked about this on IRC. I tried to work
on the other method (calculating widget width from text extents).
>
> *) I think you are relying on some ordering between these two calls,
> but the logic is not clear at all. The whole idea is that resize gets
> called when the size changes and also initially, and then redraw only
> draws on the given size.
>
(1) I've only realized it's confusing logic after I sent this patch (working too early
in the morning on this isn't helping me or anyone it seems).
Still, the correct width would be set, since allocation.width is set to 0 if widget is
uninitialized (the first way resize handler for panel is run) or set to width according
to the text extents from the clock redraw handler, which is why I used
widget_get_allocation.
Far easier way would've been to just set the size_set flag to 0.
I admit I'm far happier with using hardcoded values - less work for me.
>>> widget_set_allocation(panel->clock->widget,
>>> width - w - 8, y - h / 2, w + 1, h + 1);
>>> + }
>>> }
>>>
>>> static void
>>> @@ -492,7 +538,8 @@ panel_destroy(struct panel *panel)
>>> struct panel_launcher *tmp;
>>> struct panel_launcher *launcher;
>>>
>>> - panel_destroy_clock(panel->clock);
>>> + if (panel->clock)
>>> + panel_destroy_clock(panel->clock);
>>>
>>> wl_list_for_each_safe(launcher, tmp, &panel->launcher_list, link)
>>> panel_destroy_launcher(launcher);
>>> @@ -508,6 +555,8 @@ panel_create(struct desktop *desktop)
>>> {
>>> struct panel *panel;
>>> struct weston_config_section *s;
>>> + char *clock_format_option = NULL;
>>> + int clock_format;
>>>
>>> panel = xzalloc(sizeof *panel);
>>>
>>> @@ -522,7 +571,19 @@ panel_create(struct desktop *desktop)
>>> widget_set_redraw_handler(panel->widget, panel_redraw_handler);
>>> widget_set_resize_handler(panel->widget, panel_resize_handler);
>>>
>>> - panel_add_clock(panel);
>>> + s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
>>> + weston_config_section_get_string(s, "clock-format", &clock_format_option, "");
>>> +
>>> + if (strcmp(clock_format_option, "minutes") == 0)
>>> + clock_format = FORMAT_MINUTES;
>>> + else if (strcmp(clock_format_option, "seconds") == 0)
>>> + clock_format = FORMAT_SECONDS;
>>> + else if (strcmp(clock_format_option, "none") == 0)
>>> + clock_format = FORMAT_NONE;
>>> + else
>>> + clock_format = DEFAULT_CLOCK_FORMAT;
>>> +
>>> + panel_add_clock(panel, clock_format);
>>>
>>> s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
>>> weston_config_section_get_uint(s, "panel-color",
>>> diff --git a/man/weston.ini.man b/man/weston.ini.man
>>> index 6e92066..e9044cb 100644
>>> --- a/man/weston.ini.man
>>> +++ b/man/weston.ini.man
>>> @@ -212,6 +212,13 @@ output. Tile repeats the background image to fill the output.
>>> sets the color of the background (unsigned integer). The hexadecimal
>>> digit pairs are in order alpha, red, green, and blue.
>>> .TP 7
>>> +.BI "clock-format=" format
>>> +sets the panel clock format (string). Can be
>>> +.BR none,
>>> +.BR minutes,
>>> +.BR seconds.
>
> Extreme nitpick: the commas and the period should not be highlighted.
> See 'background-type' for an example how to do it.
>
That's easy enough to fix, will do.
>>> +By default, minutes format is used.
>>> +.TP 7
>>> .BI "panel-color=" 0xAARRGGBB
>>> sets the color of the panel (unsigned integer). The hexadecimal
>>> digit pairs are in order transparency, red, green, and blue. Examples:
>>> diff --git a/weston.ini.in b/weston.ini.in
>>> index dff9e94..14a4c0c 100644
>>> --- a/weston.ini.in
>>> +++ b/weston.ini.in
>>> @@ -7,6 +7,7 @@
>>> background-image=/usr/share/backgrounds/gnome/Aqua.jpg
>>> background-color=0xff002244
>>> background-type=tile
>>> +clock-format=minutes
>>> panel-color=0x90ff0000
>>> locking=true
>>> animation=zoom
>>> --
>
> The changes I haven't commented about seem to be going in the right
> direction.
>
>
> Thanks,
> pq
>
Thank you once again for taking your time to properly revive this and give feedback.
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/20160307/91b20725/attachment-0001.sig>
More information about the wayland-devel
mailing list