Emf image with text not shown properly

[Jēkabs Ragže]

I created image in emf format using accelrys draw, but it is not shown properly on Inkscape.
I am using Inkscape 0.92.1 r15371 64 bit version on Windows 7 x64.

[Jēkabs Ragže (djjeshk)]

Attaching screen recording of this file.

[TylerDurden (8thrule)]

Seems to open properly w/ Inkscape 0.92.1 r15371, Win 8.1 64bit

[Jēkabs Ragže (djjeshk)]

Looks like decimal separator and thousand separator issue. Attaching another screen record.

[jazzynico (jazzynico)]

Thanks for taking the time to file a report!

Reproduced on Xubuntu 16.04, Inkscape 0.91, French locales.
Not reproduced with lp:inkscape rev. 15606 and lp:inkscape/0.92.x rev. 15405, same environment.

@Jēkabs, would you be willing to test a recent trunk version (ftp://download.tuxfamily.org/inkscape/win64/) and confirm it's fixed for you too? thanks!

[Jēkabs Ragže (djjeshk)]

Can reproduce it with revisions 15615, 15546, 15536, 15412 on Latvian locale. Temporary workaround is changing decimal symbol from , (comma) to . (dot) for numbers in Control Panel > Region and Language > Additional settings... .

[jazzynico (jazzynico)]

Reproduced on Windows 7 with lp:inkscape rev. 15616 (64-bit, MSYS2 devlibs) and lp:inkscape/0.92.x rev. 15413 (official 32-bit devlibs).

[Patrick Storz (ede123)]

Fixed in
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15617

Please check if there are any regressions, otherwise I'll push to 0.92.x soon.

@mathog:
Subscribing you so you can have a look yourself and see if I missed anything.

[David Mathog (mathog)]

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.)

[Patrick Storz (ede123)]

Hi David,

> What I was trying to do previously was to change all relevant snprintf() to snprintf_dots()

Nah, you didn't... ;-)
I did in http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15568
and I deliberately chose another approach now and removed my previous changes.

This is because I have to disagree with your statement "I think there may be a code maintenance issue" and think actually the opposite is true:
* Wrapping each and every call to the printf/scanf-like functions would certainly solve the issue, but as you explain yourself the effort to debug and trace all relevant occurrences is a huge effort, let alone the extreme code duplication which I'd say does not exactly speak for the idea.
* Even if we did go with that idea it's likely we'd still miss some occurrences especially if "they are somewhere below".
* The previous point is supported by the fact that you already fixed multiple of these, I myself fixed some others, and yet we're still here getting bug reports with locale issues. That's what I'd call a maintenance issue!
* When we individually fix single printf/scanf-like functions it's only a matter of time until new code is added that will again behave unexpectedly.

Therefore at this point I'd actually much prefer my proposed approach:
* Set locale once before doing our work and reset it once after.
* I chose the places of the updated set/restore calls carefully so that only EMF/WMF code should be in between (i.e. no GUI code or similar for which we would not want to set locale to anything but the users choice)
* This way we can ensure that EMF/WMF export actually works consistently and independently of the users locale.

If you prefer to take on the burden of wrapping *all* printf/scanf-like functions that *might* cause issues *at any point* that's fine by me, but in that case we really should be thorough this time (we should be proactive, not reactive by only fixing those occurrences we get notified of by yet another bug report). For me I ruled that out as impractical, especially as there are also many occurrences in libuemf itself (and I didn't want to touch that).

[David Mathog (mathog)]

Oops, sorry, memory fart.

Are there any known locale issues in the code which are not related to the use of %f in a printf() statement? It seems likely that there are some C++ related ones resulting from one of these
types of conversion:

std::ostringstream astrstream;
astrstream << float_val;

std::string string_val = boost::lexical_cast<std::string>(float_val);

std::string string_val = to_string(float_val)

They will be harder to find since at the code line the name may not indicate that a float is involved, ie

   growing_svg << tmp;

Your global change will fix all of these. So let's stick with it.

The global change would also break the locale any float to string conversions which are supposed to land in the data part of a text string or a comment. Off the top of my head I cannot think of anywhere that happens in [EW]MF to SVG, but as we've just seen, my memory shouldn't always be trusted!

[Patrick Storz (ede123)]

> Are there any known locale issues in the code which are not related to the use of %f in a printf() statement?

I'd guess no, but I don't know for sure... I grew tired of diving into the code any deeper while debugging this issue, as I wasn't able to find an obvious printf() statement that produced wrong output. That's basically when I decided to go with the current approach.

> It seems likely that there are some C++ related ones [...]

Interestingly not! When debugging another one of these issues I found that the C++ library functions for manipulating std::string, std::ostringstream and similar are not influenced by the C locale set via "setlocale()" at all (therefore my patch can't change behavior in C++ code).
As a matter of fact the C++ standard library functions to handle strings seem to be designed better then the C string formatting functions, as one can specifically use e.g. "std::ios::imbue()" to change the locale attached to a specific stream without influencing anything else. Also as far as I can see we don't set the global locale for C++ (so it should always be "std::locale::classic()", i.e. the "C" locale).

> The global change would also break the locale any float to string conversions which are supposed to land in the data part of a text string or a comment.

Even if there are any, I'd say it's desirable to have constant output, regardless of the users locale. I don't think comments or the like should be localized (even if there were any relevant cases).
Also debug messages etc. can safely use the default "C" locale in my opinion.

There's one thing I need to fix if we stay with the current approach: From the "setlocale()" documentation [1]: "To be sure you can use the returned string encoding the currently selected locale at a later time, you must make a copy of the string. It is not guaranteed that the returned pointer remains valid over time." I suspect that the string might be overwritten on subsequent calls to setlocale()...

[1] https://www.gnu.org/software/libc/manual/html_node/Setting-the-Locale.html