[PATCH GSoC] desktop-shell: enhance weston-desktop-shell panel's clock
Pekka Paalanen
ppaalanen at gmail.com
Thu Mar 3 10:25:59 UTC 2016
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ć <armin.krezovic at fet.ba> 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
-------------- 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/20160303/441871cf/attachment-0001.sig>
More information about the wayland-devel
mailing list