From: Dana Jansens Date: Sat, 15 Oct 2011 19:11:56 +0000 (-0400) Subject: Fix error printing when parsing and save the last error. X-Git-Url: http://git.openbox.org/?a=commitdiff_plain;h=0628c049697a948b31c30e215378da8fe45a86f6;p=dana%2Fopenbox.git Fix error printing when parsing and save the last error. Errors would have a line number relative to the string of text being parsed, but if this is in a larger file, then it's useless. So add a callback that can modify the line number for parsing things nested in a larger document. This gives proper errors for parsing actions in the xml config files. Also, change how errors are given back when creating a new action. Instead of printing it directly, return an error string so the parser can show what line the invalid action was found on. --- diff --git a/openbox/action.c b/openbox/action.c index cbf150a0..b2b2b039 100644 --- a/openbox/action.c +++ b/openbox/action.c @@ -37,7 +37,7 @@ static void action_definition_ref(ObActionDefinition *def); static void action_definition_unref(ObActionDefinition *def); static gboolean action_interactive_begin_act(ObAction *act, guint state); static void action_interactive_end_act(); -static ObAction* action_find_by_name(const gchar *name); +static ObAction* action_find_by_name(const gchar *name, gchar **error); static ObAction *current_i_act = NULL; static guint current_i_initial_state = 0; @@ -179,7 +179,7 @@ static void action_definition_unref(ObActionDefinition *def) } } -static ObAction* action_find_by_name(const gchar *name) +static ObAction* action_find_by_name(const gchar *name, gchar **error) { GSList *it; ObActionDefinition *def = NULL; @@ -204,18 +204,20 @@ static ObAction* action_find_by_name(const gchar *name) act->i_cancel = NULL; act->i_post = NULL; act->options = NULL; - } else - g_message(_("Invalid action \"%s\" requested. No such action exists."), - name); + } + else if (error) + *error = g_strdup_printf( + _("Invalid action \"%s\" requested. No such action exists."), + name); return act; } -ObAction* action_new(const gchar *name, GHashTable *config) +ObAction* action_new(const gchar *name, GHashTable *config, gchar **error) { ObAction *act = NULL; - act = action_find_by_name(name); + act = action_find_by_name(name, error); if (act) { /* there is more stuff to parse here */ if (act->def->canbeinteractive) { @@ -261,11 +263,9 @@ gboolean action_run(ObAction *act, const ObActionListRun *data, struct _ObClientSet *set) { gboolean ran_interactive; - gboolean update_user_time; gboolean run, run_i; ran_interactive = FALSE; - update_user_time = FALSE; /* If we're starting an interactive action: - if the current interactive action is the same, do nothing and diff --git a/openbox/action.h b/openbox/action.h index 376540ec..720f3e2b 100644 --- a/openbox/action.h +++ b/openbox/action.h @@ -93,7 +93,7 @@ gboolean action_is_interactive(ObAction *act); @values The values of the options passed to the action, paired with the keys. These are ObActionListValue objects. */ -ObAction* action_new(const gchar *name, GHashTable *config); +ObAction* action_new(const gchar *name, GHashTable *config, gchar **error); void action_ref(ObAction *act); void action_unref(ObAction *act); diff --git a/openbox/action_parser.c b/openbox/action_parser.c index 3c975b24..62feb5bb 100644 --- a/openbox/action_parser.c +++ b/openbox/action_parser.c @@ -45,14 +45,20 @@ struct _ObConfigValue* parse_value(ObActionParser *p, gboolean allow_actions, gboolean *e); gchar* parse_string(ObActionParser *p, guchar end, gboolean *e); +static void display_error(GScanner *scanner, gchar *message, gboolean error); struct _ObActionParser { gint ref; GScanner *scan; + + ObActionParserErrorFunc on_error; + gpointer on_error_user_data; }; -ObActionParser* action_parser_new(void) +static ObActionParserError last_error; + +ObActionParser* action_parser_new(const gchar *source) { ObActionParser *p; GScannerConfig config = { @@ -84,9 +90,12 @@ ObActionParser* action_parser_new(void) .store_int64 = FALSE }; - p = g_slice_new(ObActionParser); + p = g_slice_new0(ObActionParser); p->ref = 1; p->scan = g_scanner_new(&config); + p->scan->input_name = source; + p->scan->msg_handler = display_error; + p->scan->user_data = p; return p; } @@ -108,7 +117,8 @@ ObActionList* action_parser_read_string(ObActionParser *p, const gchar *text) gboolean e; g_scanner_input_text(p->scan, text, strlen(text)); - p->scan->input_name = "(console)"; + if (!p->scan->input_name) + p->scan->input_name = "(console)"; e = FALSE; return parse_list(p, G_TOKEN_EOF, &e); @@ -131,6 +141,52 @@ ObActionList* actions_parser_read_file(ObActionParser *p, return parse_list(p, G_TOKEN_EOF, &e); } +void action_parser_set_on_error(ObActionParser *p, ObActionParserErrorFunc f, + gpointer data) +{ + p->on_error = f; + p->on_error_user_data = data; +} + +const ObActionParserError* action_parser_get_last_error(void) +{ + return last_error.message ? &last_error : NULL; +} + +void action_parser_reset_last_error(void) +{ + g_free(last_error.source); + g_free(last_error.message); + last_error.source = last_error.message = NULL; +} + +static void display_error(GScanner *scanner, gchar *message, gboolean error) +{ + ObActionParser *p = (ObActionParser*) scanner->user_data; + guint line = scanner->line; + if (!p->on_error || p->on_error(&line, message, p->on_error_user_data)) { + action_parser_reset_last_error(); + + last_error.source = g_strdup(scanner->input_name); + last_error.line = line; + last_error.message = g_strdup(message); + last_error.is_error = error; + g_message("%s:%u: %s: %s", + p->scan->input_name, + last_error.line, + (last_error.is_error ? "error" : "warning"), + last_error.message); + } +} + +gpointer parse_error(ObActionParser *p, GTokenType exp, const gchar *message, + gboolean *e) +{ + g_scanner_unexp_token(p->scan, exp, NULL, NULL, NULL, message, TRUE); + *e = TRUE; + return NULL; +} + /************* Parser functions. The list is the entry point. ************ BNF for the language: @@ -151,14 +207,6 @@ STRING := "TEXT" | (TEXT) | \\ \( \) and \" are all valid escaped characters. ************** ************/ -gpointer parse_error(ObActionParser *p, GTokenType exp, const gchar *message, - gboolean *e) -{ - g_scanner_unexp_token(p->scan, exp, NULL, NULL, NULL, message, TRUE); - *e = TRUE; - return NULL; -} - ObActionList* parse_list(ObActionParser *p, GTokenType end, gboolean *e) { ObActionList *first, *last; @@ -203,7 +251,7 @@ ObActionList* parse_action(ObActionParser *p, gboolean *e) { GTokenType t; ObActionList *al; - gchar *name; + gchar *name, *error_msg; GHashTable *config; t = g_scanner_get_next_token(p->scan); @@ -261,13 +309,22 @@ ObActionList* parse_action(ObActionParser *p, gboolean *e) t = g_scanner_peek_next_token(p->scan); } + error_msg = NULL; + al = g_slice_new(ObActionList); al->ref = 1; al->isfilterset = FALSE; - al->u.action = action_new(name, config); + al->u.action = action_new(name, config, &error_msg); al->next = NULL; g_free(name); g_hash_table_unref(config); + + if (error_msg) { + display_error(p->scan, error_msg, TRUE); + *e = TRUE; + g_free(error_msg); + } + return al; } diff --git a/openbox/action_parser.h b/openbox/action_parser.h index 1c070bd6..a5001c40 100644 --- a/openbox/action_parser.h +++ b/openbox/action_parser.h @@ -21,12 +21,36 @@ struct _ObActionList; typedef struct _ObActionParser ObActionParser; +typedef struct _ObActionParserError ObActionParserError; + +struct _ObActionParserError { + gchar *source; + guint line; + gchar *message; + gboolean is_error; /*!< If false, it was a warning and not fatal. */ +}; + +/*! A callback for when an error occurs while parsing. Return TRUE to show + the error, and FALSE otherwise. This function can adjust the line number + if it is parsing data nested within a source. +*/ +typedef gboolean (*ObActionParserErrorFunc)(guint *line, const gchar *message, + gpointer user_data); -ObActionParser* action_parser_new(void); +ObActionParser* action_parser_new(const gchar *source); void action_parser_ref(ObActionParser *p); void action_parser_unref(ObActionParser *p); +void action_parser_set_on_error(ObActionParser *p, + ObActionParserErrorFunc callback, + gpointer user_data); + +/*! Returns the last parsing error, or NULL if no errors occured. */ +const ObActionParserError* action_parser_get_last_error(void); + +void action_parser_reset_last_error(void); + struct _ObActionList* action_parser_read_string(ObActionParser *p, const gchar *text); struct _ObActionList* action_parser_read_file(ObActionParser *p, diff --git a/openbox/actions/cyclewindows.c b/openbox/actions/cyclewindows.c index a56022c3..17599975 100644 --- a/openbox/actions/cyclewindows.c +++ b/openbox/actions/cyclewindows.c @@ -107,7 +107,7 @@ static gpointer setup_func(GHashTable *config, action_list_ref(o->actions); } else { - ObActionParser *p = action_parser_new(); + ObActionParser *p = action_parser_new(NULL); o->actions = action_parser_read_string(p, "focus\n" "raise\n" diff --git a/openbox/actions/directionalwindows.c b/openbox/actions/directionalwindows.c index bdbe004d..54872cdf 100644 --- a/openbox/actions/directionalwindows.c +++ b/openbox/actions/directionalwindows.c @@ -108,7 +108,7 @@ static gpointer setup_func(GHashTable *config) action_list_ref(o->actions); } else { - ObActionParser *p = action_parser_new(); + ObActionParser *p = action_parser_new(NULL); o->actions = action_parser_read_string(p, "focus\n" "raise\n" diff --git a/openbox/config.c b/openbox/config.c index 5c1d08c4..b0f1c9f4 100644 --- a/openbox/config.c +++ b/openbox/config.c @@ -369,6 +369,16 @@ static void parse_per_app_settings(xmlNodePtr node, gpointer d) } } +static gboolean action_parser_error(guint *line, const gchar *message, + gpointer data) +{ + /* offset the position of the error by the position we are reading from in + the Xml document */ + guint *xml_line = (guint*)data; + *line = *xml_line + *line - 1; + return TRUE; +} + /* @@ -379,7 +389,7 @@ static void parse_per_app_settings(xmlNodePtr node, gpointer d) */ -static void parse_key(xmlNodePtr node, GList *keylist) +static void parse_key(xmlNodePtr node, GList *keylist, ObtXmlInst *i) { gchar *keystring, **keys, **key; xmlNodePtr n; @@ -396,21 +406,27 @@ static void parse_key(xmlNodePtr node, GList *keylist) if ((n = obt_xml_find_sibling(node->children, "keybind"))) { while (n) { - parse_key(n, keylist); + parse_key(n, keylist, i); n = obt_xml_find_sibling(n->next, "keybind"); } } else if ((n = obt_xml_find_sibling(node->children, "action"))) { + ObActionParser *p; + + p = action_parser_new(obt_xml_file_path(i)); while (n) { - ObActionParser *p; ObActionList *actions; - xmlChar *c; - - c = xmlNodeGetContent(node); - p = action_parser_new(); + gchar *c; + guint line; + + /* Set an error callback for the action parser that will offset + the line of the error by the posision of the containing + tag in the Xml document */ + c = obt_xml_node_string_raw(n); + line = obt_xml_node_line(n); + action_parser_set_on_error(p, action_parser_error, &line); actions = action_parser_read_string(p, (gchar*)c); - xmlFree(c); - action_parser_unref(p); + g_free(c); if (actions) keyboard_bind(keylist, actions); @@ -418,6 +434,7 @@ static void parse_key(xmlNodePtr node, GList *keylist) action_list_unref(actions); n = obt_xml_find_sibling(n->next, "action"); } + action_parser_unref(p); } @@ -432,6 +449,7 @@ static void parse_key(xmlNodePtr node, GList *keylist) static void parse_keyboard(xmlNodePtr node, gpointer d) { + ObtXmlInst *i = (ObtXmlInst*)d; xmlNodePtr n; gchar *key; @@ -446,7 +464,7 @@ static void parse_keyboard(xmlNodePtr node, gpointer d) if ((n = obt_xml_find_sibling(node->children, "keybind"))) while (n) { - parse_key(n, NULL); + parse_key(n, NULL, i); n = obt_xml_find_sibling(n->next, "keybind"); } } @@ -463,6 +481,7 @@ static void parse_keyboard(xmlNodePtr node, gpointer d) static void parse_mouse(xmlNodePtr node, gpointer d) { + ObtXmlInst *i = (ObtXmlInst*)d; xmlNodePtr n, nbut, nact; gchar *buttonstr; gchar *cxstr; @@ -509,6 +528,9 @@ static void parse_mouse(xmlNodePtr node, gpointer d) nbut = obt_xml_find_sibling(n->children, "mousebind"); while (nbut) { + ObActionParser *p; + + buttonstr = NULL; if (!obt_xml_attr_string(nbut, "button", &buttonstr)) goto next_nbut; if (obt_xml_attr_contains(nbut, "action", "press")) @@ -524,24 +546,31 @@ static void parse_mouse(xmlNodePtr node, gpointer d) else goto next_nbut; + p = action_parser_new(obt_xml_file_path(i)); + nact = obt_xml_find_sibling(nbut->children, "action"); while (nact) { ObActionList *actions; - ObActionParser *p; - xmlChar *c; - - c = xmlNodeGetContent(nact); - p = action_parser_new(); + gchar *c; + guint line; + + /* Set an error callback for the action parser that will + offset the line of the error by the posision of the + containing tag in the Xml document */ + c = obt_xml_node_string_raw(nact); + line = obt_xml_node_line(nact); + action_parser_set_on_error(p, action_parser_error, &line); if ((actions = action_parser_read_string(p, (gchar*)c))) mouse_bind(buttonstr, cx, mact, actions); nact = obt_xml_find_sibling(nact->next, "action"); action_list_unref(actions); - xmlFree(c); - action_parser_unref(p); + g_free(c); } - g_free(buttonstr); + + action_parser_unref(p); next_nbut: - nbut = obt_xml_find_sibling(nbut->next, "mousebind"); + g_free(buttonstr); + nbut = obt_xml_find_sibling(nbut->next, "mousebind"); } } g_free(modcxstr); @@ -938,7 +967,7 @@ static void bind_default_keyboard(void) }; ObActionParser *p; - p = action_parser_new(); + p = action_parser_new(NULL); for (it = binds; it->key; ++it) { GList *l = g_list_append(NULL, g_strdup(it->key)); ObActionList *actions = action_parser_read_string(p, it->actiontext); @@ -1012,7 +1041,7 @@ static void bind_default_mouse(void) ObActionParser *p; ObActionList *actions; - p = action_parser_new(); + p = action_parser_new(NULL); for (it = binds; it->button; ++it) { actions = action_parser_read_string(p, it->actname); mouse_bind(it->button, frame_context_from_string(it->context), @@ -1098,7 +1127,7 @@ void config_startup(ObtXmlInst *i) bind_default_keyboard(); - obt_xml_register(i, "keyboard", parse_keyboard, NULL); + obt_xml_register(i, "keyboard", parse_keyboard, i); config_mouse_threshold = 8; config_mouse_dclicktime = 500; @@ -1107,7 +1136,7 @@ void config_startup(ObtXmlInst *i) bind_default_mouse(); - obt_xml_register(i, "mouse", parse_mouse, NULL); + obt_xml_register(i, "mouse", parse_mouse, i); config_resist_win = 10; config_resist_edge = 20; diff --git a/openbox/menu.c b/openbox/menu.c index 39a42b7b..a1299e4c 100644 --- a/openbox/menu.c +++ b/openbox/menu.c @@ -44,6 +44,7 @@ typedef struct _ObMenuParseState ObMenuParseState; struct _ObMenuParseState { + const gchar *source; ObMenu *parent; ObMenu *pipe_creator; }; @@ -92,6 +93,7 @@ void menu_startup(gboolean reconfig) "openbox_menu")) { loaded = TRUE; + menu_parse_state.source = obt_xml_file_path(menu_parse_inst); obt_xml_tree_from_root(menu_parse_inst); obt_xml_close(menu_parse_inst); } else @@ -273,6 +275,14 @@ static gunichar parse_shortcut(const gchar *label, gboolean allow_shortcut, return shortcut; } +static gboolean action_parser_error(guint *line, const gchar *message, + gpointer data) +{ + guint *xml_line = (guint*)data; + *line = *xml_line + *line - 1; + return TRUE; +} + static void parse_menu_item(xmlNodePtr node, gpointer data) { ObMenuParseState *state = data; @@ -286,18 +296,25 @@ static void parse_menu_item(xmlNodePtr node, gpointer data) if (obt_xml_attr_string(node, "label", &label)) { xmlNodePtr c; - xmlChar *cc; + gchar *cc; ObActionList *acts = NULL; ObActionParser *p; c = obt_xml_find_sibling(node->children, "action"); - p = action_parser_new(); + p = action_parser_new(state->source); while (c) { ObActionList *al; + guint line; - cc = xmlNodeGetContent(c); + /* We read text from the XML file and get its line position, + Then we set an error handler which will offset errors + within the text by their position in the XML file. + */ + cc = obt_xml_node_string_raw(c); + line = obt_xml_node_line(c); + action_parser_set_on_error(p, action_parser_error, &line); al = action_parser_read_string(p, (gchar*)cc); - xmlFree(cc); + g_free(cc); acts = action_list_concat(acts, al); c = obt_xml_find_sibling(c->next, "action"); diff --git a/openbox/openbox.c b/openbox/openbox.c index 8ffe11a3..85655e53 100644 --- a/openbox/openbox.c +++ b/openbox/openbox.c @@ -17,6 +17,7 @@ See the COPYING file for a copy of the GNU General Public License. */ +#include "action_parser.h" #include "debug.h" #include "openbox.h" #include "session.h" @@ -225,8 +226,6 @@ gint main(gint argc, gchar **argv) event_reset_time(); do { - ObPrompt *xmlprompt = NULL; - if (reconfigure) obt_keyboard_reload(); { @@ -365,15 +364,24 @@ gint main(gint argc, gchar **argv) /* look for parsing errors */ { - xmlErrorPtr e = xmlGetLastError(); - if (e) { + xmlErrorPtr xe = NULL; + const ObActionParserError *ae = NULL; + + xe = xmlGetLastError(); + if (!xe) ae = action_parser_get_last_error(); + if (xe || ae) { gchar *m; - m = g_strdup_printf(_("One or more XML syntax errors were found while parsing the Openbox configuration files. See stdout for more information. The last error seen was in file \"%s\" line %d, with message: %s"), e->file, e->line, e->message); - xmlprompt = - prompt_show_message(m, _("Openbox Syntax Error"), _("Close")); + m = g_strdup_printf( + _("One or more syntax errors were found while parsing the Openbox configuration files. See stdout for more information. The last error seen was in file \"%s\" line %d, with message: %s"), + (xe ? xe->file : ae->source), + (xe ? xe->line : (signed)ae->line), + (xe ? xe->message : ae->message)); + prompt_show_message(m, _("Openbox Syntax Error"), + _("Close")); g_free(m); - xmlResetError(e); + if (xe) xmlResetError(xe); + else action_parser_reset_last_error(); } } @@ -381,11 +389,6 @@ gint main(gint argc, gchar **argv) ob_set_state(reconfigure ? OB_STATE_RECONFIGURING : OB_STATE_EXITING); - if (xmlprompt) { - prompt_unref(xmlprompt); - xmlprompt = NULL; - } - if (!reconfigure) window_unmanage_all();