D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  Actually @trufanov, would you mind doing it? I'd rather not commandeer your 
revision or commit it by hand, both of which will break the history here on 
phabricator.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  OK, will do!

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  @ngraham Feel free to do it yourself.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  @trufanov, would you like to make that change? If not I can do it, and either 
way, let's land this tonight.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid accepted this revision.
aacid added a comment.


  Personally i'd change the : for a .
  
  But i don't really care, go ahead i'd say.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

Re: Review Request 130226: CHM Generator Lib Update

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130226/#review103842
---




generators/CMakeLists.txt
Lines 45 (patched)


This description is copied from CHM lib above, please adjust.


- Christoph Feck


On Sept. 12, 2017, 11:07 p.m., Gilbert Assaf wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130226/
> ---
> 
> (Updated Sept. 12, 2017, 11:07 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This patch updates our copy of the chm lib from kchmviewer. The lib files 
> itself are an unmodified copy from kchmviewer 7.7, only our generator needed 
> some changes. In contrast to kchmviewer we still use khtml and therefor still 
> need msits.
> 
> 
> Diffs
> -
> 
>   cmake/modules/FindLibZip.cmake PRE-CREATION 
>   generators/CMakeLists.txt 5eedf4ebb61237f92d5bbf64c216123140d27fd3 
>   generators/chm/CMakeLists.txt 83abe4e24e03b24622117156badb76a1b9d735da 
>   generators/chm/generator_chm.h 1485bc8aae60d662dfc0c01afa2f664dbba6382f 
>   generators/chm/generator_chm.cpp b6a770ece0d46cb7874bfdf388bae8074d240149 
>   generators/chm/lib/bitfiddle.h eb15b0fa9b0d13b27170be76828631d8328b3109 
>   generators/chm/lib/ebook.h PRE-CREATION 
>   generators/chm/lib/ebook.cpp PRE-CREATION 
>   generators/chm/lib/ebook_chm.h PRE-CREATION 
>   generators/chm/lib/ebook_chm.cpp PRE-CREATION 
>   generators/chm/lib/ebook_chm_encoding.h PRE-CREATION 
>   generators/chm/lib/ebook_chm_encoding.cpp PRE-CREATION 
>   generators/chm/lib/ebook_epub.h PRE-CREATION 
>   generators/chm/lib/ebook_epub.cpp PRE-CREATION 
>   generators/chm/lib/ebook_search.h PRE-CREATION 
>   generators/chm/lib/ebook_search.cpp PRE-CREATION 
>   generators/chm/lib/ebook_url.h PRE-CREATION 
>   generators/chm/lib/helper_entitydecoder.h PRE-CREATION 
>   generators/chm/lib/helper_entitydecoder.cpp PRE-CREATION 
>   generators/chm/lib/helper_search_index.h PRE-CREATION 
>   generators/chm/lib/helper_search_index.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontainer.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontainer.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontent.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontent.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubtoc.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubtoc.cpp PRE-CREATION 
>   generators/chm/lib/lchmurlhandler.h 
> 35133c41d764de551a350a240d8ee43d07f84716 
>   generators/chm/lib/lchmurlhandler.cpp 
> 9d98d87e147539ef30817b3c66a040aa750575ad 
>   generators/chm/lib/libchmfile.h cb739ac7914d4856a5f0e8e6793d78e68b0c9628 
>   generators/chm/lib/libchmfile.cpp 60d03bc267eff759495af44b333448a55071b023 
>   generators/chm/lib/libchmfile_search.cpp 
> 76532b18282913ff97c8509d8e384ec0b1f48dfb 
>   generators/chm/lib/libchmfileimpl.h 
> f8d7cc11269a2688fd8b58a30d718ff911b051b3 
>   generators/chm/lib/libchmfileimpl.cpp 
> d10602028e958e7feded362b2ab58e32ca1d1ff0 
>   generators/chm/lib/libchmtextencoding.h 
> 5228b04c10718d407fc49df2e69d220d4efff67c 
>   generators/chm/lib/libchmtextencoding.cpp 
> 0ed3f0710360c7bab113a35c7896e6bbd6664d20 
>   generators/chm/lib/libchmtocimage.h 
> c0d98b3ba27596a731fd0ab24386578f6c58fdf8 
>   generators/chm/lib/libchmtocimage.cpp 
> 2952e8604d8c01360eace2826bbf5dc428155ff1 
> 
> 
> Diff: https://git.reviewboard.kde.org/r/130226/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Assaf
> 
>

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  Yeah, in the above screenshot, `reader.errorString()` seems to have returned 
"Unable to read image data" which is not very helpful. How about a single line 
and the following text:
  
  > This document appears malformed. Here is a best approximation of the 
document's intended appearance:

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


  Sounds good, do we actually get a meaningful error message in this case?
  
  If not i'd say just remove the "Error: [error message]" part.

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#167301, @lueck wrote:
  
  > In https://phabricator.kde.org/D8642#167017, @aacid wrote:
  >
  > > In https://phabricator.kde.org/D8642#164520, @lueck wrote:
  > >
  > > > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
  > > >
  > > > > Note KDE Applications 17.12 is Nov 16, it would be great if we could 
get this in, so please try to review ASAP :)
  > > >
  > > >
  > > > The dialog Warning with the option to save as document archive has a 
