Review Request: Support added for filter loading in another process
Boudewijn Rempt
boud at valdyas.org
Wed Jun 29 15:04:30 BST 2011
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101794/#review4258
-----------------------------------------------------------
Hi,
I'm sorry that I've had to be a bit finicky with the review, but this is such a core part of calligra that some extra care is a good thing. In general, I like the idea a lot, bor the progress updating, I think something more in line with the way KoProgressUpdater/KoUpdater/KoProgressProxy were designed would be better. This is an untested, uncompiled diff against your patch which sort of shows what I mean:
diff --git a/libs/main/KoFilterManager.cpp b/libs/main/KoFilterManager.cpp
index 3dc76c4..e922587 100644
--- a/libs/main/KoFilterManager.cpp
+++ b/libs/main/KoFilterManager.cpp
@@ -64,13 +64,12 @@ KoFilterManager::KoFilterManager(const QString& url, const QByteArray& mimetypeH
d->batch = false;
}
-KoFilterManager::KoFilterManager(const QByteArray& mimeType,const QStringList &extraMimes, KoUpdater *updater) :
- m_document(0), m_parentChain(0), m_graph(""), d(new Private)
+KoFilterManager::KoFilterManager(const QByteArray& mimeType,const QStringList &extraMimes, KoProgressUpdater *updater) :
+ m_document(0), m_parentChain(0), m_graph(""), d(new Private(updater))
{
d->batch = false;
d->importMimeType = mimeType;
d->extraMimesType = extraMimes;
- d->updater = updater;
}
KoFilterManager::~KoFilterManager()
diff --git a/libs/main/KoFilterManager.h b/libs/main/KoFilterManager.h
index af721f9..8e29ad1 100644
--- a/libs/main/KoFilterManager.h
+++ b/libs/main/KoFilterManager.h
@@ -66,7 +66,7 @@ public:
* @param mimeType the mimetype to import to.
*/
explicit KoFilterManager(const QByteArray& mimeType,const QStringList &extraMimes,
- KoUpdater *updater = 0);
+ KoProgressUpdater *updater = 0);
/**
* Create a filter manager for a filter which wants to embed something.
diff --git a/libs/main/OutOfProcessLoad/KoFilterLoader.cpp b/libs/main/OutOfProcessLoad/KoFilterLoader.cpp
index 1226a07..688ebc4 100644
--- a/libs/main/OutOfProcessLoad/KoFilterLoader.cpp
+++ b/libs/main/OutOfProcessLoad/KoFilterLoader.cpp
@@ -28,6 +28,7 @@
#include <KoFilterManager.h>
#include <KoUpdater.h>
+#include <KoProgressUpdater.h>
KoFilterLoader::KoFilterLoader(QString &sourcefilePath, QString &sourcefilePathTypeName, QString& mimeType, QStringList &extraMimes)
: m_localSocket(0),
@@ -53,8 +54,7 @@ int KoFilterLoader::start()
m_localSocket = new QLocalSocket(this);
m_localSocket->connectToServer(QString::number(QCoreApplication::applicationPid()));
- m_updater = new KoUpdater(0);
- connect( m_updater, SIGNAL( sigProgress( int ) ), this, SLOT( setProgress( int ) ) );
+ m_updater = new KoProgressUpdater(this);
KoFilter::ConversionStatus status;
KoFilterManager* manager = new KoFilterManager( QByteArray(m_mimeType.toLatin1()), m_extraMimes, m_updater);
@@ -74,14 +74,14 @@ int KoFilterLoader::start()
return status;
}
-void KoFilterLoader::setProgress( int percent )
+void KoFilterLoader::setValue(int value)
{
QByteArray block;
uchar header = ProgressUpdate;
- uint dataSizeSize = sizeof(percent);
+ uint dataSizeSize = sizeof(value);
block.append((const char*)&header, sizeof(header));
block.append((const char*)&dataSizeSize, sizeof(dataSizeSize));
- block.append((const char*)&percent, sizeof(percent));
+ block.append((const char*)&value, sizeof(value));
m_localSocket->write(block);
m_localSocket->flush();
}
diff --git a/libs/main/OutOfProcessLoad/KoFilterLoader.h b/libs/main/OutOfProcessLoad/KoFilterLoader.h
index 3f84100..c2707df 100644
--- a/libs/main/OutOfProcessLoad/KoFilterLoader.h
+++ b/libs/main/OutOfProcessLoad/KoFilterLoader.h
@@ -23,12 +23,13 @@
#include <qobject.h>
#include <qstring.h>
#include <qstringlist.h>
+#include <KoProgressProxy.h>
class QLocalSocket;
-class KoUpdater;
+class KoProgressUpdater;
-class KoFilterLoader : public QObjectp
+class KoFilterLoader : public QObject, public KoProgressProxy
{
Q_OBJECT
public:
@@ -49,9 +50,16 @@ public:
QString m_sourcefilePathTypeName;
QString m_mimeType;
QStringList m_extraMimes;
- KoUpdater * m_updater;
-private slots:
-void setProgress( int percent );
+ KoProgressUpdater * m_updater;
+
+private:
+
+ // KoProgressProxy implementation
+ int maximum() const { return 100; }
+ void setValue(int value);
+ void setRange(int minimum, int maximum) {}
+ void setFormat(const QString &format) {}
+
};
#endif /* KOFILTERLOADER_H */
diff --git a/words/part/KWDocument.cpp b/words/part/KWDocument.cpp
index 77fb02f..154802f 100644
--- a/words/part/KWDocument.cpp
+++ b/words/part/KWDocument.cpp
@@ -94,6 +94,7 @@ KWDocument::KWDocument(QWidget *parentWidget, QObject *parent, bool singleViewMo
m_frameLayout(&m_pageManager, m_frameSets),
m_mainFramesetEverFinished(false)
{
+ setAsynchronousLoading(true);
m_frameLayout.setDocument(this);
resourceManager()->setOdfDocument(this);
libs/main/KoDocument.h
<http://git.reviewboard.kde.org/r/101794/#comment3486>
A better name would be, I think, "enableOutOfProcessLoading", because it isn't really asynchronous. Also, the getter/setter need documentation.
libs/main/KoDocument.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3489>
Also better rename this to "loadOutOfProcess"
libs/main/KoDocument.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3487>
I don't think this is needed -- if you really need to access the parent of KoDocument, you can use the parent() method to retrieve the QObject parent of the KoDocument object.
libs/main/KoDocument.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3488>
no space after the ! sign.
libs/main/KoFilterChainLink.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3491>
Why don't we have a subtask here anymore?
libs/main/KoFilterManager.h
<http://git.reviewboard.kde.org/r/101794/#comment3495>
I'm not convinced that we need to pass a KoUpdater here -- it would be more consistent to pass a KoProgressUpdater, as in the other constructor.
Hm... and please add dox for extra params.
libs/main/KoFilterManager.h
<http://git.reviewboard.kde.org/r/101794/#comment3490>
just remove commented-out code.
libs/main/KoUpdater.h
<http://git.reviewboard.kde.org/r/101794/#comment3492>
I don't think this constructor should be public, it's only relevant for inheriting from KoUpdater, and shouldn't be publicly callable.
I see you want to construct a kind of "synthetic" KoUpdater instance in KoFilterLoader -- but it would be better to create a KoProgressUpdater instance there.
libs/main/KoUpdater.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3493>
A KoUpdater _always_ should have a d-pointer instance, so this check isn't needed. Having code that has a d-pointer that can be 0 will be very confusing because it conflicts with the pattern everywhere else.
libs/main/OutOfProcessLoad/KoFilterLoader.cpp
<http://git.reviewboard.kde.org/r/101794/#comment3497>
See the patch I included in the main review -- it touches this part.
libs/main/OutOfProcessLoad/KoOutOfProcessLoad.h
<http://git.reviewboard.kde.org/r/101794/#comment3498>
I'd move this one directory up, since this isn't part of the little loader app, is it?
libs/main/OutOfProcessLoad/KoOutOfProcessLoad.h
<http://git.reviewboard.kde.org/r/101794/#comment3499>
Erm... wrong include guard?
- Boudewijn
On June 28, 2011, 12:46 p.m., Matus Hanzes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101794/
> -----------------------------------------------------------
>
> (Updated June 28, 2011, 12:46 p.m.)
>
>
> Review request for Calligra.
>
>
> Summary
> -------
>
> It can happen that filters crash the application while loading invalid file.
> Because the fixing would be to costly I have created this work around.
>
> The idea is to place the loading code into another process and if the filter crashes the main application stays alive.
>
>
> Diffs
> -----
>
> libs/main/CMakeLists.txt 5492748
> libs/main/KoDocument.h 1cd759f
> libs/main/KoDocument.cpp 7c2d8e3
> libs/main/KoFilterChainLink.cpp 628ff95
> libs/main/KoFilterManager.h 5e05b3b
> libs/main/KoFilterManager.cpp d4bc313
> libs/main/KoFilterManager_p.h f7a4485
> libs/main/KoUpdater.h f6406c3
> libs/main/KoUpdater.cpp 484d7e4
> libs/main/KoUpdaterPrivate_p.cpp eae2e54
> libs/main/OutOfProcessLoad/CMakeLists.txt PRE-CREATION
> libs/main/OutOfProcessLoad/KoFilterLoader.h PRE-CREATION
> libs/main/OutOfProcessLoad/KoFilterLoader.cpp PRE-CREATION
> libs/main/OutOfProcessLoad/KoOutOfProcessLoad.h PRE-CREATION
> libs/main/OutOfProcessLoad/KoOutOfProcessLoad.cpp PRE-CREATION
> libs/main/OutOfProcessLoad/main.cpp PRE-CREATION
> words/part/KWDocument.cpp 77fb02f
>
> Diff: http://git.reviewboard.kde.org/r/101794/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matus
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110629/ccb2bb02/attachment.htm>
More information about the calligra-devel
mailing list