GNOME Bugzilla – Bug 622876
Migrate from dbus-glib to GApplication
Last modified: 2011-06-12 10:17:14 UTC
For GLib 2.25.5 GDBus D-Bus support was merged, providing an API to replace dbus-glib. See http://library.gnome.org/devel/gio/unstable/gdbus.html and http://library.gnome.org/devel/gio/unstable/ch28.html . According to a quick grep this module seems to use dbus-glib: ./eog/configure.ac: PKG_CHECK_MODULES([DBUS], [dbus-glib-1 >= $DBUS_GLIB_REQUIRED], have_dbus=yes, have_dbus=no) ./eog/configure.ac: EOG_MODULES="$EOG_MODULES dbus-glib-1 >= $DBUS_GLIB_REQUIRED"
Hmm, I wonder if it would make more sense to migrate the remaining dbus parts to GApplication instead as that (unique app) seems to be the only usecase of eog's dbus service right now.
I guess we should migrate to GtkApplication instead. Indeed uniqueness is the only use case. I have been looking into Gtk/GApplication these days and when I get a good idea I might give it a go.
I have migrated to inheriting EogApplication from GtkApplication, and things are working fine. The only *but* has to do with the GOptionContext parsing, which right now is quite unfriendly for GApplication (see bug 634990). Since my current changes break any command-line parameters handling, I'll keep it on hold for a while, until I figure out how to do it nicely or we fix GApplication.
Updating summary to fit the plans.
Created attachment 181994 [details] [review] Port EogApplication to GtkApplication for uniqueness This removes the direct dependency on dbus and makes use of the GApplication/GtkApplication facilities. The only regression is the removal of the --new-instance command line parameter, which is not nicely supported by GApplication.
Created attachment 181996 [details] [review] Port EogApplication to GtkApplication for uniqueness This removes the direct dependency on dbus and makes use of the GApplication/GtkApplication facilities. The only regression is the removal of the --new-instance command line parameter, which is not nicely supported by GApplication.
The patch above is basically what we need, but with the exception of the --new-instance command. I couldn't find a way to do what we need (there is wip code commented but I don't think the approach is right) so I think it would be sane to just drop it for 3.0. It is a broken use case anyway. An alternative is to split eog into a service and a launcher, similar to what evince is doing now. AFAIK GApplication has support for this but it would be a bit of extra work, so I don't consider it doable for 3.0. We can open a bug for that if desired. Felix, if this looks OK to you, I'll clean up the code and push to master.
Hi, sorry for the late response. (In reply to comment #7) > The patch above is basically what we need, but with the exception of the > --new-instance command. I couldn't find a way to do what we need (there is wip > code commented but I don't think the approach is right) so I think it would be > sane to just drop it for 3.0. It is a broken use case anyway. Well, I'd say that's something one could reimplement once the required stuff in GApplication is available. > An alternative is to split eog into a service and a launcher, similar to what > evince is doing now. AFAIK GApplication has support for this but it would be a > bit of extra work, so I don't consider it doable for 3.0. We can open a bug for > that if desired. What would be the advantage of that approach? Isn't the end-result similar to what we have now? > Felix, if this looks OK to you, I'll clean up the code and push to master. Hmm, I'd like to postpone it to 3.2. EogApplication is not really unimportant for eog and we're quite late in the 3.0 process already and that way we can have it tested more extensively.
(In reply to comment #8) > > > An alternative is to split eog into a service and a launcher, similar to what > > evince is doing now. AFAIK GApplication has support for this but it would be a > > bit of extra work, so I don't consider it doable for 3.0. We can open a bug for > > that if desired. > > What would be the advantage of that approach? Isn't the end-result similar to > what we have now? The difference would be that we would have different processes for different directories, but only one instance per directory browsed with eog. The advantage of using one-process-per-directory would be that a faulty plugin, image, or decoder would not make all other eog windows crash. This is a minor concern in eog (compared to evince, as far as I can tell), so that's why I don't necessarily see this as so important. > > Felix, if this looks OK to you, I'll clean up the code and push to master. > > Hmm, I'd like to postpone it to 3.2. EogApplication is not really unimportant > for eog and we're quite late in the 3.0 process already and that way we can > have it tested more extensively. OK, we can do that.
The 3.0 code is branched of now. So, feel free to push your code to master now once you're ready. :)
I think there is code now in glib to support non-uniqueness. I will need a bit of time to look into that, but should be able to do it in time before 3.2.
Created attachment 189578 [details] [review] Port EogApplication to GtkApplication This removes the direct dependency on dbus and makes use of the GApplication/GtkApplication facilities for uniqueness and activation. Bump the glib requirement to 2.29.4, since we need G_APPLICATION_NON_UNIQUE.
Felix, this is my final patch. It solves all the issues we had. Please review and I'll push to master.
Review of attachment 189578 [details] [review]: Very Nice! Can't spot anything problematic. :)
Oh, one thing after all. :p The patch caused git to complain with a whitespace error. Maybe you can fix that one before pushing.
Attachment 189578 [details] pushed as 3d39587 - Port EogApplication to GtkApplication
Thanks, Claudio!