Created attachment 232849 [details] [review]
Add a places dialog

Still need to decide where to put the action button so for now it is shown only using the F3 shortcut.

Created attachment 232868 [details] [review]
Add a places dialog

A few style and consistency fixes and avoids emitting link-activated
in the middle of the button press signal emission.

Created attachment 232871 [details] [review]
Add places button to the nav bar

Review of attachment 232868 [details] [review]:

Thanks a lot for the patch, some comments below.

::: src/lib/gd-places-links.c
@@ +95,3 @@
+
+
+                document_model = g_object_ref (self->priv->document_model);

Why do you need to ref the model around the signal emission?

@@ +140,3 @@
+                            -1);
+
+        if (link != NULL) {

I like it better to return FALSE early when link == NULL in this case.

@@ +223,3 @@
+        if (priv->model != NULL) {
+                g_clear_object (&priv->model);
+        }

g_clear_object doesn't do anything if priv->model is NULL

@@ +315,3 @@
+
+        /* Create tree view */
+        loading_model = create_loading_model ();

This might not be necessary; see my comment below.

@@ +396,3 @@
+
+        if (priv->document) {
+                gtk_tree_view_set_model (GTK_TREE_VIEW (priv->tree_view), NULL);

Since you already set the failed model if there are no links, I think it'd be better to set the loading model in the other case, while the job is loading. This way you don't need to NULL the model manually here and can just use g_clear_object()

@@ +405,3 @@
+                g_signal_handlers_disconnect_by_func (priv->job,
+                                                      job_finished_cb,
+                                                      self);

Shouldn't you call ev_job_cancel() instead?

@@ +460,3 @@
+                g_signal_handler_disconnect (priv->document_model, priv->page_changed_id);
+                priv->page_changed_id = 0;
+        }

Should also disconnect the notify::document handler maybe?

@@ +490,3 @@
+                gd_places_links_set_current_page (links,
+                                                  ev_document_model_get_page (links->priv->document_model));
+        }

Not a huge fan of doing this in _map. Does anything break if you remove the short-circuit when the widget is not mapped from gd_places_links_set_current_page() and let it run from the job callback normally without special cases?

@@ +608,3 @@
+                                                              GTK_TYPE_TREE_MODEL,
+                                                              G_PARAM_READWRITE |
+                                                              G_PARAM_STATIC_STRINGS));

I don't think the GtkTreeModel here is interesting enough to be exported as a property - I would have expected the EvDocumentModel instead. Why is it needed?

@@ +621,3 @@
+        GtkWidget *self;
+
+        self = g_object_new (GD_TYPE_PLACES_LINKS, NULL);

Nitpick: just use "return g_object_new (...);" on a single line.

::: src/lib/gd-places-page.c
@@ +28,3 @@
+#include "gd-places-page.h"
+
+G_DEFINE_INTERFACE (GdPlacesPage, gd_places_page, 0)

Using G_TYPE_INVALID instead of 0 is clearer.

@@ +77,3 @@
+
+const char *
+gd_places_page_get_icon_name (GdPlacesPage *places_page)

This appears to be unused for now, but it's fine if you plan to use it later.

@@ +94,3 @@
+gd_places_page_default_init (GdPlacesPageInterface *iface)
+{
+        static gboolean initialized = FALSE;

I don't think this can be called multiple times.

@@ +98,3 @@
+        if (!initialized) {
+                g_object_interface_install_property (iface,
+                                                     g_param_spec_object ("main-widget",

What's the purpose of this? Are we going to use it in Documents?

::: src/places.js
@@ +50,3 @@
+                                        title: "",
+                                        hexpand: true });
+        this.widget.add_button(_("Close"), Gtk.ResponseType.OK);

Should use Gtk.STOCK_CLOSE to avoid having to re-translate the string

@@ +64,3 @@
+
+        let contentArea = this.widget.get_content_area();
+        contentArea.pack_start(this._notebook, true, true, 2);

Why the additional 2px spacing here? If it's needed I'd rather have it defined in a single place as a notebook border/margin.

@@ +71,3 @@
+            this._gotoDest(link.action.dest);
+        }
+        this.widget.response(Gtk.ResponseType.OK);

Should be CLOSE if you fix the comment above.

@@ +82,3 @@
+        case EvDocument.LinkDestType.NAMED:
+            let doc = this._model.get_document();
+            let dest2 = doc.find_link_dest(dest.named);

Can this ever fail?

::: src/preview.js
@@ +39,3 @@
 const Searchbar = imports.searchbar;
 const Utils = imports.utils;
 const View = imports.view;

There should be code somewhere that makes use of gd_places_page_supports_document() and sets enabled = false on the action I think.

@@ +117,3 @@
             }));
+        let showPlaces = Application.application.lookup_action('places');
+        showPlaces.connect('activate', Lang.bind(this,

You can use

showPlaces.connect('activate', Lang.bind(this, this._showPlaces));

Review of attachment 232871 [details] [review]:

::: src/lib/gd-nav-bar.c
@@ +919,3 @@
 
+        priv->button_area = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 5);
+        gtk_widget_set_margin_right (priv->button_area, 5);

You should also set this margin on the left I think, or RTL layout will get a different spacing.

::: src/preview.js
@@ +358,3 @@
+        button_area.pack_start(this._placesButton, false, false, 0);
+
+                                              valign: Gtk.Align.CENTER

I think you can replace all this by just setting "action_name: 'app.places'" on the GtkButton (if that works, _placesButton won't need to be saved as a private member of "this" anymore, just declare it locally)

Review of attachment 232868 [details] [review]:

::: src/lib/gd-places-links.c
@@ +95,3 @@
+
+
+                document_model = g_object_ref (self->priv->document_model);

The intention was to avoid finalizing if the handler calls response or destroy.

Created attachment 233022 [details] [review]
Add a places dialog

Created attachment 233023 [details] [review]
Add places button to the nav bar

Review of attachment 233023 [details] [review]:

This looks good now.

Created attachment 233024 [details] [review]
Add a places dialog

Review of attachment 233024 [details] [review]:

Looks good now thanks!