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.
On Tue, Mar 08, 2016 at 07:50:16PM +0100, Armin Krezović wrote: > From: Armin Krezović <[email protected]> This From line isn't needed in the commit message, since git will pick it up from your email header already. > 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ć <[email protected]> > > 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: --- 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. > --- > 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; > + > 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. 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: > + 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. > 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 Thanks, Bryce _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
