Created attachment 281747 [details]
Without manifest and ICC

Created attachment 281748 [details]
With manifest and ICC

Manifest has to be either bundled with the executable or baked into it as a resource - only the buildsystem of the app in question can do that (that said, it might be possible to embed a manifest into DLL; i haven't tried that yet).

It may be OK to call InitCommonControlsEx() lazily, before PrintDlgEx() or similar functions.

Question is: should GTK+ be responsible for any of that? Or should it be left to the application developers? Should it be documented at least?

mmm, well gtk is the one doing the calls to the printing stuff, so I guess I'd expect it to add the manifest itself, that said if it is too much work, we could also document it for the apps to do it...

I would prefer if we could do it

Created attachment 281917 [details]
With manifest and ICC in libgtk3

Halfway there

I've tried embedding the manifest into libgtk-3-0.dll. Embedding worked, but produced no visible changes.

Then i've tried adding a manifest file alongside gtk3-demo.exe. No luck here either.

After some googling i've found a different resource type to use: 2 (ISOLATIONAWARE_MANIFEST_RESOURCE_ID) instead of 1 (CREATEPROCESS_MANIFEST_RESOURCE_ID). That did produce a result - the outer frame of the dialog got styled! Sadly, the rest of it remained mostly unstyled...

I turned back to a small testcase i wrote, and noticed that in that testcase everything works. After some pondering i have come to realize that the problem is in the application. Unless the .exe file requests common controls 6.x, all dependent libraries load system-default (5.x) comctl32 library. Libgtk3 overrides that for itself, and loads 6.x, but the dll that creates the dialog (comdlg32) does not.

Some more googling revealed that there's the Activation Context API that can be used to subvert SxS in various ways. And now i think i have a fix.

Created attachment 281919 [details] [review]
Make sure native W32 print dialog uses visual styles

For that to happen the libgtk3 is embedded with a manifest that requests
common controls library 6.x, and GTK lazily calls InitCommonControlsEx()
to initialize those. Then this manifest is used to temporarily override
the process activation contest when loading comdlg32 (which contains the
code for the print dialog), ensuring that it too depends on common
controls 6.x, even if the application that uses GTK does not.

Created attachment 281922 [details] [review]
Make sure native W32 print dialog uses visual styles

For that to happen the libgtk3 is embedded with a manifest that requests
common controls library 6.x, and GTK lazily calls InitCommonControlsEx()
to initialize those. Then this manifest is used to temporarily override
the process activation contest when loading comdlg32 (which contains the
code for the print dialog), ensuring that it too depends on common
controls 6.x, even if the application that uses GTK does not.

v2: Forgot to add the manifest file, here it is.

Review of attachment 281919 [details] [review]:

See the comments.

::: gtk/Makefile.am
@@ +1709,3 @@
 	$(win32_theme_sources)	\
 	$(gsettings_SCHEMAS)	\
+	libgtk3.manifest	\

this is generated so why add it to extra_dist?

::: gtk/gtkprintoperation-win32.c
@@ +1672,3 @@
+      icc.dwICC  = ICC_WIN95_CLASSES;
+
+      b = InitCommonControlsEx (&icc);

b <- what is that? call it inited?

@@ +1674,3 @@
+      b = InitCommonControlsEx (&icc);
+      if (!b)
+        g_warning ("Failed to InitCommonControlsEx: %lu\n", GetLastError ());

Failed to call InitCommonControlsEx...?

::: gtk/gtkwin32.c
@@ +53,3 @@
+static BOOL CALLBACK
+find_first_manifest (HMODULE hModule,
+                     LPCSTR lpszType,

align the vars and use the proper glib convention for the names

@@ +88,3 @@
+ */
+void
+_gtk_load_dll_with_libgtk3_manifest (const char *dllname)

const gchar *?

@@ +92,3 @@
+  HANDLE hactx;
+  ACTCTXA actx;
+  ULONG_PTR ulpActivationCookie;

name convention

@@ +101,3 @@
+    ACTCTX_FLAG_HMODULE_VALID |
+    ACTCTX_FLAG_SET_PROCESS_DEFAULT |
+  0;

why is this 0 aligned here?

@@ +107,3 @@
+
+  EnumResourceNames (gtk_dll, RT_MANIFEST,
+      find_first_manifest, (LONG_PTR) &resname);

align

@@ +121,3 @@
+
+  if (hactx == INVALID_HANDLE_VALUE)
+    g_warning ("Failed to CreateActCtx: %lu\n", GetLastError ());

Failed to call... or Failed calling

@@ +124,3 @@
+  else
+    {
+      ulpActivationCookie = 0;

name convention

@@ +138,3 @@
+  
+  if (resname != NULL && !IS_INTRESOURCE (resname))
+    free (resname);

you can pass NULL to free

Review of attachment 281922 [details] [review]:

See other comment

::: gtk/libgtk3.manifest.in
@@ +5,3 @@
+     name="libgtk3"
+     type="win32"/>
+  <dependency>

please fix the indentation of this file, use 2 spaces to indent?

(In reply to comment #10)
> Review of attachment 281919 [details] [review]:
> 
> See the comments.
> 
> ::: gtk/Makefile.am
> @@ +1709,3 @@
>      $(win32_theme_sources)    \
>      $(gsettings_SCHEMAS)    \
> +    libgtk3.manifest    \
> 
> this is generated so why add it to extra_dist?

At first it thought about leaving it out. But then i foudn this:
	gtk-win32.rc		\
	gtk-win32.rc.in		\
And changed my mind.


> 
> @@ +1674,3 @@
> +      b = InitCommonControlsEx (&icc);
> +      if (!b)
> +        g_warning ("Failed to InitCommonControlsEx: %lu\n", GetLastError ());
> 
> Failed to call InitCommonControlsEx...?

Since "Init" is a verb, this seems appropriate to me.
I actually expected to hear that i shouldn't use g_warning() here at all (and use maybe WIN32_API_FAILED() or somesuch).

> 
> @@ +101,3 @@
> +    ACTCTX_FLAG_HMODULE_VALID |
> +    ACTCTX_FLAG_SET_PROCESS_DEFAULT |
> +  0;
> 
> why is this 0 aligned here?

Imagine that "foo =" is "{" and "0;" is "}", then this alignment makes sense.

> 
> @@ +121,3 @@
> +
> +  if (hactx == INVALID_HANDLE_VALUE)
> +    g_warning ("Failed to CreateActCtx: %lu\n", GetLastError ());
> 
> Failed to call... or Failed calling

See above

> 
> @@ +138,3 @@
> +  
> +  if (resname != NULL && !IS_INTRESOURCE (resname))
> +    free (resname);
> 
> you can pass NULL to free

to g_free() - yes. To free() - no.
That said, maybe i should use g_strdup() and do use g_free() here.

Created attachment 282190 [details] [review]
Make sure native W32 print dialog uses visual styles

For that to happen the libgtk3 is embedded with a manifest that requests
common controls library 6.x, and GTK lazily calls InitCommonControlsEx()
to initialize those. Then this manifest is used to temporarily override
the process activation contest when loading comdlg32 (which contains the
code for the print dialog), ensuring that it too depends on common
controls 6.x, even if the application that uses GTK does not.

v3:
* renamed variables to have more gtkish names
* aligned function signatures the way gtk code is aligned
* changed _gtk_load_dll_with_libgtk3_manifest argument to const gchar *
* aligned "0;" with the rest of the flags
* aligned EnumResourceNames() arguments as per gtk style
* changed resname check before returning, changed malloc/free to g_strdup/g_free

Review of attachment 282190 [details] [review]:

See the comments.

::: gtk/Makefile.am
@@ +1709,3 @@
 	$(win32_theme_sources)	\
 	$(gsettings_SCHEMAS)	\
+	libgtk3.manifest	\

I still think this is not needed as well as gtk-win32.rc should not be here since it is just generated at configure...

::: gtk/gtkprintoperation-win32.c
@@ +43,3 @@
 
+/* from gtkwin32.c */
+static void _gtk_load_dll_with_gtk3_manifest (const char *dllname);

this should be added to gtkprivate.h and be included there, also it should not be static

::: gtk/gtkwin32.c
@@ +69,3 @@
+  return TRUE;
+}
+

no need for 2 newlines here

@@ +97,3 @@
+  memset (&activation_ctx_descriptor, 0, sizeof (activation_ctx_descriptor));
+  activation_ctx_descriptor.cbSize = sizeof (activation_ctx_descriptor);
+  activation_ctx_descriptor.dwFlags =

this should be probably set on the same line, and then indented to that point

@@ +116,3 @@
+    activation_ctx_descriptor.lpResourceName = resource_name;
+  else
+    activation_ctx_descriptor.lpResourceName = MAKEINTRESOURCEA (2);

even if you added a comment that this is an index I do not like magic numbers around, add define?

@@ +138,3 @@
+
+  if (!IS_INTRESOURCE (resource_name))
+    g_free (resource_name);

I am not completely sure about this... you are not allocating resource_name with the glib methods so probably we should use free here instead. Also I still do not understand why this should be leaked in the case of IS_INTRESOURCE, can you explain that? is this the handle that you talk on the initial comment?

(In reply to comment #14)
> Review of attachment 282190 [details] [review]:
> 
> See the comments.
> 
> ::: gtk/Makefile.am
> @@ +1709,3 @@
>      $(win32_theme_sources)    \
>      $(gsettings_SCHEMAS)    \
> +    libgtk3.manifest    \
> 
> I still think this is not needed as well as gtk-win32.rc should not be here
> since it is just generated at configure...

My guess is that this is for non-autoconf-based buildsystems (i.e. MSVS). They don't run configure, and thus can't generate these files. That said, there are lots of things that are generated, so i'm not sure how these buildsystems cope with all that stuff...I can just remove libgtk3.manifest and gtk-win32.rc from extra_dist, if you insist.

> @@ +97,3 @@
> +  memset (&activation_ctx_descriptor, 0, sizeof (activation_ctx_descriptor));
> +  activation_ctx_descriptor.cbSize = sizeof (activation_ctx_descriptor);
> +  activation_ctx_descriptor.dwFlags =
> 
> this should be probably set on the same line, and then indented to that point

Just show me how you think it should look...The explanation above is rather vague.

> 
> @@ +138,3 @@
> +
> +  if (!IS_INTRESOURCE (resource_name))
> +    g_free (resource_name);
> 
> I am not completely sure about this... you are not allocating resource_name
> with the glib methods so probably we should use free here instead. Also I still
> do not understand why this should be leaked in the case of IS_INTRESOURCE, can
> you explain that? is this the handle that you talk on the initial comment?

Easier to show than to explain. This is the definition of IS_INTRESOURCE from winuser.h:
#define IS_INTRESOURCE(_r) ((((ULONG_PTR)(_r)) >> 16)==0)
That is, if the pointer has zeroes in all bits above lower 16 bits, it's not a pointer, it's an integer id.
Which is why intresources are just assigned, never duped or freed.

Created attachment 282283 [details] [review]
Make sure native W32 print dialog uses visual styles

For that to happen the libgtk3 is embedded with a manifest that requests
common controls library 6.x, and GTK lazily calls InitCommonControlsEx()
to initialize those. Then this manifest is used to temporarily override
the process activation contest when loading comdlg32 (which contains the
code for the print dialog), ensuring that it too depends on common
controls 6.x, even if the application that uses GTK does not.

v4:
* libgtk3.manifest is not in EXTRA_DIST anymore
* no extra newlines in gtkwin32.c
* aligned flags as requested
* shuffled the code a bit
* replaced 2 with a named constant
* added function prototype to gtkprivate.h (under ifdef)

Created attachment 282284 [details] [review]
Remove generated gtk-win32.rc from EXTRA_DIST

Review of attachment 282284 [details] [review]:

I'd say start to push this one. We'll soon see if Fan complains :)

Can't do that. attachment 282284 [details] [review] depends on pushing attachment 282283 [details] [review] first (because there's the libgtk3.manifest in the diff there).

I can, obviously, rebase things and reverse the order of applying the patches, if that's what you want, then push the gtk-win32.rc-removing one first and see what happens.

yes, that's what I meant if it is not too much work

Comment on attachment 282284 [details] [review]
Remove generated gtk-win32.rc from EXTRA_DIST

Attachment 282284 [details] pushed as 3fea9ff - Remove generated gtk-win32.rc from EXTRA_DIST

Hi,

(In reply to comment #18)
> Review of attachment 282284 [details] [review]:
> 
> I'd say start to push this one. We'll soon see if Fan complains :)

I would complain about this...:)  If that is the best way for the code to go, though I might prefer gtk-win32.rc to remain in the dist (especially as this is being done with the other parts of the GTK+ stack), I might do it in two ways:

-Place a copy of the gtk-win32.rc (that is generated) into build/win32 (or so) during make dist, so it wouldn't interfere with the autotools build process.

-Use a Python script to deduce the versions required for the .rc file from configure.ac.

For the manifest file, I could use simple Python script or so to fill in the architecture of the manifest, so I would be great going for having libgtk3.manifest.in, as I need to fill in the architecture value while doing the build (the MSVS files support both Win32/x86 and x64 builds, BTW).

Thanks for letting me know and have my way on this :).

