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 cbf150a065084c7571834b34d7e562fe85ab798a..b2b2b0390f318bdebd5dbdab6fb9b39e482a492c 100644 (file)
--- 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 376540ec6f107dcb9f42c2af95628ba7f501acb1..720f3e2bcd9bd0a4f1c9552f458a51ff4e937aa9 100644 (file)
--- 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 3c975b2494dd0c53bb2302328cab970f7df8e7e1..62feb5bbb22453102137a5f6a6ce25c0cf308c09 100644 (file)
--- 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 1c070bd620570a8e158c05fc68ec40c1167667b4..a5001c400333afe1e04359897fa534fc16ff4be1 100644 (file)
--- 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 a56022c3626c4e112cca288dc880a4d376821740..17599975ac939ee92ec6fdac71e2e5c17aadd6b4 100644 (file)
--- 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 bdbe004d420a9665020708cbf4f8f1c0809fe822..54872cdff81a880b0575bf791ef3991aeeee781e 100644 (file)
--- 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 5c1d08c416db40be8d6d77c4f84b223fe2b49389..b0f1c9f462a7cf6cd5f2e8a59e1b0410e520a144 100644 (file)
--- 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;
+}
+
 /*
 
 <keybind key="C-x">
@@ -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 39a42b7be8e573c2b9726edf495daf40a70ac99d..a1299e4c984ae6591cdcf6d69eacad6d5f4700f4 100644 (file)
--- 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 8ffe11a3ff9522325a9d2794166043a7978833b6..85655e530df51ae11c42045820d8769f32895a34 100644 (file)
--- 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();