D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid marked 8 inline comments as done.
aacid added a comment.


  In https://phabricator.kde.org/D8642#167826, @rkflx wrote:
  
  > Latest version is much improved, but still not there yet (see below). I'm giving this my approval nevertheless, because I'm confident that you'll be able to do the string changes now and fix the remaining problems in time for the RC (you could pretend I found the crasher only when testing the Beta ;). Please merge at your earliest convenience, I'll try to schedule another test run before the RC.
  >
  > 24. Segfault: Add annotation, Save > Undo > Save. Introduced by https://phabricator.kde.org/R223:055f2db76d58b559b8a215cd5713cdbd7265fddd. Backtrace:
  
  
  I can't seem to reproduce this. Which file and which annotation are you using?
  
  > 25. `parttest` still fails (now requires to press button in dialog, then crashes with similar backtrace to 24.)
  
  Fixed the button press thing, was a regression of my last change to sync the modified status, good thing we have autottests :)  (still don't get a crash)
  
  > 26. Document is now named in warning dialog, but name includes full path. Instead, it should only show the file name as in KWrite, LibreOffice and the tab/window title. I guess it's just missing a `.fileName()`.
  
  Are you sure we want the filename only? what about if you have two files with the same filename and different paths?
  
  > Regarding the file format warning:
  > 
  > - I now understand why we need Continue for Save As.
  > - 27. It is still very confusing when using Save, because for the user Continue and Cancel are exactly the same. Can we special-case with a flag and hide the last sentence and the button here? As Save is the more common operation, this would streamline the user experience massively and reduce frustration. Most users won't read the text and assume Save > Continue //will// actually save the changes.
  
  Makes sense, the code is relatively complicated though, will see what i can get.
  
  > - 28. There is an inconsistency for Close: If a document `canSwapBackingFile` (e.g. for PDF), Continue will keep the document open, but if it cannot (e.g. for epub), Continue losing changes will close the document. I think Discard in the previous dialog is enough to //actually// close, so for Save (the only case triggered by closing) the Continue [l.c.] buttons can be removed, really.
  
  You mean PNG and not PDF, right? you should never get the file format warning for PDF. Otherwise makes sense too i guess, i'll see what i can do.
  
  > - (Note I assume here that all formats support saving either all or no changes, but not only some. Is this true?)
  
  At this point yes
  
  > 
  > 
  > 29. See inline comments.
  > 
  >>> 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?"
  > 
  > Got a nice idea for a very smooth reading dialog: Just remove the question, then you can change the buttons.
  
  I understand you disagree with the current wording, but honestly i like it more than your suggestion.

REPOSITORY
  R223 Okular

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

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