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
