GNOME Bugzilla – Bug 628617
Reloading does not take index creation into account
Last modified: 2014-11-15 22:13:31 UTC
Hi, reloading a document does not take index creation into account. Steps to reproduce: =================== 1) open a (pdf) document without index. The "index" choice in the side panel is unavailable. 2) update the document to add an index (for instance, use package hyperref if this is a LaTeX document). 3) reload document: the index is still not available. 4) "Open a copy" of the document: the index works. Very minor bug, but I wasted 10 minutes wondering what was wrong with my use of hyperref before figuring out it was a bug of Evince :-( Best, Gabriel
Still possible to reproduce with evince master (>3.9.2)
Created attachment 270968 [details] Testcase to reproduce the bug This is a latex file with index.
I have built the latest evince and I am able to reproduce the bug. Steps to reproduce 1.Remove index from temp.tex 2.latex temp.tex 3.pdflatex temp.tex( this will create temp.pdf) 4. run evince and open temp.pdf 5.Add index to temp.tex 6.latex temp.tex 7.pdflatex temp.tex Now you can see that there is no index in the sidepane even after reloading it.If you open it in a new window you can see index there . I would like to work on this bug,Any points to start ?
(In reply to comment #1) > Still possible to reproduce with evince master (>3.9.2) I am able to reproduce the bug and I have attached a testcase in the previous comment. I would like to solve this bug,Any pointers to start ?
Created attachment 273019 [details] [review] Enables the index option in the sidebar The change in ev-sidebar makes the 'links' item enabled. The other changes are related to the fact that, if 'links' is selected in the sidebar and the document is reloaded without links, the page would still be shown, although 'links' is disabled. To fix that, a call to setup_sidebar_from_metadata was added to ev_window_document_changed_cb. To avoid multiple calls to the sidebar setup on initialization, it was removed from setup_document_from_metadata (a call to sidebar setup is added to ev_window_open_document because of this change). ev_window_sidebar_set_current_page has an extra case that sets the sidebar to thumbnails by default.
Created attachment 273024 [details] [review] Fixing spaces/tabs and created a formated patch
Created attachment 273032 [details] [review] Enables 'links' when reloading a document Nicer commit message :)
Review of attachment 273032 [details] [review]: Thanks for the patch. We don't read the metadata again when document changes, so it's a bit weird that now we do that but only for the sidebar. Instead, I think in ev_sidebar_document_changed_cb we should check if the current page still makes sense, and change it otherwise, instead of using the metadata methods for that.
Created attachment 273435 [details] [review] Sidebar update no longer relies on metadata Thank you for the feedback Carlos. I updated the patch accordingly. I still left the default case in ev_window_sidebar_set_current_page because of the following: if the document does not support the current page and the one being set, the current page is not changed. I don't know if this happens in other cases, but I think it is safe to fallback to thumbnails.
Review of attachment 273435 [details] [review]: This is not actually specific to links, no? I guess it's the same if document doesn't have layers or attachments, and after reloading it, it has either layers or attachments, the sidebar pages are enabled. I think you could split this in 3 patches: 1) Enable pages the are supported after reloading 2) Switch to the first supported page after reloading when the current page is no longer supported 3) Fallback to thumbnails when the page set by metadata is not supported by the document Thanks! ::: shell/ev-sidebar.c @@ +496,3 @@ gboolean valid; gboolean has_pages = FALSE; + gboolean support = ev_sidebar_page_support_document (EV_SIDEBAR_PAGE (current_page), document); I think we can add a helper function ev_sidebar_current_page_support_document(). @@ +513,3 @@ + gtk_widget_set_sensitive (menu_widget, TRUE); + if (!support) { + ev_sidebar_set_page (sidebar, widget); So we change to the first one that is supported? I think it would be simpler if we save the first one supported here, and we use it also instead of has_pages. And after the loop, we check if we have pages (first_active_page != NULL) and in such case if ev_sidebar_current_page_support_document() returns FALSE we change to the active page.
Created attachment 275454 [details] [review] Sets thumbnails as the default sidebar page If the page in the metadata is not supported by the document, the sidebar falls back to thumbnails.
Created attachment 275456 [details] [review] Enable supported pages after reloading Once a document is reloaded, possible new supported pages in the sidebar are enabled.
Created attachment 275457 [details] [review] Sidebar selects first supported page after reloading When a document is reloaded and the current page in the sidebar is no longer supported, the first supported page is selected.
Comment on attachment 275454 [details] [review] Sets thumbnails as the default sidebar page Pushed, thanks!
Comment on attachment 275456 [details] [review] Enable supported pages after reloading Pushed
Review of attachment 275457 [details] [review]: Fixed my comments and pushed the patch, thanks! ::: shell/ev-sidebar.c @@ +487,3 @@ +static gboolean +ev_sidebar_current_page_support_document (EvDocumentModel *model, + EvSidebar *sidebar) This method should receive the EvSidebar as first argument, and the second one can be the document instead of the model. @@ +505,3 @@ GtkTreeIter iter; gboolean valid; + gboolean first_supported_page = NULL; This is not correct, it should be GtkWidget * not gboolean @@ +521,2 @@ gtk_widget_set_sensitive (menu_widget, TRUE); + if (first_supported_page == NULL) first_supported_page = widget; Use two lines here.
> @@ +505,3 @@ > GtkTreeIter iter; > gboolean valid; > + gboolean first_supported_page = NULL; > > This is not correct, it should be GtkWidget * not gboolean Oops... basic mistake. It was late, sorry!! Thank you for pushing :)
*** Bug 740192 has been marked as a duplicate of this bug. ***