Use gtk_show_uri_on_window() in utils_open_browser() by default

[kugel-]

Instead of requiring the user to specify a browser just try to
use the system default browser through gtk_show_uri_on_window().
This is done when tool_prefs.browser_cmd is empty which is
also the new default.

This aligns with windows where we do this already. Though we're
bypassing tool_prefs.browser_cmd there entirely, while on non-Windows
we still honor tool_prefs.browser_cmd when it's set.

This is primarily useful for flatpak support where we cannot just
execute arbitrary commands on the host (unless using flatpak-spawn).

Also, firefox is arguably a bad default these days, given the
declined marked share.

[elextr]

goto ???!!!!

[kugel-]

Yes, I've used goto. What's your point again?

[elextr]

Why? you jump to a one liner followed by return, whats the point of that? It saves nothing material in code and makes it harder to follow.

[kugel-]

No, it's not that simple. The return is only if gtk_show_uri_on_window() fails. The idea is that if the user clears the current tool_prefs.browser_cmd in the dialog and commits that and then gtk_show_uri_on_window() fails for whatever reason then the dialog pops up again and allows the user to select a different command.

Please suggest a comparably concise way to accomplish that, otherwise we have no rule that forbids goto.

[elextr]

I'm not a "never ever use goto nowhere nohow" person, I am just bemused that you did it that way which seems convoluted to me.

Why not just copy lines 93 94 in place of line 109, then its clearly inside the loop instead of jumping out the loop and re-entering it?

[Edit: line numbers refer to the changed version of the file]

[kugel-]

I think I found another solution, also without goto. Please check agian.

[elextr]

Ok fine.

My only suggestion is that the last dialog line should now say "Please correct it or enter another one, or remove it to use your system default.".

[kugel-]

I'm hesitant because it invalidates all the existing translations. Do you still think it's a good idea?

[elextr]

The next release is going to have many invalidated translations anyway, I wouldn't have thought one more would hurt.

[kugel-]

Alright, hope it's fine now.

[elextr]

LGTM

[elextr]

@kugel- Just a question thats only tangentially related to this PR, but you mentioned it in the initial commit message, if a flatpack can't execute random commands, how does an IDE that is compiling and running all sorts of weird code work as a flatpack?

[kugel-]

Well, actually the sandbox provides a wrapper command to run commands on the host and I'm going to use that for Geany, so not much changes to non-flatpack builds (I declare the permission to do that in the flatpak manifest). That would work for the browser as well but this patch is a good idea anyway (should clarify the commit message) since the current default doesn't work for lots of people.

But I also looked at what Gnome Builder does, and that doesn't use the escape command. Instead it doesn't use the "Gnome Runtime" but the "Gnome SDK" as runtime. The SDK includes make, gcc and all that stuff. But Gnome Builder is still constrained to what's in the sandbox.

[kugel-]

And the truth is that I found the escape command only after making this patch, hence the commit message.

[elextr]

I agree that this is a good idea anyway, have continued the flatpack discussion at #3199

[eht16]

According to the docs, gtk_show_uri_on_window is available only since GTK 3.22.

And it doesn't work on Windows (who is actually surprised?). It returns FALSE, emits a message in the log and set the GError.
Log message:

18:42:40.160156: GLib CRITICAL	: file ../glib-2.70.4/glib/gspawn-win32.c: line 552 (might_be_console_process): should not be reached

GError message:
Hilfsprogramm (Invalid argument) konnte nicht ausgeführt werden
(translation: the helper program could not be executed)

