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
