dfaure added inline comments.
INLINE COMMENTS
> katesecuretextbuffer.cpp:1
> +#include "katesecuretextbuffer.h"
> +
Missing copyright header
> katesecuretextbuffer.cpp:39
> +{
> + // QTemporaryFile sets permissions to 0600, so fixing this
> + if (newFile) {
Isn't it possible to call setPermissions on the QTemporaryFile instance instead?
> katesecuretextbuffer.cpp:61
> +
> + // remove target file if there is any
> + if (!newFile) {
Not sure it matters, but exists+remove+rename is racy. The file could be
deleted by another process after exists() and before remove() (which would then
fail) or the file could be created by another process after the call to
remove() (and then rename() would fail).
QSaveFile doesn't have these issues because it uses the POSIX rename() function
which is atomic (and replaces any existing files). Unfortunately QFile doesn't
expose such a method (one is supposed to use QSaveFile instead...).
> katesecuretextbuffer.h:1
> +#ifndef KATE_SECURE_TEXTBUFFER_H
> +#define KATE_SECURE_TEXTBUFFER_H
Missing copyright header
Assuming the file is not installed, I would name it _p.h (but I don't know if
the rest of this framework follows this convention - I think it's in the KF5
guidelines though).
> katesecuretextbuffer.h:11
> +
> +class SecureTextBuffer : public QObject
> +{
a bit of docu maybe?
> katetextbuffer.cpp:75
> , m_lineLengthLimit(4096)
> + , m_alwaysUseKAuthForSave(KTextEditor::EditorPrivate::unitTestMode() ?
> alwaysUseKAuth : false)
> {
Isn't it weird to ignore the constructor argument in some cases?
> katetextbuffer.cpp:912
> + KAuth::ExecuteJob *job = saveAction.execute();
> + ok = job->exec();
> + }
Nested event loops are evil. Any chance to make this asynchronous (connecting
to a slot instead, for anything that needs to be done after completion of the
job) ?
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4847
To: martinkostolny, dhaumann, #ktexteditor
Cc: dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin,
davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
