omnibus EMF issue patch

> However, since this code has only been tested on Windows I
> cannot exclude the possibility that it might do something
> untoward on another platform.

Testing 'multiple.patch' on OS X 10.7.2 Lion (64bit, llvm-gcc-4.2, debug build) against r11296:

1) patch does not apply: 'newfile' is created in topdir and overwritten for each newly added file

$ patch -p0 --dry-run < ../../_patch/todo/988601-emf-omnibus-multiple.patch
patching file ./build.xml
patching file ./src/CMakeLists.txt
patching file ./src/extension/internal/emf-win32-inout.cpp
patching file ./src/extension/internal/emf-win32-print.cpp
patching file ./src/extension/internal/emf-win32-print.h
patching file ./src/libnrtype/Layout-TNG-Output.cpp
patching file ./src/Makefile.am
(Stripping trailing CRs from patch.)
patching file newfile
(Stripping trailing CRs from patch.)
patching file newfile
(Stripping trailing CRs from patch.)
patching file newfile
(Stripping trailing CRs from patch.)
patching file newfile
(Stripping trailing CRs from patch.)
patching file newfile
patching file newfile
$

2) working around issue 1, 'make' fails due to error in 'src/Makefile.am':

automake-1.11: cannot open < src/libunicode-convert/libunicode-convert: No such file or directory
make[2]: *** [../../src/Makefile.in] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

3) after fixing issue 2 (typo in 'src/Makefile.am'), 'make' fails with this error:

src/libunicode-convert/Makefile_insert:8: bad characters in variable name `libunicode-convert_libunicode-convert_a_SOURCES'
src/Makefile.am:131: `src/libunicode-convert/Makefile_insert' included from here
src/libunicode-convert/Makefile_insert:8: variable `libunicode-convert_libunicode-convert_a_SOURCES' is defined but no program or
src/libunicode-convert/Makefile_insert:8: library has `libunicode-convert_libunicode-convert_a' as canonical name (possible typo)
src/Makefile.am:131: `src/libunicode-convert/Makefile_insert' included from here
make[2]: *** [../../src/Makefile.in] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

No idea how to correctly address this: I did manage to get 'make' succeed by renaming all instances (file names and references) of 'libunicode-convert' to 'libunicodeconvert', but I don't know if that's the appropriate fix, nor what exactly causes automake to fail (possibly the error is related to <http://www.gnu.org/software/automake/manual/html_node/Canonicalization.html#Canonicalization>)

During compilation, I get this warning:
  CC libunicodeconvert/unicodeconvert.o
../../src/libunicodeconvert/unicodeconvert.c: In function ‘table_filler’:
../../src/libunicodeconvert/unicodeconvert.c:1004: warning: comparison between signed and unsigned

Question: Does this new library have to be built and linked to on platforms other than Windows at all? Or could it be useful outside of EMF import/export for enhanced font support?
- Internal EMF import/export support in Inkscape is proprietary to the Windows port of Inkscape and not available on other platforms (the 'emf-win32-*' sources are not compiled).
- AFAICT, the specific font configuration file 'fontfix.conf' gets neither installed on non-wi...

Attaching minimal diff against original patch to configure and compile (without further tests) with autotools:

Diff against patch:
- replace 'newfile' with pathname+file
- fix typo in 'src/Makefile.am'
- fix variable naming in 'src/libunicode-convert/Makefile_insert'
- fix path for 'fontfix.conf' (keeping in 'extensions' for now)

Missing:
- entry in 'configure.ac' for 'libunicode-convert/makefile' (needed?)
- 'share/extensions/Makefile.am': add conf file to 'otherstuff' (?) in order to be installed with autotools

Unknown:
- do we need/want to build and link 'libunicode-convert' on non-Windows platforms?

Sorry about the patch business, I wasn't sure how to create a patch that added new files. It was supposed to create the directory libunicode-convert containing:

295 makefile.in
373 Makefile_insert
74 README
43942 unicode-convert.c
1708 unicode-convert.h

I also had to guess on the makefiles since btool does its own thing on Windows. The makefiles were based on those from other directories in inkscape/src/*, no big surprise if they were not quite right since they could not be tested here.

libunicode-convert is trivial to compile, all it needs is:

 gcc -c unicode-convert.c

Is it needed now on nonWindows versions? That depends on Layout-TNG-Output.cpp. In limited testing the only time the relevant code section was called was for EMF files, but that doesn't mean there isn't another one lurking somewhere else.

Path for fontfix can be whatever you want - the location was arbitrary.

The functions libunicode-convert provides are handy though, and similar unicode <->nonunicode conversions are likely to be useful, at least in the Mac inkscape, as there will be programs around that use/require Symbol or Zapf Dingbats. (The Zapf Dingbats conversions are not tested - on my Windows machine those map to Wingdings. ) Does inkscape Mac read/write PICT? If so, that is the sort of location where it would be applied.

Compilation warning, line 1004 of unicode-convert.c. Valid warning (harmless though as i is always in range 0<->0x100). btool does not show warnings if there are no errors, and when I test compiled it with -Wall on a linux system that older gcc did not complain. Anyway, the fix is to change line 1001 from

int i;

to

unsigned int i;

Cannot help you with the autoconfigure stuff - all we have on Windows is btool.

Sorry about the \r characters - they are not a problem on Windows (nor are they visible). Will see if there is a switch on the editor I am using to not use them.

Patch of comment 2 applied successfully. (Is not the diff in comment 3 a diff to the original patch?)

However, after compilation Inkscape immediately crashed. The fintfix.conf file is in windows encoding. Could the order/presence of the \r or \n characters trigger this crash?

--------------------------------------------------------------------------
E:\Inkscapecode\inkscape\inkscape>inkscape.com
** Message: Expected "f1 f2 f3 Fontname" but did not find it in file: E:\Inkscap
ecode\inkscape\inkscape\share\fonts\fontfix.conf

RegistryTool: Could not set the value 'E:\Inkscapecode\inkscape\inkscape\inkscap
e.exe'
terminate called after throwing an instance of 'char const*'

Emergency save activated!
Emergency save completed. Inkscape will close now.
If you can reproduce this crash, please file a bug at www.inkscape.org
with a detailed description of the steps leading to the crash, so we can fix it.
--------------------------------------------------------------------------

> (Is not the diff in comment 3 a diff to the original patch?)

Yes.

> However, after compilation Inkscape immediately crashed. The fintfix.conf
> file is in windows encoding. Could the order/presence of the \r or \n characters
> trigger this crash?

Maybe you could test replacing the file with the one created by the original patch? I don't recall whether I globally changed the line endings in my patch from comment #2 (that patch was only intended to document my changes when trying to build inkscape using David's original patch with autotools instead of btool).

I had a look at the patch, and it is tackling quite a few things at the same time. So how about getting this in one bit at a time?

Could David perhaps make his patch into two or three separate patches? For example, some of the items sound like pretty straightforward bug fixes, those should be relatively easy to get in. The more difficult stuff can then be looked at separately.

I don't think the available patch is - at this stage - ready to have a target milestone set.
Please add it back if you don't agree.

The attachment contains the pieces to layer over a distribution to get a working EMF import/export on Windows, Linux and most likely, OSX.

It was made with respect to r11367. Highly experimental! It uses my own EMF implementation (in C). libEMF worked great on Linux but then I ran into a ton of symbol conflicts on Windows because it reused GDI function names. So I rolled my own (libUEMF). It neither uses GDI, nor attempts to implement its functions. At some point I will post that separately on Sourceforge, but for now uemf.c and uemf.h live in src/extensions/internal.

For anybody who wants to give this code a shot:

Linux (probably OSX on Intel too):

1. Get a clean distribution.
2. unpack the attached .tar.gz over it
3. ./configure
4. make
5. copy fontfix.conf to /usr/local/share/inkscape/extensions/fontfix.conf
6. to run it: src/inkscape

Windows:

1. Get a clean distribution
2. unpack the attached file over it
3. append the suffix ".ignore" to emf-win32-print.cpp and emf-win32-inout.cpp
4. compile in the usual manner (mingwenv then btool)
5. fontfix.conf should be in the right place.
6. to run it: inkscape\inkscape

After developing this version on linux I didn't need to make a single source code change to build it on Windows!

Known problems:

1. I have no idea how to put a rule into configure so that the EMF stuff only builds on little endian systems. It will NOT work as written on big endian systems. (OSX on PowerMac, for instance.)

2. There is an ugly kludge used to smuggle the character spacing from Layout-TNG-Output.cpp to emf-print.cpp. Search
for the "smuggle" functions to see how it is done. In brief, the data is hidden after the line terminator in a C string in the
first module, and then extracted in the other.

3. The vast majority of the functions in libUEMF have not been tested, but all the pieces used by inkscape have.

4. It isn't a patch because I have no idea what a patch looks like that removes files (here the older emf-win32-*.cpp/.h files).

5. The original code allowed import of WMF, which was accomplished via a GDI call that converted WMF to EMF on the fly. Since this EMF code doesn't use GDI for anything that wasn't possible, and WMF import is lost. It could be put back in on Windows by reintroducing some GDI, or it could be accomplished with an external program. Univconverter WMF import is unaffected (but this was much less reliable than the GDI WMF import).

David, why don't you create a specific branch? It would be easier to manage and your diff would be automatically generated from Launchpad.
See http://doc.bazaar.canonical.com/latest/en/mini-tutorial

My to-do list is already very long, perhaps later in the summer.

In the meantime attached are the latest set of changes, to be applied as above. These were built on top of
r11367 of trunk. Progress since my last post include:

1. libUEMF now builds and has been tested on both Big and Little Endian machines, 32 bit and 64 bit. So that part of the code is now as portable as I can make it.

2. The image handling support in libUEMF, and so now Inkscape, has been greatly expanded. It can now handle an incoming EMR_STRETCHDIBITS as a 16, 24, or 32 bit bitmap, or using the color lookup tables to support the 2, 16, and 256 bit modes.

3. Partial handling of binary and ternary raster operations. For those of you not familiar with these the Windows GDI model
supports combinatorial operations operating on (dst, src, pen/brush). These are very complex, and in general, cannot be applied, or at least easily applied, to an object based model. (Example: dst rectangle AND (partially overlapping src rectangle) OR (brush color). Yuck, right? Anyway, a few of these modes have simple meanings and those have been implemented. All the other modes just draw a rectangle using the current pen/brush. Previous EMF versions ignored all but a single binary raster operation. Either way, EMF files that use these modes do not look exactly the same in WIndows
Preview and Inkscape, but they are closer now than they were, and I think that its better to put something into the SVG to
at least give the user a clue that there was something in the EMF which wasn't handled well.

4. Released libUEMF under gpl 2. The parts needed for Inkscape are in the attachment, but the test code, documentation, and so forth, is only in the full distro. That is at: http://libuemf.sourceforge.net

5. Extensively rearranged the logic for when path draws are made. Look at the code starting at line 993 in emf-inout.cpp in the attachment to see how complex this became. Ugly as that is, it results in Inkscape drawing the "path" parts of test EMF files I threw at it exactly like they were drawn by Windows Preview.

As before, this set of changes, and my current Windows distribution (full binary, ready to run) can be downloaded from here:

  http://saf.bio.caltech.edu/pub/software/windows/

Here is the direct link to the windows build/binary:

  http://saf.bio.caltech.edu/pub/software/windows/inkscape_r11367.zip

Just unpack it on a windows machine and double click on inkscape.exe to run it. This one also has the modification to libglib
that eliminates the 17% CPU problem on my Windows machine (bug 871968).

One minor change today to modify custom dash spacing (see the link above to get my changes file). Here are some examples.
The test_libuemf.emf is the input, test_libuemf.svg is what the modified inkscape read it as, and test_libuemf.png is a screen dump of the input EMF as viewed with Windows Preview.

Bowing to popular demand, here are all of the EMF and related changes as a patch.
This patch worked when applied to r11598 today, both on Linux (32 bit Ubuntu 12.04) and Windows (32 bit XP).

For linux:

get a clean distribution with bzr
patch -p0 -i changes_2012_08_08.patch
autoreconf -i
intltoolize #otherwise it never makes po/makefile.in.in
./configure
make

For Windows (in mingw)

get a clean distribution with bzr
get devlibs32 (recent older ones are missing an include region.h, which will stop the build)
patch -p0 -i changes_2012_08_08.patch
g++ buildtool.cpp -o btool -fopenmp

continuing in Windows in a DOS shell

# set up a mingwenv.bat to point to devlibs32
mingwenv
btool
# wait about an hour

In addition to my EMF changes, the configure.ac has been modified so that it works on Ubuntu 12.04. The version that came with r11598 would not create a working configure. Even with the modifications, there are still some warnings when autoreconf runs.

Applied here. Ubuntu Quantal, r11599

First, why use autoreconf & intltoolize as opposed to just running ./autogen.sh which is the recommended build method on linux? I compiled like normal (./autogen.sh && ./configure && make) and it generated po/Makefile.in.in as expected. Perhaps a handful of the changes to configure.ac might not be necessary?

Now, on to an observation, it segfaults if "fontfix.conf is not found. I noticed this because I initially tried just running it from my build directory without installing first. I installed, fontfix.conf didn't get installed. Attached is a slightly modified patch that adds installing that file, also removing the specific build number from build.xml in your patch.

I will try to actually test a bit tomorrow.

I should have named that better (for the link text). It is the original patch + those mentioned modifications. Also, David, thank you for all of the work you've put into this!

Right about the build.xml version number. The problem is that while a bzr download built on linux picks up the revision number and builds it into the program for "about", a windows build does not do that. The only way I know of to get it in is to modify the build.xml. Probably the linux build runs a "bzr revno" somewhere, but that won't generally work on Windows, since bzr may not
be in the path of the DOS shell being used for btool. (It isn't in my case.)

autoreconf vs. autogen.sh. I figure if autoreconf sees a line as a problem the odds are good that autogen.sh should too, even if it does not explicitly issue a warning.

Did you make the revised patch of comment 17 using "bzr diff", if so, how?

I tested your revised patch, and it is fine. However, after applying it when I tried to pull the changes
back out again with "bzr diff" it omitted the new files in the patch file it created.

The following command in a dos box will make sure the file mynewfile.cpp will be added later to the bzr repository:
bzr add mynewfile.cpp

The samen can be done through the GUI if you have tortoisebzr installed (right-click on the new file in explorer and choose add).

David: No, I did not... I will fix it. I did a diff between my local copies, so the removal part is probably borked by that. I am attaching one I manually corrected (just faster, I have way too many patches applied to my working copy of trunk atm). I also renamed the patch to the bug number as it's easier to track patches and their reports.

Latest version (based on ScislaC's second patch) tested with Inkscape 0.48+devel r11605 on OS X 10.7.4 (64bit):
- compiler: llvm-gcc-4.2 (Apple)
  i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.9.00)
- dependencies: glib 2.32.4, gtk2/x11 2.24.10, pango 1.30.1

Building:
- configured (default), compiled and installed ok
  (debug build with '-g -O0', regular install into custom prefix)
- TODO: testing bundled as osxapp (./configure --enable-osxapp)
  (whether the file 'fontfix.conf' can be loaded from within the relocatable app bundle)
- TODO: test build with Quartz backend of GTK+ (2.24.11)
- TODO: test building on Mac OS X 10.5.8 (32bit) with older versions of the major dependencies, and Apple's GCC 4.2.1

Initial tests done:
- Import: sample EMF file from comment #13 opened with local build looks the same as the SVG file attached in comment #14
  (a diff reveals only differences in the initial seed used for id numbering, but not in the actual content)
- Export: saving as EMF works in basic test (with default export options):
  (test file: 'tiger.svgz' from Inkscape's examples, opened in LibreOffice 3.6.0.4 (Build ID: 932b512) for comparison)
- TODO: more tests with both backends (X11 and Quartz)

Questions:
- AFAICT 'src/extension/internal/uemf_print.c does not get compiled - it this intentional or an oversight?
- AFAICT the patch includes proposed changes from other bug reports you filed separately (e.g. bug #1029584, bug #1002351)?
  Could you please add/maintain a list of other reports whose proposed patches have been included in this omnibus patch [1]?

---
<off-topic>
[1] Maybe it would be easier for you to continue your work on native EMF support in a public branch of current trunk - besides having the benefits of VCS (bzr) for your local changes during development, it also brings better integration with launchpad.net: it would allow to link bug reports to your branch, and close them when a final merge proposal has been reviewed, approved and merged into trunk. It also allows you to merge changes from trunk (i.e. get your feature branch up-to-date with the latest changes from trunk) more easily and - after having solved possible conflicts - produce clean diffs against current trunk.

See also:
 <https://help.launchpad.net/Code/UploadingABranch>
 <https://help.launchpad.net/Code/Review>
</off-topic>

~suv wrote:
> - TODO: test building on Mac OS X 10.5.8 (32bit) (…)

Forgot to mention: the older laptop with Mac OS X 10.5.8 Leopard (32bit) installed also has Intel arch (i386) - I don't have access to a PPC (PowerPC) Mac (wrt regard to little vs big endian systems).

~suv, yes there may be some other patches mixed in. When I get a chance I will make a list.

uemf_print.c and .h are in there because during development some of those functions were needed to dump
the contents of EMF records to stdout during parsing. I do not believe either file will be needed in production code.

One thing somebody should look at is the change to src/style.h. (At line 181 in the patched file, or
search for the string EMF_DRIVER. ) Here is that change:

/* EMF_DRIVER is a temp work-around
The problem is that SPIPaint is private, but is included in the SPStyle struct, and that is needed in emf-inout.cpp, but the compiler
won't accept it there. If the two conditional lines are moved out of the "private" they end up generating undefined reference
errors.
*/

private:
#ifndef EMF_DRIVER
    SPIPaint(SPIPaint const&);
    SPIPaint &operator=(SPIPaint const &);
#endif // EMF_DRIVER
};

Presumably the two conditional lines of code were needed for something originally, and since EMF_DRIVER would be defined on all platforms, this would break whatever function that was (everywhere). Anyway, comment out the ifndef/endif and
build to see what the errors are. It wasn't obvious to me how to rearrange style.h to fix it, other than by removing these two lines. If I remember correctly EMF_DRIVER was only used here, after its definition in emf-inout.cpp.

I just remembered, the list of related issues is already in the first post.

Finally found the access violation which was sometimes showing up on Windows. To do so had to rewrite sections so that there were fewer differences in the way things worked on Windows vs. Linux, so it could be run under valgrind on linux, since there is nothing equivalent on windows. It turned out to be an error related to the "smuggle dx" function, which is used to pass the list of character widths down to the final print routine. Latest patch, with this fix, is attached.

like to say thanks for this patch, I'm using the compiled windows version inkscape_r11367.zip from post #12. Works fine to me (exporting a SVG with embedded PNG to EMF). Now I can wait until this patch is implemented in one of the upcoming developer versions. Great Job!

Latest patch:

1. Fixed a reversion in miterlimit on EMF writes
2. Added pattern/image fill/stroke (will explain further in the next post)
3. Temporary gradient treatment: gradient fill/stroke to EMF are solid with the color that is the average of the gradient's
stops. This works for both linear and radial gradients.
4. Some code simplification.
5. Fixed a minor memory leak.

This is based on r11598. A windows binary with these patches is again available here:

  http://saf.bio.caltech.edu/pub/software/windows/

Concerning pattern and image fill...

The attachment shows the test_libuemf.emf from libUEMF v0.0.7 as viewed in WIndows XP Preview. About 1/3 of the way down on the left there is a sideways "chevron" and to the right of that are two rows of squares.

1. The first row of same sized squares is all EMF defined hatch patterns as FILL with black line as STROKE..
2. The second row of same sized squares is the same, except, after Text and BackGround colors have been changed. You can see that the last 4 "hatch" patterns depend on these settings within the EMF file. Not possible within inkscape though. So when they are read in by Inkscape the object gets the current color, and when written back to EMF they stay with that color, that is, the color is not longer a function of Text/BackGround settings.

3. The third row of expanding squares is a bitmap fill using a color image using the EMR_CREATEDIBPATTERNBRUSHPT record type. The pattern is the same one used for the larger bitmap tests further down the page. The last square in that row, which is "Fuchsia" (ff00ff) is the same bitmap using the EMR_CREATEMONOBRUSH record instead. This combination is broken in all viewers and always shows a solid Text Color, so that is how Inkscape handles it as well.

4. The fourth row of expanding squares is a monochrome bitmap fill using the EMR_CREATEMONOBRUSH record . One color maps to Text and the other BackGround (Aqua, 00FFFF). The last square in that row is the same monochrome bitmap using insted the EMR_CREATEDIBPATTERNBRUSHPT record. This shows the two colors in the bitmap, that is, it does not remap to the Text/Bk color.

5. To the right of (1) and (2) is a series of line segments. The stroke is set with the allowed EMF hatch patterns and there is no fill.

6. To the right of (3) and (4) is a another series of line segments. For each position a thin black line is drawn first. There are a lot variables affecting the stroke over the wider line that overlays it. The left group of 6 uses a color bitmap, the right group of 6 uses a monochrome bitmap. Positions (as letters A-F) vs. EMR record types are:
A,B U_BS_DIBPATTERN
C,D U_BS_DIBPATTERNPT
E,F U_BS_PATTERN
Finally, A,C,E are drawn as solid lines, whereas B,D,F are dashed.

No EMF viewer could handle the dashed image fill, yet the EMF standard does not seem to exclude it. Inkscape reads records like that and drops the dash, so when this test image is viewed in Inkscape A==B, C==D and so forth.

The 3 bitmap stroke types sort of have the same behavior as the 2 bitmap fill types.

On EMF export if Inkscape sees a pattern, but cannot identify it as one of the EMFhatch# it will fill/stroke with a hatch pattern and color FFC3C3.

PowerPoint is miserably bad at importing these sorts of records into Microsoft Draw Objects. They look fine when Paste Image (from file) - because they are still basically a GDI drawing and PowerPoint doesn't mess them up. However, ungroup twice, to convert to Microsoft Draw Objects, and the only ones that work at all are the standard EMF hatch patterns. When those are imported the foreground is right, but the background color goes to black for FILL and to white f...

Correction, this:

A,B U_BS_DIBPATTERN
C,D U_BS_DIBPATTERNPT
E,F U_BS_PATTERN

should have been

A,B EMR_DIBPATTERN
C,D EMR_DIBPATTERNPT
E,F EMR_PATTERN

 Yesterday's changes broke multipath export and input slightly. (For instance, one path surrounding another path would fill the whole thing, not just the region between them.) That is corrected in this patch and in the Windows binary currently on the download site.

New binary up on the download site.

The attached patches extend the handling of gradients. Previously these were pretty much not supported on export, with the average color of the two ends of the gradient used to fill the object. Now there is an option to emulate the gradient using a series of rectangles or elliptical rings for linear/radial gradients, respectively. This is what most graphics programs do when they have to export gradients to an EMF file. Because EMF does not handle transparency very well, gradients with stops having opacity <1.0 have their colors merged against the document's background color.

Note that this slice and dice method is not without its drawbacks. Even with overlapping slices antialiasing during rendering can result in moire patterns or other forms of interference. Also the EMF files tend to relatively large, since hundreds of thin objects are needed for the effect.

The EMF gradientfill record is still problematical. I managed to make EMF files containing triangle gradients (set a color at each corner) but not rectangular gradients, which are still toxic for all EMF applications, for unknown reasons. Near as I can tell no applications use gradientfill records, so for now Inkscape ignores them on input and does not emit them to EMF files.

Ungroup everything to see the slices.

This is what the expanded EMF Output dialog looks like now.

<off-topic>
On 24/07/2012 09:40, JazzyNico wrote:
> David, why don't you create a specific branch? It would be easier to
> manage and your diff would be automatically generated from
> Launchpad.

Agreed.
<https://help.launchpad.net/Code/BugAndBlueprintLinks>
</off-topic>

Attaching cleaned-up diff against current trunk r11664 (please verify).

That patch worked for me on linux. Have not tried it on Windows yet, but don't expect any problems there (only changes in affected files between revisions were the px/pt stuff for font size display.)

It isn't worth posting an entire patch for this, but those px/pt changes prompted me to check font sizes in converted documents and that turned up one rounding error that showed up on rotated text. In emf-print.cpp line 1973 add round() to get:

    int textheight = round(-style->font_size.computed * PX2WORLD * std::min(tf.expansionX(),tf.expansionY()));

In the one test that failed 32pt text rotated 45 degree for came out as -799 when it needed to be -800. round() fixed that.

Maybe this could be useful for coding style questions, too:
Attaching compiler warnings from clang (which is often far more verbose than GCC), for the new files in 'src/extension/internal' of <lp:~inkscape.dev/inkscape/lp988601>.

Your example in comment 155 might seem trivial, but what in the future if code structure changes? What if on the right hand side you would have a function that would return a pointer as opposed to a number?
For this kind of issues, to maintain code ssafety, there are the c++ stylecast, to distinguish between only casting away a "const", a safe cast or a dangerous cast without any control (the reinterpret_cast, let's say the dangerous C-style casting).

It is inkscape programming style to say bye bye to the C-style casts to prevent weird, hard to tackle bugs (this was a goal long before I was working on this). I went on with this task, an exception for a specific part of the codebase can not be made, so I ask you to reuse the C++ style casts.

I do not see a patch ... but ...
regarding the error's, the really good way of handling exceptions, is to show a nice error dialog instead of halting the program. Maybe you can drop a line on the devlist asking for advice or code example of a user friendly dialog box.

Also I was wondering why in the original code global variables are used. (I'd say common in C-programming, but not so good/safe to use from the overal program's point of view) Were they present in the original code? Why are they needed and could not you use a private class variable as alternative?

Oops, I now see the clang compiler warnings. They perfectly illustrate why c++ stylecasts are needed.

Re: 158

Thanks, gcc didn't pick up on the first two. I have fixed them but will wait for the next major patch to release them since neither actually affects anything. The first is in an unused (by Inkscape) function, and the second is just an extra set of
parenthesis around a value, which is harmless.

The "cubic = cubic" line in emf-print.cpp and wmf-print.cpp dates back to before I started working on the code. I can't imagine what the original purpose was , but it will be removed and if nothing breaks, it will stay out.

As for the others...
It helps to keep in mind that compilers are free to warn about anything they want, including things that the programmer intended to do knowing full well what the consequences might be.

The line "torev = torev" was added to shut up warnings on gcc that "torev" was declared but not used. That was true but intentional. "torev" was at the end of the parameter list on a whole series of function calls and I thought it better to maintain the style even if a small number of them did not actually use (in the current version) that parameter. The "torev = torev" construct silenced gcc. Apparently clang is not amused by it.

The rest of the warnings seem to this form, or a slight variant thereof:

../../src/extension/internal/emf-print.cpp:426:27: warning: cast from 'char *' to 'PU_ENHMETARECORD' (aka 'U_ENHMETARECORD *') increases required alignment from 1 to 4 [-Wcast-align]

where what the compiler is warning about was intended and is the correct cast. (If it was not the program would not work.) Note that what the compiler is warning about is not the cast itself, but the change in required alignment that results from it (-Wcast-align). The programmer is aware of the alignment issues (which were fairly horrendous for WMF, where the records are 2N bytes long, leading to embedded int32_t 's becoming misaligned) and these alignment issues are, as best as I can tell, all handled. So, changing the form of the cast to the longer C++ syntax should not eliminate this warning, since any form of cast that accomplishes the same thing is going to change the alignment requirements. I don't use clang, but if ~suv wants to do the experiment, change any one of these C style casts to the C++ syntax and recompile. The associated clang warning should still appear.

So, re 161, these warnings do not appear to indicate any problem with the C style casts.

Re: 160. The way the variables are arranged dates way back and I have just been sticking with the way it was done in the beginning. By global are you referring to the few up at the top of the .cpp files, or the ones in the .h that follow for instance here:

class PrintWmf : public Inkscape::Extension::Implementation::Implementation
{

? Moving the few global variables declared in the .cpp in with the others in the .h is on my todo list - I just keep forgetting "todo" it!

> By global are you referring to the few up at the top of the .cpp files,
Yes indeed. I am referring to the static variable declarations just following the namespace identifiers in the cpp-files.

> Moving the few global variables declared in the .cpp in with the others in the .h is on my todo list
Nice. Thanks for that.

Re: 163

They are placed there for a reason, as it turns out. Those few variables are referenced from within static member functions. So they must in turn be static variables. I tried moving them from emf-inout.cpp into emf-inout .h with no success. It was possible to get the emf-inout.cpp (for instance) to compile with these declared in emf-inout.h (public, private, or protected) but at link time these variables tossed up undefined reference errors everywhere they were used in the emf-inout.cpp file. As I understand this (ie, poorly) static variables declared in an .h files still must be allocated outside of the class in the .cpp file. Doing it the way it is done now actually seems simpler.

For instance, try adding a new variable

  int foobar;

placed anywhere within the class definition in emf-inout.h, and then add the line

  foobar = 1;

in the Emf::init method. Build. The compiler emits this:

extension/internal/emf-inout.h: In static member function ‘static void Inkscape::Extension::Internal::Emf::init()’:
extension/internal/emf-inout.h:138:8: error: invalid use of member ‘Inkscape::Extension::Internal::Emf::foobar’ in static member function
extension/internal/emf-inout.cpp:3486:5: error: from this location

Change emf-inout.h to

  static int foobar;

then the compiler gets past emf-inout.cpp, but emits this at the linkage stage:

libinkscape.a(emf-inout.o): In function `Inkscape::Extension::Internal::Emf::init()':
/usr/local/src/inkscape_lp988601/src/extension/internal/emf-inout.cpp:3486: undefined reference to `Inkscape::Extension::Internal::Emf::foobar'

Please feel free to try it yourself. It seems like the only way to accomplish this would be to get rid of the static on all of the methods that reference these variables, but I vaguely recall that static is also there for a reason, and something else breaks if that is removed. This isn't a problem on emf-print and wmf-print because they are not using static methods, or at least not any that need to reference variables of this sort.

The attached patch does the following:

1. Fixes the clang warnings noted in a post above, other than those associated with alignment caused by casting.

2. Fixes some minor rounding errors in both WMF and EMF input/output. With these fixes in place inkscape can load
test_libuemf.emf from libUEMF, save that as "one.emf", open "one.emf", save as "two.emf". Compare "one.emf" and "two.emf" (using reademf, as they are binaries) and the only difference will be the name stored in the file. (Note - to do this test the lengths of the file names must be identical, otherwise a relative offset is added to the record positions shown by reademf.) For the same test with the WMF format the two output files are identical because WMF does not save the filename in the file. In other words, round trip open/save cycles are conservative for EMF and WMF files (excluding any features that are not full supported in inkscape or the target file format, for instance, gradients, which must be emulated.)

3. Fixed a missing break in the input WMF LINETO record handling, which was falling through into the MOVETO and generating a harmless extra "M" operation in a path.

4. WMF has no POLYPOLYLINE record. However input that maps into essentially a polypolyline record in SVG is common, for instance dashed lines that have been converted to line segments. These end up in SVG as a series of M L M L draw commands in the path. Earlier each M L pair was going out as a polyline record, now they go as a series of MOVETO/LINETO records. The primary reason for this change is that without this change the behavior described in (2) does not occur.

5. Fixed an issue where polyline and polygon records in some instances ended up with an extra copy of their last point.

6. This version has the issue described in an earlier post, where dozens of warnings are generated, one for each bitmapped font installed on the system (apparently). These are harmless, but ugly.

The current windows binary incorporating these changes is here:

http://saf.bio.caltech.edu/pub/software/windows/inkscape_r11729_lp988601.zip

While testing <lp:~inkscape.dev/inkscape/lp988601> on OS X 10.7.5 with the sample SVG file from (otherwise unrelated) bug #1175894:
<https://bugs.launchpad.net/inkscape/+bug/1175894/+attachment/3663365/+files/CD.svg>
I noticed that it misses the group 'g5192' in EMF/WMF exports (tested by roundtrip-editing in Inkscape) - the group contains the logo 'COMPACT disc' outlines as paths/polygons. Converting the <polygon> elements to paths prior to exporting doesn't make a difference.

Confirmed #166. Odd. There is something about that particular set of polybeziers that causes them not to make it into the output. Edited that SVG down to just one of the polybeziers from "CDROM" and a circle, saved to EMF, and the polybezier records were not not emitted to the EMF. However another similar sort of drawing that I just created from scratch, again 1 polybezier and 1 circle, emitted just fine to EMF. Will look into this later.

I see what is going on but am not sure how to categorize the bug.

The factor that is inducing this behavior is that the problem svg has a vestigial style
associated with the graphics that are lost:

    <path
       style="fill-rule:evenodd;clip-rule:evenodd"
       inkscape:connector-curvature="0"
       d="m 217.02273,382.91999 c 0,0 [Large chunk edited out]"
       id="path5135" />

Notice that there are neither stroke nor fill parameters, so inkscape defaults are used.

The path is passed to ::fill(), but emf-print.cpp delays fill operations
if it thinks the corresponding stroke will be called next, in which case it does both operations at once. (This
is because there are stroke+fill EMF operators, which are preferred if the graphic contains both stroke and fill.)

The relevant tests for NOT postponing the fill operation are:

            (style->stroke.noneSet || style->stroke_width.computed == 0.0)

and those values are 0 and 1, respectively, so the fill() IS postponed.

The problem is that the expected stroke() is never called, so there is no output for this element.
It seems to me that the default stroke parameters (as seen in ::fill()) mandate that ::stroke() be called
after ::fill() is, but that never happens.

This problem has not been seen before, by me anyway, since in my experience Inkscape never creates vestigial styles like this. Presumably the problem file was generated by some other application?

Changing the test to this

            (style->stroke.isNone() || style->stroke.noneSet || style->stroke_width.computed == 0.0)

causes fill to execute immediately. Apparently isNone() and noneSet() can have opposite values.

Attaching sample file which exposes the same error: paths with fill and unset stroke are omitted from EMF/WF export.

Based on the SVG 1.1 specification:
Fill 'Unset' (not defined for object, no style otherwise inherited)
-> renders in solid black
Stroke 'Unset' (not defined for object, no style otherwise inhertied)
-> renders as 'none'

References:
‘fill’
    Value: <paint> (See Specifying paint)
    Initial: black
<http://www.w3.org/TR/SVG11/painting.html#FillProperties>

‘stroke’
    Value: <paint> (See Specifying paint)
    Initial: none
<http://www.w3.org/TR/SVG11/painting.html#StrokeProperties>

The attached patch does:

1. Resolves issue of message 170

2. Implements CSS 3 (and CSS 2) text-decoration support. (An example file will be posted next). Note that it does not yet provide any method of adding these features - at present it just shows whatever is in the SVG. This new code is also used to display EMF/WMF strike-through and underline text decorations when these files are read in. Those decorations may also be written out to EMF/WMF. Other text decoration features, like overline, or dotted lines, are dropped. For SVG text-decoration -line, -style, -color are all implemented. CSS3 provides two ways to represent the same state, this
code uses the compound text-decoration method rather than the 3 fields method. Also it leaves out keywords that
are not needed and would break backwards compatibility. For instance:

  text-decoration: underline solid

is valid, but would break CSS2. Solid is the default, so that sort of case is written as:

  text-decoration: underline

If the state is CSS3 specific all of the needed fields are of course include, like

 text-decoration: underline wavy red

3. Fixed more bugs in Hebrew language support than I can remember. Hebrew language export/import to EMF now works quite well. (See the examples in libTERE v 0.7.) WMF does not support unicode and for all intents and purposes Inkscape has no way to read or write Hebrew to it. Some of more important things that now work that didn't (or didn't always): Hebrew diacritical marks, R/L/center justification, and bidirectional text. The Hebrew fonts "Ezra SIL" and "EZRA SIL SR" should be installed before viewing the libTERE examples, otherwise font substitutions will cause some text shifts.

4. Implemented font failover in Text Reassemble, which makes the process more robust. (Again, see the examples in libTERE. )

Known bugs:

1. There is some problem on windows that results in Text decorations acting like a CSS 2 implementation. This might be a library issue? The build cycles are so much longer on that platform that it may take a while to find the source of this.

Example file, showing how the decorations look.

Example text decorations file, screen dump (linux)

This patch replaces the preceding one.

1. It fixes the problem on windows (one use of strndup, had to be g_strndup).
2. It incorporates the fix for bug 1181326
3. It incorporates further changes to text editing so that style can be changed on spans consisting
of only spaces when text decorations are present in the span.
4. It incorporates code to disable text decorations when text so marked is mapped onto a path.

The way Inkscape is currently mapping text onto paths is by modifying the affine matrices associated with the glyphs. But
the glyphs are passed into the actual render code in small batches corresponding to tspans, and with those tspans in unpredictable orders. This makes it really, really hard for the render code to figure out where it should put the decorations when the tspans are not straight. The code posted earlier did nothing to handle this so the decorations ended up being drawn as normals to the curve starting from the first character in each tspan. Which looked worse than not having them at all. So for now, the render code detects "bent" text and draws no decorations for it. When the text is demapped from the curve it automagically shows decorations again. Ideally I guess text mapped onto a path would generate some sort of conformal map and use that to separately draw the text decorations.

(Will post an example next.)

text on path example, source svg

text on path example, screen shot

Slight tweak to preceding patch, which it replaces. Changing colors on text-decorations on a series of spaces in a nested fashion (red within blue within yellow, for instance) was not working. The text-editing.cpp tidy_operator_styled_whitespace method was not receiving enough information from start and end to figure out that the entire region was inside a text decoration. Modified calling routines to work upwards looking for this information, and then pass it downward.

WIndows binary for this patch is now up in the usual place.

[revision number corrected, prior comment hidden)]

Compiler warnings with r11739 (Apple GCC 4.2.1, Apple clang 3.1 (tags/Apple/clang-318.0.58)):

../../src/libnrtype/Layout-TNG-Output.cpp:541:46: warning: conversion specifies type 'int' but the argument has type 'size_type' (aka 'unsigned long') [-Wformat]
    snprintf(line, sizeof(line), "spans %d\n", _spans.size());
                                            ~^ ~~~~~~~~~~~~~
                                            %lu
../../src/libnrtype/Layout-TNG-Output.cpp:543:46: warning: conversion specifies type 'int' but the argument has type 'size_type' (aka 'unsigned long') [-Wformat]
    snprintf(line, sizeof(line), "chars %d\n", _characters.size());
                                            ~^ ~~~~~~~~~~~~~~~~~~
                                            %lu
../../src/libnrtype/Layout-TNG-Output.cpp:545:46: warning: conversion specifies type 'int' but the argument has type 'size_type' (aka 'unsigned long') [-Wformat]
    snprintf(line, sizeof(line), "glyphs %d\n", _glyphs.size());
                                            ~^ ~~~~~~~~~~~~~~
                                            %lu

As it turns out, neither %d nor %lu is correct. size_t happens to be "long" on that platform but need not be. If %lu fixed it then it is also unsigned. Somewhere around C99 the "z" length modifier was added, it always corresponds to size_t.
So the right fix is "%zu".

This small patch should eliminate some compiler warnings on OS X. It should not affect how the program runs at all.

Hi, the attached file leads on my Windows 7 machine with 8 GB ram to an Emergency Save with the version inkscape_r11738_lp988601.zip. It didn't lead to this problem with the latest stable version Inkscape 0.48.4 r9939 and it also works fine with the development version Inkscape 0.48+devel r12467. I have reduced the file content to a minimum for easier debugging.

Unfortunately that test file does not cause any problems on 32 bit XP, which is my development system. I could open it and resave it. So, in order for me to figure this out you are going to have to narrow it down a bit more on the problem system.

Let's start with "leads on my Windows 7 machine with 8 GB ram to an Emergency Save". Please list every step you took until
the emergency save.

1. start Inkscape (please note which tools were open on the right side, like "Fill and Stroke", "Align" and so forth)
2. open the svg
3. ???

That SVG file is unusual for the following reasons:

1. There are guides
2. There is a grid
3. The only objects drawn are outside the page.

When I saved it to EMF the guides and grid disappeared, of course, since EMF does not support those. Does turning off 1 and 2 resolve the issue on your machine? If it does the issue is probably not in the code I work on, it may just be that there was a transitory bug in one of those functions that happened to be present when I built this version. If changing the position from outside to inside the page makes the emergency save go away, then it probably is in my code.

Sorry David, looks to me that I send you into the wrong direction: Inkscape fails, when I try to open this file, not when I like to save it or convert to emf! I am not expecting that a grid or a guide will be part of an emf by the way. This file originally was a very large technical drawing which I had done some month ago, and I recognized this issue when I wanted to open it a few days ago. I see this as an indication, that this topic is not related to your code. Usually I had a lot of drawing elements outside of the page (which I also shift quite often), and your emf export up to now and also in the past handled everything in my files quite well - thanks at this point to you! Based on your feedback, I also copy your latest InkScape version r11738_lp988601.zip into a 32bit XP system, running on a virtual machine in Virtualbox, and there I could open the file without problems. I think its not worth spending more time into this topic based on the actual info. Anyway, same issue with all guides and the grid removed. The problem seems to be related to the black connector between the two rectangles. If I delete the unconnected rectangle, the connector is redrawn with a slightly different shape, and then I can load the drawing as usual....

If you want to see what part is toxic, edit the SVG and remove various parts of the the construct below until it works. Everything down through id= looks OK to me. That connector has after that a lot of "inkscape:" features, giving it plenty of space to go wrong. The transform in particular is odd: why translate by 10^-6, which is a tiny fraction of a pixel width?

    <path
       style="fill:none;stroke:#000000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
       d="m 92.1259880066,-329.527587807 53.1496009824,0"
       id="path5530"
       inkscape:connector-type="orthogonal"
       inkscape:connector-curvature="4"
       transform="translate(0,2.5390623e-6)"
       inkscape:connection-end-point="d4"
       inkscape:connection-end="#rect5521"
       inkscape:connection-start-point="d4"
       inkscape:connection-start="#rect5517" />

This patch eliminates the introduction of an extraneous moveto operation on import from EMF, for paths that include ArcTo record.s

Patch from previous comment committed in lp988601 revision 11741.
Branch lp988601in revision 11743 also updated with trunk revisions up to trunk r12481.
Please update your local copy.

Merging the changes from trunk was not an easy job. Several changes had to be pushed manually, especially those from trunk r12471 (which may also affect further wmf/emf coding). I'd love to see the changes and bugfixes in the lp988601 branch go into the main development branch.

David, what needs to be done for this? Is there anything that is broken in this branch? Is there some cleanup needed?

> Please update your local copy.

Autotools-based build is broken in r11743 (missing files from the merge) - see PM.

Re 189

So wait on the update until that is resolved, yes?

Re: 188

Specifically, what is it about r12471 that affects WMF/EMF? I see the changes in /src/extension/internal/emf*cpp,
are there others I need to know about?

re189: should hopefully compile again on non windows systems. Sorry for the mess. Bzr did not do the file renames/moves properly.

re190: trunk revision 12471 is about a google summer of code project in which the unit convertion is rewritten. There is now a new generalised function to change units, in instead of PX_PER_IN it will be Inkscape::Util::Quantity::convert(1, "in", "px")

Am I missing something here? What is the advantage in replacing a simple and clear define like:

  PX_PER_IN

with a verbose and very unclear (which direction is the conversion, what does the 1 do?) construct like

  Inkscape::Util::Quantity::convert(1, "in", "px")

Especially when that construct has no variable parameters??? I assume that somewhere or other those methods
must act as something other than a constant, which is fine, but no reason to obfuscate the code in dozens of other methods. Would it not have been sufficient to change unit-constants.h from

 #define PX_PER_IN (PT_PER_IN / PT_PER_PX)

to

 #define PX_PER_IN Inkscape::Util::Quantity::convert(1, "in", "px")

avoiding the need to change who knows how many other files, including the EMF ones.

Above description lists some fixes which are reverted from the original patch/branch long time ago. They do not reflect the current situation anymore (are fixed in trunk and from there backported in the lp988601 branch; fix for related bug 986271 committed in trunk r12482). Updating description accordingly.

Discovered some weird behaviour with this branch with a very old emf file of my former research group at the university. The file opens relatively decent in the main development branch (besides some font issue), but the content is distorted when loading in the lp988601 branch. At first I thought there was due to some mistake when synchronising the updates with trunk, but the emf is also loaded in the same weird way if the lp988601 branch is reverted to the revision just prior to the last merge.

Forgot to mention in previous comment: system is windows vista 64 bit lp988601 revision 11746

You are referring to the branching/tree part of the dendrogram? The rest of the diagram looked OK to me.

Those branches show up as various apparently random characters in lp988601, like "o" with various accents over it.
In XP Preview those same regions show up with various outline graphics arrows, but not oriented in a reasonable way, so it still looks odd.

The reason this is happening is that those "lines" are drawn by placing characters on the drawing surface. The font used by those characters is "SPSS Marker Set", which of course is not on my system. Since the font cannot be found different
failover fonts are used in XP Preview and Inkscape, because they are using different font resolution systems. So the diagrams turn out different from each other, and neither looks like it is correct.

If you can find "SPSS Marker Set" font and install it this diagram would probably look OK.

Bottom line, the EMF file was constructed in a nonportable manner.

If there is some other issue, please clarify.

(FYI: to diagnose this sort of EMF issue, obtain libUEMF, build it, then use the "reademf" application to dump the EMF contents in a more or less understandable format as text.)

Branch <https://code.launchpad.net/~inkscape.dev/inkscape/emf-support> was merged intro trunk in revision 12488:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12488>