Re: [hackers] [slstatus][PATCH] Add flexible formatting to keyboard_indicators.

Ian,

thank you very much for your work! I have a few things to comment before
we can merge the patch:

>+/*
>for num
>+ * lock, each optionally followed by '?', in the order of indicators desired.
>+ * If followed by '?', the letter with case preserved is included in the output
>+ * if the corresponding indicator is on. Otherwise, the letter is always
>+ * included, lowercase when off and uppercase when on.
>+ */

I am not sure if keyboard_indicators.c is the correct place for the
comment? Maybe config.def.h is better? However it is pretty long.
You could also add something like "see keyboard_indicators.c for
additional documentation".
Additionally, for a minimum, add a comment for the argument and an
example in config.def.h (like for the other components).

>+ size_t fmtlen;
>+ size_t i;
>+ size_t n = 0;
>+ int togglecase;
>+ int isset;
>+ char key;

These can be simplified by creating one-liners for vars of the same
type. Additionally according to the coding style vars should be only
declared but not assigned at the beginning of a block.

>+ if (key != 'c' && key != 'n')
>+ continue;
>+ togglecase = (i + 1 >= fmtlen || fmt[i + 1] != '?');
>+ isset = (state.led_mask & (1 << (key == 'n')));
>+ if (togglecase)
>+ buf[n++] = isset ? toupper(key) : key;
>+ else if (isset)
>+ buf[n++] = fmt[i];

Additionally, according we decided to add braces for blocks always, also
for single line statements.

Cheers!
Aaron

-- 
Web: https://drkhsh.at/ or http://drkhsh5rv6pnahas.onion/
Gopher: gopher://drkhsh.at or gopher://drkhsh5rv6pnahas.onion
GPG: 0x7A65E38D55BE96FE
Fingerprint: 4688 907C 8720 3318 0D9F AFDE 7A65 E38D 55BE 96FE