D8642: Rework saving of annotations and form data

[Laurent Montel]

mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> documenttest.cpp:112
> +    QCOMPARE( m_document->isDocdataMigrationNeeded(), false );
> +    m_document->closeDocument();
> +}

closeDocument delete m_document with a deleteLater or we need to delete it before finish method ?

> parttest.cpp:785
> +    QTemporaryFile nativeFromArchiveFile( QString( "%1/okrXXXXXX.%2" ).arg( QDir::tempPath() ).arg ( extension ) );
> +    QVERIFY( archiveSave.open() ); archiveSave.close();
> +    QVERIFY( nativeDirectSave.open() ); nativeDirectSave.close();

new line before archiveSave.close() more readable

> document.cpp:4388
> +                {
> +                    qWarning() << "Unhandled undo command" << uc;
> +                }

qCWarning(...)

> document.cpp:4444
> +
> +bool Document::swapBackingFileArchive( const QString &newFileName, const QUrl & url )
> +{

Remove space before url

> document.cpp:4494
> +                {
> +                    qWarning() << "Unhandled undo command" << uc;
> +                }

qCWarnign(...)

> document.cpp:4729
> +
> +Document::OpenResult Document::openDocumentArchive( const QString & docFile, const QUrl & url, const QString & password )
> +{

coding style : remove space after &

> document.h:744
> +         *
> +         * @since 0.20 (KDE 4.14)
> +         */

4.14?

> document.h:758
> +         *
> +         * @since 0.20 (KDE 4.14)
> +         */

4.14?

> document.h:771
> +         *
> +         * @since 0.20 (KDE 4.14)
> +         */

same

> document.h:773
> +         */
> +        bool swapBackingFileArchive( const QString &newFileName, const QUrl & url );
> +

coding style: Remove space before url

> document.h:865
> +         *
> +         * @since 0.14 (KDE 4.20)
> +         */

kde 4.20 ?

> documentcommands.cpp:714
> +    {
> +        FormFieldButton *button = dynamic_cast<FormFieldButton *>(Okular::PagePrivate::findEquivalentForm( newPagesVector[m_pageNumber], oldFormButton ));
> +        m_formButtons << button;

if button is null do you think that we need to add to QList ?

> part.cpp:566
>      m_dirtyHandler->setSingleShot( true );
> -    connect( m_dirtyHandler, &QTimer::timeout,this, &Part::slotDoFileDirty );
> +    connect( m_dirtyHandler, SIGNAL(timeout()),this, SLOT(slotAttemptReload()) );
>  

You can still use
connect(m_dirtyHandler, &QTimer::timeout, this, [this]() {slotAttemptReload();})

> part.cpp:817
>  
> -    m_saveCopyAs = KStandardAction::saveAs( this, SLOT(slotSaveCopyAs()), ac );
> -    m_saveCopyAs->setText( i18n( "Save &Copy As..." ) );
> -    ac->addAction( QStringLiteral("file_save_copy"), m_saveCopyAs );
> -    ac->setDefaultShortcuts(m_saveCopyAs, KStandardShortcut::shortcut(KStandardShortcut::SaveAs));
> -    m_saveCopyAs->setEnabled( false );
> +    m_save = KStandardAction::save( this, SLOT(saveFile()), ac );
> +    m_save->setEnabled( false );

You can use new connect api too

> part.cpp:2476
> +    QScopedPointer<QTemporaryFile> tempFile;
> +    KIO::Job *copyJob; // this will be filled with the job that writes to saveUrl
>  

*copyJob = nullptr; to be sure that it's initialized

> annotationmodel.cpp:113
> +        if ( !item->annotation )
> +            qWarning() << "Lost annotation on document save, something went wrong";
> +    }

qCWarning(...)

> formwidgets.cpp:89
> +    {
> +        qWarning() << "fwButton is not a QAbstractButton" << fwButton;
> +        return;

qCWarning(...)

REPOSITORY
  R223 Okular

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

To: aacid, mlaurent
Cc: mlaurent, michaelweghorn, ngraham, #okular, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20171103/7293a5ab/attachment-0001.html>