After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 633501 - Bookshelf home screen
Bookshelf home screen
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks: document-centric
 
 
Reported: 2010-10-29 20:41 UTC by Pavel Panchekha
Modified: 2014-07-25 09:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to add bookshelf view for recent documents (93.60 KB, patch)
2013-06-20 15:32 UTC, aakash
needs-work Details | Review
Patch to add bookshelf view for recent documents (93.31 KB, patch)
2013-06-20 15:37 UTC, aakash
none Details | Review
0001 Added libgd to cut-n-paste (69.14 KB, patch)
2013-07-24 10:37 UTC, aakash
committed Details | Review
0002 Improved implementation of bookshelf (53.29 KB, patch)
2013-07-24 10:37 UTC, aakash
needs-work Details | Review
0003 Fixed a problem with visibility of find-bar (1.15 KB, patch)
2013-07-24 10:37 UTC, aakash
none Details | Review
Screenshot that shows the consequence of the bookshelf button removal (431.01 KB, image/png)
2013-09-12 18:46 UTC, Germán Poo-Caamaño
  Details
Updated patch of recent view (aka bookshelf) (26.71 KB, patch)
2014-02-24 04:46 UTC, Germán Poo-Caamaño
committed Details | Review
Integration of recent-view into Evince's shell (9.22 KB, patch)
2014-02-24 04:47 UTC, Germán Poo-Caamaño
needs-work Details | Review
Implementation of an alternative/extra toolbar for the recent view (18.68 KB, patch)
2014-02-24 04:47 UTC, Germán Poo-Caamaño
needs-work Details | Review
Removal of the recent-view menu (the old code) (17.22 KB, patch)
2014-02-24 04:48 UTC, Germán Poo-Caamaño
none Details | Review
printscreen (110.93 KB, image/png)
2014-02-24 22:59 UTC, Bogdan Petcu
  Details
Setting the "follow-state" property on the pixbuf cell renderer (823 bytes, patch)
2014-04-14 23:35 UTC, Bogdan Petcu
reviewed Details | Review
libgd: Highlight items on mouse-hover (926 bytes, patch)
2014-05-04 16:55 UTC, Germán Poo-Caamaño
committed Details | Review
recent-view: Do not propagate button_press_event in recent view (889 bytes, patch)
2014-05-04 16:56 UTC, Germán Poo-Caamaño
committed Details | Review
shell: Integrate recent view into the main window (9.20 KB, patch)
2014-05-04 16:58 UTC, Germán Poo-Caamaño
none Details | Review
ev-recent-view-toolbar: Implement a different toolbar for recent view (11.08 KB, patch)
2014-05-04 16:59 UTC, Germán Poo-Caamaño
none Details | Review
ev-window: Integrate the recent view toolbar (12.52 KB, patch)
2014-05-04 17:01 UTC, Germán Poo-Caamaño
none Details | Review
ev-window: Removed the recent menu list (17.23 KB, patch)
2014-05-04 17:01 UTC, Germán Poo-Caamaño
none Details | Review
shell: Make a class for EvToolbar to abstract for other toolbars (28.21 KB, patch)
2014-05-07 06:47 UTC, Germán Poo-Caamaño
none Details | Review
ev-recent-view-toolbar: Implement a different toolbar for recent view (28.21 KB, patch)
2014-05-07 07:50 UTC, Germán Poo-Caamaño
none Details | Review
recent-view: Verify a path exists before adding an icon (1.56 KB, patch)
2014-05-08 07:43 UTC, Germán Poo-Caamaño
committed Details | Review
ev-recent-view-toolbar: Implement a different toolbar for recent view (9.40 KB, patch)
2014-05-10 19:38 UTC, Germán Poo-Caamaño
none Details | Review
ev-window: Integrate the recent view toolbar (12.08 KB, patch)
2014-05-10 19:40 UTC, Germán Poo-Caamaño
none Details | Review
shell: Integrate recent view into the main window (9.39 KB, patch)
2014-06-25 06:18 UTC, Germán Poo-Caamaño
needs-work Details | Review
ev-toolbar: Implement toolbar with multiple modes (8.91 KB, patch)
2014-06-25 06:22 UTC, Germán Poo-Caamaño
needs-work Details | Review
shell: Remove menu for recent items (3.77 KB, patch)
2014-06-25 06:22 UTC, Germán Poo-Caamaño
committed Details | Review
ev-toolbar: Implement toolbar with multiple modes (7.50 KB, patch)
2014-07-01 20:14 UTC, Germán Poo-Caamaño
reviewed Details | Review
shell: Integrate recent view into the main window (12.81 KB, patch)
2014-07-01 20:15 UTC, Germán Poo-Caamaño
reviewed Details | Review
ev-toolbar: Implement toolbar with multiple modes (9.37 KB, patch)
2014-07-24 05:11 UTC, Germán Poo-Caamaño
reviewed Details | Review
shell: Integrate recent view into the main window (12.04 KB, patch)
2014-07-24 05:13 UTC, Germán Poo-Caamaño
needs-work Details | Review
recent-view: Integrate recent view in the shell (27.27 KB, patch)
2014-07-24 21:28 UTC, Germán Poo-Caamaño
none Details | Review
recent-view: Integrate recent view in the shell (27.68 KB, patch)
2014-07-25 06:20 UTC, Germán Poo-Caamaño
none Details | Review
recent-view: Integrate recent view in the shell (28.15 KB, patch)
2014-07-25 06:32 UTC, Germán Poo-Caamaño
none Details | Review
recent-view: Integrate recent view in the shell (28.15 KB, patch)
2014-07-25 06:38 UTC, Germán Poo-Caamaño
committed Details | Review

Description Pavel Panchekha 2010-10-29 20:41:14 UTC
Currently, starting evince without passing an argument opens a completely empty home screen. If, instead, a "bookshelf" of files were displayed (somewhat like the iOS PDF viewer), it would be much easier for users to read commonly-used files. For example, I commonly use the Ubuntu manual, but it is a hassle to find it in Nautilus and then double-click it to read it. It would be much simpler to open Evince and select the Manual from a home screen.

The bookshelf could just be collected from all of the files in a certain directory (~/Books, for example).
Comment 1 Carlos Garcia Campos 2010-10-30 13:08:45 UTC
Good idea, I think we could just use the recent document list.
Comment 2 andruk.tatum 2011-02-04 23:43:12 UTC
If the user doesn't have any recently opened documents, but does have some ebooks in ~/Books, ~/EBooks, or ~/Library or someplace like that, I think it would be a good idea to display thumbnails the Evince-openable files in those directories in the home screen.  That way Evince is still useful even though the user has no recently opened documents (or the recent documents list has been recently deleted).