field with 
  > > >  one entry "User annotations", what does it mean?
  > > >  What does the second option "Continue" in this dialog do, I see no 
difference to "Cancel"?
  > >
  > >
  > > It says that you can "Continue" saving, but if you continue the "User 
annotations" will be lost. I guess the fact that you didn't understand it means 
it needs a rewording, anything you would suggest to make it more easy to 
understand?
  >
  >
  > "You are about to save changes, but the  file format does not 
support saving the following elements. Please use the Okular document archive 
format to preserve them . Click Continue to save but you will loose these 
elements."
  
  
  I've pushed a text insipired on this.

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#167178, @rkflx wrote:
  
  > > Ah, you want save to be only enabled when the file has been modified? I 
don't see this being a common pattern for the Save action , e.g. kate doesn't 
do that, libreoffice doesn't do that.
  >
  > Indeed, I missed this (still consider it a bug, though). LibreOffice used 
to do that, but they had to change that (and caused some uproar) when changing 
the button to a split button (i.e. with a menu below). In general disabling is 
a common pattern, I remember it from older versions of MS Office. But I agree, 
let's not change it for Okular. If there is support to change that globally, it 
should be done in a coordinated manner someday.
  >
  > > Yes, cp has this problem. This problem is on the other hand not new, if 
with the "old" version of okular you do the same, you will have exactly the 
same problem.
  >
  > The old Okular only had Save As, and as this works fine with the newer 
Okular I assume it worked fine too with the older? In any case, I agree that's 
not a blocker.
  
  
  No, it is broken too, Save As is not a fix for this, the problem is that you 
replace a file A with file B and the poppler code doesn't realize and when 
saving does an incremental update as if the file was still A but now is B and 
everything breaks.
  
  > 
  > 
  >  ---
  > 
  > Thanks for fixing a lot of the problems so far, let me know if/when I 
should verify the fixes or test again before this goes in (and by when you'd 
want me to do this).
  
  On the branch (not put here in this fake diff) i've also fixed:
  
  - Open not showing okular archive option
  - Adding the (*) to the tab name when the file is dirty
  - fixing the tab name when doing a save as
  - the wording of save as that used "Okular archive" that i had manually 
written there, now i'm using the text that comes from the mimetype so it's 
consistent and doesn't need translators to translate it again since it's 
already done in kcoreaddons
  
  Still on the todo list:
  
  - syncing the modified bit to the undo/redo stack so if you add an annotation 
and then undo it, it's not marked as modified
  - Fixing the code in documentcommands.cpp suggested by Laurent
  - Bringing back the messagewidget on reload
  - Fixing duplicated code in swapBackingFile[Archive]
  - Trying to have one warning dialog instead of two
  - padding improvements on the unsupported warning dialog
  - Poppler fix so that saving doesn't fail if the file is changed behind our 
feet
  
  The todo list is still a bit long but most of the changes would be hopefully 
