aacid added a comment.
In https://phabricator.kde.org/D8642#166340, @rkflx wrote: > 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? Yes either manually or as part of the merge commit > 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?). Yes, merge is the correct way. > > > 1. Comments on the code > - `parttest` fails (missing `potato.jpg`) Wops, added. > - 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? Yes > - There is some code duplication for non-trivial amounts of code (grep for `loadSyncFile`). You mean in swapBackingFile and swapBackingFileArchive ? I'll try to merge them. > > > 1. 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. Works just fine here. > 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.) "Continue" does what the dialog says, saves losing those changes that are on the list because they are not supported in the format you're saving. I can't remove the Continue button, otherwise you can't save the file (losing data if that is what you want, you had a big-ass dialog warning you about it) > > > 1. 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.) > 1. [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". True, the modified status needs to be in sync with the undo/redo stack, at the moment it isn't, should be easy to fix. > > > 5. Adding Save to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo). I don't agree in having the Save button in the toolbar by default, the toolbar is pretty full as it is now, and while it's true that we can save, i think the majority of people using Okular just use the viewer part. > > > 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). Should be easy to do hopefully, will add to the todo list. > > > 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?". Not sure this is doable, you really want a list of the 185 annotations you added to the document to be listed there? > > > 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. I've added the file url to the text. > "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. Very minor, will only have a look if i really have time for it. > 10. Reduce default height of list if there will only ever be one or two items listed. same > 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. Probably makes sense, not sure how easy is to achieve codewise though, will have a look but can't promise anything. > Okular document archive: > > 12. File > Open does not show "Okular document archives". Wops, that bad, will fix > 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). Will try to unify. > 14. "filename.okular" in window title, but tab still says "filename.pdf" when using Save As > Okular archive. Interesting, will have a look as to why that happens and try to fix it > 15. No icons for ".okular" files in tab bar. That's because we don't have an icon for .okular files. Not a new issue, .okular files have existed since ages > "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?" Sounds good, changed. > 17. The hyphen in the title does not really work. I'd just use "Warning" with nothing in front like in the other dialog. I disagree, i like the "Save - Warning" construct > 18. Improve buttons to Continue and Cancel. Disagreed, Continue might make sense, but why "Cancel"? Cancel is not an answer to "Do you want to continue?" > 19. (Ideally, we would remember the password and the undo stack. I understand this is a technical limitation we currently have.) Remembering a password is a no go (unless you're using kwallet), noone wants their password stored in cleantext in memory. > 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.) Text looks ok to me. > `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) Will check > 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." "to a supported storage format" seems to technical, people would be scared because they don't know what that means, and in most of the cases it will hopefully it will be a pdf file and then the changes can just be saved fine wihtout the scary dialog about the okular archive format. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D8642 To: aacid, mlaurent Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
