From: Mark Jamsek <mark@jamsek.com>
Subject: Re: tog: user-defined keymap timeout
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Thu, 30 Jun 2022 00:39:50 +1000

On 22-06-29 10:21am, Omar Polo wrote:
> Mark Jamsek <mark@jamsek.com> wrote:
> > On 22-06-28 03:32pm, Omar Polo wrote:
> > > Mark Jamsek <mark@jamsek.com> wrote:
> > > > +waits before timing out. 0 disables timeout so that
> > > 
> > > new sentence, new line.
> > 
> > Sorry I missed this. I don't like making the same mistake twice and
> > you've already told me about this. You told me about man -Tlint too and
> > I have been using it but it didn't pick this one up.
> 
> np :)
> 
> i did a simple experiments and it seems that man -Tlint only issue the
> warning when there is a dot followed by some spaces and then an
> uppercase.

I had to do a double take after you pointed it out because I _thought_
I'm sure I ran `man -Tlint` but was second-guessing myself because it
would've picked up the mistake. Usually I format with `gq` in vim, which
double spaces after periods, but obviously didn't in this case because
the line was so short. But now I know `man -Tlint` requires at least
a double space, I'll try to do that as another failsafe.

> (yeah, i could have checked the sources, but experimenting the behaviour
> was more fun!)

:)

> > > >  	wclrtoeol(view->window);
> > > > @@ -927,6 +932,24 @@ get_compound_key(struct tog_view *view, int c)
> > > >  	return c;
> > > >  }
> > > > 
> > > > +static void
> > > > +set_keymap_timeout(void)
> > > > +{
> > > > +	const char *timeout;
> > > > +
> > > > +	/* fallback to default on error or if not set */
> > > > +	timeout = getenv("TOG_KEYMAP_TIMEOUT");
> > > > +	if (timeout != NULL) {
> > > > +		const char	*errstr;
> > > > +		int		 t;
> > > > +
> > > > +		t = strtonum(timeout, 0, 255, &errstr);
> > > > +		if (!errstr)
> > > > +			tog_timeout = t;
> > > 
> > > since we're not really interested in handling failure in the conversion
> > > and strtonum returns 0 on error anyway, I think you can simplify this
> > > back to
> > > 
> > > +	if (timeout != NULL)
> > > +		tog_timeout = strtonum(timeout, 0, 255, NULL);
> > > 
> > > if you're fine that with an invalid value it disables the timeout.
> > > 
> > > (and at that point you could even inline this into the main() if you
> > > want :)
> > 
> > I made the change because I thought it would be better that invalid
> > values fallback to the default value tbh, but I'll defer to your
> > judgement; if you think it's best this way, I'm happy to make the
> > change? If so, should we also document that with something like:
> > 
> >   Invalid values will disable the timeout as if TOG_KEYMAP_TIMEOUT was
> >   defined with 0.
> 
> hum, I haven't really thought about this, sorry!  it's more intuitive
> what you were doing in your original code and set it to the default
> value if the conversion failed.

I think so too and thanks for the review, but I'll wait to see how naddy
feels about this. I'd rather drop the diff if he thinks it's best; I
don't feel particularly strongly about this one way or the other so am
happy either way.

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68