not big (and imho it could really land as it is now) so if you could have a 
look as soon as possible (use the branch) would be great

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8415: Soften correctness of image file open check

[Nathaniel Graham]

ngraham added a comment.


  I am, yes. How does this sound?
  
  > This document appears malformed. Error: [error message]
  >  Here is a best approximation of the document's intended appearance:
  
  This wording (or something similar) has the benefit that it casts Okular as a 
hero--bravely doing its best to render the lousy input you're trying to feed 
it. :-)

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8415: Soften correctness of image file open check

[Albert Astals Cid]

aacid added a comment.


  @ngraham you seem like a native speaker, please commit this with proper 
english-ness

REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

D8642: Rework saving of annotations and form data

[Burkhard Lück]

lueck added a comment.


  In https://phabricator.kde.org/D8642#167017, @aacid wrote:
  
  > In https://phabricator.kde.org/D8642#164520, @lueck wrote:
  >
  > > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
  > >
  > > > Note KDE Applications 17.12 is Nov 16, it would be great if we could 
get this in, so please try to review ASAP :)
  > >
  > >
  > > The dialog Warning with the option to save as document archive has a 
field with 
  > >  one entry "User annotations", what does it mean?
  > >  What does the second option "Continue" in this dialog do, I see no 
difference to "Cancel"?
  >
  >
  > It says that you can "Continue" saving, but if you continue the "User 
annotations" will be lost. I guess the fact that you didn't understand it means 
it needs a rewording, anything you would suggest to make it more easy to 
understand?
  
  
  "You are about to save changes, but the  file format does not support 
saving the following elements. Please use the Okular document archive format to 
preserve them . Click Continue to save but you will loose these elements."

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Burkhard Lück]

lueck added a comment.


  In https://phabricator.kde.org/D8642#167017, @aacid wrote:
  
  > In https://phabricator.kde.org/D8642#164520, @lueck wrote:
  >
  > > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
  > >
  > > > Note KDE Applications 17.12 is Nov 16, it would be great if we could 
get this in, so please try to review ASAP :)
  > >
  > >
  > > Tried all steps in the TEST PLAN:
  > >
  > > 1. Open pdf file, add anotation, close app...  -> Works as expected But 
the second button in the dialog "Close Document" is labeled "Discard" or "No", 
depending how I close: Using window Close button first the second button is 
labeled "No" Using File->Close (Ctrl+W) the second button is labeled "Discard" 
Once I have used File->Close and selected the button "Cancel" in this dialog 
the dialog opened with the window Close button has the second button labeled 
"Discard" now instead of "No"
  >
  >
  > I can't reproduce that :( 
  >  Can anyone reproduce it?
  
  
  Wiped my frameworks environment completely, rebuild and this issue is now gone

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Henrik Fehlauer]

rkflx added a comment.


  > Ah, you want save to be only enabled when the file has been modified? I 
don't see this being a common pattern for the Save action , e.g. kate doesn't 
do that, libreoffice doesn't do that.
  
  Indeed, I missed this (still consider it a bug, though). LibreOffice used to 
do that, but they had to change that (and caused some uproar) when changing the 
button to a split button (i.e. with a menu below). In general disabling is a 
common pattern, I remember it from older versions of MS Office. But I agree, 
let's not change it for Okular. If there is support to change that globally, it 
should be done in a coordinated manner someday.
  
  > Yes, cp has this problem. This problem is on the other hand not new, if 
with the "old" version of okular you do the same, you will have exactly the 
same problem.
  
  The old Okular only had Save As, and as this works fine with the newer Okular 
I assume it worked fine too with the older? In any case, I agree that's not a 
blocker.
  
  ---
  
  Thanks for fixing a lot of the problems so far, let me know if/when I should 
verify the fixes or test again before this goes in (and by when you'd want me 
to do this).

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#167049, @rkflx wrote:
  
  > In https://phabricator.kde.org/D8642#167025, @aacid wrote:
  >
  > > > 1. [Ctrl] + [S] saves and is visible in Configure Shortcuts, but does 
not show up in the menu.
  > >
  > > Works just fine here.
  >
  >
  > To clarify, in the File menu, I only see Save As, but no entry for Save. 
