Hi Armin, this is a good first step, and the patch applies cleanly. I added a comment in bugzilla that you will be working on this. A detailed review follows inline. I may be more nitpicky than usual, because I take this as a teaching session. :-)
IMHO the perfect subject line would be: "[PATCH weston GSoC] desktop-shell: make panel clock configurable" or something like that. I like that you added GSoC in the subject-prefic so I can prioritise looking at GSoC-related contributions. Having "weston" in subject-prefix tells us which repository the patch is intended for, as we have several, and it's not always obvious. As for the rest of the line, it's more a matter of taste. I like being more explicit, "enhance" does not say much. On Thu, 3 Mar 2016 04:47:14 +0100 Armin Krezović <[email protected]> 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. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=57583 This is a good commit message. We also add a Signed-off-by: tag here, which has the intention described here: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n409 Section "Sign your work". You can do it easily by adding '-s' to a 'git commit' command. (Note, that unlike the kernel, reviewers use Acked-by with a weaker meaning: that a person is ok with the idea in a patch but has not properly reviewed the implementation.) > --- > clients/desktop-shell.c | 71 > ++++++++++++++++++++++++++++++++++++++++++------- > man/weston.ini.man | 3 +++ > weston.ini.in | 1 + > 3 files changed, 66 insertions(+), 9 deletions(-) > > diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c > index 6ab76dc..8aa7a99 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 { > @@ -82,6 +84,7 @@ struct panel { > struct widget *widget; > struct wl_list launcher_list; > struct panel_clock *clock; > + struct weston_config *config; > int painted; > uint32_t color; > }; > @@ -122,6 +125,9 @@ struct panel_clock { > struct panel *panel; > struct task clock_task; > int clock_fd; > + int format; > + char *format_string; > + time_t refresh_timer; > }; > > struct unlock_dialog { > @@ -330,6 +336,30 @@ panel_launcher_touch_up_handler(struct widget *widget, > struct input *input, > panel_launcher_activate(launcher); > } > > +enum { > + FORMAT_MINUTES, > + FORMAT_SECONDS, > + FORMAT_NONE > +}; > + > +static int > +panel_clock_get_format(struct panel *panel) { > + struct weston_config_section *s; > + char *clock_format = NULL; > + > + s = weston_config_get_section(panel->config, "shell", NULL, NULL); > + weston_config_section_get_string(s, "clock-format", &clock_format, ""); > + > + if(strcmp(clock_format, "minutes") == 0) Please mind the whitespace here. We use a space after keywords like 'if'. This needs to be fixed over the whole patch. > + return FORMAT_MINUTES; > + else if(strcmp(clock_format, "seconds") == 0) > + return FORMAT_SECONDS; > + else if(strcmp(clock_format, "none") == 0) > + return FORMAT_NONE; > + > + return DEFAULT_CLOCK_FORMAT; > +} > + > static void > clock_func(struct task *task, uint32_t events) > { > @@ -356,7 +386,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) > @@ -369,12 +399,14 @@ panel_clock_redraw_handler(struct widget *widget, void > *data) > 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, > - allocation.y + 3 * (allocation.height >> 2) + 1); > + cairo_move_to(cr, clock->format == FORMAT_MINUTES ? allocation.x + 10 : > + allocation.x - 10, 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, > - allocation.y + 3 * (allocation.height >> 2)); > + cairo_move_to(cr, clock->format == FORMAT_MINUTES ? allocation.x + 10 : > + allocation.x - 10, allocation.y + 3 * > + (allocation.height >> 2)); These will cause drawing outside of the widget's allocation, which possibly tramples over other content and does not match the widget's region for input (if clock reacted to input in any way). As the allocation probably is not enough for the seconds format, you have to fix that in panel_resize_handler() which sets the clock widget's allocation. > cairo_set_source_rgb(cr, 1, 1, 1); > cairo_show_text(cr, string); > cairo_destroy(cr); > @@ -385,9 +417,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"); > @@ -423,6 +455,21 @@ panel_add_clock(struct panel *panel) > clock->panel = panel; > panel->clock = clock; > clock->clock_fd = timerfd; > + clock->format = panel_clock_get_format(panel); > + > + switch(clock->format) { Missing space, 'switch' is a keyword. > + case FORMAT_MINUTES: 'case' should be on the same indent level as 'switch'. > + 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, > @@ -492,7 +539,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,10 +556,12 @@ panel_create(struct desktop *desktop) > { > struct panel *panel; > struct weston_config_section *s; > + int clock_format; > > panel = xzalloc(sizeof *panel); > > panel->base.configure = panel_configure; > + panel->config = desktop->config; > panel->window = window_create_custom(desktop->display); > panel->widget = window_add_widget(panel->window, panel); > wl_list_init(&panel->launcher_list); > @@ -522,7 +572,10 @@ 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); > + clock_format = panel_clock_get_format(panel); > + > + if(clock_format != FORMAT_NONE) > + panel_add_clock(panel); If you passed clock_format as an argument to panel_add_clock(), you would not need panel->config field at all, and panel_add_clock() would not need to re-parse the config string. > > 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..053eb7f 100644 > --- a/man/weston.ini.man > +++ b/man/weston.ini.man > @@ -212,6 +212,9 @@ 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 none, minutes (default) or > seconds. > +.TP 7 Didn't forget the manual, very good. :-) You could highlight the literal words to be used as arguments, similar to what 'background-type' has. > .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, pq
pgpZUa31LH9tQ.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
