GNOME Bugzilla – Bug 755556
Add XML parser
Last modified: 2015-11-20 14:56:41 UTC
We need a native XML parser for sources to use. The Apple Trailers uses a hand-made parsing that's pretty impossible to debug, a Sopcast plugin would need to support it as well: http://www.sopcast.com/gchlxml The API of http://doc.lubyk.org/xml.html looks like what we'd need.
Do you mean for Lua sources? or in general?
(In reply to Juan A. Suarez Romero from comment #1) > Do you mean for Lua sources? or in general? For lua, hence the lua component. For C, I don't think there's any intermediate format that would make it easier to use.
Created attachment 312634 [details] [review] lua-factory: Add XML parser Implement with libxml2 an XML parser similar to the one offered by: http://doc.lubyk.org/xml.html XML parsing from Bastien Nocera
Created attachment 312635 [details] [review] lua-factory: Fix setting filesizes Filesizes are int64, therefore, don't use grl_data_add_int() for it.
Created attachment 312636 [details] [review] lua-factory: Port Apple Trailers to new XML parser
Comment on attachment 312634 [details] [review] lua-factory: Add XML parser Pretty certain the table building isn't quite right, and that we'll see some errors from the test suite in bug 755447 when it uses the same test case as the Lubyk XML parser.
Created attachment 312668 [details] [review] lua-factory: Fix setting filesizes Filesizes are int64, therefore, don't use grl_data_add_int() for it.
Review of attachment 312635 [details] [review]: First part is good to go. ::: src/lua-factory/grl-lua-library.c @@ +278,2 @@ } break; This is correct. @@ +365,1 @@ GRL_WARNING ("'%s' requires an INT type, while a value '%s' was provided", Unless I'm wrong, this part was fixed by https://git.gnome.org/browse/grilo-plugins/commit/?id=8cc02f2b5cbd1d0974531ff6ff01b051f27d8551
I totally don't understand if I reviewed the wrong patch or what happened :) (Comment 8 from 06/10 but your last patch fixes my comment on 05/10) Sorry if it was my mistake.
Review of attachment 312668 [details] [review]: Looks good
Comment on attachment 312668 [details] [review] lua-factory: Fix setting filesizes Attachment 312668 [details] pushed as 753d3e8 - lua-factory: Fix setting filesizes
Created attachment 313028 [details] [review] lua-factory: Add XML parser Implement with libxml2 an XML parser similar to the one offered by: http://doc.lubyk.org/xml.html XML parsing from Bastien Nocera -- changes -- -> including the root node in the table as well. -> test at: https://bugzilla.gnome.org/show_bug.cgi?id=755447 -> diff between last version and current version: http://paste.fedoraproject.org/277786/51160414/ ps: thanks for the authorship
Review of attachment 313028 [details] [review]: > XML parsing from Bastien Nocera You can probably remove this now, most of my code was rewritten anyway. Looks good to commit after the nitpicky changes, if the test suites passes :) ::: src/lua-factory/lua-library/lua-xml.c @@ +62,3 @@ + + list = g_hash_table_lookup (ht, (gchar *) node->name); + list = g_list_prepend (list, node); I'm sure there's a better data type to use in GLib to store this sort of thing, but I can't find it. We can use that until we figure out a better one anyway. @@ +125,3 @@ + str = xmlNodeListGetString (doc, parent->xmlChildrenNode, 1); + if (str) { + /* We use xml to the value as it is unlikely to have an xml node */ Pretty sure that "xml" is actually a forbidden node name. At least that what the lubyk XML parser docs says: "Since the 'xml' attribute is not allowed in XML, we use this key to store the tag." @@ +132,3 @@ + } + + /* class = 'opt', */ You can remove this comment, as that's the only original comment that's left, and it doesn't help readability.
(In reply to Bastien Nocera from comment #13) > Review of attachment 313028 [details] [review] [review]: > > > XML parsing from Bastien Nocera > > You can probably remove this now, most of my code was rewritten anyway. As your patches were working and I implemented the lua part only, I would prefer to keep the reference. > > Looks good to commit after the nitpicky changes, if the test suites passes :) > > ::: src/lua-factory/lua-library/lua-xml.c > @@ +62,3 @@ > + > + list = g_hash_table_lookup (ht, (gchar *) node->name); > + list = g_list_prepend (list, node); > > I'm sure there's a better data type to use in GLib to store this sort of > thing, but I can't find it. > We can use that until we figure out a better one anyway. > Okay > @@ +125,3 @@ > + str = xmlNodeListGetString (doc, parent->xmlChildrenNode, 1); > + if (str) { > + /* We use xml to the value as it is unlikely to have an xml node */ > > Pretty sure that "xml" is actually a forbidden node name. At least that what > the lubyk XML parser docs says: "Since the 'xml' attribute is not allowed in > XML, we use this key to store the tag." > Changed to "/* We use xml to the value as it is a forbidden node name */" This is also pointed by w3schools [0] in `XML naming rules`: "Any name can be used, no words are reserved (except xml)." [0] http://www.w3schools.com/xml/xml_elements.asp > @@ +132,3 @@ > + } > + > + /* class = 'opt', */ > > You can remove this comment, as that's the only original comment that's > left, and it doesn't help readability. Removed
Comment on attachment 313028 [details] [review] lua-factory: Add XML parser Attachment 313028 [details] pushed as 069d592 - lua-factory: Add XML parser
Created attachment 313737 [details] [review] lua-factory: Port Apple Trailers to new XML parser
Attachment 313737 [details] pushed as df44679 - lua-factory: Port Apple Trailers to new XML parser