On 03.03.2016 11:25, Pekka Paalanen wrote: > Hi Armin, > Hi,
> 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. :-) > Much appreciated. > 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. > Thank you for taking your time to review this and for such fast response. I believe I've taken into account all of your suggestions in my latest patch. Feel free to be as strict as possible when reviewing it. Thanks once again. > 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 >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
