Index | Thread | Search

From:
patrick keshishian <pkeshish@gmail.com>
Subject:
Re: cal: add option to highlight the current day
To:
Job Snijders <job@bsd.nl>
Cc:
Johannes Thyssen Tishman <jtt@openbsd.org>, tech@openbsd.org, landry@openbsd.org
Date:
Sat, 4 Jul 2026 16:36:45 -0700

Download raw body.

Thread
On Sat, Jul 4, 2026 at 2:44 AM Job Snijders <job@bsd.nl> wrote:

> Hi Johannes,
>
> Thanks! I've longed for this functionality for some time now. Some
> comments below.
>
> On Fri, Jul 03, 2026 at 04:37:29PM +0000, Johannes Thyssen Tishman wrote:
> > Please find below a patch to add an option (-h) to cal to allow
> > highlighting the current day.
> >
> > For testing purposes, the -h flag takes an optional argument specifying
> > the date to highlight, e.g., cal -h2026-10-10 oct. I intend to remove
> > this before committing if the patch is accepted.
>
> Since the program tests whether standout highlighting can be used, to
> me it would make most sense to highlight the current day by default.
> Highlighting of arbitrary days seems less useful. I suggest remove '-h'
> (bonus: it reduces the size of patch).
>

Please no. The highlighting should not be default, but if you
insist making this feature the default, please allow and option
to disable this highlighting.

Sorry, not a fan of these sorts of extras.
--patrick




