GNOME Bugzilla – Bug 691254
Add ability to view and set bookmarks
Last modified: 2013-01-29 18:10:59 UTC
It would be nice to have an easy way to access and set bookmarks within a document. These could be made available through the "places" dialog alongside the table of contents. I think it makes sense to put a bookmark overlay over the view since it is associated with the page.
Created attachment 234702 [details] [review] Use properties for name and model name
Created attachment 234703 [details] [review] Add bookmarks place
Review of attachment 234702 [details] [review]: Looks good other than this ::: src/lib/gd-places-links.c @@ +44,3 @@ EvDocument *document; EvDocumentModel *document_model; + char *name; Should be const char *
Review of attachment 234703 [details] [review]: Looks quite good, thanks! I have some comments below ::: src/application.js @@ +408,3 @@ + callback: this._onActionToggle, + state: GLib.Variant.new('b', false), + accel: 'F5', F5 sounds a bit random, but I guess it could work. Ctrl+D seems to be popular for bookmarking in web browsers, even though the action itself is a little different in this context. ::: src/documents.js @@ +1017,3 @@ return; + this.metadata = new GdPrivate.Metadata({ file: file }); Should also clear it in _clearActiveModel() ::: src/lib/gd-bookmarks.c @@ +41,3 @@ + GdMetadata *metadata; + GList *items; + guint number; Why do you save and bookkeep this instead of using g_list_length()? @@ +62,3 @@ + g_list_free_full (self->items, g_object_unref); + self->items = NULL; + } NULL is a valid value for a GList, so I don't think you need to check for != NULL here before calling g_list_free_full. @@ +141,3 @@ + char *title = NULL; + + g_variant_get (child, "(us)", &page_num, &title); I think you can use g_variant_get (child, "(u&s)") and pass a const char ** instead to avoid the extra malloc. @@ +239,3 @@ + +guint +gd_bookmarks_get_number (GdBookmarks *bookmarks) "number" is a bit of an odd name...how about "length" or "n_items"? @@ +261,3 @@ + * @bookmarks: + * + * Returns: (transfer full) (element-type GdBookmark): A list of #GdBookmark objects As implemented it's not (transfer full) but (transfer container), i.e. you copy the list, but not its contents. I think that's fine tbh, but the annotation should be corrected. @@ +278,3 @@ + + if (g_list_find_custom (bookmarks->items, bookmark, (GCompareFunc)gd_bookmark_compare)) { + return; You can use gd_bookmarks_has_bookmark() here and below in remove/update ::: src/lib/gd-places-bookmarks.c @@ +136,3 @@ + + g_free (markup); + g_object_unref (link); g_clear_object, or fold the unref in the block above @@ +480,3 @@ + priv->bookmarks = g_object_ref (bookmarks); + g_signal_connect (priv->bookmarks, "changed", + G_CALLBACK (gd_places_bookmarks_changed), Can you connect_swapped and use gd_places_bookmarks_update() as a callback directly? @@ +610,3 @@ + NULL); + + renderer = gtk_cell_renderer_text_new (); You should use GdStyledTextRenderer with 'dim-label' instead. ::: src/lib/gd-places-links.c @@ +401,2 @@ static void +gd_places_links_set_document_model (GdPlacesPage *page, Why all the renaming? ::: src/places.js @@ +61,3 @@ + vexpand: false }); + this._toolbar.get_style_context().add_class(Gtk.STYLE_CLASS_MENUBAR); + contentArea.pack_start(box, true, true, 0); Pass this to the constructor as a property ::: src/preview.js @@ +82,3 @@ + Lang.bind(this, this._onActionStateChanged)); + this._onActionStateChanged(Application.application, 'bookmark-page', Application.application.get_action_state('bookmark-page')); + this.widget.connect('destroy', Lang.bind(this, I don't think PreviewView ever gets destroyed - other things in the same constructor assume it won't AFAICS.
Created attachment 234766 [details] [review] Use properties for name and model name
Created attachment 234767 [details] [review] Add bookmarks place
Review of attachment 234766 [details] [review]: Yes
Attachment 234766 [details] pushed as 6994369 - Use properties for name and model name Attachment 234767 [details] pushed as b0eb199 - Add bookmarks place