[PATCH weston GSoC v2] desktop-shell: make panel clock configurable
Pekka Paalanen
ppaalanen at gmail.com
Mon Mar 7 12:37:12 UTC 2016
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.
> > ---
> > 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?
> > + 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.
A quick quiz(*): what is the value of 'allocation' when entering this
if-branch? ;-)
> > + }
> > +
> > + 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.
*) 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.
> > 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.
> > +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
-------------- 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/20160307/9b4aec87/attachment.sig>
More information about the wayland-devel
mailing list