[GSoC] desktop-shell: enhance weston-desktop-shell panel's clock

Submitted by Armin Krezović on March 3, 2016, 3:47 a.m.

Details

Message ID 1456976834-16669-1-git-send-email-armin.krezovic@fet.ba
State Changes Requested
Headers show
Series "desktop-shell: enhance weston-desktop-shell panel's clock" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Armin Krezović March 3, 2016, 3:47 a.m.
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
---
 clients/desktop-shell.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------
 man/weston.ini.man      |  3 +++
 weston.ini.in           |  1 +
 3 files changed, 66 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

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)
+		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));
 	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) {
+		case FORMAT_MINUTES:
+			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);
 
 	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
 .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

Comments

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@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
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ć <armin.krezovic@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
>