[PATCH weston GSoC v3] desktop-shell: make panel clock configurable
Armin Krezović
armin.krezovic at fet.ba
Thu Mar 10 00:58:15 UTC 2016
On 09.03.2016 19:57, Bryce Harrington wrote:
> 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.
>
Hi Bryce,
> 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.
>
See below.
>> 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:
>
This is a bit of fail from my side. I use different name/email for my
personal repo. I forgot to change the details in ~/.gitconfig before
I did git commit -a --amend to stash the changes into an already existing
patch. The "Resend ..." line was meant to be purely informational to the
mailing list, not the actual patch. You can notice two v3 mails from this
address, but with different name (well, first one is without the surname
part).
> ---
> 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.
>
Alright. That's a nice idea.
>> ---
>> 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.
>
It will just set the desktop->config to NULL. By doing that, weston_config_section_get_anything
will return the default value, which is the fourth parameter for any of
the variants (in my case, an empty string).
> 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:
>
Comparing any string with an empty string is a valid operation. weston never
segfaulted when I commented out the entire [shell] section.
>> + 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.
>
See above. Note that the function above may set clock_format_option to
values different from 3 supported ones (a typo by user), so in such
case it's necessary to set the format to something (in my case, the
default format, which makes the most sense).
>> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160310/0e853f74/attachment.sig>
More information about the wayland-devel
mailing list