Menu

#4 XML keyboard map reader is broken (fixed; pull request)

closed-rejected
nobody
None
1
2012-07-27
2012-07-27
No

The code to read in XML keyboard maps (KeyboardMap::initializeFromXML) was broken. Particularly it choked on XML comments and silently ignored errors.

So I rewrote the code. Changes:

* New code checks for errors (i.e. bogus elements in the input XML) rather than silently skipping over them. The old behavior (combined with the bug described below) made it painful to write custom XML keyboard maps.
* Uses a DOM parser (old code used SAX for no apparent reason).
* Parses XML comments correctly. The old code would choke if the keyboard map contained comments, and silently fail to load the rest of the keymap. The DOM parser gives us this for free.

New code is at https://github.com/mechanical-snail/vmpk/tree/fix_xml_parsing

(Is this the best way to submit patches?)

Discussion

  • Mechanical snail

    • summary: Fix reading in XML keyboard maps (pull request) --> XML keyboard map reader is broken (fixed; pull request)
     
  • Pedro Lopez-Cabanillas

    • labels: 1135229 -->
    • priority: 5 --> 1
    • status: open --> closed-rejected
     
  • Pedro Lopez-Cabanillas

    SVN revision 364 allows comments included in keyboard map XML files. That required changing just 2 lines in the original code, adding "|| reader.isComment()" to the existing conditional expressions. Compare that with your 53 additions and 32 deletions.

    No. This is not the right way to submit patches, neither is the correct style. If an author chooses a method over another alternative, it is *you* as proponent who needs to justify a change (in advance, preferably). And there is a tracker called "patches", that you may want to use, or you can attach a patch to the bug report, or you can send a patch to the author or to the mailing list.

     
  • Pedro Lopez-Cabanillas

    By the way, if you want a future patch to be accepted, then please provide a real person name. I won't give credit to someone signing as "mechanical snail".

     

Log in to post a comment.