With blessings, thank you!

Review of attachment 282283 [details] [review]:

Hi,

For my other take on gtk/libgtk3.manifest.in, by the way, I just noticed, we could use the asterisk (*) for the architecture instead of generating it, or is filling it in with autotools preferred?

Fan, Chun-wei, it's not clear to me what your position exactly is. Are you OK with pushing this patch as-is (will you be able to just add MSVS-specific stuff somewhere, without changing the code that my patch introduces)? If not, which changes do you want? I have no idea how MSVS-based buildsystem generates stuff, so i can't judge this myself.

As for asterisk in architecture, i am aware of it, but:
1) GTK already generates appropriate value when preprocessing the manifest, so i didn't think it necessary to change that.
2) While "*" is known, i can't find it in MSDN, which makes it kind of dubious.
3) Since i can't find comprehensive docs about it, i'm not sure what processorArchitecure does, what it influences. What's the difference between putting "X86" and "*" into that attribute, for x86 binary? What will Windows do? Until that is known, i'll feel safer with "X86" and "amd64".

Hello LRN,

(In reply to comment #24)
> ... I have no idea how MSVS-based buildsystem generates stuff,
> so i can't judge this myself.

The MSVS build files don't readily update the version stuff from the autotools files directly, so the MSVS files don't generate the gtk-win32.rc with the up-to-date version info, so it relies on such things being dist'ed in its current state.

What I was saying, assuming that we are going with your patch for not distributing gtk-win32.rc, I could try to come with a python script that tries to read configure.ac, and run that script to create gtk-win32.rc with the correct version info, but I am not sure whether this approach would be good for all parties.  Another thing that can be done is to copy the generated gtk-win32.rc in $(srcroot)/build/win32 (or so) and dist it from there during make dist, if interfering with the autotools build is causing concerns.  Please let me know which approach is preferred.

> As for asterisk in architecture, i am aware of it, but...

It's on http://msdn.microsoft.com/en-us/library/windows/desktop/bb773175%28v=vs.85%29.aspx that you linked to in an earlier comment, but if filling it in at build time is preferred, I'm fine with that.

With blessings, thank you!

Hi,

Since the general impression that I'm getting is that generating the gtk-win32.rc file during build time is preferred (and I have yet to hear otherwise here), I've pushed a script in build/win32 that is used for this, which would be called by the Visual Studio builds[1].  Since we are already using Python to generate the DBus sources (gtkdbusgenerated.[c|h]), so I think it follows that we use Python here as well.

I will activate the manifest part when it gets committed to master (it would be great if I hear about this here in this bug, BTW), if we are going to generate it during build-time.

With blessings. :)

