GNOME Bugzilla – Bug 767611
save document to the same path it was opened from
Last modified: 2017-12-20 22:19:03 UTC
Hi, lately I have been using the new annotation support quite a lot (thanks for that!), and saving the resulting PDFs with the "Save as..." dialog. My scenario is this: I have a rather large library of research papers, manually organized in a directory hierarchy. Whenever I read a paper, I take annotations, and save the annotated copy *over* the unannotated PDF. (Because to me it makes no sense to have annotated AND unannotated versions, really, since I can always rip the annotations off if I ever need the original again.) What happens in reality is: I open a PDF from say ~/Research/my/deep/hierarchy/paper.pdf, annotate it, and then save it, but the "Save" dialog always defaults to the ~/Documents directory. And every time I have to click my way to ~/Research/my/deep/hierarchy/ to overwrite the old paper. Note that this can happen multiple times per hour. I don't think that bookmarking is a solution: if I bookmark ~/Research, it is still a long way to ~/Research/my/deep/hiearchy/. I would love if evince defaulted to saving the annotated PDF to the same directory the unannotated version is in. There is no real risk of data loss for people with different requirements, as evince already prompts the user with a "do you really want to overwrite?" dialog to prevent accidental overwrites, and clicking on "Documents" on the sidebar is already pretty convenient. Thanks a bunch.
> I would love if evince defaulted to saving the annotated PDF to the same > directory the unannotated version is in. There is no real risk of data loss > for people with different requirements, as evince already prompts the user > with a "do you really want to overwrite?" dialog to prevent accidental > overwrites, and clicking on "Documents" on the sidebar is already pretty > convenient. Sorry, I just realized that the "Save" dialog does not default to ~/Documents, but to some other directory (that is not really easy to control). In any case, AFAICS said directory has little to do with the directory the current document is in. > > Thanks a bunch.
In my case (not an extensive test) it shows the directory of the first document I opened with Evince. Does this make sense to you? This bug is somewhat related to Bug 751742
(In reply to Germán Poo-Caamaño from comment #2) > In my case (not an extensive test) it shows the directory of the first > document I opened with Evince. Does this make sense to you? Yes, thanks for the clarification. This is not the behavior I am endorsing though. I often have dozens of PDFs open at once, so I'd prefer every evince instance to point the save dialog to the directory the *currently opened* document is in. Does it sound reasonable? > > This bug is somewhat related to Bug 751742
(In reply to Stefano Teso from comment #3) > (In reply to Germán Poo-Caamaño from comment #2) > > In my case (not an extensive test) it shows the directory of the first > > document I opened with Evince. Does this make sense to you? > > Yes, thanks for the clarification. > > This is not the behavior I am endorsing though. I often have dozens of PDFs > open at once, ^^^ frequently from widely different paths > so I'd prefer every evince instance to point the save dialog > to the directory the *currently opened* document is in. > > Does it sound reasonable? > > > > > This bug is somewhat related to Bug 751742
(In reply to Stefano Teso from comment #3) > (In reply to Germán Poo-Caamaño from comment #2) > > In my case (not an extensive test) it shows the directory of the first > > document I opened with Evince. Does this make sense to you? > > Yes, thanks for the clarification. > > This is not the behavior I am endorsing though. I often have dozens of PDFs > open at once, so I'd prefer every evince instance to point the save dialog > to the directory the *currently opened* document is in. FWIW, I was trying to understand what you were seeing, not what you were expecting to see :-) I agree that is quite inconvenient the current behaviour, it seems that one may want to overwrite (or write a new copy) in the same directory than the original document, not at any "random" directory. This is particularly true when one is reading and annotating plenty of papers. > > This bug is somewhat related to Bug 751742 Related, yet not the same. At this moment I lack the context on why Evince offers to 'Save a copy' and not 'Save'. My guess is that there is (or was) a chance of data/format loss (e.g. forms support is incomplete). Carlos, José, Christian: Any idea of this behaviour?
I checked the code, and it seems to do the right thing. It tries to save the document in the last directory used either to open or save a file. If that setting is empty, then it determine the directory of the file. That is the expected behaviour one can expect if one saves a copy, an attachment or an image. Similarly as it happens with other applications. That said, when one has a document with annotations, one likely wants to overwrite the same document. But this is a corner case, or I would say, a workaround for the lack of 'Save'. I say that it seems to do the right thing because if we fix Bug 751742 then this bug will become irrelevant. Unless I am missing something. Here are the relevant parts of the code: https://git.gnome.org/browse/evince/tree/shell/ev-window.c#n2820 https://git.gnome.org/browse/evince/tree/shell/ev-window.c#n2465
(In reply to Germán Poo-Caamaño from comment #5) > (In reply to Stefano Teso from comment #3) > > (In reply to Germán Poo-Caamaño from comment #2) > > > In my case (not an extensive test) it shows the directory of the first > > > document I opened with Evince. Does this make sense to you? > > > > Yes, thanks for the clarification. > > > > This is not the behavior I am endorsing though. I often have dozens of PDFs > > open at once, so I'd prefer every evince instance to point the save dialog > > to the directory the *currently opened* document is in. > > FWIW, I was trying to understand what you were seeing, not what you were > expecting to see :-) > > I agree that is quite inconvenient the current behaviour, it seems that one > may want to overwrite (or write a new copy) in the same directory than the > original document, not at any "random" directory. This is particularly true > when one is reading and annotating plenty of papers. > > > > This bug is somewhat related to Bug 751742 > > Related, yet not the same. At this moment I lack the context on why Evince > offers to 'Save a copy' and not 'Save'. My guess is that there is (or was) > a chance of data/format loss (e.g. forms support is incomplete). > > Carlos, José, Christian: Any idea of this behaviour? Yes, PDF format it not actually and editable format, you normally open a pdf to read it. It happens that it has some "editing" features like annotations or forms, but the format itself is still considered read only. It's also common to want to keep the non annotated/filled pdf. That's the rationale, but I agree it's confusing in some cases and could cause people to lose data, like a filled form. So, I'm open to change it. Maybe instead of normal actions of editable formats Save, Save as, we could use Save modifications or something similar. But we also need to warn the user when starting to annotate or fill a form that the changes will be lost if the document is not saved, and not only when quitting without saving.
*** Bug 751742 has been marked as a duplicate of this bug. ***
Created attachment 350587 [details] [review] shell: Save document to the same path it was opened from When annotating or filling a form in a document, this must be saved as a different document as evince does not overwrite documents. The user can expect to store the modified file in the same place than the original document. Previously, evince assumed the latest directory used, or the place where an image or attachment was stored last. Such behaviour is confusing because the latest place opened might have no relation with the document modified. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=767611
Created attachment 350613 [details] [review] shell: Save document to the same path it was opened from Same patch but without mangled headers.
Review of attachment 350613 [details] [review]: ::: shell/evince-menus.ui @@ +137,3 @@ </item> <item> + <attribute name="label" translatable="yes">_Save Modifications…</attribute> So, this makes sense with documents containing annotations or forms, and even in that case, only when there are modifications. Sometimes you just want to save a copy, seeing save modifications when you haven't modified anything is just confusing. Maybe we could change the menu title depending on whether the document has modifications or not?
(In reply to Carlos Garcia Campos from comment #11) > Review of attachment 350613 [details] [review] [review]: > > ::: shell/evince-menus.ui > @@ +137,3 @@ > </item> > <item> > + <attribute name="label" translatable="yes">_Save > Modifications…</attribute> > > So, this makes sense with documents containing annotations or forms, and > even in that case, only when there are modifications. Sometimes you just > want to save a copy, seeing save modifications when you haven't modified > anything is just confusing. Maybe we could change the menu title depending > on whether the document has modifications or not? I hesitated about either way, and I still do. * "Changing" menus can be confusing. For the user point of view, it can confusing that something disappeared when looking for a specific menu (e.g. 'Where did the 'Save modifications go'?) * Having only 'Save modifications' enabled even when there are no modifications is weird. * If the user want to copy a file, they can use the file manager to do so. Maybe leave the menu as 'Save as'. That would cover both cases and it should be more familiar to users. What do you think?
So, we can rename it to save modifications, but enable it only when there are modifications, and rely on open in containing folder for all other file operations like copying.
I still think the most straightforward would be to use path to the directory of the opened file in the Save as dialog. You could still warn a user if he is to overwrite existing file. However, any solution would be great and this option is a must have.
(In reply to Carlos Garcia Campos from comment #13) > So, we can rename it to save modifications, but enable it only when there > are modifications, and rely on open in containing folder for all other file > operations like copying. To be sure we are on the same page: To enable only when the document has been modified, we will need to add some sort of notification (signal) whenever a form is modified. With annotations is straightforward because to add or remove annotation the user must interact with window, and there we can update the UI accordingly.
(In reply to Germán Poo-Caamaño from comment #15) > (In reply to Carlos Garcia Campos from comment #13) > > So, we can rename it to save modifications, but enable it only when there > > are modifications, and rely on open in containing folder for all other file > > operations like copying. > > To be sure we are on the same page: > > To enable only when the document has been modified, we will need to add some > sort of notification (signal) whenever a form is modified. > > With annotations is straightforward because to add or remove annotation the > user must interact with window, and there we can update the UI accordingly. Right, I had forgotten, we have a way to ask if there are modifications, but not a way to get notified when it happens IIRC
I'm really glad you're working on this! I was going to file an issue because the current behavior of defaulting to the previous folder and forcing overwrite (on 3.22) lead me to inadvertently overwrite the last file I had annotated, losing all of my work...
Created attachment 351719 [details] [review] libdocument: Add signal to notify a document has been modified
Created attachment 351722 [details] [review] pdf: Emit signal when a document has been modified Implements document modified signal on PDF backend
Created attachment 351723 [details] [review] shell: Save document to the same path it was opened from Update patch to update the UI and enable "Save modifications" only when the document has been modified.
Review of attachment 351719 [details] [review]: Maybe instead of a signal, we could add a read-only property "modified" that you can also query when you don't care about the specific modifications. And a simple ev_document_is_modified(). We should document that if user is interested in specific modifications should use the interfaces APIs (annots, forms).
Review of attachment 351723 [details] [review]: What do you think? ::: shell/ev-window.c @@ +435,3 @@ dual_mode = ev_document_model_get_dual_page (ev_window->priv->model); + is_modified = (EV_IS_DOCUMENT_FORMS (document) && ev_document_forms_document_is_modified (EV_DOCUMENT_FORMS (document))) || + (EV_IS_DOCUMENT_ANNOTATIONS (document) && ev_document_annotations_document_is_modified (EV_DOCUMENT_ANNOTATIONS (document))); And this would be ev_document_is_modified(); @@ +1484,3 @@ #endif + g_signal_connect (document, "document-modified", And this "notify::modified"
Created attachment 353231 [details] [review] libdocument: Add a property to determine a document has been modified
Created attachment 353232 [details] [review] pdf: Set the modified status of a document after a modification
Created attachment 353233 [details] [review] shell: Save document to the same path it was opened from
Review of attachment 353231 [details] [review]: ::: libdocument/ev-document.c @@ +217,3 @@ + */ +gboolean +ev_document_is_modified (EvDocument *document) Either this should be get_modified() or rename the property to is-modified and rename this to _get_is_modified()
Review of attachment 353232 [details] [review]: Looks good.
Review of attachment 353233 [details] [review]: But this means that one cannot save a copy for any file opened. Why?
Won't this will lead to the file chooser being in a temp directory when the document was opened directly from firefox? Which isn't what you want in that case, since you want to save somewhere *permanent*.
(In reply to Bastien Nocera from comment #28) > Review of attachment 353233 [details] [review] [review]: > > But this means that one cannot save a copy for any file opened. Why? Because of comment #12. https://bugzilla.gnome.org/show_bug.cgi?id=767611#c12 I am ok either way, if we agree in a common text that makes sense for both use cases. "Save as" sounds good to me, but Carlos had a difference preference.
(In reply to Christian Persch from comment #29) > Won't this will lead to the file chooser being in a temp directory when the > document was opened directly from firefox? Which isn't what you want in that > case, since you want to save somewhere *permanent*. Today's behaviour is not any better :-) I think we can check if the target directory is located in a temporary directory and switch to Documents or Home.
*** Bug 788083 has been marked as a duplicate of this bug. ***
Created attachment 360465 [details] [review] libdocument: Add a property to determine a document has been modified Addresses Bastien's review by changed the method name. https://bugzilla.gnome.org/show_bug.cgi?id=767611#c26
Created attachment 360466 [details] [review] shell: Save document to the same path it was opened from Addresses comment made by Bastien and Christian: * It offers always 'Save As', either to save a copy or to save the modifications made to the document. * Before saving the document, we check the document lives in a proper place, that is, not in a temporary directory.
Thanks! Just a few comments: + dir_default = g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS) ? + g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS) : + g_get_home_dir (); g_get_user_special_dir() normally returns non-NULL. I think what you want here is to test for existence of the returned directory, instead. + tmp_dir = g_file_new_for_path (g_get_tmp_dir ()); + dest_dir = dir_name && ! g_file_has_prefix (file, tmp_dir) ? dir_name : dir_default; Maybe test the standard locations /tmp and /var/tmp (in addition to g_get_tmp_dir() which is specified via env vars)?
Created attachment 360573 [details] [review] shell: Save document to the same path it was opened from When annotating or filling a form in a document, this must be saved as a different document as evince does not overwrite documents. The user can expect to store the modified file in the same place than the original document, except when the document lives in a temporary directory (e.g. downloaded automatically with a web browser), in whose case it must fallback to the Documents directory (if set) or the the home directory. Previously, evince assumed the latest directory used, or the place where an image or attachment was stored last. Such behaviour is confusing because the latest place opened might have no relation with the document modified. This patch addresses Christian's comment on #c35. It check for /tmp even though g_get_tmp_dir() returns /tmp in case that the environment variable is set to something different (just in case the directory in the variable does not exists).
Review of attachment 360465 [details] [review]: ::: libdocument/ev-document.c @@ +148,3 @@ + switch (prop_id) { + case PROP_MODIFIED: + document->priv->modified = g_value_get_boolean (value); This should call ev_document_set_modified to ensure the notify signal is emitted @@ +204,3 @@ + "Whether the document has been modified", + FALSE, + G_PARAM_READABLE | This is readwrite, backends are changing it and we have a public setter. @@ +231,3 @@ + * Set the @document modification state as @modified. + * You can monitor changes to the modification state by connecting to the + * notify::modified signal on @document. This comment make more sense in the getter, since it's telling you when you should call the getter. @@ +238,3 @@ +ev_document_set_modified (EvDocument *document, + gboolean modified) +{ Add g_return macro here @@ +240,3 @@ +{ + document->priv->modified = modified; + g_object_notify (G_OBJECT (document), "modified"); Do not emit the notify signal if the value hasn't actually changed.
Review of attachment 360573 [details] [review]: LGTM ::: shell/ev-window.c @@ +2784,3 @@ } else { ev_window_add_recent (window, EV_JOB_SAVE (job)->uri); + ev_document_set_modified (window->priv->document, FALSE); Isn't it FALSE already at this point. I didn't expect the frontends to use ev_document_set_modified, only the backends
Created attachment 360704 [details] [review] libdocument: Add a property to determine a document has been modified The use case is to update the UI to save changes only when is necessary.
Created attachment 360705 [details] [review] pdf: Set the modified status of a document after a modification A PDF can be modified by adding/editing/removing annotations or editing a form.
Created attachment 360706 [details] [review] shell: Save document to the same path it was opened from
(In reply to Carlos Garcia Campos from comment #38) > Review of attachment 360573 [details] [review] [review]: > > LGTM > > ::: shell/ev-window.c > @@ +2784,3 @@ > } else { > ev_window_add_recent (window, EV_JOB_SAVE (job)->uri); > + ev_document_set_modified (window->priv->document, FALSE); > > Isn't it FALSE already at this point. I didn't expect the frontends to use > ev_document_set_modified, only the backends I noticed that pdf_document_save was not calling ev_document_set_modified. I fixed it there and removed it here.
(In reply to Carlos Garcia Campos from comment #37) > Review of attachment 360465 [details] [review] [review]: > [...] i addressed all of your comments here, too.
*** Bug 789618 has been marked as a duplicate of this bug. ***
When will this fix be released? 3.28?
3.28, you are right.