From cc7f16007a21a3e105d6b093be615a574e0d486c Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Thu, 2 Aug 2012 17:43:00 +0200 Subject: [PATCH] Display i3-nagbar when commands lead to an error e.g. pressing Mod1+x when having the following in your configfile: bindsym Mod1+x some invalid command will lead to an i3-nagbar instance popping up, offering you to view the error log (which will contain parser errors from this commit on). --- generate-command-parser.pl | 2 +- include/all.h | 1 + include/key_press.h | 32 ++++ src/commands.c | 1 + src/commands_parser.c | 16 +- src/handlers.c | 52 ----- src/key_press.c | 303 ++++++++++++++++++++++++++++++ src/util.c | 1 + testcases/t/187-commands-parser.t | 14 +- 9 files changed, 358 insertions(+), 64 deletions(-) create mode 100644 include/key_press.h create mode 100644 src/key_press.c diff --git a/generate-command-parser.pl b/generate-command-parser.pl index 993c64fc..01cbe462 100755 --- a/generate-command-parser.pl +++ b/generate-command-parser.pl @@ -152,7 +152,7 @@ for my $state (@keys) { $cmd =~ s/[^(]+\(//; $cmd =~ s/\)$//; $cmd = ", $cmd" if length($cmd) > 0; - say $callfh qq| printf("$fmt\\n"$cmd);|; + say $callfh qq| fprintf(stderr, "$fmt\\n"$cmd);|; say $callfh '#endif'; say $callfh " state = $next_state;"; say $callfh " break;"; diff --git a/include/all.h b/include/all.h index 11eaaba4..c6b0351f 100644 --- a/include/all.h +++ b/include/all.h @@ -54,6 +54,7 @@ #include "i3.h" #include "x.h" #include "click.h" +#include "key_press.h" #include "floating.h" #include "config.h" #include "handlers.h" diff --git a/include/key_press.h b/include/key_press.h new file mode 100644 index 00000000..4d469bab --- /dev/null +++ b/include/key_press.h @@ -0,0 +1,32 @@ +/* + * vim:ts=4:sw=4:expandtab + * + * i3 - an improved dynamic tiling window manager + * © 2009-2012 Michael Stapelberg and contributors (see also: LICENSE) + * + * key_press.c: key press handler + * + */ +#ifndef _KEY_PRESS_H +#define _KEY_PRESS_H + +/** + * There was a key press. We compare this key code with our bindings table and pass + * the bound action to parse_command(). + * + */ +void handle_key_press(xcb_key_press_event_t *event); + +/** + * Kills the commanderror i3-nagbar process, if any. + * + * Called when reloading/restarting, since the user probably fixed his wrong + * keybindings. + * + * If wait_for_it is set (restarting), this function will waitpid(), otherwise, + * ev is assumed to handle it (reloading). + * + */ +void kill_commanderror_nagbar(bool wait_for_it); + +#endif diff --git a/src/commands.c b/src/commands.c index 7b6d55a7..c0681d26 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1443,6 +1443,7 @@ void cmd_exit(I3_CMD) { void cmd_reload(I3_CMD) { LOG("reloading\n"); kill_configerror_nagbar(false); + kill_commanderror_nagbar(false); load_configuration(conn, NULL, true); x_set_i3_atoms(); /* Send an IPC event just in case the ws names have changed */ diff --git a/src/commands_parser.c b/src/commands_parser.c index e505c944..9af8e981 100644 --- a/src/commands_parser.c +++ b/src/commands_parser.c @@ -389,9 +389,9 @@ struct CommandResult *parse_command(const char *input) { position[(copywalk - input)] = (copywalk >= walk ? '^' : ' '); position[len] = '\0'; - printf("%s\n", errormessage); - printf("Your command: %s\n", input); - printf(" %s\n", position); + ELOG("%s\n", errormessage); + ELOG("Your command: %s\n", input); + ELOG(" %s\n", position); /* Format this error message as a JSON reply. */ y(map_open); @@ -435,7 +435,15 @@ void debuglog(char *fmt, ...) { va_list args; va_start(args, fmt); - fprintf(stderr, "# "); + fprintf(stdout, "# "); + vfprintf(stdout, fmt, args); + va_end(args); +} + +void errorlog(char *fmt, ...) { + va_list args; + + va_start(args, fmt); vfprintf(stderr, fmt, args); va_end(args); } diff --git a/src/handlers.c b/src/handlers.c index cb76eb78..048d3a3a 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -77,58 +77,6 @@ bool event_is_ignored(const int sequence, const int response_type) { return false; } - -/* - * There was a key press. We compare this key code with our bindings table and pass - * the bound action to parse_command(). - * - */ -static void handle_key_press(xcb_key_press_event_t *event) { - - last_timestamp = event->time; - - DLOG("Keypress %d, state raw = %d\n", event->detail, event->state); - - /* Remove the numlock bit, all other bits are modifiers we can bind to */ - uint16_t state_filtered = event->state & ~(xcb_numlock_mask | XCB_MOD_MASK_LOCK); - DLOG("(removed numlock, state = %d)\n", state_filtered); - /* Only use the lower 8 bits of the state (modifier masks) so that mouse - * button masks are filtered out */ - state_filtered &= 0xFF; - DLOG("(removed upper 8 bits, state = %d)\n", state_filtered); - - if (xkb_current_group == XkbGroup2Index) - state_filtered |= BIND_MODE_SWITCH; - - DLOG("(checked mode_switch, state %d)\n", state_filtered); - - /* Find the binding */ - Binding *bind = get_binding(state_filtered, event->detail); - - /* No match? Then the user has Mode_switch enabled but does not have a - * specific keybinding. Fall back to the default keybindings (without - * Mode_switch). Makes it much more convenient for users of a hybrid - * layout (like us, ru). */ - if (bind == NULL) { - state_filtered &= ~(BIND_MODE_SWITCH); - DLOG("no match, new state_filtered = %d\n", state_filtered); - if ((bind = get_binding(state_filtered, event->detail)) == NULL) { - ELOG("Could not lookup key binding (modifiers %d, keycode %d)\n", - state_filtered, event->detail); - return; - } - } - - char *command_copy = sstrdup(bind->command); - struct CommandResult *command_output = parse_command(command_copy); - free(command_copy); - - if (command_output->needs_tree_render) - tree_render(); - - yajl_gen_free(command_output->json_gen); -} - /* * Called with coordinates of an enter_notify event or motion_notify event * to check if the user crossed virtual screen boundaries and adjust the diff --git a/src/key_press.c b/src/key_press.c new file mode 100644 index 00000000..9aaea8f9 --- /dev/null +++ b/src/key_press.c @@ -0,0 +1,303 @@ +/* + * vim:ts=4:sw=4:expandtab + * + * i3 - an improved dynamic tiling window manager + * © 2009-2012 Michael Stapelberg and contributors (see also: LICENSE) + * + * key_press.c: key press handler + * + */ +#include +#include +#include +#include +#include "all.h" + +static int current_nesting_level; +static bool success_key; +static bool command_failed; + +/* XXX: I don’t want to touch too much of the nagbar code at once, but we + * should refactor this with src/cfgparse.y into a clean generic nagbar + * interface. It might come in handy in other situations within i3, too. */ +static char *pager_script_path; +static pid_t nagbar_pid = -1; + +/* + * Handler which will be called when we get a SIGCHLD for the nagbar, meaning + * it exited (or could not be started, depending on the exit code). + * + */ +static void nagbar_exited(EV_P_ ev_child *watcher, int revents) { + ev_child_stop(EV_A_ watcher); + + if (unlink(pager_script_path) != 0) + warn("Could not delete temporary i3-nagbar script %s", pager_script_path); + + if (!WIFEXITED(watcher->rstatus)) { + fprintf(stderr, "ERROR: i3-nagbar did not exit normally.\n"); + return; + } + + int exitcode = WEXITSTATUS(watcher->rstatus); + printf("i3-nagbar process exited with status %d\n", exitcode); + if (exitcode == 2) { + fprintf(stderr, "ERROR: i3-nagbar could not be found. Is it correctly installed on your system?\n"); + } + + nagbar_pid = -1; +} + +/* We need ev >= 4 for the following code. Since it is not *that* important (it + * only makes sure that there are no i3-nagbar instances left behind) we still + * support old systems with libev 3. */ +#if EV_VERSION_MAJOR >= 4 +/* + * Cleanup handler. Will be called when i3 exits. Kills i3-nagbar with signal + * SIGKILL (9) to make sure there are no left-over i3-nagbar processes. + * + */ +static void nagbar_cleanup(EV_P_ ev_cleanup *watcher, int revent) { + if (nagbar_pid != -1) { + LOG("Sending SIGKILL (%d) to i3-nagbar with PID %d\n", SIGKILL, nagbar_pid); + kill(nagbar_pid, SIGKILL); + } +} +#endif + +/* + * Writes the given command as a shell script to path. + * Returns true unless something went wrong. + * + */ +static bool write_nagbar_script(const char *path, const char *command) { + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR | S_IXUSR); + if (fd == -1) { + warn("Could not create temporary script to store the nagbar command"); + return false; + } + write(fd, "#!/bin/sh\n", strlen("#!/bin/sh\n")); + write(fd, command, strlen(command)); + close(fd); + return true; +} + +/* + * Starts an i3-nagbar process which alerts the user that his configuration + * file contains one or more errors. Also offers two buttons: One to launch an + * $EDITOR on the config file and another one to launch a $PAGER on the error + * logfile. + * + */ +static void start_commanderror_nagbar(void) { + if (nagbar_pid != -1) { + DLOG("i3-nagbar for command error already running, not starting again.\n"); + return; + } + + DLOG("Starting i3-nagbar due to command error\n"); + + /* We need to create a custom script containing our actual command + * since not every terminal emulator which is contained in + * i3-sensible-terminal supports -e with multiple arguments (and not + * all of them support -e with one quoted argument either). + * + * NB: The paths need to be unique, that is, don’t assume users close + * their nagbars at any point in time (and they still need to work). + * */ + pager_script_path = get_process_filename("nagbar-cfgerror-pager"); + + nagbar_pid = fork(); + if (nagbar_pid == -1) { + warn("Could not fork()"); + return; + } + + /* child */ + if (nagbar_pid == 0) { + char *pager_command; + sasprintf(&pager_command, "i3-sensible-pager \"%s\"\n", errorfilename); + if (!write_nagbar_script(pager_script_path, pager_command)) + return; + + char *pageraction; + sasprintf(&pageraction, "i3-sensible-terminal -e \"%s\"", pager_script_path); + char *argv[] = { + NULL, /* will be replaced by the executable path */ + "-t", + "error", + "-m", + "The configured command for this shortcut could not be run successfully.", + "-b", + "show errors", + pageraction, + NULL + }; + exec_i3_utility("i3-nagbar", argv); + } + + /* parent */ + /* install a child watcher */ + ev_child *child = smalloc(sizeof(ev_child)); + ev_child_init(child, &nagbar_exited, nagbar_pid, 0); + ev_child_start(main_loop, child); + +/* We need ev >= 4 for the following code. Since it is not *that* important (it + * only makes sure that there are no i3-nagbar instances left behind) we still + * support old systems with libev 3. */ +#if EV_VERSION_MAJOR >= 4 + /* install a cleanup watcher (will be called when i3 exits and i3-nagbar is + * still running) */ + ev_cleanup *cleanup = smalloc(sizeof(ev_cleanup)); + ev_cleanup_init(cleanup, nagbar_cleanup); + ev_cleanup_start(main_loop, cleanup); +#endif +} + +/* + * Kills the commanderror i3-nagbar process, if any. + * + * Called when reloading/restarting, since the user probably fixed his wrong + * keybindings. + * + * If wait_for_it is set (restarting), this function will waitpid(), otherwise, + * ev is assumed to handle it (reloading). + * + */ +void kill_commanderror_nagbar(bool wait_for_it) { + if (nagbar_pid == -1) + return; + + if (kill(nagbar_pid, SIGTERM) == -1) + warn("kill(configerror_nagbar) failed"); + + if (!wait_for_it) + return; + + /* When restarting, we don’t enter the ev main loop anymore and after the + * exec(), our old pid is no longer watched. So, ev won’t handle SIGCHLD + * for us and we would end up with a process. Therefore we + * waitpid() here. */ + waitpid(nagbar_pid, NULL, 0); +} + +static int json_boolean(void *ctx, int boolval) { + DLOG("Got bool: %d, success_key %d, nesting_level %d\n", boolval, success_key, current_nesting_level); + + if (success_key && current_nesting_level == 1 && !boolval) + command_failed = true; + + return 1; +} + +#if YAJL_MAJOR >= 2 +static int json_map_key(void *ctx, const unsigned char *stringval, size_t stringlen) { +#else +static int json_map_key(void *ctx, const unsigned char *stringval, unsigned int stringlen) { +#endif + success_key = (stringlen >= strlen("success") && + strncmp((const char*)stringval, "success", strlen("success")) == 0); + return 1; +} + +static int json_start_map(void *ctx) { + current_nesting_level++; + return 1; +} + +static int json_end_map(void *ctx) { + current_nesting_level--; + return 1; +} + +static yajl_callbacks command_error_callbacks = { + NULL, + &json_boolean, + NULL, + NULL, + NULL, + NULL, + &json_start_map, + &json_map_key, + &json_end_map, + NULL, + NULL +}; + +/* + * There was a key press. We compare this key code with our bindings table and pass + * the bound action to parse_command(). + * + */ +void handle_key_press(xcb_key_press_event_t *event) { + + last_timestamp = event->time; + + DLOG("Keypress %d, state raw = %d\n", event->detail, event->state); + + /* Remove the numlock bit, all other bits are modifiers we can bind to */ + uint16_t state_filtered = event->state & ~(xcb_numlock_mask | XCB_MOD_MASK_LOCK); + DLOG("(removed numlock, state = %d)\n", state_filtered); + /* Only use the lower 8 bits of the state (modifier masks) so that mouse + * button masks are filtered out */ + state_filtered &= 0xFF; + DLOG("(removed upper 8 bits, state = %d)\n", state_filtered); + + if (xkb_current_group == XkbGroup2Index) + state_filtered |= BIND_MODE_SWITCH; + + DLOG("(checked mode_switch, state %d)\n", state_filtered); + + /* Find the binding */ + Binding *bind = get_binding(state_filtered, event->detail); + + /* No match? Then the user has Mode_switch enabled but does not have a + * specific keybinding. Fall back to the default keybindings (without + * Mode_switch). Makes it much more convenient for users of a hybrid + * layout (like us, ru). */ + if (bind == NULL) { + state_filtered &= ~(BIND_MODE_SWITCH); + DLOG("no match, new state_filtered = %d\n", state_filtered); + if ((bind = get_binding(state_filtered, event->detail)) == NULL) { + ELOG("Could not lookup key binding (modifiers %d, keycode %d)\n", + state_filtered, event->detail); + return; + } + } + + char *command_copy = sstrdup(bind->command); + struct CommandResult *command_output = parse_command(command_copy); + free(command_copy); + + if (command_output->needs_tree_render) + tree_render(); + + /* We parse the JSON reply to figure out whether there was an error + * ("success" being false in on of the returned dictionaries). */ + const unsigned char *reply; +#if YAJL_MAJOR >= 2 + size_t length; + yajl_handle handle = yajl_alloc(&command_error_callbacks, NULL, NULL); +#else + unsigned int length; + yajl_parser_config parse_conf = { 0, 0 }; + + yajl_handle handle = yajl_alloc(&command_error_callbacks, &parse_conf, NULL, NULL); +#endif + yajl_gen_get_buf(command_output->json_gen, &reply, &length); + + current_nesting_level = 0; + success_key = false; + command_failed = false; + yajl_status state = yajl_parse(handle, reply, length); + if (state != yajl_status_ok) { + ELOG("Could not parse my own reply. That's weird. reply is %.*s\n", length, reply); + } else { + if (command_failed) + start_commanderror_nagbar(); + } + + yajl_free(handle); + + yajl_gen_free(command_output->json_gen); +} diff --git a/src/util.c b/src/util.c index d337963e..76a74db1 100644 --- a/src/util.c +++ b/src/util.c @@ -303,6 +303,7 @@ void i3_restart(bool forget_layout) { char *restart_filename = forget_layout ? NULL : store_restart_layout(); kill_configerror_nagbar(true); + kill_commanderror_nagbar(true); restore_geometry(); diff --git a/testcases/t/187-commands-parser.t b/testcases/t/187-commands-parser.t index 8b57a0a1..335c775d 100644 --- a/testcases/t/187-commands-parser.t +++ b/testcases/t/187-commands-parser.t @@ -12,7 +12,7 @@ sub parser_calls { # TODO: use a timeout, so that we can error out if it doesn’t terminate # TODO: better way of passing arguments - my $stdout = qx(../test.commands_parser '$command' 2>&-); + my $stdout = qx(../test.commands_parser '$command' 2>&1 >&-); # Filter out all debugging output. my @lines = split("\n", $stdout); @@ -127,15 +127,15 @@ is(parser_calls("\nworkspace test"), ################################################################################ is(parser_calls('unknown_literal'), - "Expected one of these tokens: , '[', 'move', 'exec', 'exit', 'restart', 'reload', 'border', 'layout', 'append_layout', 'workspace', 'focus', 'kill', 'open', 'fullscreen', 'split', 'floating', 'mark', 'resize', 'rename', 'nop', 'scratchpad', 'mode'\n" . - "Your command: unknown_literal\n" . - " ^^^^^^^^^^^^^^^", + "ERROR: Expected one of these tokens: , '[', 'move', 'exec', 'exit', 'restart', 'reload', 'border', 'layout', 'append_layout', 'workspace', 'focus', 'kill', 'open', 'fullscreen', 'split', 'floating', 'mark', 'resize', 'rename', 'nop', 'scratchpad', 'mode'\n" . + "ERROR: Your command: unknown_literal\n" . + "ERROR: ^^^^^^^^^^^^^^^", 'error for unknown literal ok'); is(parser_calls('move something to somewhere'), - "Expected one of these tokens: 'window', 'container', 'to', 'workspace', 'output', 'scratchpad', 'left', 'right', 'up', 'down', 'position', 'absolute'\n" . - "Your command: move something to somewhere\n" . - " ^^^^^^^^^^^^^^^^^^^^^^", + "ERROR: Expected one of these tokens: 'window', 'container', 'to', 'workspace', 'output', 'scratchpad', 'left', 'right', 'up', 'down', 'position', 'absolute'\n" . + "ERROR: Your command: move something to somewhere\n" . + "ERROR: ^^^^^^^^^^^^^^^^^^^^^^", 'error for unknown literal ok'); ################################################################################