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