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 <m_AT_milesalan.com>
> >
> > 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
> > _AT_@ -1,70 +0,0 @@
> > -static Key keys[] = {
> > - { "1!", XK_1, 1 },
> > - { "2_AT_", 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
> > _AT_@ -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_AT_", 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
> > _AT_@ -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))
> > _AT_@ -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"
> > _AT_@ -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
> > _AT_@ -238,7 +258,6 @@ drawkeyboard(void) {
> > if(keys[i].keysym != 0)
> > drawkey(&keys[i]);
> > }
> > - XSync(dpy, False);
> > }
> >
> > void
> > _AT_@ -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;
> > }
> > _AT_@ -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.
>
> > _AT_@ -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
proycon_AT_anaproy.nl
https://proycon.anaproy.nl
https://github.com/proycon
GnuPG key: 0x39FE11201A31555C
XMPP: proycon_AT_anaproy.nl Matrix: @proycon:matrix.anaproy.nl
Telegram: proycon IRC: proycon (freenode)
Discord: proycon#8272
Mastodon: https://social.anaproy.nl/_AT_proycon (_AT_proycon@social.anaproy.nl)
Twitter: https://twitter.com/proycon
Bitcoin: 1BRptZsKQtqRGSZ5qKbX2azbfiygHxJPsd
Received on Sun Aug 02 2020 - 13:28:22 CEST