Index | Thread | Search

From:
Job Snijders <job@bsd.nl>
Subject:
Re: cal: add option to highlight the current day
To:
Johannes Thyssen Tishman <jtt@openbsd.org>, tech@openbsd.org, landry@openbsd.org
Date:
Sat, 4 Jul 2026 09:41:36 +0000

Download raw body.

Thread
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).

> 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