Created attachment 126747 [details] [review]
add python bindings for evince

This is nice, however we are one week past the API freeze deadline, so unless you can convince the GNOME release team to allow an exception this will have to wait for GNOME 2.27/2.28.

(In reply to comment #2)
> This is nice, however we are one week past the API freeze deadline, so unless
> you can convince the GNOME release team to allow an exception this will have to
> wait for GNOME 2.27/2.28.
> 

Yes, I'm willing to ask for a freeze break. Would you approve this change if they approved the break?

Sorry, but I don't see this working.

I just tried building evince in jhbuild, and it does not install any evince.pc file.  If the necessary Evince bits are not upstream yet, I don't see the point in committing this right away.

(In reply to comment #4)
> Sorry, but I don't see this working.
> 
> I just tried building evince in jhbuild, and it does not install any evince.pc
> file.  If the necessary Evince bits are not upstream yet, I don't see the point
> in committing this right away.

Yes, sorry, forgot to mention that this is blocking on the following tickets:

http://bugzilla.gnome.org/show_bug.cgi?id=567787
http://bugzilla.gnome.org/show_bug.cgi?id=567790
http://bugzilla.gnome.org/show_bug.cgi?id=568220

Comment on attachment 126747 [details] [review]
add python bindings for evince

[...]
>diff --git a/evince/evince.override b/evince/evince.override
>new file mode 100644
>index 0000000..fba768b
>--- /dev/null
>+++ b/evince/evince.override
>@@ -0,0 +1,99 @@
>+/* -*- Mode: C; c-basic-offset: 4 -*- */
>+%%
>+headers
>+#include <Python.h>

These are missing here:

#define NO_IMPORT_PYGOBJECT
#define NO_IMPORT_PYGTK

>+#include "pygobject.h"
>+#include <pygtk/pygtk.h>
>+
>+#include <ev-backends-manager.h>
>+#include <ev-document.h>
>+#include <ev-document-find.h>
>+#include <ev-document-info.h>
>+#include <ev-document-type-builtins.h>
>+//#include <ev-document-factory.h>
>+#include <ev-file-helpers.h>
>+
>+#include <ev-view.h>
>+#include <ev-page-cache.h>
>+#include <ev-jobs.h>
>+#include <ev-job-scheduler.h>
>+#include <ev-view-type-builtins.h>
>+
>+%%
>+modulename ev
>+%%
>+import gobject.GObject as PyGObject_Type
>+import gtk.Widget as PyGtkWidget_Type
>+import gtk.ScrolledWindow as PyGtkScrolledWindow_Type
>+%%
>+override ev_view_find_changed kwargs
>+/* This function usually takes a GList of search results and hence must be
>+ * wrapped manually. In our override, we modify it to take the corresponding 
>+ * job to avoid having to unpack/repack lists.
>+ */
>+static PyObject *
>+_wrap_ev_view_find_changed(PyGObject *self, PyObject *args, PyObject *kwargs)
>+{
>+    static char *kwlist[] = { "job", "page", NULL };
>+    PyGObject *job;
>+    int page;
>+
>+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O!i:EvView.find_changed",
>+            kwlist, &PyEvJobFind_Type, &job, &page))
>+        return NULL;
>+
>+    ev_view_find_changed(EV_VIEW(self->obj),
>+        ev_job_find_get_results(EV_JOB_FIND(job->obj)),
>+        page);
>+
>+    Py_RETURN_NONE;
>+}
>+%%
>+override ev_job_find_new kwargs
>+/*
>+ * We wrap this here because the object takes an EvDocument gpointer as a
>+ * construction property, and you can't do gpointers from python.
>+ * We can't make the property be a GObject parameter because EV_DOCUMENT_TYPE
>+ * is an interface and not a G_OBJECT type.
>+ */
>+static int
>+_wrap_ev_job_find_new(PyGObject *self, PyObject *args, PyObject *kwargs)
>+{
>+    static char *kwlist[] = { "document", "start_page", "n_pages", "text",
>+        "case_sensitive",  NULL };
>+    PyGObject *document;
>+    gint start_page, n_pages;
>+    const gchar *text;
>+    gboolean case_sensitive;
>+
>+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O!iizi:EvJobFind.__init__",
>+            kwlist, &PyEvDocument_Type, &document, &start_page, &n_pages,
>+            &text, &case_sensitive))
>+        return NULL;
>+
>+    pygobject_construct(self, "document", EV_DOCUMENT(document->obj),
>+        "start_page", start_page, "n_pages", n_pages, "text", text,
>+        "case_sensitive", case_sensitive, NULL);
>+    if (!self->obj) {
>+        PyErr_SetString(PyExc_RuntimeError, "could not create JobFind object");
>+        return -1;
>+    }
>+
>+    return 0;
>+}
>+%%
>+override evince_embed_init noargs
>+static PyObject *
>+_wrap_evince_embed_init(PyObject *self)
>+{
>+    /* Init glib threads asap */
>+    if (!g_thread_supported ())
>+       g_thread_init (NULL);

Don't forget to call pyg_enable_threads().

>+
>+    ev_file_helpers_init();
>+    ev_backends_manager_init();
>+
>+    Py_INCREF(Py_None);
>+    return Py_None;
>+}
>+
>diff --git a/evince/evincemodule.c b/evince/evincemodule.c
>new file mode 100644
>index 0000000..758d396
>--- /dev/null
>+++ b/evince/evincemodule.c
>@@ -0,0 +1,28 @@
>+#ifdef HAVE_CONFIG_H
>+#include "config.h"
>+#endif
>+
>+/* include this first, before NO_IMPORT_PYGOBJECT is defined */
>+#include <pygobject.h>
>+
>+void pyevince_register_classes (PyObject *d);
>+
>+extern PyMethodDef pyevince_functions[];
>+
>+DL_EXPORT(void)
>+initevince(void)
>+{
>+    PyObject *m, *d;
>+
>+    init_pygobject ();
>+
>+    m = Py_InitModule ("evince", pyevince_functions);
>+    d = PyModule_GetDict (m);
>+
>+    pyevince_register_classes (d);
>+    pyevince_add_constants(m, "EV_");
>+
>+    if (PyErr_Occurred ()) {

Should be just if (PyErr_Occurred ()) { return; }, to let the user see what error occurred (the exception is propagated).  That Py_FatalError is an error hiding anti-pattern (http://en.wikipedia.org/wiki/Error_hiding).

>+        Py_FatalError ("can't initialise module evince");
>+    }
>+}
>diff --git a/evince/wscript b/evince/wscript
>new file mode 100644
>index 0000000..1ca79b0
>--- /dev/null
>+++ b/evince/wscript
>@@ -0,0 +1,24 @@
>+# -*- python -*-
>+# encoding: utf-8
>+
>+def configure(conf):
>+    conf.env.append_value('MODULES_AVAILABLE', 'evince')
>+    if 'evince' in conf.env['ENABLE_MODULES'] or 'all' in conf.env['ENABLE_MODULES']:
>+        if conf.pkg_check_modules('EVINCE', 'evince pygobject-2.0 pygtk-2.0',
>+                                  mandatory=False):
>+            conf.env.append_value('MODULES_TO_BUILD', 'evince')
>+
>+
>+
>+def build(bld):
>+    
>+    if 'evince' in bld.env['MODULES_TO_BUILD']:
>+        bld.codegen('evince')
>+        pyext = bld.create_pyext()
>+        pyext.source = 'evincemodule.c evince.c'
>+        pyext.target = 'evince'
>+        pyext.uselib = 'EVINCE'
>+        pyext.install_path = '${PYTHONDIR}/gtk-2.0'
>+
>+        bld.install_files('${DATADIR}/pygtk/2.0/defs', 'evince.defs')
>+
>diff --git a/wscript b/wscript
>index 83ddcd8..6249222 100644
>--- a/wscript
>+++ b/wscript
>@@ -110,6 +110,7 @@ def configure(conf):
>     conf.sub_config('gnomeapplet')
>     conf.sub_config('gnomedesktop')
>     conf.sub_config('gnomeprint')
>+    conf.sub_config('evince')
>     conf.sub_config('evolution')
>     conf.sub_config('gtksourceview')
>     conf.sub_config('gtop')
>@@ -246,6 +247,7 @@ def build(bld):
>     bld.add_subdirs('gnomeapplet')
>     bld.add_subdirs('gnomedesktop')
>     bld.add_subdirs('gnomeprint')
>+    bld.add_subdirs('evince')
>     bld.add_subdirs('evolution')
>     bld.add_subdirs('gtksourceview')
>     bld.add_subdirs('gtop')
>-- 
>1.5.6.3
>

OK, if/when those bugs are unblocked, and the patch is fixed, and freeze break approval is obtained, I'll commit it.

Created attachment 126774 [details] [review]
comments addressed

+#include <ev-backends-manager.h>
+#include <ev-document.h>
+#include <ev-document-find.h>
+#include <ev-document-info.h>
+#include <ev-document-type-builtins.h>
+#include <ev-file-helpers.h>
+
+#include <ev-view.h>
+#include <ev-page-cache.h>
+#include <ev-jobs.h>
+#include <ev-job-scheduler.h>
+#include <ev-view-type-builtins.h>

You need to include them as <ev-backend/ev-backends-manager.h> etc and #include <ev-view/ev-view.h> etc. under the include scheme agreed on in bug 568220 comment 6.

(In reply to comment #9)
> 
> You need to include them as <ev-backend/ev-backends-manager.h> etc and #include
> <ev-view/ev-view.h> etc. under the include scheme agreed on in bug 568220
> comment 6.

Yeah, the error I pasted in bug 568220 happened after I did as you said. The missing "ev-backend/" is in ev-view.h's includes.

Created attachment 127208 [details] [review]
updated to last changes in evince

(In reply to comment #7)
> OK, if/when those bugs are unblocked, and the patch is fixed, and freeze break
> approval is obtained, I'll commit it.

I think we are finally ready ;)

http://mail.gnome.org/archives/release-team/2009-January/msg00112.html

Committed revision 529.

Before closing, just one more thing I noticed now.  Why _wrap_evince_embed_init?  Why is this stuff not in the module init function (initevince) as is usual done for python modules?

(In reply to comment #13)
> Committed revision 529.
> 
> Before closing, just one more thing I noticed now.  Why
> _wrap_evince_embed_init?  Why is this stuff not in the module init function
> (initevince) as is usual done for python modules?

Don't really know but I can move it there if you want.

I would prefer to move it there if possible.  Calling init() functions in Python usually does not make sense.  If you could test it and see if nothing bad happens, that would be great.

PS: you might need to call pygtk_init() in initevince.

Created attachment 127366 [details] [review]
let evince module be built via autotools

This patch adds support for the evince module when building with autotools, not waf.

Created attachment 127387 [details] [review]
this patch initializes evince in initevince()

(In reply to comment #16)
> Created an attachment (id=127366) [edit]
> let evince module be built via autotools
> 
> This patch adds support for the evince module when building with autotools, not
> waf.
> 

Waste of your time, but since you already spent it you might as well commit :P


(In reply to comment #17)
> Created an attachment (id=127387) [edit]
> this patch initializes evince in initevince()
> 

Minor nitpick: instead of removing evince_embed_init you could simply add this to the .override file:

%%
ignore
    evince_embed_init

Commited as rev 530.

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=127387) [edit]
> > this patch initializes evince in initevince()
> > 
> 
> Minor nitpick: instead of removing evince_embed_init you could simply add this
> to the .override file:
> 
> %%
> ignore
>     evince_embed_init

Hmm, but why do we want to keep that func? You mean so we don't break the older code that may use it?

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Created an attachment (id=127387) [edit]
> > > this patch initializes evince in initevince()
> > > 
> > 
> > Minor nitpick: instead of removing evince_embed_init you could simply add this
> > to the .override file:
> > 
> > %%
> > ignore
> >     evince_embed_init
> 
> Hmm, but why do we want to keep that func? You mean so we don't break the older
> code that may use it?
> 

No, I mean not remove it from the .defs, so that future h2def.py scanning continues to work.  But putting that ignore directive in the override file will cause the function wrapper to not be generated.  In the end the function would be gone from the Python bindings.

Created attachment 127398 [details] [review]
leave evince_embed_init in the .defs file and add pyevince_add_constants declaration

(In reply to comment #22)
> Created an attachment (id=127398) [edit]
> leave evince_embed_init in the .defs file and add pyevince_add_constants
> declaration
> 

Thank you!