(In reply to comment #0)
>... so it does
> appear to me that some updates are needed for demos/gtk-demo/gtk-demo.py so
> that it would continue to run on Windows as it used to, as the script used to
> run on Windows before the changes in GLib/GIO.

What changes needed to gtk-demo.py? It seems like that would be considered an API break or a bug in pygobject internals or glib/gio...

Hi,

It seems that this issue was caused by a change in how GApplication works on Windows, as g_application_run() now calls g_win32_get_command_line() in GLib, which then calls the Windows API CommandLineToArgvW().

As shebang lines aren't supported in the standard Windows shell (not sure about MSYS, but most non-programmer users of programs will most probably not have it installed anyways...), Python scripts would be run with something like "python gtk-demo.py", which would be picked up by CommandLineToArgvW() to be like
argv[0]: python
argv[1]: gtk-demo.py

Whereas the former way that this is run would result in:
argv[0]: gtk-demo.py

As GApplication would not "recognize" python.exe, hence the error:
(python.exe:5092): GLib-GIO-CRITICAL **: This application can not open files.

would be thrown.

As this is the case, it seems that other bindings would be hit by this issue as well, as those scripts would be triggered by something like "perl abc.pl" or "ruby abc.rb" and so on.  Seems that we need some extra checks around for such scenarios or so, or find out whether those interpreters interact with the Windows console in Unicode and use the old method in such situations.

With blessings, thank you!

Created attachment 287910 [details] [review]
g_application_run(): Fix on Windows during the use of scripts using GNOME bindings.

Hi,

(Please scratch out the part of using envvars that I mentioned in bug 722025 comment 49-there seems to be a more comprehensive yet simpler fix that would affect less parties)

It seems that acquiring the arguments starting from the argument where the arguments returned by g_win32_get_command_line() matches argv[0], which is the invocation command of the program (or the script itself), would fix the problem in scenarios where we run scripts using GNOME bindings by running "python gtk-demo.py" or "python -v gtk-demo.py" or so.  The other added bonus is that we only acquire the arguments up to the point specified by argc, instead of the whole command line that is used to invoke the program/script that makes use of g_application_run(), which IMHO would be more in line with the way that g_application_run() is originally intended to work.

This is my proposed fix for this-I am unsure whether it is common for argv[0] to contain unicode characters that would fall out of one's codepage, which I couldn't test as cmd.exe basically rejects such commands as it could not find it) but in my gut guess I think that should be quite rare.

With blessings, thank you!

Created attachment 288026 [details] [review]
g_application_run(): Fix on Windows during the use of scripts using GNOME bindings. (take ii)

Hi,

The previous version of this patch did not account for two things:
-Free win32_arguments, causing a leak
-Handle the case where argc == 0 (such as when gtk3-demo-application)
 is run, which causes it to crash as we are always trying to acquire
 argv[0], which is a bad pointer in this case.  In this case, just
 acquire arguments using the method used before the application of
 this patch.

The version of the patch attempts to fix these as well.

With blessings, thank you!

Created attachment 288362 [details] [review]
g_application_run(): Fix on Windows during the use of scripts using GNOME bindings. (take iii)

Hi,

I made some more updates to this patch, as:

-Looking at what is done on non-Windows, the arguments weren't
 processed at all if argc is 0, so probably we should do likewise
 on Windows.
-Improved comments in the patch, and cleaned up the patch a little
 bit.

With blessings, thank you!

*** Bug 737332 has been marked as a duplicate of this bug. ***

Hi,

Any comments about this patch?  It does seem to me that this hits people using GApplication especially in bindings on Windows (which can make it unusable, so I think I should raise the serverity level of this bug).

With blessings, thank you!

Yes this should get attention, I maintain windows build http://sourceforge.net/projects/pygobjectwin32 and was avoided 2.40 and 2.42, revert the changes and finally use this patch at the moment. This also affect GJS which also use App class.

Hello,

Pinging here, as this would hit bindings running on Windows that use GApplication, and it seems to have stemmed from commit 673ee54 (win32: add g_win32_get_command_line()), making them unusable as a result.