This is a good idea even without my addition though!
Comment 3 Federico Mena Quintero 2011-10-18 16:42:27 UTC
"Recently downloaded" (e.g. ~/Downloads/*.pdf) would be very useful as well :)
Comment 4 William Jon McCann 2011-10-18 17:23:14 UTC
In the past I've advocated for this feature as well. However, I think the bookshelf app is going to be Documents. I'd like to see Evince and evolve into a just a simple viewer plugin for viewing documents directly in a web browser. Or used as a library from Sushi or Documents.
Comment 5 Cosimo Cecchi 2011-10-18 18:31:42 UTC
(In reply to comment #4)
> In the past I've advocated for this feature as well. However, I think the
> bookshelf app is going to be Documents. I'd like to see Evince and evolve into
> a just a simple viewer plugin for viewing documents directly in a web browser.
> Or used as a library from Sushi or Documents.

Yeah, Documents is an improved version of this bookshelf idea, so I don't think it makes sense to duplicate it into Evince.
I still think Evince as a standalone viewer makes sense though, mostly for opening files not indexed by Documents, or just double clicking a PDF in Nautilus, but I think I'd like to make it less prominently a standalone app (e.g. having a launcher in the shell application list for it, launching an empty viewer doesn't really make sense to me).
Comment 6 José Aliste 2013-04-28 01:19:23 UTC
Ok, since we removed the "NoDisplay=True" from the evince desktop file, I think it makes sense to finally fix this bug. This is not going to duplicate Gnome-Documents feature since Documents shows everything you have indexed and also recently opened, evince would only show recently opened.
Comment 7 William Jon McCann 2013-04-29 17:10:00 UTC
My recommendation would be to split Evince into two separate projects. 1. Move the document rendering part into a "preview" library that Epiphany, Sushi, and GTK can use 2. Make the app more like Acrobat with editing and form filling features.
Comment 8 aakash 2013-06-20 15:32:12 UTC
Created attachment 247335 [details] [review]
Patch to add bookshelf view for recent documents

Hi,

I have made this patch for adding the bookshelf view. I have used libgd for making the bookshelf. This patch is definitely not ready for shipping yet. So, I would like your comments on improving it.

Thanks!
Comment 9 aakash 2013-06-20 15:37:22 UTC
Created attachment 247337 [details] [review]
Patch to add bookshelf view for recent documents

Removed some unused code.
Comment 10 José Aliste 2013-06-20 16:39:55 UTC
Review of attachment 247335 [details] [review]:

so this is a review of the ev-window part of the patch, the widget will be reviewed later.

::: shell/ev-bookshelf.c
@@ +94,3 @@
+	G_OBJECT_CLASS (ev_bookshelf_parent_class)->dispose (obj);
+}
+

there is no need to override the set_property and get_property when there are no properties

::: shell/ev-window.c
@@ +7533,3 @@
+	g_signal_connect_object (ev_window->priv->bookshelf,
+	                         "item-activated",
+	gtk_container_add (GTK_CONTAINER (ev_window->priv->scrolled_window),

In ev_window_init() we do not know whether evince has been called with a Document or not... I don't like the approach, because on every call to evince you are basically creating the BookshelfWidget and then throwing it away just some milliseconds later because the window will load a document. 

The only case where the Bookshelf should be created is when Evince is launched without any files to load. This is achieved in ev-application with the ev_application_open_window() which just creates an empty EvWindow. So,ev_application_open_window() is only called in this case, so the approach I'd like to see is to make ev_application_open_window() to create the bookshelf. For this you will probably need a new function on ev-window. Something like ev_window_new_with_bookshelf() that creates a new ev_window and then creates the bookshelf. In this way, you could make the bookshelf to go away just after the load job of the document on the activated item has finished.
Comment 11 Carlos Garcia Campos 2013-06-20 17:06:25 UTC
I'm not sure, ideally the bookshelf view would replace the recent menu list, so we would always need it. Maybe it could be created on demand when evince is opened with a document. I think we should keep the metadata in the gvfs metadata and the thumbnail in the thumbnails cache, so that we don't need to load all recent files at start up. Also note that we only have one worker thread, so we can't load all documents in parallel. We can use evince-thumbnailer to create the thumbnails so that it can run in parallel. Getting the metadata from gio can also be run in parallel.
Comment 12 José Aliste 2013-06-20 21:19:26 UTC
oh, I haven't though about UI for replacing the recent list before so you have a point...but...  how would that work? In google-chrome, you get the bookshelf of recent pages whenever you open a new tab or new window. 

So one way of getting this list would be to have a "see bookshelf or see recent documents" action. Then I think this should open a new empty window with the bookshelf. 

I don't think we want to close the current Document when going to the bookshelf (and thus keep the same window) but   I am not sure.
Comment 13 Carlos Garcia Campos 2013-06-21 07:50:17 UTC
I would use a button in the toolbar to load the bookshelf in the same window, marking the currently opened document somehow. If a different document is selected it's loaded in a new window (the same that when you select a file from the recent list), if the current document is selected or if evince was opened without a document the selected document is opened in the existing window (and again this is what we currently do with the recent list menu). So, I don't think this should be a new window, but rather like a back button, similar to the pages button in the Epiphany mockups (https://github.com/gnome-design-team/gnome-mockups/raw/master/web/web-webpage.png).
Comment 14 Germán Poo-Caamaño 2013-07-09 06:44:44 UTC
Regarding to the current implementation (up to https://gitorious.org/aakash-evince-repo/gsoc2013/commit/ea6b7c7517e885a7d3f62590b1a464470b102099):

I noticed the thumbnails that do not exist are generated when it the bookshelf is loaded.  It is ok for the first time is loaded.

However, they could be created once the document has been loaded. This is noticeable in documents where first page takes a while to render.

To check it:
1. Grab http://ubuntuone.com/30y4VcyBtQb0CTZBVHicSn
2. Open it with evince
3. Open other documents.
4. Close all evince windows.
5. Open a new evince without any document

Results:
The thumbnail of the document in 1 will take time to be shown.

Expected results:
The thumbnail should be immediately show (the page was already rendered a couple of minutes ago).

Once the thumbnail exists, it is loaded pretty fast.  But if evince opens the documents and adds it to the list of recent opened document, why not adding immediately the thumbnail?
Comment 15 Germán Poo-Caamaño 2013-07-09 06:53:29 UTC
Some comments I made on IRC but I leaving them here to not be forgotten:

1. There is a critical warning that appears twice every time
   the bookshelf is shown:

(evince:4292): Gtk-CRITICAL **: gtk_range_get_adjustment: assertion `GTK_IS_RANGE (range)' failed

2. The button in the toolbar is enabled by default when opening
   evince without documents.  Clicking the button hides the
   bookshelf. Clicking it again will show the bookshelf and
   disable the button.  This behaviour seems odd.

3. Highlight the document where the cursor is over (as nautilus
   does).  This would give feedback to the user regarding the
   document to be opened.  Because the document is opened at
   the first click, it caught my as surprise.
Comment 16 Carlos Garcia Campos 2013-07-21 10:47:00 UTC
I've been trying it out too, it looks pretty good in general, but I have some comments:

  - I think we should use a different toolbar when in bookshelf view, since having the current document toolbar with almost everything disabled doesn't make sense to me. It could be a very simply toolbar with close and open options.

  - The bookshelf button should not be in the bookshelf toolbar, the current active document should be highlighted somehow and to go back to the document view you would just click on it.

  - I'm not sure EvBookshelf is the better name, I would use something like EvRecentView, gnome-documents is more the bookshelf to me, this is just a recent document view. 

  - It should replace the recent menu, so it should be removed.

  - The critical warning is because you are trying to use the scrolled window before it has been constructed. Do not call ev_bookshelf_rebuild in init method, use the constructed one instead.

  - Using _("") is not a good idea, it's not a translatable string and I've ended up with something like this:

metadata::evince::author: Project-Id-Version: evince.master
Report-Msgid-Bugs-To: http://bugzilla.gnome.org/enter_bug.cgi?product=evince&keywords=I18N+L10N&component=general
POT-Creation-Date: 2013-06-08 23:47+0000
PO-Revision-Date: 2013-06-12 13:12+0200
Last-Translator: Daniel Mustieles <daniel.mustieles@gmail.com>
Language-Team: Español <gnome-es-list@gnome.org>
Language: 
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Plural-Forms: nplurals=2; plural=(n!=1);
X-Generator: Gtranslator 2.91.5

in the metada of my recent files that don't have an author.

  - It seems that thumbnails are cached in /tmp and it's not cleaned when evince finishes. And now I also have all my recent files with a metadata key pointing to a file in /tmp:

metadata::evince::thumbnail-path: /tmp/lt-evince-6991/thumb.2LC9ZW

  - We could use the libgnome-desktop as an optional dependency to cache the thumbnails properly, instead of using the evince metadata.

  - To get a proper review, please, rebase your patches, squash them when appropriate, clean them up, and submit them here for review.
Comment 17 Germán Poo-Caamaño 2013-07-21 17:37:37 UTC
(In reply to comment #16)
> I've been trying it out too, it looks pretty good in general, but I have some
> comments:
> [...]
>   - It should replace the recent menu, so it should be removed.

Maybe we should replace the menu to something like "Show recent items view" or similar.  Otherwise, the recent menu would be only accessible with the toolbar.
Comment 18 Carlos Garcia Campos 2013-07-21 17:57:38 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > I've been trying it out too, it looks pretty good in general, but I have some
> > comments:
> > [...]
> >   - It should replace the recent menu, so it should be removed.
> 
> Maybe we should replace the menu to something like "Show recent items view" or
> similar.  Otherwise, the recent menu would be only accessible with the toolbar.

And what's the problem? The menu is only accessible with the toolbar too. We don't have options duplicated in toolbar and menu anymore.
Comment 19 aakash 2013-07-24 10:37:31 UTC
Created attachment 250016 [details] [review]
0001 Added libgd to cut-n-paste

Hi

I am attaching 3 patches for an improved implementation of the EvRecentView (as it is now called).

Most of the suggestions by gpoo and KaL have been taken care of. The following things remain

  - Keep libgnome-desktop as an optional dependency and make use of it when available. And also, don't save the thumbnails in /tmp, and keep the metadata clean. 

  - Mark the currently open document when in bookshelf/recent view

  - Highlight an item on mouse hover

I have rebased everything, and squashed into 3 commits/patches -

0001 Added libgd to cut-n-paste - Jose pointed out that we might want to use libgd as a submodule rather in cut-n-paste. Hence, the addition of libgd to cut-n-paste is in a separate commit so that it can be undone easily.

0002 Improved implementation - As the name suggests, it is a revised and improved version of the bookshelf/recent view

0003 Fixed a problem where the find-bar would be displayed while in the recent-view

I am working on the remaining 3 things. It would be great if this could be reviewed in the meanwhile.

Thanks!
Comment 20 aakash 2013-07-24 10:37:44 UTC
Created attachment 250017 [details] [review]
0002 Improved implementation of bookshelf
Comment 21 aakash 2013-07-24 10:37:50 UTC
Created attachment 250018 [details] [review]
0003 Fixed a problem with visibility of find-bar
Comment 22 Carlos Garcia Campos 2013-07-28 11:14:59 UTC
(In reply to comment #19)
> Created an attachment (id=250016) [details] [review]
> 0001 Added libgd to cut-n-paste
> 
> Hi
> 
> I am attaching 3 patches for an improved implementation of the EvRecentView (as
> it is now called).

Thanks!

> Most of the suggestions by gpoo and KaL have been taken care of. The following
> things remain
> 
>   - Keep libgnome-desktop as an optional dependency and make use of it when
> available. And also, don't save the thumbnails in /tmp, and keep the metadata
> clean. 

Maybe we don't really need to use libgnome-desktop at all. To read the thumbnails we can easily use the gio API (G_FILE_ATTRIBUTE_THUMBNAIL_PATH), and we can use evince to create the thumbnails, and the custom code to store it in the cache (pretty simple that we can just copy and paste from libgnome-desktop).

>   - Mark the currently open document when in bookshelf/recent view
> 
>   - Highlight an item on mouse hover
> 
> I have rebased everything, and squashed into 3 commits/patches -
> 
> 0001 Added libgd to cut-n-paste - Jose pointed out that we might want to use
> libgd as a submodule rather in cut-n-paste. Hence, the addition of libgd to
> cut-n-paste is in a separate commit so that it can be undone easily.

Yes, and makes the review easier :-)

> 0002 Improved implementation - As the name suggests, it is a revised and
> improved version of the bookshelf/recent view
> 
> 0003 Fixed a problem where the find-bar would be displayed while in the
> recent-view
> 
> I am working on the remaining 3 things. It would be great if this could be
> reviewed in the meanwhile.
> 
> Thanks!

I'll review the current patch.
Comment 23 Carlos Garcia Campos 2013-07-28 14:49:41 UTC
Review of attachment 250017 [details] [review]:

Looks much better, thanks!

::: shell/Makefile.am
@@ +56,3 @@
 	ev-metadata.h			\
+	ev-minimal-toolbar.c		\
+	ev-minimal-toolbar.h		\

I would call this ev-recent-view-toolbar instead

::: shell/ev-minimal-toolbar.c
@@ +37,3 @@
+};
+
+G_DEFINE_TYPE (EvMinimalToolbar, ev_minimal_toolbar, GTK_TYPE_TOOLBAR)

You should make current EvToolbar an abstract base class with all the common code, and this one and current EvToolbar (that should be renamed as EvMainToolbar or something like that) would inherit from it.

@@ +156,3 @@
+	gtk_widget_show (button);
+	gtk_container_add (GTK_CONTAINER (ev_minimal_toolbar), tool_item);
+	gtk_widget_show (tool_item);

What about moving this buttons to a gear menu for consistency with the main toolbar?

::: shell/ev-minimal-toolbar.h
@@ +49,3 @@
+};
+
+GType      ev_minimal_toolbar_get_type           (void);

Leave only one space between function name and arguments for the longest function.

::: shell/ev-recent-view.c
@@ +43,3 @@
+struct _EvRecentViewPrivate {
+	GtkWidget         *view;
+	GtkListStore      *list_store;

Call this model instead.

@@ +45,3 @@
+	GtkListStore      *list_store;
+	GtkRecentManager  *recent_manager;
+	gchar             *button_press_item_path;

Name is a bit confusing, I had to read the code to understand what this was. What about pressed_time_path? And why not saving the GtkTreePath so that it's even clear this is a path in a treeview and not a filename?

@@ +49,3 @@
+
+enum {
+	ITEM_ACTIVATED = 1,

Why do you need to start with 1 this enum?

@@ +74,3 @@
+ev_recent_view_dispose (GObject *obj)
+{
+	EvRecentView *self = EV_RECENT_VIEW (obj);

Use ev_recent_view or just recent_view instead of self, we don't typically use self in evince code.

@@ +85,3 @@
+		g_signal_handlers_disconnect_by_func (self->priv->recent_manager,
+		                                      ev_recent_view_refresh,
+		                                      self);

We could save the signal id, and use g_signal_handler_disconnect.

@@ +99,3 @@
+	switch (prop_id) {
+	default:
+		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);

Do not add a set_property implementation if the object doesn't have properties.

@@ +104,3 @@
+
+static gboolean
+metadata_is_stale (EvMetadata *metadata, GFile *file)

What do you need this for?

@@ +142,3 @@
+	gchar     *thumbnail_path = NULL;
+	
+	thumbnail_file = ev_mkstemp_file ("thumb.XXXXXX", &error);

Don't save the thumbnail unless you are going to cache it.

@@ +153,3 @@
+				 "png", &error, NULL);
+		if (!error)
+			ev_metadata_set_string (metadata, "thumbnail-path", thumbnail_path);

Do not save the thumbnail in path in metadata, even less if the path is in /tmp. We should use the thumbs cache or not cache at all.

@@ +169,3 @@
+	switch (prop_id) {
+	default:
+		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);

Do not add a get_property implementation if the object doesn't have properties.

@@ +186,3 @@
+		ev_job_cancel (job);
+		g_signal_handlers_disconnect_by_func (job, thumbnail_job_completed_callback, data);
+		g_signal_handlers_disconnect_by_func (job, document_load_job_completed_callback, data);

I don't think this is correct, the job instance is not the same for both callbacks.

@@ +230,3 @@
+get_generic (EvRecentView *self)
+{
+	if (self->priv->view != NULL)

How can this be NULL if it's created on widget construction? Do we really need a function just to cast GtkWidget to GdMainViewGeneric?

@@ +239,3 @@
+on_button_release_event (GtkWidget *view,
+                         GdkEventButton *event,
+                         gpointer user_data)

You can use EvRecentView * directly here.

@@ +256,3 @@
+		button_release_item_path = gtk_tree_path_to_string (path);
+		if (g_strcmp0 (self->priv->button_press_item_path, button_release_item_path) == 0)
+			same_item = TRUE;

In what conditions can the item in press be different than the item in release? Drag and drop? If you save the GtkTreePath so don't need to duplicate the newly created strings and you can use gtk_tree_path_compare

@@ +265,3 @@
+
+	if (!same_item)
+		res = FALSE;

I prefer an early return here, 

if (!same_item) {
        gtk_tree_path_free (path);
        return FALSE;
}

@@ +270,3 @@
+		gchar *uri;
+
+		if (self->priv->list_store == NULL)

How can be list_store NULL here? it's created in the init method and deleted in dispose.

@@ +274,3 @@
+
+		if (!gtk_tree_model_get_iter (GTK_TREE_MODEL (self->priv->list_store), &iter, path))
+			goto exit;

I prefer if (foo) { code } instead of the goto here.

@@ +323,3 @@
+	border.right = 3; 
+	border.top = 3;
+	border.bottom = 6;

Where do these numbers come from? Shouldn't they be taken from the theme?

@@ +325,3 @@
+	border.bottom = 6;
+
+	pixbuf = ev_document_misc_render_thumbnail_with_frame (GTK_WIDGET (ev_recent_view), job->thumbnail);

Why do you render the thumbnail with a frame if you are going to use the thumbnail-frame.png?

@@ +362,3 @@
+	EvMetadata         *metadata;
+
+	document = job_load->parent.document;

This should be EV_JOB (job_load)->document.

@@ +378,3 @@
+		gdouble          scale;
+
+		model = ev_document_model_new_with_document (document);

What do you need the model for?

@@ +379,3 @@
+
+		model = ev_document_model_new_with_document (document);
+		page = ev_document_model_get_page (model);

This is always 0 for a newly created model

@@ +385,3 @@
+
+		scale = (gdouble)ICON_VIEW_SIZE / height < (gdouble)ICON_VIEW_SIZE / width ? 
+			(gdouble)ICON_VIEW_SIZE / height : (gdouble)ICON_VIEW_SIZE / width;

Save these in local variables to improve the readability. thumb_width, thum_height or something like that.

@@ +388,3 @@
+		job_thumbnail = ev_job_thumbnail_new (document,
+		                                      page,
+		                                      ev_document_model_get_rotation (model),

This is always 0 for a newly created model.

@@ +412,3 @@
+
+		if (metadata) { 
+			ev_metadata_set_string (metadata, "author", info->author == NULL ? "" : info->author);

Never set empty values in metadata, simply don't save it if not present.

@@ +428,3 @@
+			GdkPixbuf *thumbnail;
+
+			ev_metadata_set_string (metadata, "author", "");

Ditto.

@@ +441,3 @@
+			g_object_unref (metadata);
+			g_object_unref (thumbnail);
+		}

I wouldn't change the metadata at all if the document failed to load.

@@ +456,3 @@
+	items = g_list_sort (items, (GCompareFunc) compare_recent_items);
+
+	gtk_list_store_clear (ev_recent_view->priv->list_store);

Maybe we could try to not reload all documents when recent files change. Iterating the recent list and only adding the new item if needed or something similar.

@@ +498,3 @@
+		load_document:
+
+			author = "";

Why not using NULL instead of ""?

@@ +500,3 @@
+			author = "";
+			thumbnail = gtk_recent_info_get_icon (info, ICON_VIEW_SIZE);
+			job_load = ev_job_load_new (uri);

It's unfortunate and complex that we need to two jobs for getting author and thumbnail. I think we could add a custom job that loads the document, creates the thumbnail and returns the doc info. It simplifies this codee and it's also more efficient.

@@ +510,3 @@
+
+		gtk_list_store_set (ev_recent_view->priv->list_store, &iter,
+		                    GD_MAIN_COLUMN_ID, _("id"),

What's this id and why it needs to be translated?

@@ +545,3 @@
+
+static void
+ev_recent_view_rebuild (EvRecentView *ev_recent_view)

Why is this called rebuild? it's only used once the first time the object is constructed. I think you can get rid of this confusing method and add this code directly to constructed.

@@ +574,3 @@
+{
+	EvRecentView *ev_recent_view = EV_RECENT_VIEW (object);
+	G_OBJECT_CLASS (ev_recent_view_parent_class)->constructed (object);

Leave an empty line between declarations block and the code.

@@ +575,3 @@
+	EvRecentView *ev_recent_view = EV_RECENT_VIEW (object);
+	G_OBJECT_CLASS (ev_recent_view_parent_class)->constructed (object);
+	ev_recent_view_rebuild (ev_recent_view);

I think you can move the current rebuild code to the init method and simply call ev_recent_view_refresh() here.

@@ +583,3 @@
+	ev_recent_view->priv = G_TYPE_INSTANCE_GET_PRIVATE (ev_recent_view, EV_TYPE_RECENT_VIEW, EvRecentViewPrivate);
+	ev_recent_view->priv->recent_manager = gtk_recent_manager_get_default ();
+	ev_recent_view->priv->list_store = gtk_list_store_new (11,

You can use NUM_COLUMNS instead of 11, here.

@@ +643,3 @@
+	ev_recent_view = EV_RECENT_VIEW (g_object_new (EV_TYPE_RECENT_VIEW, NULL));
+
+	return ev_recent_view;

All this can be a single line return EV_RECENT_VIEW (g_object_new (EV_TYPE_RECENT_VIEW, NULL));

::: shell/ev-window.c
@@ +513,3 @@
 	gboolean can_find_in_page = FALSE;
 	gboolean dual_mode = FALSE;
+	gboolean fullscreen_mode = FALSE;

fullscreen_mode is unconditionally set, so you don't need to initialize it here.

@@ +4621,3 @@
+{
+	ev_window_show_recent_view (ev_window);
+	return;

Useless return at the end of a void function.

@@ +5268,2 @@
 static void
+ev_window_try_swap_out_recent_view (EvWindow *ev_window)

Why try? can this fail?

@@ +5270,3 @@
+{
+	if (ev_window->priv->recent_view)
+	{

Move the { to the previous line.

@@ +5274,3 @@
+		gtk_widget_hide (GTK_WIDGET (ev_window->priv->minimal_toolbar));
+		g_object_unref (ev_window->priv->recent_view);
+		ev_window->priv->recent_view = NULL;

Are we destroying and creating the recent view every time you switch between document and recent view?

@@ +5290,3 @@
+					 gtk_window_get_screen (GTK_WINDOW (ev_window)),
+					 NULL, 0, NULL, gtk_get_current_event_time ());
+	return;

Useless return at the end of a void function.

@@ +5940,3 @@
+	/* View of recent items */
+	{ "RecentViewShow", "view-grid-symbolic", N_("Recent Items"), NULL,
+	  N_("Show recent items"),

recent documents maybe instead of items?

@@ +6102,3 @@
+	{ "ToolbarCloseWindow", "window-close-symbolic", N_("Close"), NULL,
+	  N_("Close this window"),
+	  G_CALLBACK (ev_window_cmd_file_close_window) },

I think you can reuse the existing actions and we don't need a specific action group for the recent view toolbar.

@@ +7190,2 @@
 	ev_window->priv->model = ev_document_model_new ();
+	ev_window->priv->recent_view = NULL;

This is already NULL at this point, the struct is filled with 0 when allocated.

@@ +7594,3 @@
 			   GDK_ACTION_COPY);
 	gtk_drag_dest_add_uri_targets (GTK_WIDGET (ev_window));
+

Extra line here.

@@ +7639,3 @@
+	ev_window_setup_action_sensitivity (ev_window);
+
+	return;

Useless return at the end of a void function.
Comment 24 Germán Poo-Caamaño 2013-08-13 17:30:19 UTC
(In reply to comment #16)
> I've been trying it out too, it looks pretty good in general, but I have some
> comments:
> 
>   - I think we should use a different toolbar when in bookshelf view, since
> having the current document toolbar with almost everything disabled doesn't
> make sense to me. It could be a very simply toolbar with close and open
> options.
> 
>   - The bookshelf button should not be in the bookshelf toolbar, the current
> active document should be highlighted somehow and to go back to the document
> view you would just click on it.

I have been using the current set of patches for a while, and I think the removal of the bookshelf button is confusing because it modifies the behaviour of the most recent button used.  It changes the bookshelf button to a open a new file button.  In other words, "how do I undo my this?" is not obviously possible.

It is true the current set of patches does not highlight the active document (to allow the user go back), but I am unsure how well this will work with several documents opened, because the user will have to visually search for it.  I am also unsure how this could work for a11y users to go back to the active document.

Besides the action of showing the recent view documents, the bookshelf button in the toolbar also shows where the UI is.  We have two states: recent view or document view.

Having the bookshelf button in the recent view toolbar seems odd only when opening Evince without a document.  But I believe this is not the common case, given we have set "NoDisplay=True".
Comment 25 Germán Poo-Caamaño 2013-09-12 18:46:12 UTC
Created attachment 254804 [details]
Screenshot that shows the consequence of the bookshelf button removal

The screenshot shows the problem I expressed in comment #24.

Can the user go back quickly to the document?

Hint: the icon for the document is in the 3rd row, 2nd column.

I had to look for the filename at the toolbar and then look for the same filename in the list of icons, which is suboptimal IMVHO.

FWIW, I opened most of the other documents when I was browsing a library's website.
Comment 26 Carlos Garcia Campos 2013-09-14 08:21:18 UTC
(In reply to comment #24)
> Having the bookshelf button in the recent view toolbar seems odd only when
> opening Evince without a document.  But I believe this is not the common case,
> given we have set "NoDisplay=True".

This is no longer true since 3.8, and with a recent files view makes no sense at all anyway.
Comment 27 Germán Poo-Caamaño 2013-10-01 06:37:18 UTC
I rebased aakash's branch against master. Comment #23 are
already addressed there.

I squashed a bunch of commits, re-worded some of them, make
them fit in 72 columns, fixed the author name in 4 of them.

I uploaded a branch in:
https://github.com/gpoo/evince/commits/wip/recent-view
Comment 28 Germán Poo-Caamaño 2014-01-24 03:00:53 UTC
There is an issue triggered by this set of patches when switching to fullscreen (F11):

Program received signal SIGSEGV, Segmentation fault.
0x00000000 in ?? ()


I can only reproduce the issue with this branch, not with master.  It happens after waiting a littler to get the toolbar hidden, that is:

1. Open a PDF
2. Press F11 to set fullscreen on
3. Wait without activity (keyboard or mouse)
4. Crash
Comment 29 Carlos Garcia Campos 2014-02-23 12:16:52 UTC
(In reply to comment #27)
> I rebased aakash's branch against master. Comment #23 are
> already addressed there.
> 
> I squashed a bunch of commits, re-worded some of them, make
> them fit in 72 columns, fixed the author name in 4 of them.
> 
> I uploaded a branch in:
> https://github.com/gpoo/evince/commits/wip/recent-view

Why don't you use gnome git?
Comment 30 Germán Poo-Caamaño 2014-02-24 04:09:58 UTC
(In reply to comment #29)
> (In reply to comment #27)
> > I rebased aakash's branch against master. Comment #23 are
> > already addressed there.
> > 
> > I squashed a bunch of commits, re-worded some of them, make
> > them fit in 72 columns, fixed the author name in 4 of them.
> > 
> > I uploaded a branch in:
> > https://github.com/gpoo/evince/commits/wip/recent-view
> 
> Why don't you use gnome git?

It was not my tree.  Anyhow, I re-ordered the commits, squash them
whenever possible and made sense.  It should be more manageable.

https://git.gnome.org/browse/evince/log/?h=wip/recent-view
Comment 31 Germán Poo-Caamaño 2014-02-24 04:46:21 UTC
Created attachment 270089 [details] [review]
Updated patch of recent view (aka bookshelf)

This patch has the complete/current bookshelf implementation
Comment 32 Germán Poo-Caamaño 2014-02-24 04:47:02 UTC
Created attachment 270090 [details] [review]
Integration of recent-view into Evince's shell
Comment 33 Germán Poo-Caamaño 2014-02-24 04:47:36 UTC
Created attachment 270091 [details] [review]
Implementation of an alternative/extra toolbar for the recent view
Comment 34 Germán Poo-Caamaño 2014-02-24 04:48:09 UTC
Created attachment 270092 [details] [review]
Removal of the recent-view menu (the old code)
Comment 35 Germán Poo-Caamaño 2014-02-24 05:09:57 UTC
Here is an update of the a conversation on #evince a while ago (Sep 20, 2013),
I am pasting it here for giving more context of the remaining tasks:

<aakash>  gpoo: However, a few things out of it still remain, mainly the part
                regarding saving and creating of thumbnails.
<gpoo>    aakash: do you consider the evtoolbar abstract class done?
<aakash>  gpoo: for instance I was saving its path in the metadata and it has
          been suggested that it should be done using some way similar to gnome-desktop
          On my part, I think its done. However, I am not very confident about
          if I have done it correctly since I am not totally familiar with GObjects
          and also, I had added actions for those buttons whereas it had been suggested
          that I could reuse the ones used in the main toolbar.
<gpoo>	  aakash: at some point, IIRC, the bookshelf highlighted the thumbnail when the
          point was over it.  Am I right?
<aakash>  gpoo: About that, its some kind of bug in Gtk
          I had talked to Cosimo about it during GUADEC, and he did try to get it working.
          but after some point I think he left it. It had mainly to do with the prelight
          not working properly in icon view
<aakash>  gpoo: I am not sure what you mean by 'dragged', could you please elaborate a bit ?
          ok. So you mean you are being able to drag and drop now and it wasn't so before?
          Oh. That seems like a bug. I had drag and drop there since quite the beginning,
          but it used to work as expected.
Comment 36 Cosimo Cecchi 2014-02-24 16:33:27 UTC
FWIW the bug we refer to in the conversation above has since been fixed in GTK.
Comment 37 Bogdan Petcu 2014-02-24 22:59:27 UTC
Created attachment 270198 [details]
printscreen  

Selecting a document from the recent-view window and quickly moving the mouse cursor drags the document. Should I file this bug separately? 

Way to reproduce:

1. enter evince in recent-view window
2. click on a document for opening
3. move the mouse cursor before the document opens
4. the item should be dragged until clicking again
Comment 38 Saurav Agarwalla 2014-02-26 19:10:58 UTC
I have been testing this out for a while. I realised that the documents are opened in a new window every time. Is this designed knowingly?

It feels strange to me. The document should open in the same window, I guess. Since, a button to show recent documents is also included in the toolbar, opening a document in a new window doesn't look very user friendly to me.
Comment 39 Germán Poo-Caamaño 2014-02-26 19:24:35 UTC
(In reply to comment #37)
> Created an attachment (id=270198) [details]
> printscreen  
> 
> Selecting a document from the recent-view window and quickly moving the mouse
> cursor drags the document. Should I file this bug separately?

No, because the issue is in the patch set, not in Evince.
Comment 40 Germán Poo-Caamaño 2014-02-26 19:24:51 UTC
(In reply to comment #38)
> I have been testing this out for a while. I realised that the documents are
> opened in a new window every time. Is this designed knowingly?
> 
> It feels strange to me. The document should open in the same window, I guess.
> Since, a button to show recent documents is also included in the toolbar,
> opening a document in a new window doesn't look very user friendly to me.

See https://bugzilla.gnome.org/show_bug.cgi?id=633501#c13
Comment 41 Saurav Agarwalla 2014-02-26 19:59:04 UTC
(In reply to comment #40)
> (In reply to comment #38)

> See https://bugzilla.gnome.org/show_bug.cgi?id=633501#c13

I went through it before too. But I think there is a difference. I somehow still feel that it should not open in a new window. Here's why - 
Selecting a file from the recent list is different than this because in this case the document is, in essence, closed for the user (although it may not in the background). 
Comparing this to the case of a web browser, when a user clicks on the home page button and clicks a new link there, the link is opened in the same tab because the user wanted to do the same thing. 
If he wanted to open a new link from his home page, he would perhaps open a new tab and then select the link (assuming the new tab opens his home page).

In the case of evince this translates as if the user wants to open a new document, he should open a new window and choose the document he wants to open from the new window as the user of a web browser will open a new tab and click the link rather than go to the home page to open the link in a new tab.

I am fully aware that people far more experienced than me have given a lot of thought over this, but I just felt that I should express my opinion.
Comment 42 Bogdan Petcu 2014-03-06 20:46:26 UTC
Regarding the "Mark the currently open document when in bookshelf/recent view" issue I also find it quite bothering that when I switch from a document to bookshelf-view I have no info on what was the document I had opened. As I saw in the comments above one of the possibilities would be highlighting the currently opened document. I thought that this may be solved by opening documents as tabs in the main window (like in a web browser), not as separate windows so in this way marking the currently opened document would not be necessary any more. This way we could avoid that confusing multiple highlighting issue when opening several documents that Germán Poo-Caamaño mentioned in comment 24.

Also I noticed that there is no way to remove items from the bookshelf. Maybe from various reasons you would not want a file to appear here. So maybe a trash button could be added to the toolbar and document opening could be made by double-click, leaving single-click for simply selecting an item (highlighting it). This way pressing the trash button would remove the selected item. In addition a multiple select feature could be implemented(like adding a check-box to each item) allowing this way multiple-delete, or even multiple open(by adding an "open" button that will open the documents selected)
Comment 43 Bogdan Petcu 2014-03-10 20:20:39 UTC
I noticed that trying to open a file that doesn't exist anymore displays the "Error opening file: No such file or document" message and offers the "Close" button. If pressed, the "Close" button will provide an empty Evince window having the "Recent-View" button inactive. I guess it would be better if pressing "Close" will return to the Recent-View window refreshing it(removing the missing file).
Comment 44 Bogdan Petcu 2014-04-14 23:35:26 UTC
Created attachment 274319 [details] [review]
Setting the "follow-state" property on the pixbuf cell renderer

This patch along with Cosimo's fix on: https://bugzilla.gnome.org/show_bug.cgi?id=726271 makes the items in recent-view to be highlighted while hovered
Comment 45 Germán Poo-Caamaño 2014-05-01 20:37:54 UTC
Review of attachment 274319 [details] [review]:

This looks good to me and works fine with gtk+ 3.13.1.

For some reason, the patch did not apply cleanly but I didn't
dig in to figure out why.
Comment 46 Germán Poo-Caamaño 2014-05-04 04:51:01 UTC
I rebased the branch and fixed some issues that were driving me crazy
(I've been using this branch + other patches as my main evince since
a while):

One special remark:

(In reply to comment #16)
> [...]
>   - The bookshelf button should not be in the bookshelf toolbar, the current
> active document should be highlighted somehow and to go back to the document
> view you would just click on it.

I brought back the button because is the simplest solution, so far.

I have found myself with documents opened for several days or a week
(for example, when working on a paper) or opening several documents
for crossing references, leaving only the main one opened during
the session.

In both cases, it does not take it long that the active documents
disappear from the list of recent documents opened, so the recent
view becomes a burden when I cannot back to the document.

An alternative solution that came to my mind was to set the current
document as the first item the window's recent view. However, that
would mean adding more code and complexity for a marginal gain.

The trick of adding the document to the recent manager just before
switching to the recent view works in a very simple case. Having
more than one document opened, the icons might start jumping
(depending of the use) and the UI becomes confusing.
Comment 47 Carlos Garcia Campos 2014-05-04 09:47:29 UTC
Review of attachment 270089 [details] [review]:

Not sure this is the latest version, but I'll review this one anyway.

::: shell/ev-recent-view.c
@@ +145,3 @@
+				 "png", &error, NULL);
+		if (!error)
+			ev_metadata_set_string (metadata, "thumbnail-path", thumbnail_path);

I still think we should not save a temporary file in the metadata. We should use the thumbnails cache for this.

@@ +166,3 @@
+		ev_job_cancel (job);
+		g_signal_handlers_disconnect_by_func (job, thumbnail_job_completed_callback, data);
+		g_signal_handlers_disconnect_by_func (job, document_load_job_completed_callback, data);

We could probably use g_signal_handlers_disconnect_by_data here.

@@ +215,3 @@
+
+	return NULL;
+}

I don't understand this method, view is created in constructed, so how can it be NULL here? Also, the caller is not checking the returned value is not NULL.

@@ +222,3 @@
+                         EvRecentView   *ev_recent_view)
+{
+	GdMainViewGeneric *generic = get_generic (ev_recent_view);

I think we should just cast here.

@@ +235,3 @@
+		return result;
+
+	if (ev_recent_view->priv->pressed_item_tree_path == NULL) {

We could check this before getting the path

@@ +261,3 @@
+		                    GD_MAIN_COLUMN_SELECTED, TRUE,
+		                    -1);
+		g_signal_emit (ev_recent_view, signals[ITEM_ACTIVATED], 0, uri);

uri is leaked, it should be freed here.

@@ +493,3 @@
+
+	g_list_foreach (items, (GFunc) gtk_recent_info_unref, NULL);
+	g_list_free (items);

WE can use g_list_free_full here

@@ +499,3 @@
+
+static void
+ev_recent_view_rebuild (EvRecentView *ev_recent_view)

This is called rebuild, but it's only used once from constructed.
Comment 48 Carlos Garcia Campos 2014-05-04 11:12:33 UTC
Review of attachment 270089 [details] [review]:

::: shell/ev-recent-view.c
@@ +355,3 @@
+		g_object_set_data_full (G_OBJECT (job_thumbnail), "tree_iter",
+		                        gtk_tree_iter_copy (iter),
+		                        (GDestroyNotify) gtk_tree_iter_free);

I don't think it's safe to save the GtkTreeIter, we should use a GtkTreeRowReference instead.
Comment 49 Carlos Garcia Campos 2014-05-04 11:49:31 UTC
Review of attachment 274319 [details] [review]:

::: cut-n-paste/libgd/gd-toggle-pixbuf-renderer.c
@@ +200,3 @@
+   g_object_set (renderer, "follow-state", TRUE, NULL);
+   
+   return renderer;

I guess it would be simpler to do 

return g_object_new (GD_TYPE_TOGGLE_PIXBUF_RENDERER, "follow-state", TRUE, NULL);
Comment 50 Carlos Garcia Campos 2014-05-04 13:32:34 UTC
Review of attachment 270089 [details] [review]:

::: shell/ev-recent-view.c
@@ +308,3 @@
+	gtk_tree_model_get (GTK_TREE_MODEL (priv->model),
+	                    iter,
+	                    EV_RECENT_VIEW_DOCUMENT_COLUMN, &document,

This looks unused, I guess we don't really need to save the document in the model
Comment 51 Carlos Garcia Campos 2014-05-04 13:39:24 UTC
Review of attachment 270089 [details] [review]:

::: shell/ev-recent-view.c
@@ +35,3 @@
+typedef enum {
+	EV_RECENT_VIEW_JOB_COLUMN = GD_MAIN_COLUMN_LAST,
+	EV_RECENT_VIEW_THUMBNAILED_COLUMN,

I also wonder what this column is for.
Comment 52 Carlos Garcia Campos 2014-05-04 14:14:50 UTC
Review of attachment 250016 [details] [review]:

I've just pushed this to master, thanks!
Comment 53 Carlos Garcia Campos 2014-05-04 14:20:12 UTC
Review of attachment 270089 [details] [review]:

I've reworked this:

 * Simplified and cleaned up the code
 * Removing the metadata part, we should use the thumbanils cache instead. 
 * Updated the use cairo surfaces instead of pixbufs
 * Updated to use target size instead of scale for the thumbnail job
 * Updated to use GtkTreeRowReference instead of GtkTreeIter
 * Show title and author as primary/secondary text, falling back to the filename when not present
 * Removed unused code

It's still unused but at least I think it will be easier continue working on this bug now that this is in master.
Comment 54 Carlos Garcia Campos 2014-05-04 14:21:00 UTC
Review of attachment 274319 [details] [review]:

For some reason this doesn't work when using surfaces instead of pixbufs for the cell renderer
Comment 55 Carlos Garcia Campos 2014-05-04 14:43:07 UTC
Review of attachment 270090 [details] [review]:

::: shell/ev-toolbar.c
@@ +161,3 @@
+	/* View of recent items */
+	action = gtk_action_group_get_action (action_group, "RecentViewShow");
+	button = ev_toolbar_create_toggle_button (ev_toolbar, action);

I'm not sure we want a toggle button here, since this is never shown when active.

@@ +165,3 @@
+	gtk_container_add (GTK_CONTAINER (tool_item), button);
+	gtk_widget_show (button);
+	gtk_widget_set_margin_right (tool_item, 12);

This should take the text direction into account like the other items

::: shell/ev-window.c
@@ +505,3 @@
 	ev_window_set_action_sensitive (ev_window, PAGE_SELECTOR_ACTION, has_pages);
 	ev_window_set_action_sensitive (ev_window, ZOOM_CONTROL_ACTION,  has_pages);
+	ev_window_set_action_sensitive (ev_window, HISTORY_ACTION, has_document);

Is this unrelated to this patch?

@@ +529,3 @@
+	ev_window_set_action_sensitive (ev_window, "RecentViewShow", 
+	                                ev_window->priv->document ||
+	                                !ev_window->priv->recent_view);

Why is this unsensitive when there isn't a recent_view? When is recent_view NULL? we should always be able to go to the recent list.

@@ +1668,2 @@
 	ev_window_hide_loading_message (ev_window);
+	ev_window_try_swap_out_recent_view (ev_window);

Why try? how can this fail?

@@ +4725,3 @@
+                                  EvWindow  *ev_window)
+{
+	if (!ev_window->priv->recent_view)

You should check first the state of the toggle button, although I think this shouldn't be a toggle action.

@@ +4731,3 @@
+		ev_window_setup_action_sensitivity (ev_window);
+	}
+	return;

This is useless.

@@ +5388,3 @@
+ev_window_try_swap_out_recent_view (EvWindow *ev_window)
+{
+	if (ev_window->priv->recent_view)

So, the "try" is because recent_view can be NULL? I prefer the if_needed sufix instead. I think we could rework the current page mode, and use something like ev_window_set_view_mode with values DOCUMET, PASSWORD and RECENT and use the same code to switch modes.

@@ +5389,3 @@
+{
+	if (ev_window->priv->recent_view)
+	{

Move this to the previous line.

@@ +5390,3 @@
+	if (ev_window->priv->recent_view)
+	{
+		gtk_widget_hide (GTK_WIDGET (ev_window->priv->recent_view));

Why do you need to hide it right before unrefing it?

@@ +5392,3 @@
+		gtk_widget_hide (GTK_WIDGET (ev_window->priv->recent_view));
+		g_object_unref (ev_window->priv->recent_view);
+		ev_window->priv->recent_view = NULL;

You should use gtk_widget_destroy, instead of unref. But, do we really want to destroy the widget? or only hiding it? Maybe we could use a timeout, so that if the recent view is not visited in x seconds, the widget is destroyed to free the memory, but if the recent view is visited often, it will be faster if we don't have to create it again. Maybe it makes more sense to destroy it every time once we have the metadata (title, author) and thumbnail properly cached.

@@ +5402,3 @@
+                               EvWindow     *ev_window)
+{
+	if (ev_window->priv->uri && strcmp (ev_window->priv->uri, uri) == 0)

Use g_strcmp0 that already handles NULL

@@ +5407,3 @@
+					 gtk_window_get_screen (GTK_WINDOW (ev_window)),
+					 NULL, 0, NULL, gtk_get_current_event_time ());
+	return;

This is useless

@@ +6143,3 @@
+	{ "RecentViewShow", "view-grid-symbolic", N_("Recent Items"), NULL,
+	  N_("Toggle between view of recent items and open document"),
+	  G_CALLBACK (ev_window_cmd_toggle_recent_view) },

Why don't we reuse the existing RecentFiles action?

@@ +7410,2 @@
 	ev_window->priv->model = ev_document_model_new ();
+	ev_window->priv->recent_view = NULL;

This is not needed.

@@ +7850,3 @@
+	if (!ev_window->priv->recent_view) {
+		ev_window->priv->recent_view = ev_recent_view_new ();
+		g_object_ref (ev_window->priv->recent_view);

Why are you taking a ref here? The container will consume the floating reference, we only need to take a reference if we are going to reparent the widget, but you are destroying it.

@@ +7863,3 @@
+	ev_window_setup_action_sensitivity (ev_window);
+
+	return;

This is useless
Comment 56 Carlos Garcia Campos 2014-05-04 14:51:05 UTC
Review of attachment 270091 [details] [review]:

I think we should not duplicate code in ev-toolbar and ev-whatever-toolbar.

::: shell/Makefile.am
@@ +37,3 @@
 	ev-metadata.h			\
+	ev-minimal-toolbar.c		\
+	ev-minimal-toolbar.h		\

These should called ev-recent-toolbar or something like that, but not minimal

::: shell/ev-minimal-toolbar.c
@@ +149,3 @@
+
+	/* Close Button */
+	action = gtk_action_group_get_action (action_group, "ToolbarCloseWindow");

I don't think we want to include a close button, it's already in the window manager or in the header bar once we move to header bar.

::: shell/ev-window.c
@@ +535,3 @@
+	                                ev_window->priv->document &&
+	                                !ev_window->priv->recent_view &&
+	                                !fullscreen_mode);

This looks unrelated to this patch. Is this a fix for the previous one?

@@ +4277,3 @@
 	if (window->priv->metadata && !ev_window_is_empty (window))
 		ev_metadata_set_boolean (window->priv->metadata, "fullscreen", TRUE);
+	ev_window_update_actions_sensitivity (window);

Ditto.

@@ +4322,3 @@
 	if (window->priv->metadata && !ev_window_is_empty (window))
 		ev_metadata_set_boolean (window->priv->metadata, "fullscreen", FALSE);
+	ev_window_update_actions_sensitivity (window);

Ditto.

@@ +4733,3 @@
                                   EvWindow  *ev_window)
 {
+	ev_window_show_recent_view (ev_window);

Ditto.

@@ +5872,3 @@
+	if (priv->minimal_toolbar_action_group) {
+		g_object_unref (priv->minimal_toolbar_action_group);
+		priv->minimal_toolbar_action_group = NULL;

g_clear_object

@@ +7470,3 @@
 	gtk_window_add_accel_group (GTK_WINDOW (ev_window), accel_group);
 
+	action_group = gtk_action_group_new ("MinimalToolbarActions");

RecentToolbarActions
Comment 57 Germán Poo-Caamaño 2014-05-04 16:42:30 UTC
Review of attachment 274319 [details] [review]:

::: cut-n-paste/libgd/gd-toggle-pixbuf-renderer.c
@@ +200,3 @@
+   g_object_set (renderer, "follow-state", TRUE, NULL);
+   
+   return renderer;

Maybe because the fix in GTK uses pixbufs:
https://bugzilla.gnome.org/review?bug=726271&attachment=274135
Comment 58 Germán Poo-Caamaño 2014-05-04 16:52:31 UTC
Review of attachment 270091 [details] [review]:

::: shell/ev-window.c
@@ +5872,3 @@
+	if (priv->minimal_toolbar_action_group) {
+		g_object_unref (priv->minimal_toolbar_action_group);
+		priv->minimal_toolbar_action_group = NULL;

I think this was made to make it consistent with the rest of the code
Comment 59 Germán Poo-Caamaño 2014-05-04 16:55:01 UTC
Created attachment 275823 [details] [review]
libgd: Highlight items on mouse-hover
Comment 60 Germán Poo-Caamaño 2014-05-04 16:56:15 UTC
Created attachment 275824 [details] [review]
recent-view: Do not propagate button_press_event in recent view

This was already fixed in the wip branch, but not uploaded here. So
I am updating the patches.
Comment 61 Germán Poo-Caamaño 2014-05-04 16:58:15 UTC
Created attachment 275825 [details] [review]
shell: Integrate recent view into the main window

Still need to address ev_window_set_mode suggested in a comment.
Maybe in another patch.
Comment 62 Germán Poo-Caamaño 2014-05-04 16:59:24 UTC
Created attachment 275826 [details] [review]
ev-recent-view-toolbar: Implement a different toolbar for recent view

Missing: using an abstract class to reuse the same code in
both toolbars
Comment 63 Germán Poo-Caamaño 2014-05-04 17:01:16 UTC
Created attachment 275827 [details] [review]
ev-window: Integrate the recent view toolbar
Comment 64 Germán Poo-Caamaño 2014-05-04 17:01:51 UTC
Created attachment 275828 [details] [review]
ev-window: Removed the recent menu list
Comment 65 Carlos Garcia Campos 2014-05-05 17:56:05 UTC
Review of attachment 275823 [details] [review]:

Remove the unused variable before landing, please.

::: cut-n-paste/libgd/gd-toggle-pixbuf-renderer.c
@@ +195,3 @@
 gd_toggle_pixbuf_renderer_new (void)
 {
+   GtkCellRenderer *renderer;

This is unused.
Comment 66 Carlos Garcia Campos 2014-05-05 17:57:52 UTC
Review of attachment 275824 [details] [review]:

hmm, I used your branch, not the attached patches, I guess I messed it up somehow. Thanks.
Comment 67 Carlos Garcia Campos 2014-05-05 17:59:04 UTC
(In reply to comment #61)
> Created an attachment (id=275825) [details] [review]
> shell: Integrate recent view into the main window
> 
> Still need to address ev_window_set_mode suggested in a comment.
> Maybe in another patch.

And all other review comments
Comment 68 Germán Poo-Caamaño 2014-05-07 06:47:23 UTC
Created attachment 276045 [details] [review]
shell: Make a class for EvToolbar to abstract for other toolbars

Just make the current toolbar a base class for other toolbars.
For now, EvToolbarMain, in another patch EvRecentViewToolbar.
Comment 69 Germán Poo-Caamaño 2014-05-07 07:50:41 UTC
Created attachment 276048 [details] [review]
ev-recent-view-toolbar: Implement a different toolbar for recent view

Re-implementation of the toolbar as a subclass of EvToolbar
Comment 70 Germán Poo-Caamaño 2014-05-08 07:43:45 UTC
Created attachment 276136 [details] [review]
recent-view: Verify a path exists before adding an icon

The recent view in master does not verify if a path is valid, this
triggers some warnings when switching from the recent view to the
document back and forth.

This patch fixes the issue.
Comment 71 Germán Poo-Caamaño 2014-05-10 19:38:06 UTC
Created attachment 276294 [details] [review]
ev-recent-view-toolbar: Implement a different toolbar for recent view

Rebased against master, that is, removed the about button from
the toolbar to match the switch to the app menu.
Comment 72 Germán Poo-Caamaño 2014-05-10 19:40:14 UTC
Created attachment 276295 [details] [review]
ev-window: Integrate the recent view toolbar

Rebased against master and removing the about action.
Comment 73 Carlos Garcia Campos 2014-06-20 05:56:40 UTC
(In reply to comment #68)
> Created an attachment (id=276045) [details] [review]
> shell: Make a class for EvToolbar to abstract for other toolbars
> 
> Just make the current toolbar a base class for other toolbars.
> For now, EvToolbarMain, in another patch EvRecentViewToolbar.

I'm afraid this approach doesn't work for us anymore. I noticed it when we ported to the headerbar. Once you set the headerbar as decorator of the toplevel you can't remove it and place another one, like we do in fullscreen mode. So, I think we need to use a different approach here, probably based on modes, where you do ev_toolbar_set_mode(). Possible modes would be DOCUMENT, RECENT, PREVIEW, etc. and the toolbar itself shows/hides or creates/destroys its items to implement every mode.
Comment 74 Germán Poo-Caamaño 2014-06-25 06:18:53 UTC
Created attachment 279170 [details] [review]
shell: Integrate recent view into the main window

Rebased patch, rewritten to use gaction.
Comment 75 Germán Poo-Caamaño 2014-06-25 06:22:00 UTC
Created attachment 279171 [details] [review]
ev-toolbar: Implement toolbar with multiple modes

Given the restrictions of GtkHeaderBar for reparenting, we must
implement multiple modes for the toolbar, which can be customized
for Recent View, Full Screen, Preview and Document (normal).

For now, Full Screen, Preview and Document are the same.
Comment 76 Germán Poo-Caamaño 2014-06-25 06:22:46 UTC
Created attachment 279172 [details] [review]
shell: Remove menu for recent items
Comment 77 Germán Poo-Caamaño 2014-06-25 06:26:28 UTC
(In reply to comment #73)
> (In reply to comment #68)
> > Created an attachment (id=276045) [details] [review] [details] [review]
> > shell: Make a class for EvToolbar to abstract for other toolbars
> > 
> > Just make the current toolbar a base class for other toolbars.
> > For now, EvToolbarMain, in another patch EvRecentViewToolbar.
> 
> I'm afraid this approach doesn't work for us anymore. I noticed it when we
> ported to the headerbar. Once you set the headerbar as decorator of the
> toplevel you can't remove it and place another one, like we do in fullscreen
> mode. So, I think we need to use a different approach here, probably based on
> modes, where you do ev_toolbar_set_mode(). Possible modes would be DOCUMENT,
> RECENT, PREVIEW, etc. and the toolbar itself shows/hides or creates/destroys
> its items to implement every mode.

The patch with modes seems smaller and cleaner to me.

I also rebased the branch wip/recent-menu against master (7bba09073).

I think this part is almost done (besides the review). Next step is
to improve the recent view itself (use cache, etc.)
Comment 78 Carlos Garcia Campos 2014-06-30 16:23:06 UTC
Review of attachment 279170 [details] [review]:

::: shell/ev-application.c
@@ +656,3 @@
 {
 	GtkWidget *new_window = ev_window_new ();
+	ev_window_show_recent_view (EV_WINDOW (new_window));

It's a bit weird that this is called by ev-application. Since empty windows don't make sense anymore, maybe we could merge the methods that creates the windows and loads their contents. Something like:

ev_window_new () -> bookshelf
ev_window_new_for_document () -> new + open_document
ev_window_new_for_uri () ->  new + open_uri

what do you think?

::: shell/ev-window.c
@@ +487,3 @@
 
+	/* Recent View */
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);

Isn't this always enabled?

@@ +489,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (has_document));

I don't think setting the state belongs here, this method is only about actions sensitivity.

@@ +516,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (!has_document));

Same here. I wonder if we should merge setup_actions_sensitivity and update_actions_sensitivity. This made sense before because we couldn't switch back to a no document state, but now we can switch to recent view.

@@ +1647,2 @@
 	ev_window_hide_loading_message (ev_window);
+	ev_window_swap_out_recent_view_if_needed (ev_window);

This should probably happen in ev_window_set_document

@@ +1650,3 @@
 	/* Success! */
 	if (!ev_job_is_failed (job)) {
+

Extra line here?

@@ +2179,3 @@
 	ev_window_clear_load_job (ev_window);
 	ev_window_clear_local_uri (ev_window);
+	ev_window_swap_out_recent_view_if_needed (ev_window);

If this is called in ev_window_set_document() you don't need it here either

@@ +4574,3 @@
+		ev_window_show_recent_view (ev_window);
+	else {
+		ev_window_swap_out_recent_view_if_needed (ev_window);

I would call this hide_recent_view, for consistency with show_recent_view.

@@ +4577,3 @@
+		ev_window_setup_action_sensitivity (ev_window);
+	}
+	return;

This is useless.

@@ +5063,3 @@
+ev_window_swap_out_recent_view_if_needed (EvWindow *ev_window)
+{
+	if (ev_window->priv->recent_view)

How can this be called with a NULL recent_view?

@@ +5064,3 @@
+{
+	if (ev_window->priv->recent_view)
+	{

This should be in the previous line

@@ +5066,3 @@
+	{
+		gtk_widget_hide (GTK_WIDGET (ev_window->priv->recent_view));
+		ev_window->priv->recent_view = NULL;

I don't think this is correct, we either hide the widget and leave the pointer, or destroy the widget and reset the pointer. We are creating a new recent view everytime we switch, but without destroying the previous one.

@@ +5076,3 @@
+                               EvWindow     *ev_window)
+{
+	if (ev_window->priv->uri && g_strcmp0 (ev_window->priv->uri, uri) == 0) {

g_strcmp0 is null safe
Comment 79 Carlos Garcia Campos 2014-06-30 16:31:18 UTC
Review of attachment 279171 [details] [review]:

::: shell/ev-toolbar.c
@@ +224,3 @@
+                                                  _("Toggle between view of recent items and open document"));
+        ev_toolbar->priv->recent_view_button = button;
+        tool_item = GTK_WIDGET (gtk_tool_item_new ());

We don't use GtkToolItem now that the toolbar is no longer a GtkToolbar.

@@ +237,3 @@
+                                           "document-open-symbolic",
+                                           _("Open an existenting document"));
+        tool_item = GTK_WIDGET (gtk_tool_item_new ());

Ditto.

::: shell/ev-window.c
@@ +496,3 @@
+	else
+		ev_toolbar_set_mode (EV_TOOLBAR (ev_window->priv->toolbar),
+			             EV_TOOLBAR_MODE_RECENT_VIEW);

I don't think this is the right place for this, it should be done when the actual switch happens, in show/hide recent_view I guess.

@@ +4582,2 @@
 		ev_window_show_recent_view (ev_window);
+		ev_toolbar_set_mode (toolbar, EV_TOOLBAR_MODE_RECENT_VIEW);

This should be called by show_recent_view I think

@@ +4585,2 @@
 		ev_window_swap_out_recent_view_if_needed (ev_window);
+		ev_toolbar_set_mode (toolbar, EV_TOOLBAR_MODE_NORMAL);

And this by hide_recent_view only when it actually happens
Comment 80 Carlos Garcia Campos 2014-06-30 16:32:15 UTC
(In reply to comment #77)
> (In reply to comment #73)
> > (In reply to comment #68)
> > > Created an attachment (id=276045) [details] [review] [details] [review] [details] [review]
> > > shell: Make a class for EvToolbar to abstract for other toolbars
> > > 
> > > Just make the current toolbar a base class for other toolbars.
> > > For now, EvToolbarMain, in another patch EvRecentViewToolbar.
> > 
> > I'm afraid this approach doesn't work for us anymore. I noticed it when we
> > ported to the headerbar. Once you set the headerbar as decorator of the
> > toplevel you can't remove it and place another one, like we do in fullscreen
> > mode. So, I think we need to use a different approach here, probably based on
> > modes, where you do ev_toolbar_set_mode(). Possible modes would be DOCUMENT,
> > RECENT, PREVIEW, etc. and the toolbar itself shows/hides or creates/destroys
> > its items to implement every mode.
> 
> The patch with modes seems smaller and cleaner to me.
> 
> I also rebased the branch wip/recent-menu against master (7bba09073).
> 
> I think this part is almost done (besides the review). Next step is
> to improve the recent view itself (use cache, etc.)

Done, I've just pushed a patch to cache metadata and thumbnails.
Comment 81 Germán Poo-Caamaño 2014-07-01 20:10:46 UTC
Review of attachment 279170 [details] [review]:

::: shell/ev-application.c
@@ +656,3 @@
 {
 	GtkWidget *new_window = ev_window_new ();
+	ev_window_show_recent_view (EV_WINDOW (new_window));

I agree, although I don't see the use of ev_window_new_for_document () or maybe I misunderstood something (see the new to check it out)

::: shell/ev-window.c
@@ +487,3 @@
 
+	/* Recent View */
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);

No. There is one circumstance when this should be disabled: when you launch evince for the first time with no argument. In such case, you cannot switch to the document view, hence, it must be disabled.
Comment 82 Germán Poo-Caamaño 2014-07-01 20:14:06 UTC
Created attachment 279714 [details] [review]
ev-toolbar: Implement toolbar with multiple modes

Updated patch. It addresses the comments in #c79 plus some extra changes to make the integration easier (add method to get_mode and don't show the open icon by default).
Comment 83 Germán Poo-Caamaño 2014-07-01 20:15:31 UTC
Created attachment 279715 [details] [review]
shell: Integrate recent view into the main window

Updated patch. It addresses the comments in #c78
Comment 84 Germán Poo-Caamaño 2014-07-01 20:22:06 UTC
Review of attachment 279715 [details] [review]:

::: shell/ev-window.c
@@ +1661,3 @@
 	/* Success! */
 	if (!ev_job_is_failed (job)) {
+

Forget this extra line.  wip/recent-view has the patch without this line.
Comment 85 Germán Poo-Caamaño 2014-07-21 21:52:46 UTC
I found an issue with the recent view:

If a document produces a segfault in evince, then, every time the recent view is activated and the document is in the recent list, evince will crash when trying to create the thumbnail of the document.
Comment 86 Carlos Garcia Campos 2014-07-23 10:07:32 UTC
Review of attachment 279714 [details] [review]:

::: shell/ev-toolbar.c
@@ +232,3 @@
+
+        /* Don't show the open button by detault, only when activated,
+           only when requested explicitly according to the toolbar mode */

I think we should probably remove all gtk_widget_show from here, since we already show/hide all the appropriate widgets when set_mode is called. By default everything is hidden until set mode is called for the first time.

@@ +344,3 @@
         ev_toolbar_setup_bookmarks_menu (ev_toolbar, bookmarks_submenu_model);
 
+        ev_toolbar->priv->toolbar_mode = EV_TOOLBAR_MODE_NORMAL;

This is not needed, since NORMAL is 0 and the struct is filled with 0 when allocated, but if you still want to add this explicitly, do it in init instead of constructed.

@@ +440,3 @@
+
+        if (mode == priv->toolbar_mode)
+                return;

If we default to everything hidden, this will not work unless we have a mode for the initial state. I think we can simply remove this check, since gtk_widget_show/hide do nothing if the widget is already visible/hidden, and in most of the cases this will not be called twice for the same mode.

@@ +448,3 @@
+        case EV_TOOLBAR_MODE_FULL_SCREEN:
+        case EV_TOOLBAR_MODE_PRESENTATION:
+        case EV_TOOLBAR_MODE_PREVIEW:

Let's add all other modes when they are needed, for now NORMAL and RECENT_VIEW are enough.
Comment 87 Carlos Garcia Campos 2014-07-23 10:14:10 UTC
Review of attachment 279715 [details] [review]:

::: shell/ev-application.c
@@ +596,3 @@
 	ev_window = ev_application_get_empty_window (application, screen);
 	if (!ev_window)
+		ev_window = EV_WINDOW (ev_window_new_for_uri ());

I actually meant to merge the methods that create the window and load the contents, so that ev_window_new_for_uri would receive the uri to create the window and load the uri. But I'm seeing now that it's not so easy, because ev-applications does more things. So, I think we can leave the create window and then load contents approach. In that case we could add ev_window_open_recent_view for consistency with open_uri and open_document. All of them should be called after a window is created.

::: shell/ev-window.c
@@ +502,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (!has_document));

As I said, I don't think this is the right place for this, this method is only about actions sensitivity, not about their state.

@@ +4579,3 @@
+	} else {
+		ev_window_hide_recent_view (ev_window);
+		ev_toolbar_set_mode (toolbar, EV_TOOLBAR_MODE_NORMAL);

Could we call ev_toolbar_set_mode in ev_window_show/hide_recent_view? it seems it's always called after those calls.
Comment 88 Germán Poo-Caamaño 2014-07-24 05:09:48 UTC
Review of attachment 279715 [details] [review]:

::: shell/ev-window.c
@@ +502,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (!has_document));

As per comment https://bugzilla.gnome.org/show_bug.cgi?id=633501#c78 :

I merged both setup_actions_sensitivity and update_actions_sensitivity.  Maybe we should change the name of the method or set back and have both methods, or maybe there is another alternative that I am missing here.
Comment 89 Germán Poo-Caamaño 2014-07-24 05:11:06 UTC
Created attachment 281545 [details] [review]
ev-toolbar: Implement toolbar with multiple modes

Updated patch with comments addressed
Comment 90 Germán Poo-Caamaño 2014-07-24 05:13:10 UTC
Created attachment 281546 [details] [review]
shell: Integrate recent view into the main window

Updated patch with comments addressed (except the one in comment #68 where I need more feedback).
Comment 91 Carlos Garcia Campos 2014-07-24 10:34:18 UTC
(In reply to comment #88)
> Review of attachment 279715 [details] [review]:
> 
> ::: shell/ev-window.c
> @@ +502,3 @@
> +    ev_window_set_action_enabled (ev_window, "recent-view", has_document);
> +    action = g_action_map_lookup_action (G_ACTION_MAP (ev_window),
> "recent-view");
> +    g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean
> (!has_document));
> 
> As per comment https://bugzilla.gnome.org/show_bug.cgi?id=633501#c78 :
> 
> I merged both setup_actions_sensitivity and update_actions_sensitivity.  Maybe
> we should change the name of the method or set back and have both methods, or
> maybe there is another alternative that I am missing here.

Yes, but one thing is merging both methods, but still caring only about actions sensitivity, not their state.
Comment 92 Carlos Garcia Campos 2014-07-24 10:36:12 UTC
Review of attachment 281545 [details] [review]:

This patch looks good to me, but it doesn't make sense independently, since it doesn't show anything because set_mode is never called. And even if it were called, you would see a recent view button that does nothing. So, please, merge this with the next patch into a single one.
Comment 93 Carlos Garcia Campos 2014-07-24 10:55:25 UTC
Review of attachment 281546 [details] [review]:

Looks much better, but there are still some serious issues with the patch

::: shell/ev-application.c
@@ +682,3 @@
 	}
+
+	ev_window_open_recent_view (EV_WINDOW (new_window));

I think we should do this at the beginning, right after the window is created, so that when it's shown, the recent view has already been created. Similar to what we do when creating a window to load a uri. Also I think we should rename this method now to something like ev_application_open_recent_view.

::: shell/ev-window.c
@@ +410,2 @@
 	const EvDocumentInfo *info = NULL;
 	gboolean has_document = FALSE;

There's another problem with the actions sensitivity now. has_document is no longer enough to decide, because we don't want to allow document actions in recent_view mode even when there's a document loaded. For example, open evince in recen view mode, you can't find, shown the sidebar, start a presentation, etc., open a file from the recent view and switch back to recent view mode, now you can find, start a presentation, etc. Most of the actions that we currently disable when has_document is FALSE should also be disabled now when we are in recent view.

@@ +502,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (!has_document));

So, this is done twice now, and still at the wrong place. I think you can simply leave the ev_window_set_action_enabled and remove the set_state, since it's already done correctly in other places.

@@ +4085,3 @@
 	if (window->priv->metadata && !ev_window_is_empty (window))
 		ev_metadata_set_boolean (window->priv->metadata, "fullscreen", TRUE);
+	ev_window_update_actions_sensitivity (window);

Why do you need this here now?. We currently allow to enter in fullscreen mode for an empty view, but I think we should not allow that now in recen view mode. Also you need to call ev_toolbar_set_mode() right after creating the fulscreen toolbar. We should make sure the recent view button is disabled when in fullscreen mode, or even not shown adding the preview toolbar mode.

@@ +4121,3 @@
 	if (window->priv->metadata && !ev_window_is_empty (window))
 		ev_metadata_set_boolean (window->priv->metadata, "fullscreen", FALSE);
+	ev_window_update_actions_sensitivity (window);

Ditto.

@@ +4571,3 @@
+static void
+ev_window_cmd_toggle_recent_view (GSimpleAction *action,
+                                  GVariant      *parameter,

This is not a parameter but the state.

@@ +4577,3 @@
+	EvToolbar *toolbar = EV_TOOLBAR (ev_window->priv->toolbar);
+
+	if (ev_toolbar_get_mode (toolbar) == EV_TOOLBAR_MODE_NORMAL)

Why not checking the state parameter?

@@ +4581,3 @@
+	else
+		ev_window_hide_recent_view (ev_window);
+}

This is the right place to set the state, I would say

@@ +6822,3 @@
 			    ev_window->priv->find_bar,
 			    FALSE, TRUE, 0);
+	

Please, remove these whitespaces.

@@ +7117,3 @@
  *
+ * Returns: the #GtkWidget that represents the window with the Recent View
+ * activated..

This is no longer the case.

@@ +7156,3 @@
+	}
+
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);

You should call ev_window_update_actions_sensitivity instead here to make sure all actions that should be disabled in recent view mode are actually disabled.

@@ +7158,3 @@
+	ev_window_set_action_enabled (ev_window, "recent-view", has_document);
+	action = g_action_map_lookup_action (G_ACTION_MAP (ev_window), "recent-view");
+	g_simple_action_set_state (G_SIMPLE_ACTION (action), g_variant_new_boolean (has_document));

I think this is only true the first time, you could move this inside the previous if
Comment 94 Germán Poo-Caamaño 2014-07-24 21:28:38 UTC
Created attachment 281639 [details] [review]
recent-view: Integrate recent view in the shell

Updated patch.
Comment 95 Germán Poo-Caamaño 2014-07-24 21:49:52 UTC
Review of attachment 281639 [details] [review]:

::: shell/ev-window.c
@@ +4628,3 @@
+	EvWindow *ev_window = user_data;
+
+	if ( g_variant_get_boolean (state))

This typo is fixed in the wip/recent-view branch
Comment 96 Germán Poo-Caamaño 2014-07-25 06:20:22 UTC
Created attachment 281660 [details] [review]
recent-view: Integrate recent view in the shell

I noticed the previous patch did not handle properly the toolbar in fullscreen mode. Now it does.
Comment 97 Germán Poo-Caamaño 2014-07-25 06:32:07 UTC
Created attachment 281661 [details] [review]
recent-view: Integrate recent view in the shell

Maybe it's a good idea to not show the 'recent-view' toggle button in fullscreen mode.
Comment 98 Germán Poo-Caamaño 2014-07-25 06:38:08 UTC
Created attachment 281662 [details] [review]
recent-view: Integrate recent view in the shell

It seems I uploaded a wrong patch.
Comment 99 Carlos Garcia Campos 2014-07-25 08:40:26 UTC
(In reply to comment #97)
> Created an attachment (id=281661) [details] [review]
> recent-view: Integrate recent view in the shell
> 
> Maybe it's a good idea to not show the 'recent-view' toggle button in
> fullscreen mode.

I don't think so, but we can do it later if we really want it, for now, I would not allow to enter fullscreen mode in recent view mode, you can just maximize the window
Comment 100 Carlos Garcia Campos 2014-07-25 09:06:36 UTC
Comment on attachment 281662 [details] [review]
recent-view: Integrate recent view in the shell

Pushed with some minor changes
Comment 101 Carlos Garcia Campos 2014-07-25 09:11:07 UTC
Comment on attachment 279172 [details] [review]
shell: Remove menu for recent items

Removed also the ev-recent-menu-model files and pushed to git master.
Comment 102 Carlos Garcia Campos 2014-07-25 09:11:35 UTC
This is done now, thank you all guys!