Review of patch #730243 - Port to vte-2.91

[Dodji Seketeli <dodji seketeli org>]

Hello Debarshi,

First of all sorry for taking so long before looking at the patches
that you filed at https://bugzilla.gnome.org/show_bug.cgi?id=730243.

Thank you very much for taking care of porting Nemiver to vte-2.91.
This is really appreciated.

Please allow me to review these patches here as I find email more
convenient and efficient for patch reviews than a web browser.

Let's start with the firs patch titled "730243 Consolidate
GErrorSafePtr definitions".

Concerning the diff hunks in the file

diff --git a/src/common/nmv-error.h b/src/common/nmv-error.h

[...]

+struct GErrorRef {
+    void operator () (GError *a_error);
+};
+
+struct GErrorUnref {
+    void operator () (GError *a_error);
+};
+
+typedef SafePtr<GError, GErrorRef, GErrorUnref> GErrorSafePtr;

I think it's better to add these to nmv-safe-ptr-utils.h where there
are already other functors to be used with SafePtr.

I have done that in the patch set that I am proposing as a follow-up
of this message.

Likewise, the changes in the file

    diff --git a/src/common/nmv-error.cc b/src/common/nmv-error.cc


become unnecessary if the SafePtr helper functors are added to
nmv-safe-ptr-utils.h.

Likewise for the hunks in the file:

    diff --git a/src/common/Makefile.am b/src/common/Makefile.am


diff --git a/src/common/nmv-ustring.cc b/src/common/nmv-ustring.cc
index f5dfe49..076258c 100644
--- a/src/common/nmv-ustring.cc
+++ b/src/common/nmv-ustring.cc
@@ -30,6 +30,7 @@
 #include "config.h"
 #include <cstring>
 #include <sstream>
+#include "nmv-error.h"

This now becomes unnecessary as the necessary header is
nmv-safe-ptr-utils.h.  Luckily it's already included by this file.

[...]

diff --git a/src/confmgr/nmv-gconf-mgr.cc
-    b/src/confmgr/nmv-gconf-mgr.cc
index fd7ff1c..f0f97ad 100644
--- a/src/confmgr/nmv-gconf-mgr.cc
+++ b/src/confmgr/nmv-gconf-mgr.cc
@@ -24,6 +24,7 @@
  */
 #include <gconf/gconf-client.h>
 #include "nmv-i-conf-mgr.h"
+#include "common/nmv-error.h"

Likewise.

[...]


Now for the second patch entitled "730243 Port to vte-2.91 API".

Subject: [PATCH 2/2] 730243 Port to vte-2.91 API

       * configure.ac: Build against vte-2.91.
       * src/uicommon/nmv-terminal (init_body): Use
       vte_terminal_set_font instead of
       vte_terminal_set_font_from_string.
       (init_pty): vte_terminal_set_pty expects a VtePty instead of a
       fd.  Adjust according to that.
---
 configure.ac                 |  4 ++--
 src/uicommon/nmv-terminal.cc | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 04f5a26..272acd9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -353,7 +353,7 @@ dnl library dependencies for the nemiver common UI
module
 DEP_UICOMMON=" gtkmm-3.0 >= $LIBGTKMM_VERSION \
                gtk+-3.0 >= $LIBGTK_VERSION \
                gtksourceviewmm-3.0 >= $LIBGTKSOURCEVIEWMM_VERSION \
-               vte-2.90 >= $LIBVTE_VERSION"
+               vte-2.91 >= $LIBVTE_VERSION"

Shouldn't we modify the LIBVTE_VERSION version number as well?
 
[...]

diff --git a/src/uicommon/nmv-terminal.cc
 b/src/uicommon/nmv-terminal.cc
index 717c094..e7432c1 100644
--- a/src/uicommon/nmv-terminal.cc
+++ b/src/uicommon/nmv-terminal.cc

[...]
+#include "common/nmv-error.h"

This header is now unnecessary as we just need nmv-safe-ptr-utils.h
which is already indirectly included by this file.

[...]

 #include "nmv-ui-utils.h"
 
 NEMIVER_BEGIN_NAMESPACE(nemiver)
@@ -134,7 +136,8 @@ struct Terminal::Priv {
         THROW_IF_FAIL (vte);
 
         // Mandatory for vte 0.14      
-        vte_terminal_set_font_from_string (vte, "monospace");
+        Pango::FontDescription font_desc ("monospace");
+        vte_terminal_set_font (vte, font_desc.gobj());
 
         vte_terminal_set_scroll_on_output (vte, TRUE);
         vte_terminal_set_scrollback_lines (vte, 1000);
@@ -264,7 +267,14 @@ struct Terminal::Priv {
         THROW_IF_FAIL (slave_pty);
         THROW_IF_FAIL (master_pty);
 
-        vte_terminal_set_pty (vte, master_pty);
+        GError *err = 0;
+        VtePty *pty = vte_pty_new_foreign_sync (master_pty, 0, &err);
+        GErrorSafePtr error (error);
+        THROW_IF_FAIL2 (!error, error->message);
+
+        vte_terminal_set_pty (vte, pty);
+        g_object_unref (pty);

Note that we try to make the code be generally exception safe.  That
is, if an exception is raised between the first assignment to 'pty'
and this point where we g_object_unref(pty), we'll likely leak the
resources of 'pty'.  So I'd rather stick pty into a SafePtr.  The
patch I'll send as a follow-up of this message should do that.

Otherwise, the patch looks good.

Thank you very much for your time!

-- 
                Dodji