Hi Hiltjo et al, Thanks for your feedback. It took a bit before I could dive back into it. Comments inline:
On 20-07-25 01:26, Hiltjo Posthuma wrote: > On Fri, Jul 24, 2020 at 09:49:55PM +0200, Maarten van Gompel wrote: > > From: Miles Alan <[email protected]> > > > > Fix SIGTERM handler - flip terminate flag in sigterm handler & cleanup > > properly > > > > Modify run function to use select() with a timeout since X events will be > > blocked otherwise and terminate wouldn't apply for a while. > > > > Run XFlush instead of XSync before starting main loop; fixes bug where > > rending > > of keys fails when used in conjunction w/ dwm dock patch > > > > Add pipe key to backslash key > > --- > > layout.en.h | 70 ------------------------------- > > layout.sxmo.h | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > svkbd.c | 73 +++++++++++++++++++++++++++++---- > > 3 files changed, 175 insertions(+), 79 deletions(-) > > delete mode 100644 layout.en.h > > create mode 100644 layout.sxmo.h > > > > diff --git a/layout.en.h b/layout.en.h > > deleted file mode 100644 > > index b7291b5..0000000 > > --- a/layout.en.h > > +++ /dev/null > > @@ -1,70 +0,0 @@ > > -static Key keys[] = { > > - { "1!", XK_1, 1 }, > > - { "2@", XK_2, 1 }, > > - { "3#", XK_3, 1 }, > > - { "4$", XK_4, 1 }, > > - { "5%", XK_5, 1 }, > > - { "6^", XK_6, 1 }, > > - { "7&", XK_7, 1 }, > > - { "8*", XK_8, 1 }, > > - { "9(", XK_9, 1 }, > > - { "0)", XK_0, 1 }, > > - { "-_", XK_minus, 1 }, > > - { "=+", XK_plus, 1 }, > > - { "<-", XK_BackSpace, 2 }, > > - { 0 }, /* New row */ > > - { "->|", XK_Tab, 1 }, > > - { 0, XK_q, 1 }, > > - { 0, XK_w, 1 }, > > - { 0, XK_e, 1 }, > > - { 0, XK_r, 1 }, > > - { 0, XK_t, 1 }, > > - { 0, XK_y, 1 }, > > - { 0, XK_u, 1 }, > > - { 0, XK_i, 1 }, > > - { 0, XK_o, 1 }, > > - { 0, XK_p, 1 }, > > - { "[", XK_bracketleft, 1 }, > > - { "]", XK_bracketright, 1 }, > > - { "Return", XK_Return, 3 }, > > - { 0 }, /* New row */ > > - { 0, XK_Caps_Lock, 2 }, > > - { 0, XK_a, 1 }, > > - { 0, XK_s, 1 }, > > - { 0, XK_d, 1 }, > > - { 0, XK_f, 1 }, > > - { 0, XK_g, 1 }, > > - { 0, XK_h, 1 }, > > - { 0, XK_j, 1 }, > > - { 0, XK_k, 1 }, > > - { 0, XK_l, 1 }, > > - { ":;", XK_semicolon, 1 }, > > - { "'\"", XK_exclam, 1 }, > > - { "\\|", XK_backslash, 1 }, > > - { 0 }, /* New row */ > > - { 0, XK_Shift_L, 3 }, > > - { 0, XK_z, 1 }, > > - { 0, XK_x, 1 }, > > - { 0, XK_c, 1 }, > > - { 0, XK_v, 1 }, > > - { 0, XK_b, 1 }, > > - { 0, XK_n, 1 }, > > - { 0, XK_m, 1 }, > > - { ",", XK_colon, 1 }, > > - { ".", XK_period, 1 }, > > - { "/?", XK_slash, 1 }, > > - { 0, XK_Shift_R, 2 }, > > - { 0 }, /* New row */ > > - { "Ctrl", XK_Control_L, 2 }, > > - { "Alt", XK_Alt_L, 2 }, > > - { "", XK_space, 5 }, > > - { "Alt", XK_Alt_R, 2 }, > > - { "Ctrl", XK_Control_R, 2 }, > > - { "[X]", XK_Cancel, 1}, > > -}; > > - > > -Buttonmod buttonmods[] = { > > - { XK_Shift_L, Button2 }, > > - { XK_Alt_L, Button3 }, > > -}; > > - > > diff --git a/layout.sxmo.h b/layout.sxmo.h > > new file mode 100644 > > index 0000000..52ce781 > > --- /dev/null > > +++ b/layout.sxmo.h > > @@ -0,0 +1,111 @@ > > +static Key keys[40] = { NULL }; > > + > > +static Key keys_en[40] = { > > + { 0, XK_q, 1 }, > > + { 0, XK_w, 1 }, > > + { 0, XK_e, 1 }, > > + { 0, XK_r, 1 }, > > + { 0, XK_t, 1 }, > > + { 0, XK_y, 1 }, > > + { 0, XK_u, 1 }, > > + { 0, XK_i, 1 }, > > + { 0, XK_o, 1 }, > > + { 0, XK_p, 1 }, > > + > > + { 0 }, /* New row */ > > + > > + { 0, XK_a, 1 }, > > + { 0, XK_s, 1 }, > > + { 0, XK_d, 1 }, > > + { 0, XK_f, 1 }, > > + { 0, XK_g, 1 }, > > + { 0, XK_h, 1 }, > > + { 0, XK_j, 1 }, > > + { 0, XK_k, 1 }, > > + { 0, XK_l, 1 }, > > + { ";:", XK_colon, 1 }, > > + /*{ "'", XK_apostrophe, 2 },*/ > > + > > + { 0 }, /* New row */ > > + > > + { 0, XK_z, 1 }, > > + { 0, XK_x, 1 }, > > + { 0, XK_c, 1 }, > > + { 0, XK_v, 1 }, > > + { 0, XK_b, 1 }, > > + { 0, XK_n, 1 }, > > + { 0, XK_m, 1 }, > > + /*{ "/?", XK_slash, 1 },*/ > > + { "Tab", XK_Tab, 1 }, > > + { "<-", XK_BackSpace, 2 }, > > + > > + { 0 }, /* New row */ > > + { "Layer 2", XK_Cancel, 1}, > > + { "Shift", XK_Shift_L, 1 }, > > + /*{ "L", XK_Left, 1 },*/ > > + { "D", XK_Down, 1 }, > > + { "U", XK_Up, 1 }, > > + /*{ "R", XK_Right, 1 },*/ > > + { "", XK_space, 2 }, > > + { "Esc", XK_Escape, 1 }, > > + { "Ctrl", XK_Control_L, 1 }, > > + /*{ "Alt", XK_Alt_L, 1 },*/ > > + { "Enter", XK_Return, 2 }, > > +}; > > + > > +static Key keys_symbols[40] = { > > + { "1!", XK_1, 1 }, > > + { "2@", XK_2, 1 }, > > + { "3#", XK_3, 1 }, > > + { "4$", XK_4, 1 }, > > + { "5%", XK_5, 1 }, > > + { "6^", XK_6, 1 }, > > + { "7&", XK_7, 1 }, > > + { "8*", XK_8, 1 }, > > + { "9(", XK_9, 1 }, > > + { "0)", XK_0, 1 }, > > + > > + { 0 }, /* New row */ > > + > > + { "'\"", XK_apostrophe, 1 }, > > + { "`~", XK_grave, 1 }, > > + { "-_", XK_minus, 1 }, > > + { "=+", XK_plus, 1 }, > > + { "[{", XK_bracketleft, 1 }, > > + { "]}", XK_bracketright, 1 }, > > + { ",<", XK_comma, 1 }, > > + { ".>", XK_period, 1 }, > > + { "/?", XK_slash, 1 }, > > + { "\\|", XK_backslash, 1 }, > > + > > + { 0 }, /* New row */ > > + > > + { " ", XK_Shift_L|XK_bar, 1 }, > > + { " ", XK_Shift_L|XK_bar, 1 }, > > + { "L", XK_Left, 1 }, > > + { "R", XK_Right, 1 }, > > + { " ", XK_Shift_L|XK_bar, 1 }, > > + { " ", XK_Shift_L|XK_bar, 1 }, > > + { " ", XK_Shift_L|XK_bar, 1 }, > > + { "Tab", XK_Tab, 1 }, > > + { "<-", XK_BackSpace, 2 }, > > + > > + { 0 }, /* New row */ > > + { "Layer 1", XK_Cancel, 1}, > > + { "Shift", XK_Shift_L, 1 }, > > + /*{ "L", XK_Left, 1 },*/ > > + { "D", XK_Down, 1 }, > > + { "U", XK_Up, 1 }, > > + /*{ "R", XK_Right, 1 },*/ > > + { "", XK_space, 2 }, > > + { "Esc", XK_Escape, 1 }, > > + { "Ctrl", XK_Control_L, 1 }, > > + /*{ "Alt", XK_Alt_L, 1 },*/ > > + { "Enter", XK_Return, 2 }, > > +}; > > + > > +Buttonmod buttonmods[] = { > > + { XK_Shift_L, Button2 }, > > + { XK_Alt_L, Button3 }, > > +}; > > + > > Is this layout specific for sxmo or the Pinephone in general? Let's say it is optimised for smartphone-sized displays, for which the regular layouts were too big (buttons too small). Perhaps you may want to get rid of the specific sxmo name for this layout? Perhaps something like "layout.mini-en.h" and the one introduced by the later patch in this series as "layout.mini-intl.h" ? > > > > diff --git a/svkbd.c b/svkbd.c > > index 337f769..7d93d78 100644 > > --- a/svkbd.c > > +++ b/svkbd.c > > @@ -13,6 +13,9 @@ > > #include <X11/Xutil.h> > > #include <X11/Xproto.h> > > #include <X11/extensions/XTest.h> > > +#include <signal.h> > > +#include <sys/select.h> > > + > > > > /* macros */ > > #define MAX(a, b) ((a) > (b) ? (a) : (b)) > > @@ -98,6 +101,8 @@ static int rows = 0, ww = 0, wh = 0, wx = 0, wy = 0; > > static char *name = "svkbd"; > > > > Bool ispressing = False; > > +Bool baselayer = True; > > +Bool sigtermd = False; > > > > /* configuration, allows nested code to access above variables */ > > #include "config.h" > > @@ -185,6 +190,21 @@ buttonrelease(XEvent *e) { > > > > void > > cleanup(void) { > > + int i; > > + > > + // E.g. Generally in scripts we call SIGTERM on svkbd in which case > > + // if the user is holding for example the enter key (to execute > > + // the kill or script that does the kill), that causes an issue > > + // since then X doesn't know the keyup is never coming.. (since > > + // process will be dead before finger lifts - in that case we > > + // just trigger out fake up presses for all keys > > + if (sigtermd) { > > + for (i = 0; i < LENGTH(keys); i++) { > > + XTestFakeKeyEvent(dpy, XKeysymToKeycode(dpy, > > keys[i].keysym), False, 0); > > + } > > + } > > + XSync(dpy, False); > > + > > if(dc.font.set) > > XFreeFontSet(dpy, dc.font.set); > > else > > @@ -238,7 +258,6 @@ drawkeyboard(void) { > > if(keys[i].keysym != 0) > > drawkey(&keys[i]); > > } > > - XSync(dpy, False); > > } > > > > void > > @@ -393,7 +412,10 @@ unpress(Key *k, KeySym mod) { > > if(k != NULL) { > > switch(k->keysym) { > > case XK_Cancel: > > - exit(0); > > + togglelayer(); > > + break; > > + case XK_Break: > > + running = False; > > default: > > break; > > } > > @@ -432,13 +454,29 @@ unpress(Key *k, KeySym mod) { > > void > > run(void) { > > XEvent ev; > > - > > - /* main event loop */ > > - XSync(dpy, False); > > - while(running) { > > - XNextEvent(dpy, &ev); > > - if(handler[ev.type]) > > - (handler[ev.type])(&ev); /* call handler */ > > + int xfd; > > + fd_set fds; > > + struct timeval tv; > > + > > + > > + xfd = ConnectionNumber(dpy); > > + tv.tv_usec = 0; > > + tv.tv_sec = 2; > > + > > + //XSync(dpy, False); > > + XFlush(dpy); > > + > > + while (running) { > > + FD_ZERO(&fds); > > + FD_SET(xfd, &fds); > > + if (select(xfd + 1, &fds, NULL, NULL, &tv)) { > > + while (XPending(dpy)) { > > + XNextEvent(dpy, &ev); > > + if(handler[ev.type]) { > > + (handler[ev.type])(&ev); /* call > > handler */ > > + } > > + } > > + } > > } > > } > > > > The select code looks wrong to me. Instead of the workaround as described near > the comment and XTestFakeKeyEvent the error condition of select() should be > checked also. On many systems select() should also receive EINTR and return -1 > when interrupted by a signal, maybe that solves it? Ok, I'll give that a try and see if that leads to cleaner code, there is some added complexity introduced in the later patches too. > > > @@ -580,11 +618,28 @@ usage(char *argv0) { > > exit(1); > > } > > > > +void > > +togglelayer() { > > + memcpy(&keys, baselayer ? &keys_symbols : &keys_en, sizeof(keys_en)); > > + updatekeys(); > > + drawkeyboard(); > > + baselayer = !baselayer; > > +} > > + > > +void > > +sigterm(int sig) > > +{ > > + running = False; > > + sigtermd = True; > > +} > > + > > int > > main(int argc, char *argv[]) { > > int i, xr, yr, bitm; > > unsigned int wr, hr; > > > > + memcpy(&keys, &keys_en, sizeof(keys_en)); > > + signal(SIGTERM, sigterm); > > for (i = 1; argv[i]; i++) { > > if(!strcmp(argv[i], "-v")) { > > die("svkbd-"VERSION", © 2006-2016 svkbd engineers," > > -- > > 2.27.0 > > > > > > Furthermore can you clean up the code-style a bit and also separate them into > incremental commits? This would make reviewing them easier. Okay, I had squashed a few commits by Miles into one as things got a bit lengthy otherwise and I didn't really know what you guys preferred. I will resubmit them individually then. Expect a new patch series soon. Regards, -- Maarten van Gompel [email protected] https://proycon.anaproy.nl https://github.com/proycon GnuPG key: 0x39FE11201A31555C XMPP: [email protected] Matrix: @proycon:matrix.anaproy.nl Telegram: proycon IRC: proycon (freenode) Discord: proycon#8272 Mastodon: https://social.anaproy.nl/@proycon (@[email protected]) Twitter: https://twitter.com/proycon Bitcoin: 1BRptZsKQtqRGSZ5qKbX2azbfiygHxJPsd
