GTK4: Convert Geeqie to a GtkApplication 1 #1525
Conversation
This is a request for comment, not a real pull request. GTK4 is a pain in the neck, but is not going to go away. So this change has to be done sometime. This is the conversion of Geeqie to be a GtkApplication. The change should also simplify command line option and argument handling. The following options are deleted: --fullscreen-start, --fullscreen-stop: Use --fullscreen which is a toggle --slideshow-start, --slideshow-stop: Use --slideshow which is a toggle --tools-show, --tools-hide: Use --tools which is a toggle --disable-clutter: Start with GQ_DISABLE_CLUTTER= geeqie --cache-maintenance: Start with GQ_CACHE_MAINTENANCE= geeqie (disabled) --new-instance: Start with GQ_NON_UNIQUE= geeqie --blank, --list-add, --list-clear: I do not see the need for them [I think the intent was for a hidden command line collection to be used with the above options adding/removing images from it] GQ_CACHE_MAINTENANCE= geeqie does not work, and so is disabled - I will fix that sometime In meson.build, image checks, is_parallel is set to False. I assume parallel is not possible because of a problem with dbus, but I cannot find a solution. Image tests run OK locally, but fail running on GitHub. It looks like the first instance launches and runs OK, but the second instance runs also as a primary and not as a remote. Again it seems like a dbus problem and I did not find a solution. The changes are in C and not C++ - unfortunately, that's the way things are.
| @@ -63,6 +65,13 @@ chmod 0700 "$XDG_RUNTIME_DIR" | |||
| cd | |||
| mkdir -p "$XDG_CONFIG_HOME" | |||
|
|
|||
| # Primary and remote instances of GtkApplication communicate with dbus | |||
There was a problem hiding this comment.
I can take a look at this, but dbus-launch will be using the environment variables of the user running the test, which will break the isolation here.
That said, going by the manpage, you should be able to add dbus-launch on line 83, instead of running it in advance.
So env -i G_DEBUG="..." [...] XDG_RUNTIME_DIR="..." dbus-launch "$@"
From the docs:
You may specify a program to be run; in this case, dbus-launch will
launch a session bus instance, set the appropriate environment
variables so the specified program can find the bus, and then execute
the specified program, with the specified arguments. See below for
examples.
| @@ -88,35 +88,36 @@ def main(argv) -> int: | |||
| # signal to the geeqie process. See | |||
| # <https://unix.stackexchange.com/questions/291804/howto-terminate-xvfb-run-properly>. | |||
| # So we should modify or replace xvfb-run to allow us to kill an errant geeqie process. | |||
| geeqie_proc = subprocess.Popen(args=[*geeqie_cmd_prefix, test_image_path]) | |||
| geeqie_proc = subprocess.Popen(args=[*geeqie_cmd_prefix, test_image_path], env=os.environ) | |||
There was a problem hiding this comment.
I understand that this was probably while battling with dbus, but this is the default value, so should not be necessary (please correct me if you found otherwise, though)
| * | ||
| * collection_unref(cd); | ||
| */ | ||
| options->marks_save ? marks_save(TRUE) : marks_save(FALSE); |
There was a problem hiding this comment.
I believe @qarkai simplified this to marks_save(options->marks_save);
| GList *work; | ||
| GList *selected; | ||
| FileData *fd; | ||
| fprintf(stderr, \ |
There was a problem hiding this comment.
Could this be written with auto-string-concatenation to avoid having to break indentation?
Like:
fprintf(stderr,
_("Can't initialize clutter-gtk. \n"
"To start Geeqie use: \n"
"GQ_DISABLE_CLUTTER=1 geeqie\n\n"));
| if (g_getenv("GQ_CACHE_MAINTENANCE")) | ||
| { | ||
| /* Disabled at the moment */ | ||
| log_printf("Command line cache maintenance is disabled"); |
There was a problem hiding this comment.
If this is going to be submitted (even temporarily), it might be good to add "...is disabled until the code can be fixed" or something. I can imagine a user seeing this warning as it's currently written and wondering what they're doing wrong to enable it.
| Icon=geeqie | ||
| Type=Application | ||
| Terminal=false | ||
| # Startup notification disabled, since the remote -r switch may not open a new window... |
There was a problem hiding this comment.
Is this comment still relevant?
There was a problem hiding this comment.
Will review this file after remote.cc and remote.h are deleted, so that git will auto-detect this as a partial move rather than a completely new file.
Thanks, but I think I am some way to finding the solution. Apparently I tried it and dismissed it for some reason. And I suppose somewhere in the midst of multiple wrong directions and even more going round in circles that I forgot about that. |
This is a request for comment, not a real pull request.
GTK4 is a pain in the neck, but is not going to go away. So this change has to be done sometime.
This is the conversion of Geeqie to be a GtkApplication.
The change should also simplify command line option and argument handling.
The following options are deleted:
--fullscreen-start, --fullscreen-stop: Use --fullscreen which is a toggle
--slideshow-start, --slideshow-stop: Use --slideshow which is a toggle
--tools-show, --tools-hide: Use --tools which is a toggle
--remote: No longer needed
--disable-clutter: Start with GQ_DISABLE_CLUTTER= geeqie
--cache-maintenance: Start with GQ_CACHE_MAINTENANCE= geeqie (disabled)
--new-instance: Start with GQ_NON_UNIQUE= geeqie
--blank, --list-add, --list-clear: I do not see the need for them
[I think the intent was for a hidden command line collection to be used with the above options adding/removing images from it]
GQ_CACHE_MAINTENANCE= geeqie does not work, and so is disabled - I will fix that sometime
In meson.build, image checks, is_parallel is set to False. I assume parallel is not possible because of a problem with dbus, but I cannot find a solution.
Image tests run OK locally, but fail running on GitHub. It looks like the first instance launches and runs OK, but the second instance runs also as a primary and not as a remote. Again it seems like a dbus problem and I did not find a solution.
The changes are in C and not C++ - unfortunately, that's the way things are.