[PATCH weston GSoC v3] desktop-shell: make panel clock configurable
Pekka Paalanen
ppaalanen at gmail.com
Thu Mar 10 11:37:32 UTC 2016
On Thu, 10 Mar 2016 01:58:15 +0100
Armin Krezović <armin.krezovic at fet.ba> wrote:
> On 09.03.2016 19:57, Bryce Harrington wrote:
> > Hi Armin,
> >
> > This is coming along nicely, keep up the good work. I'm going to follow
> > pq's lead here in pointing out more than I usually would, in interest of
> > education.
Thanks Bryce, I essentially agree with everything you said on the
commit message.
Armin, please always use reply-to-all, it's polite to keep people CC'd.
I'm sure we are all subscribed to the list here, but I also have a
highlight on all emails that CC me personally for instance.
> Hi Bryce,
>
> > On Tue, Mar 08, 2016 at 07:50:16PM +0100, Armin Krezović wrote:
> >> From: Armin Krezović <armin.krezovic at fet.ba>
> >
> > This From line isn't needed in the commit message, since git will pick
> > it up from your email header already.
> >
>
> See below.
>
> >> 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 first review.
> >> v3: Implement suggestions from Pekka's second review.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=57583
> >> Signed-off-by: Armin Krezović <armin.krezovic at fet.ba>
> >>
> >> Resend with correct name in ~/.gitconfig
> >
> > Conventionally, people will put the v2, v3 information and other such
> > notes here. You place three dashes, and then add your patch
> > "metacomments". Anything after the three dashes is omitted by git when
> > the patch lands. So, that would look like:
> >
>
> This is a bit of fail from my side. I use different name/email for my
> personal repo. I forgot to change the details in ~/.gitconfig before
> I did git commit -a --amend to stash the changes into an already existing
> patch. The "Resend ..." line was meant to be purely informational to the
> mailing list, not the actual patch. You can notice two v3 mails from this
> address, but with different name (well, first one is without the surname
> part).
Armin, if you really want different name/email per git repo, you could
probably put it in the repo config instead of the global config. See
'git help config' on how to pick which config it targets. That way you
don't have edit it all the time.
> > ---
> > v2: Implement suggestions from Pekka's first review.
> > v3: Implement suggestions from Pekka's second review.
> > Resend with correct name in ~/.gitconfig
> >
> > Also, "Implement suggestions" is fairly ambiguous; it's fairly obvious
> > that a v2 would fix review comments from v1, not really worth
> > mentioning. If you do wish to mention changes, then I'd suggest
> > detailing exactly what those suggestions were. For instance (just
> > making stuff up here):
> >
> > ---
> > v2: Implement suggestions from Pekka's first review.
> > - Fix misspellings/grammar
> > - Improve commit message
> > - Rejigger the fitzworther
> > v3: Implement suggestions from Pekka's second review.
> > - Resend with correct name in ~/.gitconfig
> > - Add link to bug tracker
> >
> > Often whomever reviews your patches early on, will not be the person who
> > reviews it later, so having these items detailed saves them some time.
> > For example, if the last version changed something major, that clues
> > them in to review that item in detail. Or if the last few versions are
> > merely copyedits, that can be a signal the patch is probably ready to be
> > landed.
> >
>
> Alright. That's a nice idea.
>
> >> ---
> >> clients/desktop-shell.c | 59 +++++++++++++++++++++++++++++++++++++++++++------
> >> man/weston.ini.man | 7 ++++++
> >> weston.ini.in | 1 +
> >> 3 files changed, 60 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> >> index 6ab76dc..8171f62 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 CLOCK_FORMAT_MINUTES
> >> +
> >> extern char **environ; /* defined by libc */
> >>
> >> struct desktop {
> >> @@ -83,6 +85,7 @@ struct panel {
> >> struct wl_list launcher_list;
> >> struct panel_clock *clock;
> >> int painted;
> >> + int clock_format;
> >> uint32_t color;
> >> };
> >>
> >> @@ -122,6 +125,8 @@ struct panel_clock {
> >> struct panel *panel;
> >> struct task clock_task;
> >> int clock_fd;
> >> + 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,7 +373,8 @@ 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_font_extents(cr, &font_extents);
> >> +
> >> cairo_move_to(cr, allocation.x + 5,
> >> allocation.y + 3 * (allocation.height >> 2) + 1);
> >> cairo_set_source_rgb(cr, 0, 0, 0);
> >> @@ -385,9 +391,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 +413,18 @@ panel_destroy_clock(struct panel_clock *clock)
> >> free(clock);
> >> }
> >>
> >> +enum {
> >> + CLOCK_FORMAT_MINUTES,
> >> + CLOCK_FORMAT_SECONDS,
> >> + CLOCK_FORMAT_NONE
> >> +};
> >> +
> >> static void
> >> panel_add_clock(struct panel *panel)
> >> {
> >> + if (panel->clock_format == CLOCK_FORMAT_NONE)
> >> + return;
I would slightly prefer if this check was in the caller of
panel_add_clock() instead of here. That way plane_add_clock() would
always do what it says on the tin.
The same way you already have panel_destroy_clock() conditional on the
clock existing in the first place. Symmetry is beautiful.
> >> +
> >> struct panel_clock *clock;
> >> int timerfd;
> >>
> >> @@ -424,6 +439,17 @@ panel_add_clock(struct panel *panel)
> >> panel->clock = clock;
> >> clock->clock_fd = timerfd;
> >>
> >> + switch (panel->clock_format) {
> >> + case CLOCK_FORMAT_MINUTES:
> >> + clock->format_string = "%a %b %d, %I:%M %p";
> >> + clock->refresh_timer = 60;
> >> + break;
> >> + case CLOCK_FORMAT_SECONDS:
> >> + clock->format_string = "%a %b %d, %I:%M:%S %p";
> >> + clock->refresh_timer = 1;
> >> + break;
> >> + }
> >> +
> >> clock->clock_task.run = clock_func;
> >> display_watch_fd(window_get_display(panel->window), clock->clock_fd,
> >> EPOLLIN, &clock->clock_task);
> >> @@ -450,8 +476,13 @@ panel_resize_handler(struct widget *widget,
> >> x, y - h / 2, w + 1, h + 1);
> >> x += w + 10;
> >> }
> >> - h=20;
> >> - w=170;
> >> +
> >> + h = 20;
> >> +
> >> + if (panel->clock_format == CLOCK_FORMAT_SECONDS)
> >> + w = 190;
> >> + else /* CLOCK_FORMAT_MINUTES */
> >> + w = 170;
> >>
> >> if (panel->clock)
> >> widget_set_allocation(panel->clock->widget,
> >> @@ -492,7 +523,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 +540,7 @@ panel_create(struct desktop *desktop)
> >> {
> >> struct panel *panel;
> >> struct weston_config_section *s;
> >> + char *clock_format_option = NULL;
> >>
> >> panel = xzalloc(sizeof *panel);
> >>
> >> @@ -522,6 +555,18 @@ panel_create(struct desktop *desktop)
> >> widget_set_redraw_handler(panel->widget, panel_redraw_handler);
> >> widget_set_resize_handler(panel->widget, panel_resize_handler);
> >>
> >> + s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
> >> + weston_config_section_get_string(s, "clock-format", &clock_format_option, "");
> >
> > Note that if there is no "shell" defined in the config file,
> > weston_config_get_section will return NULL. Now, it looks like it's ok
> > to pass s==NULL into weston_config_section_get_string, but it will
> > set clock_format_option==NULL.
> >
>
> It will just set the desktop->config to NULL. By doing that,
> weston_config_section_get_anything will return the default value,
> which is the fourth parameter for any of the variants (in my case, an
> empty string).
clock_format_option rather, but yes, the config stuff is pretty
NULL-safe.
However, the string in clock_format_option will be malloc'd, and there
is no free() for it.
> > I believe strcmp will segfault if you pass NULL as its first
> > parameter, that would result in a crash in this next section of
> > code:
>
> Comparing any string with an empty string is a valid operation.
> weston never segfaulted when I commented out the entire [shell]
> section.
>
> >> + if (strcmp(clock_format_option, "minutes") == 0)
> >> + panel->clock_format = CLOCK_FORMAT_MINUTES;
> >> + else if (strcmp(clock_format_option, "seconds") == 0)
> >> + panel->clock_format = CLOCK_FORMAT_SECONDS;
> >> + else if (strcmp(clock_format_option, "none") == 0)
> >> + panel->clock_format = CLOCK_FORMAT_NONE;
> >> + else
> >> + panel->clock_format = DEFAULT_CLOCK_FORMAT;
> >> +
> >
> > I think your initial if statement there should check for
> > clock_format_option==NULL, and set the panel->clock_format to
> > DEFAULT_CLOCK_FORMAT in that case.
> >
>
> See above. Note that the function above may set clock_format_option to
> values different from 3 supported ones (a typo by user), so in such
> case it's necessary to set the format to something (in my case, the
> default format, which makes the most sense).
>
> >> panel_add_clock(panel);
> >>
> >> s = weston_config_get_section(desktop->config, "shell",
> >> NULL, NULL); diff --git a/man/weston.ini.man b/man/weston.ini.man
> >> index 6e92066..8cdc837 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" "."
> >> +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
Everything else in the patch is perfect now. If it wasn't for the
clock_format_option memory leak, I would just push this patch upstream
after testing it. All the other complaints are very minor. There is
also one hunk that doesn't really belong in this patch. ;-)
With the leak fixed, this is:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
(The above sentence means that you can just add my R-b line in your
next version of this patch below your S-o-b line, if you made exactly
the changes I asked for and nothing else.)
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/20160310/9b9336c7/attachment.sig>
More information about the wayland-devel
mailing list