[1]: https://git.gnome.org/browse/gtk+/commit/?id=d62bd12b861742ba5aa6549aebb33d541b0cad1e

With the agreement of Fan, let's push the remaining stuff here.

Attachment 282283 [details] pushed as 0d02cc8 - Make sure native W32 print dialog uses visual styles

Hi,

Just to make sure, is the image in attachment 281917 [details] the one you should see after applying the patch?

Oh, BTW, the update on gtkprintoperation-win32.c missed an include, gtkprivate.h, which I pushed a fix to master for that.

I do get the following warning when using the print demo from gtk3-demo:
(gtk3-demo.exe:3664): Gtk-WARNING **: Failed to CreateActCtx for module 00007FFCAC1C0000, resource 0000000000000002: 14011

Will check whether this is a problem on my side for the projects though...

With blessings.

Created attachment 282637 [details] [review]
gtkprintoperation-win32.c: Create Activation Context for libgtk3.manifest only when needed

Hi,

> ...Gtk-WARNING **: Failed to CreateActCtx for module
> 00007FFCAC1C0000, resource 0000000000000002: 14011

It seems that this problem is caused as we repeatedly tried to create the activation context, for Visual Studio 2005+ builds.  The runtime, being aware of the manifests, will automatically create the activation contexts from the manifest files that are embedded into the DLL, so doing that again manually will cause this, as this error code means ERROR_SXS_PROCESS_DEFAULT_ALREADY_SET, meaning that the activation context was already created.

