Re-assigning all Gimp bugs to default component owner (Gimp bugs list)

Created attachment 660 [details] [review]
patch to make gimp-remote start gimp from the same path

Attached is a patch that addresses at least one of the problems.
It determines the path of the gimp-remote executable and tries to
start gimp from this path. If this fails it falls back to the old
behaviour. Could need some testing....

Is this a 1.2.3 thing?

I don't like my patch that much, otherwise I would have committed it.
I'm not sure if g_getenv ("_") is portable at all.

Someone might want to look into this for 1.2.4.

I did some research. The environment variable "_" apparently gets set
by the shell - but not in all shells. bash and ksh apparently set
this, but tcsh does not (and if you invoke tcsh from a bash "_" will
contain the path of the tcsh always...).

I do not know if the use of "_" is specified in any standard for the
sh family of shells. It is however a good start for searching the
executable. We could check for "gimp-remote" as a substring of "_" and
if we find it use the path specified in front of it. If this fails we
could fallback to argv[0] and check if it contains a '/' - which would
mean that there is a path specification in it (this might be relative
to the CWD).

As a last fallback use the current approach to search the path with
execvp().

Implemented my latest suggestion in CVS:

2003-01-08  Simon Budig  <simon@gimp.or>

        * tools/gimp-remote.c: Implemented a heuristics that tries to
        start the gimp in the same directory as gimp-remote-1.3.
        If this fails it falls back to the regular PATH search.

        For sanity checks this feature requires that the command name
        starts with "gimp-remote". If you do weird symlinks the 
        heuristics will fall back to the PATH search silently.

I wonder what to do about the "behaves differently" part should be done.
A fork would be an easy solution, however, there would be no way to
report back if the command was successful or had failed and textual
output of the child would appear after the shell prompt.

A signal handler for SIGCHILD plus a timeout is probably too much
overhead...

Should be easy to do using g_spawn_async_with_pipes(). However I think
that the current behaviour is perfectly OK. gimp_remote exits when it
has done its job.

Suggest moving this one to 1.2.5 as well.

Dave.

Moving to 1.2.6 milestone, since it is looking like there will be a pretty quick
1.2.( release.

Moving this to the 2.2 milestone since I don't think we will do
anything about it in the 1.2 branch. But we could finally redo
gimp-remote for 2.2.

*** Bug 138395 has been marked as a duplicate of this bug. ***

Raising priority. If we want to get this done for 2.2, we should figure out how
to do it soon.

What is it that should be done?  The issue concerning the toolbox seems to be
the only unsolved and uncontroversial one.

Well, gimp-remote should die. The idea is that the gimp executable itself should
be able to detect that an interactive gimp is already running on the same
display and instead of starting it should pass the request to load an image to
this gimp instance. The problem is to do this in a portable way. I don't think
we will get away w/o any platform-specific code but it shouldn't be too much of it.

It wouldn't be desirable to prevent the user from running more than one instance
of Gimp at a time, I think.  So maybe a reasonable plan would be that if Gimp is
invoked using the application name "gimp-remote", then it looks for an existing
instance to pass off the image to, and if none is found behaves like it
currently does?  I know this will work in Unixish OS's; not sure about Windows.

IMO it should be the default behaviour that only one instance is ever run on the
same display. We can add a command-line flag that changes this behaviour.

If we don't want to put the remote code into the core application we could also
change the name of the gimp binary to (for example) gimp.bin and have rename
gimp-remote to gimp. A lot of apps are doing it this way.

What really bothers me about the current implementation of gimp-remote is the
hackish way that the filenames are passed to the application. We did it this way
because gimp-remote was introduced in the middle of a stable series and there
was no way to add a proper IPC scheme. One problem of the current implementation
is that it doesn't work if the toolbox is minimized. We'd have the chance to do
it better now.

We could consider to use dbus for this. Perhaps in the next development cycle...

This would also fix bug #332835. Let's finally do this and integrate the gimp-remote functionality into the gimp application.

In the long run, we might want to consider using whatever comes out of this SoC project: http://live.gnome.org/SingleInstanceApps

gtkunique is now in gnomecvs (http://cvs.gnome.org/viewcvs/gtkunique/). See http://cvs.gnome.org/viewcvs/gtkunique/README?view=markup for a quick introduction. We should consider to use this in GIMP 2.6.

Created attachment 80681 [details] [review]
build gimp-remote functionality into gimp by means of d-bus

This patch is bascially working but needs some minor improvements before it can go into the trunk. I am attaching it here for review. Comments appreciated.

Created attachment 80693 [details] [review]
revised patch

The code is now in the trunk. Will likely be tweaked further over the next days. We also still need to decide if, how and when we want to deprecate gimp-remote. But I think we can already close this report as FIXED.


2007-01-19  Sven Neumann  <sven@gimp.org>

	* INSTALL
	* configure.in: check for D-Bus GLib bindings.

	* app/Makefile.am
	* app/main.c: check if an interactive GIMP instance proposes
	itself on the D-Bus and delegate to it. Allow this behaviour to be
	overridden by using the --new-instance command-line option.

	* app/widgets/Makefile.am
	* app/widgets/gimpdbusservice.[ch]
	* app/widgets/dbus-service.xml: added an object that offers a
	D-Bus service.

	* app/gui/Makefile.am
	* app/gui/gui.c: connect to the D-Bus and export the GimpDBusService.

A couple ways you might improve the patch:

 - the way dbus names representing whole apps are meant to be handled is:
    1) request the name
    2) if you did not get it, exit (or otherwise gracefully allow 
       another instance to be "the main gimp"). This avoids an 
       admittedly unlikely race if you do something like 
       "gimp & gimp & gimp & gimp &" and start a bunch at once.
    3) if you did get it,
       watch for the NameLost signal indicating that you lost the 
       name; if you lose the name, then shut down gracefully 
       (this is to implement a "--replace" feature like many WMs 
        have, very useful in debugging instead of having to kill 
        or manually quit the old instance)
    the short summary is maintain the invariant that the process is 
    only running while it owns the org.gimp.GIMP name

 - Open() should probably support some extra information such as the 
   startup notification info; if you look at the "GUnique" library 
   some people have blogged about, their API for opening an app 
   has an example of some of the arguments you might want. 
   I would suggest maybe a dict of {string,variant} as an extensible
   set of properties you could pass to Open

Why should the application shut down when it loses the name? I don't think users would be happy if their gimp instance shut down when someone steals the name. We explicitely allow multiple instances if the user wants them.

I am not sure if I understand the startup notification issues but I will have a look.

I am not sure if it is worth re-opening this bug report, but the second point in the original bug report has not been addressed by this patch:

> - It behaves differently for the first and subsequent instances: if the
>   Gimp was not running, then gimp-remote executes it and the control
>   does not return to the caller until the Gimp exits.  If it was already
>   running, then gimp-remote returns immediately after having sent the
>   image.

The answer to this point clearly is WONTFIX. Such a behaviour would be completely unreasonable.