> + gboolean val = *(gboolean *)optentry->arg_data;
> + gboolean reverse = (optentry->flags & G_OPTION_FLAG_REVERSE);
> + if ((val && !reverse) || (!val && reverse)) /* logical XOR */
> + s = g_strdup_printf("--%s", optentry->long_name);
> + }
> + break;
> +
> + case G_OPTION_ARG_INT:
> + if (*(gint *)optentry->arg_data)
> + s = g_strdup_printf("--%s=%d", optentry->long_name, *(gint *)optentry->arg_data);
> + break;
> +
> + case G_OPTION_ARG_STRING:
> + case G_OPTION_ARG_FILENAME:
> + if (*(gchar **)optentry->arg_data)
> + s = g_strdup_printf("--%s=%s", optentry->long_name, *(gchar **)optentry->arg_data);
`STRING` and `FILENAME` have different encoding, I'm not sure if a conversion is needed.
[GLib's docs](https://developer.gnome.org/glib/unstable/glib-Commandline-option-par… say:
> On UNIX systems, the argv that is passed to `main()` has no particular encoding, even to the extent that different parts of it may have different encodings. In general, normal arguments and flags will be in the current locale and filenames should be considered to be opaque byte strings. Proper use of [G_OPTION_ARG_FILENAME](https://developer.gnome.org/glib/unstable/glib-Comma… vs [G_OPTION_ARG_STRING](https://developer.gnome.org/glib/unstable/glib-Command… is therefore important.
---
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/637/files#r38643575
> Basic logic looks fine, and I personally like the table driven approach [...]
To be clear, I didn't mean to imply it was a bad approach or anything, just that it's relatively complex and having it combined in with the other changes makes it more difficult to review, fully understand (especially in the future), or event to back-out specific parts if bugs are found, etc.
> Disagree with @codebrainz on the need to split a PR of this size now its not made in steps [...]
Depends if the intermediate commits exist and were just squashed together or not. If they exist, it's trivial unsplit, otherwise it might arguably be too much hassle.
---
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/637#issuecomment-137301459
Basic logic looks fine, and I personally like the table driven approach, but as I have said on other versions of this change, I don't agree with forcing the same config so I won't be testing or committing it.
Disagree with @codebrainz on the need to split a PR of this size now its not made in steps. That is just a relative judgement thing, but agree with him that in general it can be useful if things are done stepwise in *complete, compilable, bisectable* commits.
Agree with @codebrainz that this PR makes some improvements for most users, so it could be used until some magic wand makes the right solution possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/637#issuecomment-137297627
> If you mean doc_path, it does indeed look like a full path (not necessarily a realpath, since it may be remote). So -- is almost surely unneeded.
I haven't looked super close at the code that assigns `doc_path` but I assume it uses `tm_get_realpath()` (ie. `realpath()`). I think Geany doesn't handle remote URIs or such, but defers to GVFS to handle mounting remote shares to local file systems.
---
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/637#issuecomment-137297104
> are there any notes on the logic of this attempt to fix the "New Window" function
I like the end result.
It would be easier to review (and look back to later) if it was broken down into the general stages of the change, like in a few different commits. I like the fact that it uses the `spawn_*()` API now, instead of `g_find_program_in_path()` + `utils_spawn_*()` (change 1), and agree with the changes from @b4n's PR that it incorporates getting the installed/resource binary path (change 2, without using his commit+attribution), but the stuff with the options lookup table, and new globals, functions, and data types (change 3..N) seems like a lot of complexity (though it may indeed be warranted), and the part that guards out the `is_osx_bundle()` seems unrelated (change N+1).
Because of the above I have not done a thorough code review and likely won't merge it myself, but I'm +1 for the end result in general.
> The ultimate Right Thing for that will be to open a new window in the same process, but that will take time, and we have to patch "New Window" somehow until then.
Agree, there's no point holding off on an intermediate solution until we endeavour to improve the underlying implementation enough to support the ideal solution.
---
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/637#issuecomment-137294359