Any further insight about this would be highly appreciated.

With blessings, thank you, and Happy Holidays!

Hi Ryan,

I think I need to add you to the CC list due to the issue here.  Do you mind to take a look at this when the time is ok for you?

With blessings, thank you!  Happy Holidays!

Review of attachment 288362 [details] [review]:

::: gio/gapplication.c
@@ +2236,3 @@
+         dealing with the arguments returned by g_win32_get_command_line()
+         after the one matching argv[0] */
+      /* We could be calling this via a script that uses GNOME

Coding style: please, use curly braces around the body of the for() loop…

@@ +2245,1 @@
+	  g_return_val_if_fail(win32_arguments[i] != NULL, 1);

… because then it clear that this g_return_val_if_fail() goes outside the for() loop, even if the indentation is wrong. Though, please: fix the indentation as well. :-)

I would not use g_return_val_if_fail(), as that can be compiled out, and lead to crashes later on.

Use an explicit condition and a g_critical() message, or use a g_assert() if this should never happen.

@@ +2252,3 @@
+    }
+  else
+      arguments[j] = NULL;

Coding style: please, add braces around the body of the else block.

Created attachment 317696 [details] [review]
Proposed fix

Strange coincidence, I too was about to update this bug, I guess adding myself to the cc sparked some emails :-) .. anyway.

Hi all,

We also ran into this problem on MSYS2 when g_application_run is called from Python2 (pygtk). We had it for ages also with our meld package (meld a.txt b.txt would bring up a 3-way compare with meld.py a.txt b.txt as the 3 panes) but I didn't connect the dots until today while investigating http://sourceforge.net/p/msys2/discussion/general/thread/e905716d/

Hi Chun-Wei,

I must admit that I don't quite agree with your approach in this patch, specifically, you have the comment:

"What if argv[0] is in Unicode outside of the system codepage, though quite unlikely?" well, why is it not possible that "gtk-demo.py" could not contain any characters, it's just a filename after all.

Isn't this the reason that g_win32_get_command_line is being called in the first place? I took a different approach which is to return the same number of trailing arguments in g_win32_get_command_line as the argc passed to g_application_run which doesn't rely on the assumed-sensible string comparison.

Best regards, and (a nearly a whole year later) happy holidays (only lower-case though) also!

This gets more complicated.

On MSYS2 my patch fixed: http://sourceforge.net/p/msys2/discussion/general/thread/e905716d/ 
But the meld issue isn't fixed it has just changed shape a bit. Now, "meld --help" doesn't work, but "meld aaaaaaa --help" does .. very strange.

Of course it could be that the meld developers implemented a workaround for this bug that I'm now in conflict with! (and I didn't check if the latest meld was working before I made this change either ..)

Created attachment 317705 [details] [review]
Same as last one, presumes the n dropped leading args are still to be dropped

Of course it could be I made a mistake in my patch.
Finally, I have a working meld on MSYS2 :-)
Now to see if I can wean myself off of Beyond Compare 4.
I also tested it with the gtk demo and it works there too.

Please review.

Review of attachment 317705 [details] [review]:

Hi Ray,

I'm glad that someone is looking at this bug as well, and it has been a while:)  I obsoleted my own patch in favor of yours as yours surely look better-just a few things though...

Also, perhaps use 'git format-patch' for generating your patch, would be better.

Thanks though, with blessings, and Happy Holidays.

::: glib-2.46.2/gio/gapplication.c.orig
@@ +2260,3 @@
+    gint new_argc = 0;
+    arguments = g_win32_get_command_line ();
+    /**

The /** ... **/ is used for a special purpose (gtk-doc, for code documentation), so I don't think we want to use them here.  Also, have havok should read havoc. :)

Created attachment 317722 [details] [review]
Win32: Fix g_application_run() when using bindings

Hi,

This is Ray's patch with the updates in his comment block that I mentioned a few moments ago, in 'git format-patch' format.