No idea what this means and why the helper program could not be executed :(.

Btw, this is loosely related to #2444.

[elextr]

Hmmm, well our Glade file targets GTK 3.20 and I think there are other things needing later GTKs even though both Autotools and Meson just check for 3.0 and the Mingw CI passes this PR using GTK 3.8 (AFAICT).

So I'm confused what GTK version we really need.

[eht16]

I've edited my previous post and added a link to the docs which mention 3.22 as requirement.

The Mingw CI with GTK 3.8 passes only by accident in this regard because this PR's changes are #ifdefed for Linux :).

About the rest of the version confusion, I'm lost as well. I don't know how serious is the Glade file requirement is.
It seems even Ubuntu 18.04 had already GTK 3.22, so it's probably not a big issue and we could maybe make Meson and Autotools require GTK 3.22 as well.

[kugel-]

So how to proceed with the version requirement? I think it would best to codify the 3.22 requirement in configure.ac but that would break the mingw ci.

I wonder if it would make sense to create a new bundle based on the msys2 packages and use that for CI. What do you think?

[eht16]

So how to proceed with the version requirement? I think it would best to codify the 3.22 requirement in configure.ac but that would break the mingw ci.

Apart from the version constraint chaos, before merging this we need to fix the Windows problem. As mentioned ealier in this PR, it does NOT work on Windows and in the current state it will break the behavior for Windows users.

I wonder if it would make sense to create a new bundle based on the msys2 packages and use that for CI. What do you think?

I'm actually on replacing the current Mingw CI builds by a more recent approach using MSYS2 packages: https://github.com/eht16/geany-plugins/blob/ci_add_windows_build/.github/workflows/mingw-w64.yaml

I hope I can finish this in a reasonable time frame, then GTK 3.22 would be no problem for the CI.
My plan was to first get G-P running and then replace the Mingw CI build in Geany. But I might change the priorities and do Geany first.

[kugel-]

As mentioned ealier in this PR, it does NOT work on Windows and in the current state it will break the behavior for Windows users.

Huh? Windows should not change, it's still ifdef'd and uses win32_open_browser() (which outright ignores the browser_cmd pref).

[kugel-]

@eht16 ping

[kugel-]

@eht16 ping

[eht16]

As mentioned ealier in this PR, it does NOT work on Windows and in the current state it will break the behavior for Windows users.

Huh? Windows should not change, it's still ifdef'd and uses win32_open_browser() (which outright ignores the browser_cmd pref).

Sorry, I was very unclear in my post :(.

I assumed the TODO comment in the Windows code branch was meant to use the new code also on Windows and remove the previous code. This would be nice but won't work as gtk_show_uri_on_window doesn't work on Windows (as described).

Now, that we checked the new code won't work on Windows, we can happily merge this PR as is, maybe just remove the TODO comment in the Windows code branch.

[kugel-]

@eht16 still unresolved is the (new) requirement for GTK 3.22. FWIW, we can probably formally require the 4-year-old 3.24 which is also the last GTK3 release. It's available in MSYS2 but our windows releases and mingw CI builds still use that ancient version 3.8 isn't it?

[elextr]

Agree, 3.24 is in Ubuntu 20.04 LTS which is the oldest still supported.

[eht16]

I agree to require GTK 3.24. The Windows releases use up2date versions of the packages, I usually perform a full "pacman -Syu" in release preparation.
For the CI I will try to get my updated build scripts finished soon. IMO we do not need to wait with this PR for it (even if Windows CI would break).

[kugel-]

I'm hesitant to break the CI as that affects not only this PR but all future PRs for other contributors. I would prefer if we (you) could get the CI prepared beforehand.

[eht16]

Alright, will do.

[eht16]

Ok, we have geany/geany-plugins#1201 for Geany-Plugins and geany/infrastructure#7 for the necessary supporting scripts.
Once we agree to merge these, I'll start adding the job for Geany and then we finally bump the GTK requirement.

So, feedback on the above PRs is welcome to get them ready to land soon.

[kugel-]

@eht16 The mingw-w64 CI seems to recreate the Docker image from scratch. You implemented a cache didn't you?

[eht16]

@eht16 The mingw-w64 CI seems to recreate the Docker image from scratch. You implemented a cache didn't you?

I noticed that the prebuilt Docker image cannot be pulled from the registry and so it is rebuilt on each CI run.
There is no cache itself for the Docker image but it is pushed to the Docker package registry of the geany organization at Github. To my understanding of the docs, the image should be pullable by CI jobs started on the master branches of the Geany and G-P repositories but it seems not so.

I will change the image visibility to public so that everyone can pull it from the registry. I hesitated to do this because once it is public it cannot be deleted any longer. But it is probably OK to do it now, then also PRs can use the prebuilt image.

[b4n]

If we do care about pre-3.22 GTK, it'd be easy enough to do something like that:

#if GTK_CHECK_VERSION(3, 22, 0)
			if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
#else
			if (gtk_show_uri(gtk_widget_get_screen(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
#endif

[kugel-]

But we don't

[b4n]

Just be aware of one thing: gtk_show_uri*() will happily open a lot more URIs than what the web browser might support. It basically can open any URI, be it http:, mailto: or my-custom-scheme:. It's nice and all, but some suggest that it should be handled with care not to open random programs using random schemes.

Admittedly, there's two things going for us here:

• we're not a chat app or such, we don't present mangled untrusted URIs
• all internal URIs are safe and likely (?) to open in the browser (https:, file: -- need to check that last one though)

But we cannot know what plugins might do.

Though we should check how are file: URIs handled. We use that in help, and when using the builtin build exec command.

[elextr]

we're not a chat app or such, we don't present mangled untrusted URIs

Its worse than that!!!! If the build command is "builtin" utils_open_browser() is used to open stuff I wrote 😁

[kugel-]

Should I change the patch or was this just a "beware!" remark?

[b4n]

That was just a "beware!" remark, but it'd be nice to check what happens with file:// URIs we might have to local help

[kugel-]

Seems we already feed file:// URIs to the function, e.g. for file:///usr/local/share/doc/geany/html/index.html. So at least file:// must be supported, although without the change we explicitly opened the configured browser, and now we rely on GTK do find the right "browser".

I hesitate to limit the inputs the function accepts. Webdevelopment plugins may want this function to open arbitrary stuff in the browser.

If anything, we might limit to https://, http:// and file:// but then we might exclude fancy webapps that some plugins could use (they could use gtk_show_uri_on_window() directly but lets help them to work on windows as well).

And then, on Windows we also do a generic open, the browser_cmd configuration is not a thing there.

Long story shot: I leave this as-is until problems arise

[kugel-]

Would anyone mind formally approving the current patch?

[eht16]

Tested and works fine.
@kugel- I'm fine to merge.