[PATCH GSoC] desktop-shell: enhance weston-desktop-shell panel's clock
Armin Krezović
armin.krezovic at fet.ba
Thu Mar 3 14:53:14 UTC 2016
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 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: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160303/8ea8307d/attachment-0001.sig>
More information about the wayland-devel
mailing list