D4847: KAuth integration in document saving

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170305/fc3ba2fc/attachment-0001.html>