Any special trick to make it show up? (I just used `make install` to 
`~/somedir` and set the paths as outlined here 
.)
  
  
  Ah, part.rc needs a version increase. Done in the branch.
  
  > 
  > 
  >>> 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.
  > 
  > I fully agree, but that's not what I meant. "Adding" was about "When I 
manually add the button to the toolbar",  otherwise I would have written 
"Please add <…>, because ". You may have missed the main part of the 
sentence, which was about the (non)disabled state.
  
  Ah, you want save to be only enabled when the file has been modified? I don't 
see this being a common pattern for the Save action , e.g. kate doesn't do 
that, libreoffice doesn't do that.
  
  > 
  > 
  >>> 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 "" 
have been modified. 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?
  > 
  > Good point. Just use my wording suggestion then (which does not imply 
having the list).
  
  Sincerely i don't see the need of saying "Annotations and/or forms", users 
are not that stupid. As mentioned in other comments i've already added the 
filename to the text.
  
  > 
  > 
  >>> 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.
  > 
  > Okay, my main motivation was more about rewording the last part which I 
stumbled upon. Could you change this to ", if you want to continue to edit the 
document."?
  
  Changed.
  
  > 
  > 
  > In https://phabricator.kde.org/D8642#167037, @aacid wrote:
  > 
  >> > 23. Data loss on external change despite trying to save: Open, add 
annotation, change file externally, wait for warning, Save. Instead of 
overwriting, there is an error in the UI ("Could not open file:///…") and on 
the console ("The document hasn't been reloaded/swapped correctly"). After 
this, the file is corrupted but non-zero sized, i.e. Okular is not able to open 
it (MuPDF works, amazingly). Binary diff shows annot stream added at the end, 
perhaps some problem there? Note that Cancel and Save as with a different 
filename does work correctly. We do seem to have the data, but there is an 
issue saving to a file with the same name (not sure what this means in terms of 
file handles).
  >>
  >> I can't reproduce this, what do you mean by change file externally 
exactly? touch? edit it? remove it? copy something else over? ( i tried touch 
and my awesome pdf editing skills with vim)
  > 
  > 
  > Cannot check right now, IIRC this was with both `latexmk` and `cp 
somefile.pdf file_opened_in_okular.pdf` (comment if you still cannot reproduce 
and I will check in the evening). That's obviously not part of a regular 
workflow, but could happen by accident in reality.
  
  Yes, cp has this problem. This problem is on the other hand not new, if with 
the "old" version of okular you do the same, you will have exactly the same 
problem. I will try to fix it since you're right you can get data loss but it's 
nothing new and thus i don't think we should block the landing of this feature 
because of that.

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#166482, @rkflx wrote:
  
  > > 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).
  >
  > Steps to reproduce:
  >
  > - Download and Open sample document 
,
 add annotation, Save.
  > - Observe warning on console ("Lost annotation on document save, something 
went wrong").
  > - Review sidebar shows only multiple "Page 1" where before the annotations 
where listed correctly (they are still shown in the document, though).
  > - Manual Reload restores all old annotations as well as the newly added 
one, Review sidebar no longer broken.
  >
  >   Given no actual data was lost, I'd not consider this critical anymore. 
However, it is quite scary and should be fixed.
  >
  >   The same behaviour can be seen when Okular opens 
`autotests/data/file1.pdf` annotated on Android with "Adobe Acrobat" or "MuPDF".
  >
  >   ---
  >
  >   Edit: To clarify, I assume this is fixable in the backend, no UI warning 
necessary as originally proposed.
  
  
  Found the problem and fixed in the branch (still need to update the diff here)

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Henrik Fehlauer]

rkflx added a comment.


  In https://phabricator.kde.org/D8642#167025, @aacid wrote:
  
  > > 1. [Ctrl] + [S] saves and is visible in Configure Shortcuts, but does not 
show up in the menu.
  >
  > Works just fine here.
  
  
  To clarify, in the File menu, I only see Save As, but no entry for Save. Any 
