GNOME Bugzilla – Bug 770437
general: fix issues to allow evolution integration
Last modified: 2016-09-02 17:00:03 UTC
The recent modifications in autoar API made the evolution integration not work anymore. In order to patch the evolution autoar integration, a few issues had to be fixed in autoar. See the patches for more details. Evolution bug report: https://bugzilla.gnome.org/show_bug.cgi?id=770380.
Created attachment 334218 [details] [review] general: add enum-types header to the library headers
Created attachment 334219 [details] [review] AutoarCompressor, AutoarExtractor: remove redundant declaration The type getters were mistakenly redeclared in the header files.
Created attachment 334220 [details] [review] autoar-gtk: update includes The sources and headers for the gnome-autoar-gtk library should include headers from the library itself, not from gnome-autoar.
Created attachment 334221 [details] [review] AutoarExtractor: make "decide-destination" emission synchronous Since "decide-destination" requires the client to provide a new destination, it cannot be executed asynchronously in the main context. In order to fix this, use g_signal_emit instead.
Review of attachment 334220 [details] [review]: ::: tests/test-ui.c @@ -3,2 +3,2 @@ #include <gnome-autoar/gnome-autoar.h> -#include <gnome-autoar/autoar-gtk.h> +#include <gnome-autoar/autoar-gtk-chooser.h> Please use '#include <gnome-autoar-gtk/autoar-gtk.h>' here. autoar-gtk-chooser.h is a private header.
(In reply to Razvan Chitu from comment #4) > Created attachment 334221 [details] [review] [review] > AutoarExtractor: make "decide-destination" emission synchronous > > Since "decide-destination" requires the client to provide a new destination, > it > cannot be executed asynchronously in the main context. In order to fix this, > use > g_signal_emit instead. Signals were emitted in the main context because user-provided callbacks were usually used to update the graphical interface. I remember this feature was used in autoar integration patches made for epiphany and empathy. It was probably not used in evolution because I didn't find a way to display the information.
(In reply to Ting-Wei Lan from comment #6) > (In reply to Razvan Chitu from comment #4) > > Created attachment 334221 [details] [review] [review] [review] > > AutoarExtractor: make "decide-destination" emission synchronous > > > > Since "decide-destination" requires the client to provide a new destination, > > it > > cannot be executed asynchronously in the main context. In order to fix this, > > use > > g_signal_emit instead. > > Signals were emitted in the main context because user-provided callbacks > were usually used to update the graphical interface. I remember this feature > was used in autoar integration patches made for epiphany and empathy. It was > probably not used in evolution because I didn't find a way to display the > information. Hey again :). I figured this eventually, but we kind of needed this decide-destination signal if we went with not automatically solving file conflicts.
(In reply to Ting-Wei Lan from comment #5) > Review of attachment 334220 [details] [review] [review]: > > ::: tests/test-ui.c > @@ -3,2 +3,2 @@ > #include <gnome-autoar/gnome-autoar.h> > -#include <gnome-autoar/autoar-gtk.h> > +#include <gnome-autoar/autoar-gtk-chooser.h> > > Please use '#include <gnome-autoar-gtk/autoar-gtk.h>' here. > autoar-gtk-chooser.h is a private header. If I try to do this, I get: fatal error: gnome-autoar-gtk/autoar-gtk.h: No such file or directory #include <gnome-autoar-gtk/autoar-gtk.h> Any idea what could cause this?
(In reply to Razvan Chitu from comment #7) > (In reply to Ting-Wei Lan from comment #6) > > (In reply to Razvan Chitu from comment #4) > > > Created attachment 334221 [details] [review] [review] [review] [review] > > > AutoarExtractor: make "decide-destination" emission synchronous > > > > > > Since "decide-destination" requires the client to provide a new destination, > > > it > > > cannot be executed asynchronously in the main context. In order to fix this, > > > use > > > g_signal_emit instead. > > > > Signals were emitted in the main context because user-provided callbacks > > were usually used to update the graphical interface. I remember this feature > > was used in autoar integration patches made for epiphany and empathy. It was > > probably not used in evolution because I didn't find a way to display the > > information. > > Hey again :). I figured this eventually, but we kind of needed this > decide-destination signal if we went with not automatically solving file > conflicts. Can we have two kinds of decide-destination signal? Applications which don't want to change the destination can simply use the async signal. Applications which want to do checks and possibly change the destination can connect to both sync and async signals. The sync version is emitted first, and the async version gets emitted later with the new destination. (In reply to Razvan Chitu from comment #8) > (In reply to Ting-Wei Lan from comment #5) > > Review of attachment 334220 [details] [review] [review] [review]: > > > > ::: tests/test-ui.c > > @@ -3,2 +3,2 @@ > > #include <gnome-autoar/gnome-autoar.h> > > -#include <gnome-autoar/autoar-gtk.h> > > +#include <gnome-autoar/autoar-gtk-chooser.h> > > > > Please use '#include <gnome-autoar-gtk/autoar-gtk.h>' here. > > autoar-gtk-chooser.h is a private header. > > If I try to do this, I get: > > fatal error: gnome-autoar-gtk/autoar-gtk.h: No such file or directory > #include <gnome-autoar-gtk/autoar-gtk.h> > > Any idea what could cause this? A recent commit changed the installation location of autoar-gtk.h from gnome-autoar to gnome-autoar-gtk, but autoar-gtk.h is still put in gnome-autoar of the source directory. I think a possible fix is to make gnome-autoar-gtk directory in the source directory and move gnome-autoar-gtk source and pc files into it.
(In reply to Ting-Wei Lan from comment #9) I see that we still have quite a few things to discuss about autoar's development. Would you like to do it on the Nautilus IRC? I bet Carlos Soriano (and maybe also Cosimo, if he's around) would like to participate too. We are kind of flooding the evolution bug with an ~unrelated discussion :D.
(In reply to Ting-Wei Lan from comment #9) So, besides the header issue, are you fine with these patches?
(In reply to Razvan Chitu from comment #11) > (In reply to Ting-Wei Lan from comment #9) > > So, besides the header issue, are you fine with these patches? What I meant to say is that even though there still are a few issues about the current state of autoar (some of them being the ones you mentioned here), these patches put it in a stable state for the 3.22 release. After that we can focus on a proper overhaul of the library and not just changing it bit by bit. Would you be fine with this approach?
Review of attachment 334219 [details] [review]: Yes, I didn't find it because it didn't cause compiler warnings. :)
Review of attachment 334218 [details] [review]: This patch causes circular dependencies in the Makefile: gmake: Circular gnome-autoar/autoar-enum-types.h <- gnome-autoar/autoar-enum-types.h dependency dropped. gnome-autoar/autoar-enum-types.h: gnome-autoar/autoar-enum-types.h.template $(libgnome_autoar_la_headers) $(AM_V_GEN) (cd $(srcdir) && $(GLIB_MKENUMS) --template gnome-autoar/autoar-enum-types.h.template $(libgnome_autoar_la_headers) ) > $@
Review of attachment 334221 [details] [review]: If you think this patch is required to fix evolution, you can commit this patch to gnome-3-22 branch (but not master branch). I think we will have a better solution in next cycle.
Review of attachment 334220 [details] [review]: I think you have to move gnome-autoar/*autoar-gtk* to gnome-autoar-gtk/ directory in the source tree because autoar-gtk headers are now installed in $(includedir)/gnome-autoar-gtk-0/gnome-autoar-gtk.
Created attachment 334657 [details] [review] Makefile: change gnome-autoar-gtk include directory
Created attachment 334658 [details] [review] general: add enum-types header to the library headers
Created attachment 334661 [details] [review] general: change gnome-autoar-gtk include directory
Review of attachment 334658 [details] [review]: With this the enums are included as is done in GLib, which is good. This patch no longer causes a loop dependency either, which was what Ting-Wey pointed out. So this LGTM
Review of attachment 334661 [details] [review]: This reverts the change of creating a different folder for the -gtk library, which is what Ting-Wei pointed out, and fix the include problems in evolution. So this seems to be fine too. Thanks!
Attachment 334219 [details] pushed as dad5bf8 - AutoarCompressor, AutoarExtractor: remove redundant declaration Attachment 334221 [details] pushed as 4ac0fd4 - AutoarExtractor: make "decide-destination" emission synchronous Attachment 334658 [details] pushed as f88b408 - general: add enum-types header to the library headers Attachment 334661 [details] pushed as cb2b595 - general: change gnome-autoar-gtk include directory
Attachment 334219 [details] pushed as dad5bf8 - AutoarCompressor, AutoarExtractor: remove redundant declaration Attachment 334658 [details] pushed as f88b408 - general: add enum-types header to the library headers Attachment 334661 [details] pushed as cb2b595 - general: change gnome-autoar-gtk include directory