D8642: Rework saving of annotations and form data

[Milian Wolff]

mwolff added a comment.


  I started with a cursory glance at the changes, but the change set is huge, which makes it really hard to review, especially for people who have no extensive prior experience with okular. I think some of my comments could be "solved" by adding a comment here or there, as it seems like they are non-issues but rather arise from misunderstanding or lack of information from my side. Help future people looking at this code by writing some more comments. And consider splitting up such monster commits in the future into smaller patches, one building on the other - if possible. Or is this the case already? I.e. could I instead review a feature branch? I know that phabricator sucks in that regard, but at least it would simplify my life at reviewing, even if I'd read patches on the command line and then add comments here

INLINE COMMENTS

> documenttest.cpp:105
> +
> +    // Pretend the user has done the migration
> +    m_document->docdataMigrationDone();

can we test the actual migration, too? i.e. instead of pretending, actually do it?

> documenttest.cpp:112
> +    QCOMPARE( m_document->openDocument( testFilePath, testFileUrl, mime ), Okular::Document::OpenSuccess );
> +    QCOMPARE( m_document->page( 0 )->annotations().size(), 0 );
> +    QCOMPARE( m_document->isDocdataMigrationNeeded(), false );

shouldn't this have an `QEXECTED_FAIL`, considering that in principle the annotations should have been migrated?

> parttest.cpp:827
> +            {
> +                closeDialogHelper.reset(new CloseDialogHelper( &part, QDialogButtonBox::No  )); // this is the "you're going to lose the annotations" dialog
> +            }

this overrides the helper from above, is this desired? do you need a stack of helpers? or do you maybe miss a wait step before (signal spy?) to ensure the previous dialog has been shown and closed?

> parttest.cpp:854
> +            // so we want to give her a last chance to save on close with the "you have changes dialog"
> +            closeDialogHelper.reset(new CloseDialogHelper( &part, QDialogButtonBox::No  )); // this is the "do you want to save or discard" dialog
> +        }

dito, this again potentially overrides the close helper, no?

> document.cpp:1162
>      // 2.1. Save page attributes (bookmark state, annotations, ... ) to DOM
> -    QDomElement pageList = doc.createElement( QStringLiteral("pageList") );
> -    root.appendChild( pageList );
> -    PageItems saveWhat = AllPageItems;
> -    if ( m_annotationsNeedSaveAs )
> -    {
> -        /* In this case, if the user makes a modification, he's requested to
> -            * save to a new document. Therefore, if there are existing local
> -            * annotations, we save them back unmodified in the original
> -            * document's metadata, so that it appears that it was not changed */
> -        saveWhat |= OriginalAnnotationPageItems;
> -    }
> -    // <page list><page number='x'>.... </page> save pages that hold data
> -    QVector< Page * >::const_iterator pIt = m_pagesVector.constBegin(), pEnd = m_pagesVector.constEnd();
> -    for ( ; pIt != pEnd; ++pIt )
> -        (*pIt)->d->saveLocalContents( pageList, doc, saveWhat );
> +    //  -> do this there are not-yet-migrated annots or forms in docdata/
> +    if ( m_docdataMigrationNeeded )

missing "if"

> document.cpp:1170
> +        // read when we opened the file and ignore any change made by the user.
> +        // Since we don't store annotations and forms docdata/ any more, this is
> +        // necessary to preserve annotations/forms that previous Okular version

missing "in"

> generator.h:296
> +         */
> +        virtual SwapBackingFileResult swapBackingFile( const QString &newFileName, QVector<Okular::Page*> & newPagesVector );
>  

whitespace issue after &

> page.cpp:502
>  
> +Annotation * Page::annotation( const QString & uniqueName ) const
> +{

ws issues like above

> page.h:256
> +         */
> +        Annotation * annotation( const QString & uniqueName ) const;
> +

whitespace issue after &

REPOSITORY
  R223 Okular

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

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