SUMMARY Observed with okular versions since 1.3.3 on Ubuntu 18.04. When opening a PDF with annotations using the old "internal format" storage the user is prompted to save the annotations to the PDF file. "This document contains annotations or form data that were saved internally by a previous Okular version. Internal storage is no longer supported. Please save to a file in order to move them if you want to continue to edit the document." When saving the annotations, the original create and modify time stamps are lost and replaced with the current datetime. STEPS TO REPRODUCE 1. use current version of okular to open a PDF with annotations stored in the original internal storage (eg. okular xml file for pdf in ~/.kde/share/apps/okular/docdata/) 2. the need to migrate is detected 3. saving the file to a new PDF preserves the original annotation. 4. the create and modify time stamps of from the original annotation are lost and replaced with the current date. OBSERVED RESULT The original time stamps for create and modify of an annotation are lost on import to the PDF and replaced with the current date. EXPECTED RESULT The original time stamps for create and modify of an annotation are should be preserved and imported to the PDF. SOFTWARE/OS VERSIONS Windows: macOS: Linux/KDE Plasma: Ubuntu 18.04 is test system but any version of okular seems to behave this way. (available in About System) KDE Plasma Version: KDE Frameworks Version: 5.44.0 and above Qt Version: 5.9.5 (built against 5.9.5) and above ADDITIONAL INFORMATION This appears to be part of how the annotations are imported. The annotation is properly reconstructed from the original annotation but the metadata on the annotation is not migrated to the new annotation format. The migration logic seems to begin here: https://github.com/KDE/okular/blob/master/core/document.cpp#L1192 This appears to reach the code where the local data is converted to new annotation format. https://github.com/KDE/okular/blob/master/core/page.cpp#L811 The newly instantiated annotations don't appear to have their time stamps explicitly set from the original so the new instance timestamps are the ones that get saved.
Sure, valid issue, but at this point how many people are left using such old versions that this is actually an issue we want to spend time fixing?
Created attachment 130742 [details] attachment-26762-0.html It’s an issue in 1.10 so wouldn’t call it old. 1.3 is in Ubuntu 18.04 and is a valid upgrade path from soon to expire Ubuntu 16.04 which included a prior okular release that still used the internal storage for annotation. This is a bug still present in the data migration routines of 1.10 that actually causes data loss by ignoring the metadata of the original annotation. For those of us who use the annotation feature to take notes in Okular and are confronted by the loss of timestamps of a large collection of annotations, it’s a real bug that doesn't have any resolution but in the code base of okular during migration steps. I filed the bug as the first step of problem resolution with the intent of coding a solution. I believe it comes down to a line or so of code: set the properties from the original annotation after creating the migrated Annotation instance. See the code sections referenced in the report. I’m looking for bug reviewers that can help guide that implementation. On Sun, Aug 9, 2020 at 5:13 PM Albert Astals Cid <bugzilla_noreply@kde.org> wrote: > https://bugs.kde.org/show_bug.cgi?id=425148 > > Albert Astals Cid <aacid@kde.org> changed: > > What |Removed |Added > > ---------------------------------------------------------------------------- > CC| |aacid@kde.org > > --- Comment #1 from Albert Astals Cid <aacid@kde.org> --- > Sure, valid issue, but at this point how many people are left using such > old > versions that this is actually an issue we want to spend time fixing? > > -- > You are receiving this mail because: > You reported the bug.
Here's a proposed fix to the also set the properties when creating the new annotations. https://invent.kde.org/graphics/okular/-/merge_requests/239 I look forward to suggestions on whether this fix would be sufficient. I'm new to KDE dev and okular so don't understand the code base well, so accept that this may be naive and require additional considerations.
(In reply to Albert Astals Cid from comment #1) > Sure, valid issue, but at this point how many people are left using such old > versions that this is actually an issue we want to spend time fixing? Yes, I think we should support upgrading from such old versions. At least when the fix does not require structural changes in Okular. The submitted MR 239 looks promising to me, although it doesn’t pass tests right now.
The first pipeline failed On clang because of tab vs space. I added a new commit to fis it and the second one passed. I have yet to test it because I’m still getting my local build env set up. This is currently based on a code read.
(In reply to David Hurka from comment #4) > (In reply to Albert Astals Cid from comment #1) > > Sure, valid issue, but at this point how many people are left using such old > > versions that this is actually an issue we want to spend time fixing? > > Yes, I think we should support upgrading from such old versions. At least > when the fix does not require structural changes in Okular. The submitted MR > 239 looks promising to me, although it doesn’t pass tests right now. To be fair we support upgrading from such old versions, one could argue that the annotation timestamp being changed is even correct since now it's inside the PDF and previously wasn't, but i guess one could argue that this is a technicality and that the annotation was really added in the past to the users point of view.
(In reply to Albert Astals Cid from comment #6) > > To be fair we support upgrading from such old versions, one could argue that > the annotation timestamp being changed is even correct since now it's inside > the PDF and previously wasn't, but i guess one could argue that this is a > technicality and that the annotation was really added in the past to the > users point of view. Being the user, I naturally favor the second interpretation. ;) In that line of thinking, the annotation is a data object created by the user. That it was once stored in an an okular xml file and is now stored in the pdf itself is really of little importance to the user. It's the data that I created that I care about. In the current implementation, it's not just the timestamp that I would lose. Since the new annotation element is created by me, I'm the owner, the old object may in fact have a different owner listed in the xml. That owner information is also lost by not migrating the metadata and this seems a even more significant than the time stamp. Thanks for the additional thoughts and support in working on the migration. I'm still in the process of getting a pristine okular to build in my build env so that I can test out the proposed fix with my data. At the moment, most of the build dependencies succeed (78/84). I'm running into an issue with polkit-qt-1.
(In reply to jprorama from comment #7) > I'm running into an issue with polkit-qt-1. Just don't build polkit-qt-1, it's not needed for what you want to test.
I've had a chance to debug this further. It appears that the annotations are properly restored with original metadata (author, create, mod dates etc) during the import. When the message about the internal storage conversion is present, you can look at the annotations and see all the original properties are present. It appears that the date stamps are overwritten when the PDF is saved. This makes my original source code mode unecessary since the attributes are in fact read from internal storage correctly. I'm trying to track down where/why the restored dates are lost during the save.
I'm starting to wonder if this might actually be an issue in Poppler but need some guidance in understanding the logic that Okular follows in the conversion process. As noted, Okular is correctly restoring the original create/mod dates of the annotation from its original storage. It prompts the user to save the PDF to a file to preserve the annotations with the PDF. At this point I have traced lots of code, but it seems to come down to Okular handing of the Save operation to the PDFGenerator::save() method. There is a QHashIteration across the annotations that compares unique ids between okular annotations and poppler annotations, but doesn't seem to have any other affect. The actual save of the current Okular document that needs migration appears to be handed off to the Poppler::PDFConverter(). It seems like this utility takes the current Okular PDF document and saves it out to a temporary file. It's during this operation that the original annotation create/modify times are lost. The temp file is then copied to the saveAs target, but by that time the times are already set to the current date. Is this understanding correct? If so, is there any way to control the conversion of the annotations within Okular to avoid this issue. I thought the PopplerAnnotationProxy might be able to, but it doesn't seem to be invoked by Poppler during the PDFConverter.
There's some issues in the poppler side setting the modification date, i'm working on them, no idea if it will fix this or not, but it's at least a pre-requisite
Created attachment 130978 [details] attachment-29420-0.html Great, thanks! Happy to review/test/contribute. This is a personal need to convert pdfs with preserved timestamps. So i have interest and can make time. 😉 On Tue, Aug 18, 2020 at 2:30 PM Albert Astals Cid <bugzilla_noreply@kde.org> wrote: > https://bugs.kde.org/show_bug.cgi?id=425148 > > --- Comment #11 from Albert Astals Cid <aacid@kde.org> --- > There's some issues in the poppler side setting the modification date, i'm > working on them, no idea if it will fix this or not, but it's at least a > pre-requisite > > -- > You are receiving this mail because: > You reported the bug.
This should help you i think https://gitlab.freedesktop.org/poppler/poppler/-/merge_requests/612
Thanks for the link. I'm working to test it but having difficulty getting my okular build to use the poppler I built of that branch. I'm using kdesrc-build for okular and ninja for poppler (as described by the CI build steps). My kde build env is in ~/kde. I build configured poppler with: cmake -G Ninja -DCMAKE_INSTALL_PREFIX=~/kde/ .. and run install to get the libs in ~/kde/usr/lib ninja install I set okular CMake
ugh. pressed enter too soon. I set Okular CMake to look for poppler version 0.102 which looks like that's the tag on the libraries from the branch. find_package(Poppler "0.102.0" COMPONENTS Qt5) I set LD_LIBRARY_PATH to include ~/kde/usr/lib at the front. Then build okular. When I look at the backend components in okular after loading a pdf, it still reports poppler 0.85 as the build target. Needless to say my documenttest runs aren't showing any impact of the poppler improvements from your branch. Thanks for any build hints.
I'm sorry but bugzilla is not the place to explain how to compile things, if you want drop by the #okular or #poppler IRC channel in freenode and ask for help, if I (or someone else is there) we may help you.
Created attachment 131106 [details] attachment-28902-0.html Got it, thanks! On Sat, Aug 22, 2020 at 4:31 PM Albert Astals Cid <bugzilla_noreply@kde.org> wrote: > https://bugs.kde.org/show_bug.cgi?id=425148 > > --- Comment #16 from Albert Astals Cid <aacid@kde.org> --- > I'm sorry but bugzilla is not the place to explain how to compile things, > if > you want drop by the #okular or #poppler IRC channel in freenode and ask > for > help, if I (or someone else is there) we may help you. > > -- > You are receiving this mail because: > You reported the bug.
I was able to test your latest patch and it does preserve the creation time but still updates the modification time to the current time of the conversion. I've updated the DocumentTest for migration to deal with the dates a little more sanely.
(In reply to jprorama from comment #18) > I was able to test your latest patch and it does preserve the creation time > but still updates the modification time to the current time of the > conversion. Which from poppler's point of view is correct, you're modifying the annotation, so the modification time changes, if you want the modification time to be preserved, you need to make sure we set the modification time the last when recreating the poppler annotation from the okular annotation.
Not sure I’m following. It seems the document is resident in okular with correct create/modify times during migration when loaded from the internal xml. The document is handed to poppler for writing (pdfconvert() i think). It seems poppler should serialize what is there in the okular runtime. This behavior suggest to me that okular has done what you are suggesting: set modification time last during save. Could you clarify? Thanks. > On Aug 24, 2020, at 4:30 PM, Albert Astals Cid <bugzilla_noreply@kde.org> wrote: > > https://bugs.kde.org/show_bug.cgi?id=425148 > > --- Comment #19 from Albert Astals Cid <aacid@kde.org> --- > (In reply to jprorama from comment #18) >> I was able to test your latest patch and it does preserve the creation time >> but still updates the modification time to the current time of the >> conversion. > > Which from poppler's point of view is correct, you're modifying the annotation, > so the modification time changes, if you want the modification time to be > preserved, you need to make sure we set the modification time the last when > recreating the poppler annotation from the okular annotation. > > -- > You are receiving this mail because: > You reported the bug.
(In reply to jprorama from comment #20) > Not sure I’m following. > > It seems the document is resident in okular with correct create/modify times > during migration when loaded from the internal xml. The document is handed > to poppler for writing (pdfconvert() i think). It seems poppler should > serialize what is there in the okular runtime. > > This behavior suggest to me that okular has done what you are suggesting: > set modification time last during save. > > Could you clarify? Thanks. You need to understand the code and add the missing line yourself. There's basically a setModificationDate() call missing after creating the poppler annotation from the stored okular annotation in okular code. The only way for me to be clearer is for me to do the change by myself and then tell you "see if you change this line it works", which i already said i wasn't interested in working on, and I've already spent more time than i wanted on this.