special trick to make it show up? (I just used `make install` to `~/somedir` 
and set the paths as outlined here 
.)
  
  >> 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.
  
  I fully agree, but that's not what I meant. "Adding" was about "When I 
manually add the button to the toolbar",  otherwise I would have written 
"Please add <…>, because ". You may have missed the main part of the 
sentence, which was about the (non)disabled state.
  
  >> 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 "" 
have been modified. 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?
  
  Good point. Just use my wording suggestion then (which does not imply having 
the list).
  
  >> 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.
  
  Okay, my main motivation was more about rewording the last part which I 
stumbled upon. Could you change this to ", if you want to continue to edit the 
document."?
  
  In https://phabricator.kde.org/D8642#167037, @aacid wrote:
  
  > > 23. Data loss on external change despite trying to save: Open, add 
annotation, change file externally, wait for warning, Save. Instead of 
overwriting, there is an error in the UI ("Could not open file:///…") and on 
the console ("The document hasn't been reloaded/swapped correctly"). After 
this, the file is corrupted but non-zero sized, i.e. Okular is not able to open 
it (MuPDF works, amazingly). Binary diff shows annot stream added at the end, 
perhaps some problem there? Note that Cancel and Save as with a different 
filename does work correctly. We do seem to have the data, but there is an 
issue saving to a file with the same name (not sure what this means in terms of 
file handles).
  >
  > I can't reproduce this, what do you mean by change file externally exactly? 
touch? edit it? remove it? copy something else over? ( i tried touch and my 
awesome pdf editing skills with vim)
  
  
  Cannot check right now, IIRC this was with both `latexmk` and `cp 
somefile.pdf file_opened_in_okular.pdf` (comment if you still cannot reproduce 
and I will check in the evening). That's obviously not part of a regular 
workflow, but could happen by accident in reality.

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#166462, @rkflx wrote:
  
  > Most of my final checks passed, there is one more serious problem though:
  >
  > 23. Data loss on external change despite trying to save: Open, add 
annotation, change file externally, wait for warning, Save. Instead of 
overwriting, there is an error in the UI ("Could not open file:///…") and on 
the console ("The document hasn't been reloaded/swapped correctly"). After 
this, the file is corrupted but non-zero sized, i.e. Okular is not able to open 
it (MuPDF works, amazingly). Binary diff shows annot stream added at the end, 
perhaps some problem there? Note that Cancel and Save as with a different 
filename does work correctly. We do seem to have the data, but there is an 
issue saving to a file with the same name (not sure what this means in terms of 
file handles).
  
  
  I can't reproduce this, what do you mean by change file externally exactly? 
touch? edit it? remove it? copy something else over? ( i tried touch and my 
awesome pdf editing skills with vim)

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

[okular] [Bug 386316] Faster scroll speed for arrow keys

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=386316

--- Comment #2 from muntoo...@gmail.com ---
The issue with PgUp/PgDown is that it does the entire screen and you lose
surrounding context.

Perhaps fast-scrolling can be done by a modifier like: `Shift-Up/Down` or
`Ctrl-Up/Down`. This is similar to video players.

As further justification, other PDF readers have much faster scroll rates.
Okular seems to be an anomaly in this regard.

-- 
You are receiving this mail because:
You are the assignee for the bug.

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

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  
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 

D8642: Rework saving of annotations and form data

[Michael Weghorn]

michaelweghorn added a comment.


  In https://phabricator.kde.org/D8642#167020, @aacid wrote:
  
  > In https://phabricator.kde.org/D8642#165611, @michaelweghorn wrote:
  >
  > > > Open pdf file, add anotation, close app
  > > >  You get dialog about losing changes, check that save, discard, cancel 
all do what they say
  > >
  > > This works as described. One additional thought on that:
  > >  Some users may want to save the modified version to a new file instead 
of overwriting the existing one. While this is already possible via "File" -> 
"Save As" (or Ctrl+Shift+S), I think it might be helpful to add an additional 
button "Save as" in that dialog as well. This would also help avoid users 
accidently overwriting the original file. (Evince, for example only offers a 
"Save As" option and users switching between the two programs might not be 
aware of the difference at once.)
  >
  >
  > I disagree, does libreoffice suggest "Save As" when you do open an existing 
