D8642: Rework saving of annotations and form data

Fri Nov 10 22:48:20 UTC 2017

rkflx added a comment.

  Another great feature \o/. Good news, I cannot reproduce or even describe the single crash I got and **there are only small issues to be fixed before this can land (IMHO). The rest is optional polishing.** (Thanks for the extensive test plan, BTW, made it easier to review. Sorry it took a while to find enough time for reviewing. Still some things to check, will continue tomorrow.)

  As you've commented on some bugs with the Phab URL, I guess you'll close them manually? Have found several more related bugzillas, will triage and maybe close some once the Diff is in (I assume you'll just merge the branch to preserve Fabio's copyright?).

  Comments on the code
  --------------------

  - `parttest` fails (missing `potato.jpg`)
  - see Laurent's comments
  - more `@since` wrong (e.g. `TODO`, `0.21`, …)
  - Looked only briefly at the individual commits, it's just too much code to analyze in depth (i.e. "I trust you on the `const_cast`" and the tests you added :). In general the `swapBackingFile` mechanism seems to add a huge amount of complexity, but I guess if there was an easier path you would have followed it.
  - Did you run this with ASAN and friends to check for any problems?
  - There is some code duplication for non-trivial amounts of code (grep for `loadSyncFile`).

  UI: Important issues
  --------------------

  (I consider these must-haves for the RC latests, but some might break string freeze)

  1. [Ctrl] + [S] saves and is visible in Configure Shortcuts, but does not show up in the menu.

  2. Got several "Lost annotation on document save, something went wrong" on the console. However, to prevent data loss this should show a warning in the UI and allow aborting (just show the warning dialog from below and amend the list appropriately). I'll try to add steps to reproduce as soon as I can (might be an annotation created elsewhere, i.e. already present in the document and thus should not be lost indeed).

  3. Only looking at the format warning dialog (screenshot below), what will the Continue button do? It is confusing and potentially harmful. In addition, the wording does not maintain a reference to the original intention of saving. Suggestions for rewording: "You are about to save changes, but the //<format>// file format does not support saving the following elements. Please use the //Okular document archive// format to preserve them." Remove the Continue button (in case of "saving+closing" instead of only "saving", text and behaviour could just be changed to Discard, but removal would be okay too). (See also 11. and 13.)

  UI: Polishing
  -------------

  (Concerning the overall user experience of the Diff and related functionality, not blocking this patch. Will file issues for those that won't/cannot be fixed in the near term.)

  4. [Ctrl] + [Z] to the point where the document state corresponds to the state on disk (e.g. last save point or complete undo for non-yet-saved files). The document is still marked as changed ("*" shown in title, warning dialog despite there being no changes). Instead, the state should return to "unchanged", i.e. same behaviour as in every "editor".

  5. Adding Save to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo).

  6. Show dirty state in tab title too, not only in window title (avoids surprises and clicking back and forth when there are multiple changed tabs).

  7. "Do you want to save your changes or discard them?": Would be nice to mention what changed exactly ("Huh, I did not change //anything//!") because for PDFs this might not be obvious, e.g. by using a similar dialog to the one with the list you get when Saving a PNG, or just by changing the text. Also mention the filename. Suggestion (separate text for forms, if possible, to avoid the "and/or"): "Annotations and/or forms in the document "<filename>" have been modified. <linebreak>Do you want to save your changes or discard them?".

  8. When closing multiple dirty tabs, the warning dialog brings the document in question to the front, which is nice. However, this might not be obvious for every user and for small window sizes especially. It should be sufficient to read the text in the dialog, instead you have to read the title of the (potentially darkened by KWin) window behind. Mention at least the filename in the dialog text, optionally use the same dialog with a checkable list of documents as Kate already has.

  "unsupported" warning dialog:
  F5486097: okular-unsupported-warning.png <https://phabricator.kde.org/F5486097>

  9. Padding below text is much larger than below list. Reduce, maybe also top-align so the icon does not look so out of place.
  10. Reduce default height of list if there will only ever be one or two items listed.
  11. It would be worth thinking about not cascading the warning dialogs ("changes" + "wrong format"). Instead, directly show the second dialog, but with a Discard button.

  Okular document archive:

  12. File > Open does not show "Okular document archives".
  13. Inconsistent wording for ".okular" format, e.g. "Okular archive" in Save As, but "Okular document archive" in warning dialog for Save of e.g. PNG (check sources and docs for more). I'd probably prefer "Okular archive" because it is shorter (important for buttons) and "document" is not really meaningful (in general, but also for images).
  14. "filename.okular" in window title, but tab still says "filename.pdf" when using Save As > Okular archive.
  15. No icons for ".okular" files in tab bar.

  "password" warning dialog:
  F5486094: okular-password-warning.png <https://phabricator.kde.org/F5486094>

  16. Grammar of "has password" is wrong, but I'd suggest some rewording anyway (e.g. to get rid of "we", "stack" and more): "The current document is protected with a password. In order to save, the file needs to be reloaded. You will be asked for the password again and your undo/redo history will be lost. Do you want to continue?"
  17. The hyphen in the title does not really work. I'd just use "Warning" with nothing in front like in the other dialog.
  18. Improve buttons to Continue and Cancel.
  19. (Ideally, we would remember the password and the undo stack. I understand this is a technical limitation we currently have.)
  20. Note for the other new dialogs which are similar (grep for `i18n`):
    - Apply same changes.
    - "Continue losing changes": Sounds weird, maybe use Continue in both cases or Discard? (I'd need to see the dialog in context.)

  `KMessageWidget` storage migration warning:

  21. Assume the user closes the warning at first or by accident, but now wants it back. [F5] or reopening will not help, only restarting Okular brings it back. Is there any reason for this behaviour? If not, the warning should be shown everytime the document is (re)loaded. (← very minor issue, as the migration is probably not used very often)
  22. Wording: Remove linebreak and replace "and continue editing the document" with "to a supported storage format, if you want to continue to edit the document."

INLINE COMMENTS

> index.docbook:992
>  					<listitem>
> -					  <para><action>Saves</action> the document under a new name including all the changes (annotations, form contents, &etc;), provided the document backend supports saving changes. With the &PDF; backend it is possible to save the document with the changed values of the form fields.<!--Only useful for pdf? what happens with other formats/backends?--> It can be possible (provided that the data were not secured using DRM) to save annotations with &PDF; files.</para>
> -                                        <note>
> -                                        <para>
> -                                            Note that, due to the way this is implemented, even if there are no changes to the file, the new file need not to be an exact bit-for-bit copy of the original file (⪚ can have a different SHA-1 hash, &etc;).
> -                                        </para>
> -                                        </note>
> +					  <para><action>Saves</action> the document including all the changes (annotations, form contents, &etc;), provided the document backend supports saving those changes, if the backend does not support saving the changes the user will be give the option to lose them or to save as &okular; archive.</para>
>  					</listitem>

Same changes as in other comment, please.

> index.docbook:1007
>  					<listitem>
> -						<para><action>Saves</action> a copy of the original document under a new name (completely bypassing the document backend). The saved document will be a bit-for-bit copy of the original.</para>
> +					  <para><action>Saves</action> the document under a new name including all the changes (annotations, form contents, &etc;), provided the document backend supports saving changes, if the backend does not support saving the changes the user will be give the option to lose them or to save as &okular; archive.</para>
> +                                        <note>

- "if the backend": start new sentence
- "the user will": add comma before
- "will be give" → "will be given"
- "to lose them…" → "to either discard or to save them together with the document in an &okular; archive."

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8642

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20171110/cf5cd64b/attachment-0001.html>