>
> > The highlight_day function was mostly taken from FreeBSD[1]. Their
> > implementation falls back to underlining the date (IIUC) if the terminal
> > has no standout mode. I decided to leave this out for now and simply
> > ignore the -h flag in this case.
>
> Good idea.
>
> > To test this and check for regressions, I used the attached script. The
> > script tests the in-tree patched cal against the unpatched installed
> > one. It probably doesn't cover all scenarios, but hopefully enough.  In
> > fact, it is probably a little excessive, as it tests every day of 28
> > years, hehe. There's probably a smarter way to do this.
> >
> > Lastly, I'm not super confident in my C skills yet so please don't hold
> > back on your feedback. I've added some comments with questions in parts
> > I'm not so sure about. I'd be grateful for any input on these.
>
> I recommend removing all the debug code to make the proposed changeset
> more concise. Please see below for some more comments.
>
> > Index: Makefile
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/cal/Makefile,v
> > diff -u -p -r1.3 Makefile
> > --- Makefile  21 Sep 1997 11:48:29 -0000      1.3
> > +++ Makefile  3 Jul 2026 15:22:08 -0000
> > @@ -1,5 +1,6 @@
> >  #    $OpenBSD: Makefile,v 1.3 1997/09/21 11:48:29 deraadt Exp $
> >
> > -PROG=        cal
> > +PROG=                cal
> > +LDADD+=              -lcurses
> >
> >  .include <bsd.prog.mk>
> > Index: cal.1
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/cal/cal.1,v
> > diff -u -p -r1.34 cal.1
> > --- cal.1     2 Jul 2026 20:40:53 -0000       1.34
> > +++ cal.1     3 Jul 2026 15:22:08 -0000
> > @@ -41,7 +41,7 @@
> >  .Nd displays a calendar
> >  .Sh SYNOPSIS
> >  .Nm cal
> > -.Op Fl jmwy
> > +.Op Fl hjmwy
> >  .Op Ar month
> >  .Op Ar year
> >  .Sh DESCRIPTION
> > @@ -51,6 +51,8 @@ Calendars may be displayed by month or b
> >  .Pp
> >  The options are as follows:
> >  .Bl -tag -width Ds
> > +.It Fl h
> > +Highlight the current day.
> >  .It Fl j
> >  Display Julian dates (days one-based, numbered from January 1).
> >  The options
>
> If -h is removed man page update not needed
>
> > Index: cal.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/cal/cal.c,v
> > diff -u -p -r1.36 cal.c
> > --- cal.c     2 Jul 2026 20:40:53 -0000       1.36
> > +++ cal.c     3 Jul 2026 15:22:08 -0000
> > @@ -42,6 +42,8 @@
> >  #include <string.h>
> >  #include <time.h>
> >  #include <unistd.h>
> > +#include <curses.h>
> > +#include <term.h>
> >
> >  #define      THURSDAY                4               /* for reformation
> */
> >  #define      SATURDAY                6               /* 1 Jan 1 was a
> Saturday */
> > @@ -127,8 +129,11 @@ const char       *day_headings = NULL;
> >  int julian;
> >  int mflag = 0;
> >  int wflag = 0;
> > +int highlight = 0;
> > +struct tm *local_time;
> >
> >  void ascii_day(char *, int);
> > +void highlight_day(char *, int, int *);
> >  void center(const char *, int, int);
> >  void day_array(int, int, int *);
> >  int  day_in_week(int, int, int);
> > @@ -141,21 +146,26 @@ void    trim_trailing_spaces(char *);
> >  void usage(void);
> >  void yearly(int);
> >  int  parsemonth(const char *);
> > +int  is_today(int, int, int);
> >
> >  int
> >  main(int argc, char *argv[])
> >  {
> > -     struct tm *local_time;
> >       time_t now;
> >       int ch, month, year, yflag;
> >       const char *errstr;
> > +     const char *hl_date = NULL; /* debug only */
> >
> > -     if (pledge("stdio", NULL) == -1)
> > +     if (pledge("stdio rpath tty", NULL) == -1)
> >               err(1, "pledge");
> >
> >       yflag = year = 0;
> > -     while ((ch = getopt(argc, argv, "jmwy")) != -1)
> > +     while ((ch = getopt(argc, argv, "h::jmwy")) != -1)
> >               switch(ch) {
> > +             case 'h':
> > +                     hl_date = optarg; /* debug only */
> > +                     highlight = isatty(STDOUT_FILENO);
> > +                     break;
>
>
> It might be better to move the detection of highlighting ability into
> a separate function. In that new function, not only use isatty() to
> help determine the nature of the terminal, but also check whether
> tgetstr("so", &ptr) and tgetstr("se", &ptr) return 1.
>
>
> >               case 'j':
> >                       julian = 1;
> >                       break;
> > @@ -190,6 +200,13 @@ main(int argc, char *argv[])
> >               day_headings = DAY_HEADINGS_JS;
> >       }
> >
> > +     (void)time(&now);
> > +     local_time = localtime(&now);
> > +     if (local_time == NULL) {
> > +             perror("localtime");
> > +             exit(1);
> > +     }
> > +
> >       month = 0;
> >       switch(argc) {
> >       case 2:
> > @@ -202,8 +219,6 @@ main(int argc, char *argv[])
> >                       if (yflag)
> >                               errx(1, "specifying a month conflicts with
> -y");
> >                       month = parsemonth(*argv);
> > -                     (void)time(&now);
> > -                     local_time = localtime(&now);
> >                       year = local_time->tm_year + 1900;
> >               } else {
> >                       year = strtonum(*argv, 1, 9999, &errstr);
> > @@ -212,8 +227,6 @@ main(int argc, char *argv[])
> >               }
> >               break;
> >       case 0:
> > -             (void)time(&now);
> > -             local_time = localtime(&now);
> >               year = local_time->tm_year + 1900;
> >               if (!yflag)
> >                       month = local_time->tm_mon + 1;
> > @@ -222,6 +235,28 @@ main(int argc, char *argv[])
> >               usage();
> >       }
> >
> > +     /* debug only */
> > +     if (hl_date != NULL) {
> > +             if (sscanf(hl_date, "%4d-%2d-%2d", &local_time->tm_year,
> > +                 &local_time->tm_mon, &local_time->tm_mday) != 3 ||
> > +                 strlen(hl_date) != 10)
> > +                     errx(1, "illegal date format: use yyyy-mm-dd");
> > +
> > +             if (local_time->tm_year < 1 || local_time->tm_mon < 1 ||
> > +                 local_time->tm_mday < 1 || local_time->tm_year > 9999
> ||
> > +                 local_time->tm_mon > 12 || local_time->tm_mday > 31)
> > +                     errx(1, "illegal date format: use yyyy-mm-dd");
> > +
> > +             local_time->tm_year -= 1900;
> > +             local_time->tm_mon -= 1;
> > +             local_time->tm_hour = 0;
> > +             local_time->tm_min = 0;
> > +             local_time->tm_sec = 0;
> > +             local_time->tm_isdst = -1;
> > +             if (mktime(local_time) == -1 && local_time->tm_wday == -1)
> > +                     errx(1, "specified date is outside allowed range");
> > +     }
> > +
> >       if (month)
> >               monthly(month, year);
> >       else if (julian)
> > @@ -298,8 +333,13 @@ isoweek(int day, int month, int year)
> >  void
> >  monthly(int month, int year)
> >  {
> > -     int col, row, len, days[MAXDAYS], firstday;
> > -     char *p, lineout[30];
> > +     int col, row, len, hl_len, d, days[MAXDAYS], firstday;
> > +
> > +     /*
> > +      * At least 9 bytes for standout mode escape sequence. What is a
> safe
> > +      * size for this? Could this differ from terminal to terminal?
> > +      */
> > +     char *p, lineout[48];
> >
> >       day_array(month, year, days);
> >       (void)snprintf(lineout, sizeof(lineout), "%s %d",
> > @@ -310,15 +350,21 @@ monthly(int month, int year)
> >           lineout, day_headings);
> >       for (row = 0; row < 6; row++) {
> >               firstday = SPACE;
> > +             hl_len = 0;
> >               for (col = 0, p = lineout; col < 7; col++,
> >                   p += julian ? J_DAY_LEN : DAY_LEN) {
> > -                     if (firstday == SPACE && days[row * 7 + col] !=
> SPACE)
> > -                             firstday = days[row * 7 + col];
> > -                     ascii_day(p, days[row * 7 + col]);
> > +                     d = days[row * 7 + col];
> > +                     if (firstday == SPACE && d != SPACE)
> > +                             firstday = d;
> > +                     if (is_today(d, month, year) && highlight) {
> > +                             highlight_day(p, d, &hl_len);
> > +                             p += hl_len;
> > +                     } else
> > +                             ascii_day(p, d);
> >               }
> >               *p = '\0';
> >               trim_trailing_spaces(lineout);
> > -             (void)printf("%-20s", lineout);
> > +             (void)printf("%-*s", 20 + hl_len, lineout);
> >               if (wflag && firstday != SPACE)
> >                       printf(" [%2d]", week(firstday, month, year));
> >               printf("\n");
> > @@ -328,8 +374,13 @@ monthly(int month, int year)
> >  void
> >  j_yearly(int year)
> >  {
> > -     int col, *dp, i, month, row, which_cal;
> > +     int col, *dp, i, month, row, which_cal, hl_len;
> >       int days[12][MAXDAYS];
> > +
> > +     /*
> > +      * At least 9 bytes for standout mode escape sequence. What is a
> safe
> > +      * size for this? Could this differ from terminal to terminal?
> > +      */
> >       char *p, lineout[80];
> >
> >       (void)snprintf(lineout, sizeof(lineout), "%d", year);
> > @@ -346,15 +397,30 @@ j_yearly(int year)
> >                   J_HEAD_SEP, "", day_headings);
> >
> >               for (row = 0; row < 6; row++) {
> > +                     hl_len = 0;
> >                       for (which_cal = 0; which_cal < 2; which_cal++) {
> > -                             p = lineout + which_cal * (J_WEEK_LEN + 2);
> > +                             p = lineout + which_cal * (J_WEEK_LEN + 2)
> + hl_len;
>
> These lines are overly long
>
> >                               dp = &days[month + which_cal][row * 7];
> > -                             for (col = 0; col < 7; col++, p +=
> J_DAY_LEN)
> > -                                     ascii_day(p, *dp++);
> > +                             for (col = 0; col < 7; col++, p +=
> J_DAY_LEN) {
> > +                                     if (is_today(*dp, month +
> which_cal + 1,
> > +                                         year) && highlight) {
> > +                                             highlight_day(p, *dp++,
> &hl_len);
> > +                                             p += hl_len;
> > +                                     } else
> > +                                             ascii_day(p, *dp++);
> > +                             }
> >                       }
> >                       *p = '\0';
> >                       trim_trailing_spaces(lineout);
> >                       (void)printf("%s\n", lineout);
> > +
> > +                     /*
> > +                      * Highlighting a day causes escape sequences to
> > +                      * misalign the rows. Rather than tracking offsets
> to
> > +                      * overwrite rows day-by-day, clear the row.
> > +                      */
> > +                     if (highlight)
> > +                             (void)memset(lineout, ' ', sizeof(lineout)
> - 1);
> >               }
> >       }
> >       (void)printf("\n");
> > @@ -363,9 +429,14 @@ j_yearly(int year)
> >  void
> >  yearly(int year)
> >  {
> > -     int col, *dp, i, month, row, which_cal, week_len, wn, firstday;
> > +     int col, *dp, i, month, row, which_cal, week_len, hl_len, wn,
> firstday;
> >       int days[12][MAXDAYS];
> > -     char *p, lineout[81];
> > +
> > +     /*
> > +      * At least 9 bytes for standout mode escape sequence. What is a
> safe
> > +      * size for this? Could this differ from terminal to terminal?
> > +      */
> > +     char *p, lineout[96];
> >
> >       week_len = WEEK_LEN;
> >       if (wflag)
> > @@ -386,15 +457,21 @@ yearly(int year)
> >                   HEAD_SEP + (wflag ? WEEKNUMBER_LEN : 0), "",
> day_headings);
> >
> >               for (row = 0; row < 6; row++) {
> > +                     hl_len = 0;
> >                       for (which_cal = 0; which_cal < 3; which_cal++) {
> > -                             p = lineout + which_cal * (week_len + 2);
> > +                             p = lineout + which_cal * (week_len + 2) +
> hl_len;
> >
> >                               dp = &days[month + which_cal][row * 7];
> >                               firstday = SPACE;
> >                               for (col = 0; col < 7; col++, p +=
> DAY_LEN) {
> >                                       if (firstday == SPACE && *dp !=
> SPACE)
> >                                               firstday = *dp;
> > -                                     ascii_day(p, *dp++);
> > +                                     if (is_today(*dp, month +
> which_cal + 1,
> > +                                         year) && highlight) {
> > +                                             highlight_day(p, *dp++,
> &hl_len);
> > +                                             p += hl_len;
> > +                                     } else
> > +                                             ascii_day(p, *dp++);
> >                               }
> >                               if (wflag && firstday != SPACE) {
> >                                       wn = week(firstday,
> > @@ -408,6 +485,14 @@ yearly(int year)
> >                       *p = '\0';
> >                       trim_trailing_spaces(lineout);
> >                       (void)printf("%s\n", lineout);
> > +
> > +                     /*
> > +                      * Highlighting a day causes escape sequences to
> > +                      * misalign the rows. Rather than tracking offsets
> to
> > +                      * overwrite rows day-by-day, clear the row.
> > +                      */
> > +                     if (highlight)
> > +                             (void)memset(lineout, ' ', sizeof(lineout)
> - 1);
> >               }
> >       }
> >       (void)printf("\n");
> > @@ -514,6 +599,50 @@ ascii_day(char *p, int day)
> >  }
> >
> >  void
> > +highlight_day(char *p, int day, int *hl_len)
> > +{
> > +     const char *term_so, *term_se;
> > +     char cbuf[512];
> > +     char tbuf[1024], *b;
> > +
> > +     term_se = term_so = NULL;
> > +
> > +     /*
> > +      * why NULL as name here?
> > +      * termcap(3) doesn't seem document this
> > +      */
>
> From termcap(3):
>
>      The tgetstr routine returns the string entry for id, or zero if it is
> not
>      available.  Use tputs to output the returned string.  The area
> parameter
>      is used as follows:
>
>         * It is assumed to be the address of a pointer to a buffer managed
>             by the calling application.
>
>         * However, ncurses checks to ensure that area is not NULL, and also
>             that the resulting buffer pointer is not NULL.  If either check
>             fails, the area parameter is ignored.
>
>         * If the checks succeed, ncurses also copies the return value to
> the
>             buffer pointed to by area, and the area value will be updated
> to
>             point past the null ending this value.
>
>         * The return value itself is an address in the terminal description
>             which is loaded into memory.
>
>
> > +     if (tgetent(tbuf, NULL) == 1) {
> > +             b = cbuf;
> > +
> > +             /*
> > +              * why not NULL here for area instead of &b?
> > +              * cbuf is never used
> > +              */
> > +             term_so = tgetstr("so", &b);
> > +             term_se = tgetstr("se", &b);
> > +     }
>
> I think the above tgetstr() calls only need to be done once (at program
> start). See above suggestion to move setup of the terminal into a
> separate function.
>
> > +     if (term_so != NULL && term_se != NULL) {
> > +             /* highlight on */
> > +             memcpy(p, term_so, strlen(term_so));
> > +             p += strlen(term_so);
> > +
> > +             /* the actual text */
> > +             ascii_day(p, day);
> > +             p += (julian ? J_DAY_LEN : DAY_LEN) - 1;
> > +
> > +             /* highlight off */
> > +             memcpy(p, term_se, strlen(term_se));
> > +             p += strlen(term_se);
> > +             *p = ' ';
> > +             *hl_len = strlen(term_so) + strlen(term_se);
> > +     } else {
> > +             ascii_day(p, day);
> > +             *hl_len = 0;
> > +     }
> > +}
> > +
> > +void
> >  trim_trailing_spaces(char *s)
> >  {
> >       char *p;
> > @@ -539,8 +668,7 @@ center(const char *str, int len, int sep
> >  void
> >  usage(void)
> >  {
> > -
> > -     (void)fprintf(stderr, "usage: cal [-jmwy] [month] [year]\n");
> > +     (void)fprintf(stderr, "usage: cal [-hjmwy] [month] [year]\n");
> >       exit(1);
>
> no change needed here if -h is removed
>
> >  }
> >
> > @@ -560,4 +688,12 @@ parsemonth(const char *s)
> >       if (v <= 0 || v > 12)
> >               errx(1, "invalid month: use 1-12 or a name");
> >       return (v);
> > +}
> > +
> > +int
> > +is_today(int day, int month, int year)
> > +{
> > +     return (year == local_time->tm_year + 1900 &&
> > +             month == local_time->tm_mon + 1 &&
>
> Please use tabs instead of spaces
>
> > +             day == (julian ? local_time->tm_yday + 1 :
> local_time->tm_mday));
>
> This line is overly long, maybe rename 'local_time' to just 'tm' or 'lt'?
>
> Kind regards,
>
> Job
>
>