file, do some changes and close the app? Do you know of any program that does 
offer "Save As" in that situation?
  
  
  As mentioned, Evince does. Apart from that, I am currently not aware of 
another program that does similarly. I don't consider the "Save as" button 
necessary if the clarification as mentioned below is implemented.
  
  > 
  > 
  >> Alternatively (or in addition to that), clarifying the question in the 
dialog to something like "Do you want to save your changes TO THE DOCUMENT 
 or discard them?" (capital letters only here to indicate what has 
been added) might possibly be an option as well.
  > 
  > That makes sense and is again what libreoffice does for example, will add.
  
  Thanks! LibreOffice is actually from where I adapted the idea. :-)

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

[okular] [Bug 386111] Selecting text moves it one pixel towards the bottom

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=386111

David Edmundson  changed:

   What|Removed |Added

 CC||k...@davidedmundson.co.uk
   Assignee|okular-devel@kde.org|k...@davidedmundson.co.uk

-- 
You are receiving this mail because:
You are the assignee for the bug.

[okular] [Bug 386110] ctrl+f highlighting only works for low zoom levels

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=386110

David Edmundson  changed:

   What|Removed |Added

 CC||k...@davidedmundson.co.uk
   Assignee|okular-devel@kde.org|k...@davidedmundson.co.uk

-- 
You are receiving this mail because:
You are the assignee for the bug.

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8642#165611, @michaelweghorn wrote:
  
  > > Open pdf file, add anotation, close app
  > >  You get dialog about losing changes, check that save, discard, cancel 
all do what they say
  >
  > This works as described. One additional thought on that:
  >  Some users may want to save the modified version to a new file instead of 
overwriting the existing one. While this is already possible via "File" -> 
"Save As" (or Ctrl+Shift+S), I think it might be helpful to add an additional 
button "Save as" in that dialog as well. This would also help avoid users 
accidently overwriting the original file. (Evince, for example only offers a 
"Save As" option and users switching between the two programs might not be 
aware of the difference at once.)
  
  
  I disagree, does libreoffice suggest "Save As" when you do open an existing 
file, do some changes and close the app? Do you know of any program that does 
offer "Save As" in that situation?
  
  > Alternatively (or in addition to that), clarifying the question in the 
dialog to something like "Do you want to save your changes TO THE DOCUMENT 
 or discard them?" (capital letters only here to indicate what has 
been added) might possibly be an option as well.
  
  That makes sense and is again what libreoffice does for example, will add.

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

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


  In https://phabricator.kde.org/D8642#164520, @lueck wrote:
  
  > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
  >
  > > Note KDE Applications 17.12 is Nov 16, it would be great if we could get 
this in, so please try to review ASAP :)
  >
  >
  > Tried all steps in the TEST PLAN:
  >
  > 1. Open pdf file, add anotation, close app...  -> Works as expected But the 
second button in the dialog "Close Document" is labeled "Discard" or "No", 
depending how I close: Using window Close button first the second button is 
labeled "No" Using File->Close (Ctrl+W) the second button is labeled "Discard" 
Once I have used File->Close and selected the button "Cancel" in this dialog 
the dialog opened with the window Close button has the second button labeled 
"Discard" now instead of "No"
  
  
  I can't reproduce that :( 
  Can anyone reproduce it?
  
  > 
  > 
  > 2. Open png file, add anotation, close app... -> Works as expected Same 
issue with the label of the second button (Discard/No) in the dialog Close 
Document as in 1)
  
  
  
  > The dialog Warning with the option to save as document archive has a field 
with 
  >  one entry "User annotations", what does it mean?
  >  What does the second option "Continue" in this dialog do, I see no 
difference to "Cancel"?
  
  It says that you can "Continue" saving, but if you continue the "User 
annotations" will be lost. I guess the fact that you didn't understand it means 
it needs a rewording, anything you would suggest to make it more easy to 
understand?
  
  > 
  > 
  > 3. With old version of okular, add annotation to pdf file... -> Works as 
expected
  > 4. Open pdf file with forms... ->Works as expected
  > 5. Open png file add an annotation and save it as okular archive... ->Works 
