On 10.03.2016 12:37, Pekka Paalanen wrote: > On Thu, 10 Mar 2016 01:58:15 +0100 > Armin Krezović <[email protected]> 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ć <[email protected]> >>> >>> 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ć <[email protected]> >>>> >>>> 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 <[email protected]> > > (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.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