My attempt to fix this is to only call _gtk_load_dll_with_libgtk3_manifest() only when necessary, when the runtime is not natively aware of embedded manifests.

With blessings, thank you!

CreteActCtx fails for me in gtk3-demo as well, but with a different error code. I'm rebuilding gtk at the moment, will try to debug it soon.

Review of attachment 282637 [details] [review]:

Looks good. but add a comment about this.

Created attachment 282638 [details] [review]
gtkprintoperation-win32.c: Create Activation Context for libgtk3.manifest only when needed (comments added)

Hello Nacho,

I added the comments.  Thanks for the reviews.

With blessings, thank you!

Fan, I think the patch is correct but let's wait for LRN to see what he says about the mingw part.

Created attachment 282641 [details] [review]
Add missing include to gtk-win32.rc

Otherwise ISOLATIONAWARE_MANIFEST_RESOURCE_ID and RT_MANIFEST constants
can't be expanded.

Created attachment 282642 [details] [review]
Don't try to override process default activation context

Attachment 282641 [details] fixes *my* problem (resource enumeration fails).

Attachment 282642 [details] may be able to fix Fan's problem, please test.
I could try to test it myself, but i can never perfectly reproduce your situation.

Review of attachment 282641 [details] [review]:

Looks good.

Hello LRN

