Inkscape

Bug #1675755
Comment #9

Comment 9 for bug 1675755

Revision history for this message

David Mathog (mathog) wrote on 2017-04-07:

Well a couple of things.

I take it that the bug involves some snprintf() or other standard C++ function I missed in earlier bugs related to this?

Anyway, I think there may be a code maintenance issue in the way you did things.

What I was trying to do previously was to change all relevant snprintf() to snprintf_dots() so that the functions which contained those lines would not need to be surrounded by locale set/restore calls. The presence of that special function would still tell the programmer that there is a locale issue at that specific line which had been handled. In retrospect not the best name, it should have been snprintf_clocale(). As I recall the issue for the bug which started this work was limited to conversion of floats to text going into SVG, which demanded "." and not "," or "$" or whatever, so only those snprintf() with %f formats were affected. There were 3 of them in metafile-print.cpp.

Looks like I didn't change the name on the one in emf_print.cpp which was already surrounded by set/restore locale calls. Had I noticed that I would have removed the surrounding locale code, as you did, but rather than move it up, I would have changed snprintf() to snprintf_dots().

There are now set/restore locale calls around calls to print_document_to_file(). There are not, as far as I can tell, any standard library calls directly in that function though, they are somewhere below. I'm afraid that forcing everything downwards to use a C locale will break something else at some point. That is, I think it would be best to fix only those lines which are known to cause specific locale related problems, rather than changing the environment and letting it propagate down to who knows how many other lines of code.

If you undo that patch (locally) and make the snprintf() to snprintf_dots() change in emf-print.cpp, and there is still a problem with the OPs example, then the lines of code which are causing issues should be identified and fixed explicitly. I'm guessing that the bug will still be there, since as far as I can tell the 4 snprintf() calls (for EMF) I know of which cause problems would all be using C locale at that stage.

Here's how you debug problems like this in the EMF code. Using the command line:

export LANG="lv_LV" #whatever Latvian locale
export INKSCAPE_DBG_EMF=FINALCOMMENT
inkscape 2>/dev/null >/tmp/converted.txt

read in the problem file, and quit once it loads. That will dump the raw SVG to the file before the rest of Inkscape can modify it. Dig through the SVG text in converted.txt and somewhere in there you will find a ',[digit]' or '[digit],' which should have used a '.'. The nearest comment lines will provide a hint where to look in the code. (It is the same process for WMF, just use INKSCAPE_DBG_WMF instead.)

Well a couple of things.

I take it that the bug involves some snprintf() or other standard C++ function I missed in earlier bugs related to this?

Anyway, I think there may be a code maintenance issue in the way you did things.

What I was trying to do previously was to change all relevant snprintf() to snprintf_dots() so that the functions which contained those lines would not need to be surrounded by locale set/restore calls.  The presence of that special function would still tell the programmer that there is a locale issue at that specific line which had been handled.  In retrospect not the best name, it should have been snprintf_clocale().  As I recall the issue for the bug which started this work was limited to conversion of floats to text going into SVG, which demanded "." and not "," or "$" or whatever, so only those snprintf() with %f formats were affected.  There were 3 of them in metafile-print.cpp.

Looks like I didn't change the name on the one in emf_print.cpp which was already surrounded by set/restore locale calls.  Had I noticed that I would have removed the surrounding locale code, as you did, but rather than move it up, I would have changed snprintf() to snprintf_dots().

There are now set/restore locale calls around calls to print_document_to_file().  There are not, as far as I can tell, any standard library calls directly in that function though, they are somewhere below.  I'm afraid that forcing everything downwards to use a C locale will break something else at some point.  That is, I think it would be best to fix only those lines which are known to cause specific locale related problems, rather than changing the environment and letting it propagate down to who knows how many other lines of code.

If you undo that patch (locally) and make the snprintf() to snprintf_dots() change in emf-print.cpp, and there is still a problem with the OPs example, then the lines of code which are causing issues should be identified and fixed explicitly.  I'm guessing that the bug will still be there, since as far as I can tell the 4 snprintf() calls (for EMF) I know of which cause problems would all be using C locale at that stage.

Here's how you debug problems like this in the EMF code.  Using the command line:

export LANG="lv_LV"  #whatever Latvian locale
export INKSCAPE_DBG_EMF=FINALCOMMENT
inkscape 2>/dev/null >/tmp/converted.txt

read in the problem file, and quit once it loads.  That will dump the raw SVG to the file before the rest of Inkscape can modify it.  Dig through the SVG text in converted.txt and somewhere in there you will find a ',[digit]' or '[digit],' which should have used a '.'.   The nearest comment lines will provide a hint where to look in the code.  (It is the same process for WMF, just use INKSCAPE_DBG_WMF instead.)