With blessings, thank you!

Created attachment 317723 [details] [review]
Win32: Fix g_application_run() when using bindings

Hi,

I re-uploaded the patch as there were some leftovers on my part in there...sorry!

Review of attachment 317723 [details] [review]:

Hey,

please have a look at the comments.

::: gio/gapplication.c
@@ +2279,3 @@
 #ifdef G_OS_WIN32
+  {
+    gint new_argc = 0;

empty line after the variable declarations

@@ +2283,3 @@
+
+    /*
+     * If may be the case that something (e.g. Python) dropped arguments

this comment is really not clear to me. Can you rewrite it and maybe put some example of the problem?

@@ +2289,3 @@
+     */
+    while (arguments[new_argc])
+      {

no need for braces since it is a single line

@@ +2291,3 @@
+      {
+        new_argc++;
+      }

empty line after before the if

@@ +2296,3 @@
+        memmove (&arguments[0],
+                 &arguments[new_argc - argc],
+                 sizeof(arguments[0]) * (argc + 1));

space before ( on the sizeof

Created attachment 317727 [details] [review]
Win32: Fix g_application_run() when using bindings (take ii)

Hi Nacho,

This is one bug that does bother a number of people... :)

(In reply to Ignacio Casal Quinteiro (nacho) from comment #18)
>  #ifdef G_OS_WIN32
> +  {
> +    gint new_argc = 0;
> 
> empty line after the variable declarations

Got it.

> 
> @@ +2283,3 @@
> +
> +    /*
> +     * If may be the case that something (e.g. Python) dropped arguments
> 
> this comment is really not clear to me. Can you rewrite it and maybe put
> some example of the problem?

Are the comments better now?
> 
> @@ +2289,3 @@
> +     */
> +    while (arguments[new_argc])
> +      {
> 
> no need for braces since it is a single line
Got it

> @@ +2291,3 @@
> +      {
> +        new_argc++;
> +      }
> 
> empty line after before the if

Got it.
> 
> @@ +2296,3 @@
> +        memmove (&arguments[0],
> +                 &arguments[new_argc - argc],
> +                 sizeof(arguments[0]) * (argc + 1));
> 
> space before ( on the sizeof

Got it.

With blessings, thank you!

Review of attachment 317727 [details] [review]:

::: gio/gapplication.c
@@ +2293,3 @@
+     */
+    while (arguments[new_argc])
+     * pulls in the whole command line that is used to call the program.  This is

this while loop can be replaced by `g_strv_length()`, which is actually clearer.

@@ +2297,3 @@
+    if (new_argc > argc)
+      {
+     * program is a called via a script, such as PyGObject's gtk-demo.py, which is normally

I'm not entirely sure about this memmove(); you're leaking the first `new_argc - argc` elements of the `arguments` array, which are allocated by `g_win32_get_command_line()`.

You need to free the elements from `arguments[0]` to `arguments[new_argc - argc]` and then `memmove()` the rest of the elements down by `new_argc - argc` slots.

Created attachment 317768 [details] [review]
Win32: Fix g_application_run() when using bindings (take ii)

Hi,

This is the new patch with the fixes Emmanuele suggested.

With blessings, thank you!

Review of attachment 317768 [details] [review]:

Minor nitpick from my side :)

::: gio/gapplication.c
@@ +2298,3 @@
+        gint i;
+
+        for (i = 0; i < new_argc - argc; i ++)

no space before ++

Created attachment 317775 [details] [review]
Win32: Fix g_application_run() when using bindings

here it goes...:)

Review of attachment 317775 [details] [review]:

Looks good to me, now.

Great, thanks alot guys. Sorry about the memory leak :-)

Hi Emmanuele/Nacho,

Thanks for the pre-Christmas reviews and for getting this done with, as a Christmas present for Windows users:).

---
Hi Ray,

Thanks for getting this Christmas present made more ready!
---

Patch was pushed as:
master: bec6a9a
glib-2-46: 017dd9b

With blessings, thank you, and Happy Holidays!