as expected

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid

D8415: Soften correctness of image file open check

[Alexander Trufanov]

trufanov added a comment.


  Ok, I've added a warning for this. Please check wording - english isn't my 
native language. Feel free to change it.
  Attached are screenshots of Okular showing current warning for eng and rus 
locales.F5489936: Screenshot_20171113_113818.png 

  
  F5489935: Screenshot_20171113_113834.png 


REPOSITORY
  R223 Okular

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

To: trufanov, ngraham, #okular, aacid
Cc: aacid, progwolff, ngraham

KDE CI: Applications okular kf5-qt5 SUSEQt5.9 - Build # 4 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Applications%20okular%20kf5-qt5%20SUSEQt5.9/4/
 Project:
Applications okular kf5-qt5 SUSEQt5.9
 Date of build:
Mon, 13 Nov 2017 08:03:55 +
 Build duration:
15 min and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 15 test(s), Skipped: 0 test(s), Total: 16 test(s)Failed: TestSuite.parttest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report88%
(22/25)51%
(138/271)51%
(138/271)34%
(13196/38462)23%
(6082/26457)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(5/5)100%
(5/5)54%
(917/1685)36%
(453/1263)autotests100%
(14/14)100%
(14/14)98%
(1954/1984)48%
(1195/2477)conf6%
(1/17)6%
(1/17)7%
(53/805)0%
(1/272)conf.autotests100%
(1/1)100%
(1/1)100%
(17/17)50%
(7/14)core85%
(41/48)85%
(41/48)49%
(4461/9159)35%
(2139/6031)core.script0%
(0/9)0%
(0/9)0%
(0/494)0%
(0/230)generators.comicbook29%
(2/7)29%
(2/7)3%
(10/397)0%
(0/430)generators.dvi2%
(1/46)2%
(1/46)0%
(9/3571)0%
(1/2423)generators.epub100%
(3/3)100%
(3/3)55%
(184/335)43%
(78/183)generators.fax17%
(1/6)17%
(1/6)2%
(7/432)0%
(0/585)generators.fictionbook60%
(3/5)60%
(3/5)3%
(16/531)0%
(2/454)generators.kimgio100%
(1/1)100%
(1/1)63%
(41/65)31%
(5/16)generators.kimgio.tests100%
(1/1)100%
(1/1)100%
(49/49)46%
(24/52)generators.mobipocket60%
(3/5)60%
(3/5)11%
(13/115)0%
(0/120)generators.ooo27%
(3/11)27%
(3/11)1%
(14/1125)0%
(0/736)generators.plucker100%
(1/1)100%
(1/1)4%
(4/98)0%
(0/36)generators.plucker.unpluck0%
(0/6)0%
(0/6)0%
(0/1652)0%
(0/876)generators.poppler60%
(3/5)60%
(3/5)49%
(646/1317)37%
   

D8642: Rework saving of annotations and form data

[Albert Astals Cid]

aacid marked 12 inline comments as done.
aacid added inline comments.

INLINE COMMENTS

> mlaurent wrote in documenttest.cpp:112
> closeDocument delete m_document with a deleteLater or we need to delete it 
> before finish method ?

delete added.

> mlaurent wrote in document.cpp:4388
> qCWarning(...)

I don't agree, this is something that won't be triggered unless something 
really wrong happened, thus i don't want it hidden by a qCWarning

> mlaurent wrote in document.cpp:4494
> qCWarnign(...)

Same as above, don't agree.

> mlaurent wrote in document.cpp:4729
> coding style : remove space after &

no, that's the original okular style, as you can see on the diff it was exactly 
like this already.

> mlaurent wrote in document.h:744
> 4.14?

The branch was old ;)

> mlaurent wrote in documentcommands.cpp:714
> if button is null do you think that we need to add to QList ?

Yes and no, we need to detect error here, i'll work on an improvement.

> mlaurent wrote in annotationmodel.cpp:113
> qCWarning(...)

again i don't agree with the qcwarning here.

> mlaurent wrote in formwidgets.cpp:89
> qCWarning(...)

disagree again

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid