Bug 22305

Poppler already has a way to set the data dir, that is creating the GlobalParams with the custom data dir, isn't that enough for you?

Also about using GlobalParamsWin.cc i think this should be a compile time option and not forced because i know some people bulding with mingw but using fontconfig.

Thanks, Albert, for your comment.
Now, as for the relocatable dll: On windows the runtime position and build-time position of the dll is normally different. So, it is a common place that all windows dlls are relocatable, it means that the prefix where they are installed does not matter. I saw the possibility to set another data directory. Nevertheless, this patch makes the right thing without the developer having the burden himself to determine where the data is installed. It is really not a very intrusive change and makes allows the programs using poppler to work out of the box on windows.
Concerning the GlobalParamsWin.cc, I tested on windows evince trunk with poppler 0.11.0 and the fontconfig was giving inferior results. There were bunches of text that were actually not readable. With the windows engine, the pdfs are readable. But I agree, one could do that a configure option, since if someone really needs (for whatever reason) use fontconfig, she should be able. With my changes the dlls produced by Visual Studio and by MinGW behave the same way. And if you don't build the splash output, the dll is not depending on fontconfig/freetype combo which can be useful for some.

Ok, let me rephrase my comment. Could you please do this?
 a) Make relocatable dll be a compile time option
 b) Make using GlobalParamsWin.cc a compile time option

If this happens i have no problem merging in the patch otherwise i'm a bit hesitant

Created attachment 32374 [details] [review]
0001-Make-fontconfig-optional-with-mingw-compiler.patch

Hi,

I have split Fridrich's patch in two parts and added compile time options.

This is the first part, making it possible to use the win32 font configuration system instead of fontconfig. 

When you use configure, you can select the font configuration system with --with-font-configuration=fontconfig|win32.

For the mingw compiler, it defaults to win32. For all other compilers, it defaults to fontconfig. I think that is the most sensible default.

Created attachment 32375 [details] [review]
0002-Make-poppler-optionally-relocatable-on-Windows.patch

This is the second part, it allows poppler to find it's data on Windows in a directory that is not known at compile time.

This only works on Windows. If you do no like it, you can disable this feature with --disable-relocatable. The default is --enable-relocatable on native Windows, which is the sane thing to do.

On other operation systems, it defaults to --disable-relocatable because the feature is not available there and because that is common practice on Unix.

Wondering if in first patch 
-#ifdef _MSC_VER
+#if defined(_MSC_VER) || WITH_FONTCONFIGURATION_WIN32
should just be
-#ifdef _MSC_VER
+#if WITH_FONTCONFIGURATION_WIN32
because you did 
-#ifndef _MSC_VER
+#if WITH_FONTCONFIGURATION_FONTCONFIG
and i'd prefer it to be kind of symmetric.

And
AM_CPPFLAGS = -D_WIN32_IE=0x0501
needs a comment for us non windows people to understand what it is

In the second i guess
#if ENABLE_RELOCATABLE && defined(_WIN32)
could just be
#if ENABLE_RELOCATABLE
?
Maybe even calling it ENABLE_WIN32_RELOCATABLE ?

Also i guess asking you to write the cmake counterparts is too much, right?

(In reply to comment #6)

Hi,

First, sorry for my late reply, I forget to subscribe to this bug when submitting the patches.

> Wondering if in first patch 
> -#ifdef _MSC_VER
> +#if defined(_MSC_VER) || WITH_FONTCONFIGURATION_WIN32
> should just be
> -#ifdef _MSC_VER
> +#if WITH_FONTCONFIGURATION_WIN32

That would certainly be the better thing to do, as that would specify the build based on the configuration parameter WITH_FONTCONFIGURATION_WIN32 only, not on the compiler used in the build. The reason I did not do that is that it would also require changes in the configuration settings for the Microsoft Visual C compiler (in makefile.vc), and in the CMake file. I have created a new patch, but I cannot test it on Windows myself. So I hope there is someone else who can test this patch on Windows with cmake and/or msvc.

> And
> AM_CPPFLAGS = -D_WIN32_IE=0x0501
> needs a comment for us non windows people to understand what it is

Hmm, indeed, that is not very clear. Thinking about it a bit more, it also should be in configure.ac because all parts of poppler should be compiled with this setting. I changed this in my new patch.

> In the second i guess
> #if ENABLE_RELOCATABLE && defined(_WIN32)
> could just be
> #if ENABLE_RELOCATABLE
> ?
> Maybe even calling it ENABLE_WIN32_RELOCATABLE ?

Okay, I renamed it to ENABLE_WIN32_RELOCATABLE.

> Also i guess asking you to write the cmake counterparts is too much, right?

Maybe, maybe not: In the new patches, I did my best to make it work with cmake, but I have only tested those on Linux. I cannot say if they work on Windows as well, but I think they do.

Created attachment 32559 [details] [review]
0001-Make-fontconfig-optional-with-mingw-compiler.patch

I have only tested this patch on:

  1. linux with configure
  2. linux with cmake
  3. linux cross compiling to windows with configure

I have not tested it on:

  1. windows with cmake
  2. windows with msvc's nmake -f makefile.vc
  3. windows with msys/configure

If some one can test it on one of these last three configuration, it would be appreciated.

Created attachment 32560 [details] [review]
0002-Make-poppler-optionally-relocatable-on-Windows.patch

Patches commited, i'll close the bug, it'll be a plus if you add a CMake patch for the second feature, but if not i'll leave for someone that wants it to code it