[PATCH weston GSoC v3] desktop-shell: make panel clock configurable
Bryce Harrington
bryce at osg.samsung.com
Wed Mar 9 18:57:01 UTC 2016
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ć <armin.krezovic at fet.ba>
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ć <armin.krezovic at fet.ba>
>
> 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
More information about the wayland-devel
mailing list