[PATCH weston GSoC v3] desktop-shell: make panel clock configurable
Armin Krezović
armin.krezovic at fet.ba
Thu Mar 10 16:41:39 UTC 2016
On 10.03.2016 12:37, Pekka Paalanen wrote:
> On Thu, 10 Mar 2016 01:58:15 +0100
> Armin Krezović <armin.krezovic at fet.ba> wrote:
>
>> 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.
>
> Thanks Bryce, I essentially agree with everything you said on the
> commit message.
>
> Armin, please always use reply-to-all, it's polite to keep people CC'd.
> I'm sure we are all subscribed to the list here, but I also have a
> highlight on all emails that CC me personally for instance.
>
Whoops, my bad. I'll use Reply all from now on.
>> 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).
>
> Armin, if you really want different name/email per git repo, you could
> probably put it in the repo config instead of the global config. See
> 'git help config' on how to pick which config it targets. That way you
> don't have edit it all the time.
>
I'll take a look, thanks for the hint.
>>> ---
>>> 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;
>
> I would slightly prefer if this check was in the caller of
> panel_add_clock() instead of here. That way plane_add_clock() would
> always do what it says on the tin.
>
> The same way you already have panel_destroy_clock() conditional on the
> clock existing in the first place. Symmetry is beautiful.
>
Fair enough.
>>>> +
>>>> 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).
>
> clock_format_option rather, but yes, the config stuff is pretty
> NULL-safe.
>
> However, the string in clock_format_option will be malloc'd, and there
> is no free() for it.
>
I didn't catch that, thanks for pointing it out.
>>> 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
>
> Everything else in the patch is perfect now. If it wasn't for the
> clock_format_option memory leak, I would just push this patch upstream
> after testing it. All the other complaints are very minor. There is
> also one hunk that doesn't really belong in this patch. ;-)
>
I suppose you're talking about the whitespace fix? I can revert that
part in the whole if necessary. I've just accidentaly fixed that when
I played with getting the widget size from text extents.
> With the leak fixed, this is:
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> (The above sentence means that you can just add my R-b line in your
> next version of this patch below your S-o-b line, if you made exactly
> the changes I asked for and nothing else.)
>
>
> Thanks,
> pq
>
Thank you once more for taking your time to review and give advices.
-------------- 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/d26fe41e/attachment.sig>
More information about the wayland-devel
mailing list