Re: word wrap for logview

[Emmanuele Bassi <ebassi gmail com>]

On Fri, 2008-08-22 at 00:35 +0200, David Hugh-Jones wrote:
> This patch (not very beautiful, apologies) adds word wrap to logview.
> It also shows whole lines in tooltips.

I can appreciate that logview does not have a consistent coding style,
but please: avoid introducing something completely alien like this:

+static void logview_scrollwindow_cb (GtkScrolledWindow *sw, GtkAllocation *al,
+                                LogviewWindow *logview) {
+    logview_update_grid_width(logview, al->width); 
+}

it makes reviewing painful - and makes logview even worse to maintain.

that should be:

static void
logview_scrollwindow_cb (GtkScrolledWindow *sw,
                         GtkAllocation     *allocation,
                         LogviewWindow     *logview)
{
  logview_update_grid_width (logview, allocation->width);
}

C is not for cuddly, touchy-feely variable names, but seriously: "al"?
come on. ;-)

+    logview->wordwrap = gtk_toggle_action_get_active(GTK_TOGGLE_ACTION (action));
+    logview_update_grid_width(logview, -1);
+    logview_save_prefs (logview);

and please: consistency when using the parentesis; preferably, use a
space between function name and arguments - this also stands for macros.

+    if (!gtk_tree_view_get_tooltip_context(tree_view, &x, &y,
keyboard_mode, 
+              &model, &path, &iter)) return FALSE;

newline after the if.

@@ -932,8 +1009,12 @@ logview_init (LogviewWindow *logview)
                      G_CALLBACK (row_toggled_cb), logview);
    g_signal_connect (G_OBJECT (logview->view), "row-collapsed",
                      G_CALLBACK (row_toggled_cb), logview);
+   g_signal_connect (G_OBJECT(logview->view), "query-tooltip", 
+                     G_CALLBACK(query_tooltip_cb), NULL);
    g_signal_connect (G_OBJECT (logview), "configure_event",
                      G_CALLBACK (window_size_changed_cb), logview);
+   g_signal_connect (G_OBJECT (logview->scrolled), "size-allocate",
+                     G_CALLBACK (logview_scrollwindow_cb), logview);

do not cast to GObject - it's useless (g_signal_connect() takes a
gpointer) and might mask errors. yes, the existing lines do it: they
should not.

+    if (logview->wordwrap) {
+        if (w == -1) w =
GTK_WIDGET(logview->scrolled)->allocation.width;
+        if (w > 100) w = w - 50; // hack to avoid the scrollbar and
first column
+        g_object_set(G_OBJECT(cell_list->data), "wrap-width", w,
+                                "wrap-mode", PANGO_WRAP_WORD_CHAR,
+                                NULL);
+    }

this is really fugly (not just for the coding style), and will probably
break for some combination of font-size and theme; what unit is the
cut-off value of "100"? pixels? why one hundred pixels and not 80
characters at the current font size and dpi?

also you never check if cell_list is NULL or if data is NULL.

ciao,
 Emmanuele.

-- 
Emmanuele Bassi,
W: http://www.emmanuelebassi.net
B: http://log.emmanuelebassi.net