> Attachment 282642 [details] may be able to fix Fan's problem, please test.
> I could try to test it myself, but i can never perfectly reproduce your
> situation.

Unfortunately it doesn't... probably it's best to leave out the _gtk_load_dll_with_libgtk3_manifest() call for MSVC 2005+.

Thanks though.

With blessings!

(In reply to comment #39)
> Hello LRN
> 
> > Attachment 282642 [details] [details] may be able to fix Fan's problem, please test.
> > I could try to test it myself, but i can never perfectly reproduce your
> > situation.
> 
> Unfortunately it doesn't... probably it's best to leave out the
> _gtk_load_dll_with_libgtk3_manifest() call for MSVC 2005+.
> 
> Thanks though.
> 
Have you debugged it? In your MSVC-compiled binaries it should get current context, context should be non-0, then it should take the short path, release the context it took, load library and return immediately.

If it does, and that doesn't work, try commenting out the LoadLibraryA() call and see if that fixes things.

If current context is 0 for you, then i don't know what to do.

Hello LRN,

The context acquired by GetCurrentActCtx() is 0, but then, one can safely call ReleaseActCtx() with NULL passed into it without problems (essentially a no-op)[1], also whether or not the LoadLibraryA() call was issued, the results are the same for me.

Probably we can determine whether we want to go with your patch (and not worrying about the context acquired by GetCurrentActCtx(), or just skip the _gtk_load_dll_with_libgtk3_manifest() call on MSVC 2005+

With blessings, thank you!

[1]: http://msdn.microsoft.com/en-us/library/aa375713%28v=vs.85%29.aspx

Created attachment 282650 [details] [review]
Handle ERROR_SXS_PROCESS_DEFAULT_ALREADY_SET

Try this patch instead of attachment 282642 [details] [review]

Also, does your original patch (which ifdefs the call to the whole function)
fix the theming of the print dialog for you? Was it broken in the first place?
The attachment 281917 [details] shows half-broken theming (outer frame is themed, inner
frame isn't; spinbutton is also themed for some weird reason). Correctly themed
dialog should have everything themed.

Reopening since this is clearly not fixed.

Hello LRN,

I will try your patch tomorrow, but to let you know, what I had was the image that was similar to attachment 281917 [details], or I need to check on how I had the manifest embedded (apparently doing it through gtk-win32.rc didn't work for me, I might have missed something in the process).

Thanks though, with blessings.

Make sure you apply attachment 282641 [details] [review]

Comment on attachment 282641 [details] [review]
Add missing include to gtk-win32.rc

Attachment 282641 [details] pushed as ae14928 - Add missing include to gtk-win32.rc

Hello LRN,

Your latest patch (282650) works-what happened is that I would need to add a define in my project file property sheets, which I will commit to master in a bit, as I am heading out.  To maintain compatibility with later MSVC versions, I will need to remove the last line that you added in gtk-win32.rc when I process gtk-win32.rc.in (which I will keep untouched), to let you know.  I will post here when I get the MSVC-specific patches up.

With blessings, thank you!

Do i apply attachment 282650 [details] [review], or will the fixes on your side eliminate the problem? The code samples i've googled up indicate that ERROR_SXS_PROCESS_DEFAULT_ALREADY_SET is a good error (i.e. it indicates a normal, if a bit unusual, course of events and shouldn't cause a warning). If that is so, attachment 282650 [details] [review] is just a correct handling of an error and should be pushed regardless of what you're going to do to remedy the situation on your side. OTOH, if i push it, it'll sweep ERROR_SXS_PROCESS_DEFAULT_ALREADY_SET under the rug, and if that error indicates that something is borked in MSVC projects, silencing it will prevent people in the future from getting a warning when things break up (which is bound to happen eventually).

Hello LRN,

I have pushed the Visual C++-related fixes for this bug as 37321f6f, for records.

By all means, please push attachment 282650 [details] [review]... I think it's good on my side too, so we can probably close this bug for good after attachment 282650 [details] [review] is committed and pushed.

With blessings, thank you!

Attachment 282650 [details] pushed as 3b916e4 - Handle ERROR_SXS_PROCESS_DEFAULT_ALREADY_SET