[Differential] [Requested Changes] D4847: KAuth integration in document saving

[Dominik Haumann]

dhaumann requested changes to this revision.
dhaumann added a comment.
This revision now requires changes to proceed.


  This is a good patch.
  
  My main concerns are:
  
  - please use const for variables that do not change anymore
  - please don't use const & for primitive data types (like bool) in function arguments, that does not make sense
  - naming conventions: Instead of TextBufferSecure, the word SecureTextBuffer sounds much more natural. Similarly, the header file can be renamed.
  
  Could you provide an updated revision?
  
  Some general questions
  
  - Does that affect other platforms such as Windows in any way? I.e., does KAuth exist on Windows?
  - The text buffer itself is unit tested pretty well. Is it possible to have a unit test for this? PS: We already have a " bool KTextEditor::EditorPrivate::unitTestMode()" function that we could use to virtually trigger the KAuth case somehow, not sure.

INLINE COMMENTS

> katetextbuffer.cpp:768-769
> +    // prepare parameters for calling helper method
> +    QString textCodec = QString::fromLatin1(m_textCodec->name());
> +    int eolMode = static_cast<int>(endOfLineMode());
> +    QList<QVariant> dataToSave;

const QString textCodec = ...
const int eolMode = ...

Please use const whenever possible for locally defined variables.

> katetextbuffer_secure.cpp:1
> +#include "katetextbuffer_secure.h"
> +#include "katetextline.h"

I would prefer katesecuretextbuffer.h as filename. Intermixing underscrores is rather uncommon in KDE's sources.

> katetextbuffer_secure.cpp:26-31
> +    QString filename = args[QLatin1String("filename")].toString();
> +    QString mimeTypeForFilterDev = args[QLatin1String("mimeTypeForFilterDev")].toString();
> +    QString textCodec = args[QLatin1String("textCodec")].toString();
> +    bool generateByteOrderMark = args[QLatin1String("generateByteOrderMark")].toBool();
> +    int endOfLineMode = args[QLatin1String("endOfLineMode")].toInt();
> +    bool newLineAtEof = args[QLatin1String("newLineAtEof")].toBool();

Please prepend 'const' for all local variables whenever possible.

> katetextbuffer_secure.cpp:34
> +
> +    bool ok = saveInternal(filename, mimeTypeForFilterDev, textCodec, generateByteOrderMark, endOfLineMode, newLineAtEof, dataToSave);
> +

const bool ok = ...

> katetextbuffer_secure.cpp:87
> +    // just dump the lines out ;)
> +    int lineCount = dataToSave.count();
> +    for (int i = 0; i < lineCount; ++i) {

const int lineCount = ...

> katetextbuffer_secure.h:10
> +
> +using namespace KAuth;
> +

Please no using namespace KAuth here, especially since this is a header file.

> katetextbuffer_secure.h:12
> +
> +class TextBufferSecure : public QObject
> +{

Please call it SecureTextBuffer, that feels much more natural than adding the suffix "Secure".

> katetextbuffer_secure.h:18
> +
> +    TextBufferSecure() {};
> +

Pedantic: no semicolon after a closing function brace, i.e.:

  SecureTextBuffer() {}

> katetextbuffer_secure.h:22
> +
> +    bool saveInternal(const QString &filename, const QString &mimeTypeForFilterDev, const QString &textCodec, const bool &generateByteOrderMark,
> +                      const int &endOfLineMode, const bool &newLineAtEof, const QList<QVariant> &dataToSave);

Please never use const references for primitive types. Here:

- const bool & --> bool

REPOSITORY
  R39 KTextEditor

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: martinkostolny, #ktexteditor, dhaumann
Cc: dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170301/18edb615/attachment-0001.html>