Created attachment 232786 [details] [review]
Add a new page navigation bar

Review of attachment 232786 [details] [review]:

Thanks for the patch! I agree this would be great to have.
Can you also remove GdThumbNav and GdSidebarThumbnails in a later patch?

I have some comments on the code below.

::: src/lib/gd-nav-bar.c
@@ +47,3 @@
+        gint width;
+        gint height;
+} EvThumbsSize;

Might as well use a GtkRequisition instead of a custom struct.

@@ +54,3 @@
+        gint uniform_height;
+        EvThumbsSize *sizes;
+} EvThumbsSizeCache;

Can you rename this and the matching methods to use the Gd prefix instead?

@@ +230,3 @@
+        char *text;
+
+        text = g_strdup_printf ("Page %u of %u", self->priv->current_page + 1, self->priv->n_pages);

This needs to be marked as translatable.

@@ +277,3 @@
+        item->pixbuf = pixbuf;
+        item->loaded = TRUE;
+        item->job = NULL;

Don't you need to unref item->job here too?

@@ +301,3 @@
+                        ev_job_cancel (item->job);
+                        g_object_unref (item->job);
+                        item->job = NULL;

Use g_clear_object()

@@ +400,3 @@
+                item->pixbuf = preview_get_loading_icon (self, width, height);
+                item->loaded = FALSE;
+                item->job = NULL;

Shouldn't this be item->job = job?

@@ +511,3 @@
+                return;
+
+        priv->model = g_object_ref (model);

Since this can be set multiple times, you should unref priv->model if it's != NULL, and disconnect the notify::document signal.

@@ +591,3 @@
+
+        gtk_style_context_save (context);
+        gtk_style_context_set_state (context, gtk_widget_get_state_flags (widget));

This (and so the save/restore calls) shouldn't be needed, since you never touch the widget state manually AFAICS.

@@ +610,3 @@
+{
+        GObjectClass *oclass = (GObjectClass *) class;
+        GtkWidgetClass *wclass = (GtkWidgetClass *) class;

Use G_OBJECT_CLASS and GTK_WIDGET_CLASS for these casts

@@ +711,3 @@
+        show_preview (self);
+
+        self->priv->update_id = 0;

Should be self->priv->show_id = 0;

@@ +766,3 @@
+           coming soon */
+        if (self->priv->show_id == 0) {
+                self->priv->show_id = g_timeout_add (300, (GSourceFunc)show_preview_timeout, self);

Coding style: some parts of the the file uses no braces for single-line if blocks and some parts use braces. With tabs or 8-space indentation, I prefer using always braces, but either way we should be consistent.

::: src/preview.js
@@ -73,3 @@
-        this._thumbBar.view.connect('selection-changed', Lang.bind(this,
-            function() {
-                this._thumbSelectionChanged = true;

I think we still need to account for this with the new widget, no? I see the controls sometimes disappearing when sliding, which might be related.

@@ +330,3 @@
     _init: function(model) {
+        this.widget = new GdPrivate.NavBar({ document_model: model });
+        //this.widget.get_style_context().add_class('osd');

I guess we don't really support styling an OSD range like that? Anyway, feel free to remove it entirely until theming is ready.

Created attachment 232791 [details] [review]
Add a new page navigation bar

Created attachment 232792 [details] [review]
Remove old thumbnail bar

Should address all the comments. Some of those issues were apparently inherited from the code I copied out of the old thumb bar.

I added back the osd style. But I don't really like it. I'm not really liking it in general any more. But we should change it globally not here only.

Review of attachment 232791 [details] [review]:

I still have a couple more minor comments

::: src/lib/gd-nav-bar.c
@@ +283,3 @@
+
+        /* check to see if preview needs updating */
+        if (self->priv->preview_page == job->page && pixbuf != NULL) {

Can it happen that pixbuf == NULL here? AFAICS we would possibly fail previously when calling e.g. ev_document_misc_invert_pixbuf(). If it's a case that can happen, we should either have a fallback icon or consistently check for pixbuf != NULL.

@@ +417,3 @@
+
+                g_clear_object (&item->job);
+                g_clear_object (&item->pixbuf);

item->label is an allocated string that we're never free-ing - I think I'd like to have a preview_item_free() function that does all this.

@@ +491,3 @@
+        /* Connect to the signal and trigger a fake callback */
+        g_signal_connect (priv->model, "page-changed",
+                          G_CALLBACK (page_changed_cb),

I'd rather use g_signal_connect_swapped() with update_page as a callback directly here.

@@ +517,3 @@
+
+        if (priv->model != NULL) {
+                g_signal_handlers_disconnect_by_func (priv->model, "notify::document", self);

This should be: g_signal_handlers_disconnect_by_func (priv->model, gd_nav_bar_document_changed_cb, self);
I think you should also disconnect the handlers you connect on priv->model in gd_nav_bar_document_changed_cb; it might be better to connect all the handlers to the model in this function instead.

@@ +602,3 @@
+        context = gtk_widget_get_style_context (widget);
+
+        gtk_style_context_set_state (context, gtk_widget_get_state_flags (widget));

This still shouldn't be needed. Does anything break if you remove it?

::: src/preview.js
@@ +317,3 @@
             this._fsToolbar.setModel(model);
+            this._model.connect('page-changed',
+                                Lang.bind(this,

Nitpick: indentation should be

this._model.connect('page-changed', Lang.bind(this,
  function() {
    // do stuff
  }));

Review of attachment 232792 [details] [review]:

Looks good once the other one is ready.

Created attachment 232809 [details] [review]
Add a new page navigation bar

Fix comments and a few other cleanups.

Review of attachment 232809 [details] [review]:

Other than this, looks good to commit.

::: src/lib/gd-nav-bar.c
@@ +598,3 @@
+        }
+
+        g_clear_object (&self->priv->model);

Should also clear self->priv->document since we now take a ref.

Thanks for the good reviews!