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. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3178
-- Commit Summary --
* Use gtk_show_uri_on_window() in utils_open_browser() by default
-- File Changes --
M data/geany.glade (1) M src/keyfile.c (4) M src/utils.c (18)
-- Patch Links --
https://github.com/geany/geany/pull/3178.patch https://github.com/geany/geany/pull/3178.diff
`goto` ???!!!!
Yes, I've used goto. What's your point again?
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.
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.
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?
@kugel- pushed 1 commit.
80cc5857fffb07e6081ad696dface7cf0b11c1e6 squash! Use gtk_show_uri_on_window() in utils_open_browser() by default
@kugel- pushed 1 commit.
719080cc8001d86da77c18f4e30244e3b75bdfda Use gtk_show_uri_on_window() in utils_open_browser() by default
I think I found another solution, also without goto. Please check agian.
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.".
I'm hesitant because it invalidates all the existing translations. Do you still think it's a good idea?
The next release is going to have many invalidated translations anyway, I wouldn't have thought one more would hurt.
@kugel- pushed 1 commit.
3b35cb891843703a60a305bff2ef8b956ad3a314 fixup! Use gtk_show_uri_on_window() in utils_open_browser() by default
Alright, hope it's fine now.
@kugel- pushed 1 commit.
94eab007ad00bdd9ee9e5c0cf56a25061c4d2e37 fixup! Use gtk_show_uri_on_window() in utils_open_browser() by default
LGTM
@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?
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.
And the truth is that I found the escape command only after making this patch, hence the commit message.
I agree that this is a good idea anyway, have continued the flatpack discussion at #3199
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 https://github.com/geany/geany/pull/2444.
Hmmm, well our Glade file targets [GTK 3.20](https://github.com/geany/geany/blob/37d20a823fb1bca67cea57ef73aa9e64009d8138...) 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.
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 `#ifdef`ed 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.
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?
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/wor...
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.
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).
@eht16 ping
@eht16 ping
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- pushed 2 commits.
68bf112adb125f28bb1070ac7cf6602bf74ad56e Use gtk_show_uri_on_window() in utils_open_browser() by default 602624a502fff10f8cf316fa1837bdfffbd5e2ba fixup! Use gtk_show_uri_on_window() in utils_open_browser() by default
@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?
Agree, 3.24 is in Ubuntu 20.04 LTS which is the oldest still supported.
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).
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.
Alright, will do.
Ok, we have https://github.com/geany/geany-plugins/pull/1201 for Geany-Plugins and https://github.com/geany/infrastructure/pull/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- pushed 1 commit.
58863312967babe8dfad1f20c25ff169e341e92d Use gtk_show_uri_on_window() in utils_open_browser() by default
@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 commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
If we do care about pre-3.22 GTK, it'd be easy enough to do something like that: ```c #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- commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
But we don't
@b4n commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
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 commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
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 :grin:
@kugel- commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
Should I change the patch or was this just a "beware!" remark?
@b4n commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
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- commented on this pull request.
{
- gchar *new_cmd = dialogs_show_input(_("Select Browser"), GTK_WINDOW(main_widgets.window), - _("Failed to spawn the configured browser command. " - "Please correct it or enter another one."), + /* Uses the user's default browser akin to xdg-open (in flatpak through a portal) */ + if (EMPTY(tool_prefs.browser_cmd)) + { + if (gtk_show_uri_on_window(GTK_WINDOW(main_widgets.window), uri, GDK_CURRENT_TIME, NULL))
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
Would anyone mind formally approving the current patch?
Tested and works fine.
@eht16 approved this pull request.
Merged #3178 into master.
github-comments@lists.geany.org