[Kde-pim] [Differential] [Requested Changes To] D2038: A new akonadi resource type implementing the Tomboy REST API

dvratil (Daniel Vrátil) noreply at phabricator.kde.org
Tue Jun 28 23:41:37 BST 2016


dvratil requested changes to this revision.
dvratil added a reviewer: dvratil.
dvratil added a comment.
This revision now requires changes to proceed.


  Generally looks very good. Please don't get scared from the number of comments, most of it is just coding style nitpicks and me being overly pedantic with `const` and lots of the comments repeat. I haven't spotted any serious issue so far.
  
  Regarding the QLatin1String/QStringLiteral: you should never use a bare string in Qt, unless the method takes `const char *` (like KConfigGroup ctor does) or `QByteArray` argument (like KMime API does). If it takes `QString` (like in Akonadi::Item::setMimeType()`) , you should wrap the string in `QStringLiteral`. If the method takes `QLatin1String` (or has an overload that takes `QLatin1String`) (like `QJsonObject::operator[]()` or `QString::operator==()`), you should wrap the string in `QLatin1String`.
  
  One question: does the OAuth token expire, and if yes, how do you refresh it?
  You only seem to retrieve the token when the resource configuration is changed. If the token expires after some time however, the resource should automatically refresh it (otherwise errors everywhere! :))
  
  Regarding the license header, just look into some other files in kdepim-runtime which are under GPLv2, copy the header and just change the name there :)
  
  Once you fix those issues I'll do another round of review, but yeah, so far it looks pretty good :)

INLINE COMMENTS

> CMakeLists.txt:6-58
> +find_package(ECM 5.13.0 CONFIG REQUIRED)
> +
> +set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules ${ECM_MODULE_PATH} ${CMAKE_MODULE_PATH})
> +
> +include(FeatureSummary)
> +include(KDEInstallDirs)
> +include(KDECMakeSettings)

All this can be dropped, it's handled in root CMakeLists.txt

> CMakeLists.txt:123-125
> +########### summary ############
> +
> +feature_summary(WHAT ALL FATAL_ON_MISSING_REQUIRED_PACKAGES)

This can be dropped as well

> LICENSE:1
> +                    GNU GENERAL PUBLIC LICENSE
> +                       Version 2, June 1991

This file can be removed, a GPLv2 LICENSE file should already be present in the root of the repository

> acknowledgements.md:5
> + 
> +> Copyright (c) 2012, Akos Polster
> +> All rights reserved.

Remove o2 copyright, since you no longer ship it.

> configdialog.cpp:19
> +    // KSettings stuff
> +    mSettings = settings;
> +    mManager = new KConfigDialogManager(this, settings);

Move to initialization list

> configdialog.cpp:54
> +
> +    if(ui->kcfg_collectionName->text() == "") {
> +        ui->kcfg_collectionName->setText("Tomboy Notes " + Settings::serverURL());

Use `isEmpty()` instead

> configdialog.h:19
> +public:
> +    explicit ConfigDialog(Settings* settings, QWidget *parent=Q_NULLPTR);
> +    ~ConfigDialog();

Spaces around "="

> o1tomboy.h:9
> +public:
> +    explicit O1Tomboy(QObject *parent = 0) : O1(parent) {
> +

Move implementation of all the methods in this class to .cpp file please

> o1tomboy.h:14
> +    void setBaseURL(const QString &value) {
> +        setRequestTokenUrl(QUrl(value + "/oauth/request_token"));
> +        setAuthorizeUrl(QUrl(value + "/oauth/authorize"));

Wrap the bare strings here and below in `QLatin1String()` or `QStringLiteral()`

> o1tomboy.h:21
> +
> +    QString getRequestToken() {
> +        return requestToken_;

const

> o1tomboy.h:25
> +
> +    QString getRequestTokenSecret() {
> +        return requestTokenSecret_;

const

> tomboycollectionsdownloadjob.cpp:1
> +#include <QDesktopServices>
> +#include <QJsonDocument>

Missing license header

> tomboycollectionsdownloadjob.cpp:11
> +{
> +    mCollectionName = collectionName;
> +}

Move to initializer list

> tomboycollectionsdownloadjob.cpp:34
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move on the same line as the `if`

> tomboycollectionsdownloadjob.cpp:43
> +
> +    QJsonObject jo = document.object();
> +    qCDebug(log_tomboynotesresource) << "TomboyCollectionsDownloadJob: " << jo;

`const QJsonObject`

> tomboycollectionsdownloadjob.cpp:45
> +    qCDebug(log_tomboynotesresource) << "TomboyCollectionsDownloadJob: " << jo;
> +    QJsonValue collectionRevision = jo["latest-sync-revision"];
> +    qCDebug(log_tomboynotesresource) << "TomboyCollectionsDownloadJob: " << collectionRevision;

- `const QJsonValue`
- wrap bare string into `QLatin1String()`

> tomboycollectionsdownloadjob.cpp:49
> +    Akonadi::Collection c;
> +    c.setParentCollection( Akonadi::Collection::root());
> +    c.setRemoteId( mContentURL );

Remove space after "("

> tomboycollectionsdownloadjob.cpp:50
> +    c.setParentCollection( Akonadi::Collection::root());
> +    c.setRemoteId( mContentURL );
> +    c.setName( mCollectionName );

Remove spaces around "(" and ")"

> tomboycollectionsdownloadjob.cpp:51
> +    c.setRemoteId( mContentURL );
> +    c.setName( mCollectionName );
> +    c.setRemoteRevision(QString::number(collectionRevision.toInt()));

Remove spaces

> tomboycollectionsdownloadjob.cpp:53
> +    c.setRemoteRevision(QString::number(collectionRevision.toInt()));
> +    qCDebug(log_tomboynotesresource) << "TomboyCollectionsDownloadJob: Sync revision " << collectionRevision.toString();
> +

Does `collectionRevision.toString()` actually work if `collectionRevision` contains a number? If not, fix this, if it does, use it above instead of `QString::number`

> tomboycollectionsdownloadjob.cpp:56
> +    QStringList mimeTypes;
> +    mimeTypes << QLatin1String("text/x-vnd.akonadi.note");
> +    c.setContentMimeTypes( mimeTypes );

Use `Akonadi::NoteUtils::noteMimeType()` from `AkonadiNotes/NoteUtils` instead of manually hardcoding the mimetype (you will have to link against `KF5::AkonadiNotes` in `target_link_libraries()` in CMakeList.txt)

> tomboycollectionsdownloadjob.cpp:57
> +    mimeTypes << QLatin1String("text/x-vnd.akonadi.note");
> +    c.setContentMimeTypes( mimeTypes );
> +

C++11 initializer_list FTW: `c.setContentMimeTypes({ Akonadi::NoteUtils::noteMimeType() });`

> tomboycollectionsdownloadjob.h:1
> +#ifndef TOMBOYCOLLECTIONSDOWNLOADJOB_H
> +#define TOMBOYCOLLECTIONSDOWNLOADJOB_H

Missing license header

> tomboycollectionsdownloadjob.h:14
> +    // returns the parsed results wrapped in Akonadi::Collection::List, see bellow
> +    Akonadi::Collection::List collections();
> +

const

> tomboyitemdownloadjob.cpp:1
> +#include <QJsonArray>
> +#include <QJsonDocument>

Missing license header

> tomboyitemdownloadjob.cpp:11
> +{
> +    mResultItem = Akonadi::Item(item);
> +}

1. Move to initializer list
2. why constructing a new `Akonadi::Item`? Just do `mResultItem = item`?

> tomboyitemdownloadjob.cpp:23
> +    mContentURL.chop(1);
> +    QNetworkRequest request(mContentURL + "/" + mResultItem.remoteId());
> +    mReply = mRequestor->get(request, QList<O0RequestParameter>());

Wrap the bare character to `QLatin1Char()`

> tomboyitemdownloadjob.cpp:34
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move on the same line as the `if`

> tomboyitemdownloadjob.cpp:42
> +    // Parse received data as JSON
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +

`const QJSonDocument`

> tomboyitemdownloadjob.cpp:44
> +
> +    QJsonObject jsonNote = document.object();
> +

`const QJsonObject`

> tomboyitemdownloadjob.cpp:48
> +
> +    mResultItem.setRemoteRevision(QString::number(jsonNote["last-sync-revision"].toInt()));
> +    qCDebug(log_tomboynotesresource) << "TomboyItemDownloadJob: Sync revision " << mResultItem.remoteRevision();

Wrap bare string to `QLatin1String()`

> tomboyitemdownloadjob.cpp:53
> +    // Set timestamp
> +    QString timeStampJson = jsonNote["last-change-date"].toString();
> +    QDateTime modificationTime = QDateTime::fromString(timeStampJson, Qt::ISODate);

- `const QString`
- wrap bare string to `QLatin1String()`

> tomboyitemdownloadjob.cpp:54
> +    QString timeStampJson = jsonNote["last-change-date"].toString();
> +    QDateTime modificationTime = QDateTime::fromString(timeStampJson, Qt::ISODate);
> +    mResultItem.setModificationTime(modificationTime);

`const QDateTime`

> tomboyitemdownloadjob.cpp:58
> +    // Set note title
> +    KMime::Message::Ptr akonadiNote = KMime::Message::Ptr(new KMime::Message);
> +    akonadiNote->subject(true)->fromUnicodeString( jsonNote["title"].toString().toUtf8(), "utf-8" );

Not really an issue, just fyi - you can write it shorter :-)

`auto akonadiNote = KMime::Message::Ptr::create();`
or
`KMime::Message::Ptr akonadiNote(new KMime::Message);`

> tomboyitemdownloadjob.cpp:59
> +    KMime::Message::Ptr akonadiNote = KMime::Message::Ptr(new KMime::Message);
> +    akonadiNote->subject(true)->fromUnicodeString( jsonNote["title"].toString().toUtf8(), "utf-8" );
> +

Wrap "title" to `QLatin1String()`

> tomboyitemdownloadjob.cpp:65
> +    akonadiNote->contentTransferEncoding(true)->setEncoding(KMime::Headers::CEquPr);
> +    akonadiNote->mainBodyPart()->fromUnicodeString(jsonNote["note-content"].toString().toUtf8());
> +

Wrap bare string into `QLatin1String()`

> tomboyitemdownloadjob.h:1
> +#ifndef TOMBOYITEMDOWNLOADJOB_H
> +#define TOMBOYITEMDOWNLOADJOB_H

Missing license header

> tomboyitemsdownloadjob.cpp:1
> +#include <QJsonArray>
> +#include <QJsonDocument>

Missing license header

> tomboyitemsdownloadjob.cpp:11
> +{
> +    mCollectionId = id;
> +}

Move to initialization list

> tomboyitemsdownloadjob.cpp:34
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move to the same line as the `if`

> tomboyitemsdownloadjob.cpp:41
> +    // Parse received data as JSON
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +

`const QJsonDocument`

> tomboyitemsdownloadjob.cpp:43
> +
> +    QJsonObject jo = document.object();
> +    QJsonArray notes = jo["notes"].toArray();

`const QJsonObject`

> tomboyitemsdownloadjob.cpp:44
> +    QJsonObject jo = document.object();
> +    QJsonArray notes = jo["notes"].toArray();
> +

`const JsonArray`

> tomboyitemsdownloadjob.cpp:46
> +
> +    foreach (auto note, notes) {
> +        Akonadi::Item item( "text/x-vnd.akonadi.note" );

Use `Q_FOREACH` and `const auto &note`, or use C++ range-based loop (`for (const auto &note : notes)`)

> tomboyitemsdownloadjob.cpp:47
> +    foreach (auto note, notes) {
> +        Akonadi::Item item( "text/x-vnd.akonadi.note" );
> +        item.setRemoteId(note.toObject()["guid"].toString());

Use `Akonadi::NoteUtils::noteMimeType()`

> tomboyitemsdownloadjob.cpp:48
> +        Akonadi::Item item( "text/x-vnd.akonadi.note" );
> +        item.setRemoteId(note.toObject()["guid"].toString());
> +        mResultItems << item;

Wrap bare string to `QLatin1String()`

> tomboyitemsdownloadjob.h:1
> +#ifndef TOMBOYITEMSDOWNLOADJOB_H
> +#define TOMBOYITEMSDOWNLOADJOB_H

Missing license header

> tomboyitemuploadjob.cpp:1
> +#include <QJsonArray>
> +#include <QJsonDocument>

Missing license header

> tomboyitemuploadjob.cpp:11
> +{
> +    mSourceItem = Akonadi::Item(item);
> +    if (item.hasPayload<KMime::Message::Ptr>()) {

- move to initialization list
- no need to explicitly create a new `Akonadi::Item()`

> tomboyitemuploadjob.cpp:22
> +    if (jobType == JobType::addItem) {
> +        mSourceItem.setRemoteId(KRandom::randomString(37));
> +    }

Would maybe using `QUuid `to generate remoteID better? `QUuid` has better guarantees of being unique.

> tomboyitemuploadjob.cpp:40
> +    QJsonObject jsonNote;
> +    jsonNote["guid"] = mSourceItem.remoteId();
> +    switch (mJobType) {

`QLatin1String()` here and everywhere below

> tomboyitemuploadjob.cpp:43
> +    case JobType::deleteItem:
> +        jsonNote["command"] = "delete";
> +        break;

Wrap "delete" in `QStringLiteral()`

> tomboyitemuploadjob.cpp:47
> +        jsonNote["create-date"] = getCurrentISOTime();
> +    case JobType::modifyItem:
> +        jsonNote["title"] = mNoteContent->headerByType("subject")->asUnicodeString();

Missing `break` for `JobType::addItem`? If this is intentional, please add a comment that the fall-through is intentaional.

> tomboyitemuploadjob.cpp:50
> +        jsonNote["note-content"] = mNoteContent->mainBodyPart()->decodedText();
> +        jsonNote["note-content-version"] = "1";
> +        jsonNote["last-change-date"] = getCurrentISOTime();

Should that be "1" (then wrap in `QStringLiteral()`) or 1 (i.e. integer)?

> tomboyitemuploadjob.cpp:65
> +    QNetworkRequest request(mContentURL);
> +    request.setHeader(QNetworkRequest::ContentTypeHeader, "application/json; boundary=7d44e178b0439");
> +    request.setHeader(QNetworkRequest::ContentLengthHeader, postData.toJson().length());

- The boundary string  feels rather arbitrary - where is that coming from?
- wrap in `QStringLiteral()`

> tomboyitemuploadjob.cpp:76
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move to the same line as the `if`

> tomboyitemuploadjob.cpp:84
> +    // Parse received data as JSON
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +

`const QJsonDocument`

> tomboyitemuploadjob.cpp:86
> +
> +    QJsonObject jo = document.object();
> +    QJsonArray notes = jo["notes"].toArray();

`const QJSonObject`

> tomboyitemuploadjob.cpp:87
> +    QJsonObject jo = document.object();
> +    QJsonArray notes = jo["notes"].toArray();
> +

- `const QJsonArray`
- wrap bare string into `QLatin1String()`

> tomboyitemuploadjob.cpp:91
> +    bool found = false;
> +    foreach (auto note, notes) {
> +        found = (note.toObject()["guid"].toString() == mSourceItem.remoteId());

`Q_FOREACH (const auto & ...` or C++11 range-based `for` loop

> tomboyitemuploadjob.cpp:92
> +    foreach (auto note, notes) {
> +        found = (note.toObject()["guid"].toString() == mSourceItem.remoteId());
> +        if (found) {

`QLatin1String()`

> tomboyitemuploadjob.cpp:99
> +        setError(TomboyJobError::PermanentError);
> +        setErrorText("Sync error. Server status not as expected!");
> +        emitResult();

`i18n()`

> tomboyitemuploadjob.cpp:105
> +        setError(TomboyJobError::PermanentError);
> +        setErrorText("Sync error. Server status not as expected!");
> +        emitResult();

`i18n()`

> tomboyitemuploadjob.h:1
> +#ifndef TOMBOYITEMUPLOADJOB_H
> +#define TOMBOYITEMUPLOADJOB_H

Missing license header

> tomboyitemuploadjob.h:10
> +{
> +    addItem,
> +    modifyItem,

Generally enum values start with capital letter

> tomboyitemuploadjob.h:24
> +
> +    JobType jobType();
> +

const

> tomboyitemuploadjob.h:35
> +    // Workaround for https://bugreports.qt-project.org/browse/QTBUG-26161 Qt bug
> +    QString getCurrentISOTime();
> +

const

> tomboyjobbase.cpp:8-10
> +    mO1 = new O1Tomboy(this);
> +    mManager = manager;
> +    mRequestor = new O1Requestor(mManager, mO1, this);

Move to initialization list

> tomboyjobbase.cpp:16
> +    mO1->setBaseURL(apiurl);
> +    mApiURL = apiurl + "/api/1.0";
> +    mContentURL = contenturl;

Wrap bare string in `QStringLiteral()`

> tomboyjobbase.h:1
> +#ifndef TOMBOYJOBBASE_H
> +#define TOMBOYJOBBASE_H

Missing license header

> tomboynotesresource.cpp:1
> +#include <QtDBus/QDBusConnection>
> +#include <QSslCipher>

Missing license header

> tomboynotesresource.cpp:34
> +    mStatusMessageTimer->setSingleShot(true);
> +    connect(mStatusMessageTimer, &QTimer::timeout, this, &TomboyNotesResource::clearStatusMessage);
> +    connect(this, &AgentBase::error, this, &TomboyNotesResource::showError);

Sicne `clearStatusMessage()` is so simple and not called from anywhere else, I'd put it into lambda here

> tomboynotesresource.cpp:47
> +{
> +    delete mStatusMessageTimer;
> +}

No need to explictly delete the QTimer here

> tomboynotesresource.cpp:53
> +    qCDebug(log_tomboynotesresource) << "Retriving collections started";
> +
> +    auto job = new TomboyCollectionsDownloadJob(Settings::collectionName(), mManager, this);

Should probably also include `if (configurationNotValid())` check?

> tomboynotesresource.cpp:67
> +    if (configurationNotValid()) {
> +        cancelTask("Resource configuration is not valid");
> +        return;

`i18n()`

> tomboynotesresource.cpp:116
> +    }
> +    else {
> +        showError(job->errorText());

Move on the same line as `}`

> tomboynotesresource.cpp:188
> +    if (Settings::ignoreSslErrors()) {
> +        reply->ignoreSslErrors();
> +    }

Ouch, really? Look into using `KIO::SslUi::askIgnoreSslErrors()` or `KSslInfoDialog`...

> tomboynotesresource.cpp:208
> +
> +    // Run the configuration dialog an sve settings if accepted
> +    if (dialog.exec() != QDialog::Accepted) {

"and save"

> tomboynotesresource.cpp:217
> +    if (configurationNotValid())
> +    {
> +        auto job = new TomboyServerAuthenticateJob(mManager, this);

Move on the same line as the `if`

> tomboynotesresource.cpp:219
> +        auto job = new TomboyServerAuthenticateJob(mManager, this);
> +        job->setServerURL(Settings::serverURL(), "");
> +        connect(job, &KJob::result, this, &TomboyNotesResource::onAuthorizationFinished);

Use `QString()` instead of `""` (or `QString::Null`)

> tomboynotesresource.cpp:224
> +    }
> +    else {
> +        synchronize();

Move on the same line as `}`

> tomboynotesresource.cpp:233
> +    if (Settings::readOnly() || configurationNotValid()) {
> +        cancelTask("Resource is read-only");
> +        return;

`i18n()`

> tomboynotesresource.cpp:238
> +    if (mUploadJobProcessRunning) {
> +        retryAfterFailure("");
> +        return;

Use `QString()` instead of `""` (or `QString::Null`)

> tomboynotesresource.cpp:254
> +    if (Settings::readOnly() || configurationNotValid()) {
> +        cancelTask("Resource is read-only");
> +        return;

`i18n()`

> tomboynotesresource.cpp:259
> +    if (mUploadJobProcessRunning) {
> +        retryAfterFailure("");
> +        return;

`QString()` or `QString::Null`

> tomboynotesresource.cpp:274
> +    if (Settings::readOnly() || configurationNotValid()) {
> +        cancelTask("Resource is read-only");
> +        return;

`i18n()`

> tomboynotesresource.cpp:279
> +    if (mUploadJobProcessRunning) {
> +        retryAfterFailure("");
> +        return;

`QString()` or `QString::Null`

> tomboynotesresource.h:1
> +#ifndef TOMBOYNOTESRESOURCE_H
> +#define TOMBOYNOTESRESOURCE_H

Missing license header

> tomboynotesresource.h:12
> +class TomboyNotesResource : public Akonadi::ResourceBase,
> +                           public Akonadi::AgentBase::Observer
> +{

Wrong indentation (I know, I'm really pedantic... :-) )

> tomboynotesresource.h:54
> +    // Status handling
> +    void showError(const QString errorText);
> +    QTimer *mStatusMessageTimer;

`const QString &`

> tomboyserverauthenticatejob.cpp:1
> +#include <QDesktopServices>
> +#include <QJsonArray>

Missing license header

> tomboyserverauthenticatejob.cpp:12-16
> +    connect(mO1, SIGNAL(linkedChanged()), this, SLOT(onLinkedChanged()));
> +    connect(mO1, SIGNAL(linkingFailed()), this, SLOT(onLinkingFailed()));
> +    connect(mO1, SIGNAL(linkingSucceeded()), this, SLOT(onLinkingSucceeded()));
> +    connect(mO1, SIGNAL(openBrowser(QUrl)), this, SLOT(onOpenBrowser(QUrl)));
> +    connect(mO1, SIGNAL(closeBrowser()), this, SLOT(onCloseBrowser()));

Use the new `connect()` syntax (with function pointers)

> tomboyserverauthenticatejob.cpp:47
> +    setError(TomboyJobError::PermanentError);
> +    setErrorText("Authorization has been failed!");
> +    emitResult();

`i18n()`

> tomboyserverauthenticatejob.cpp:67
> +{
> +    QDesktopServices::openUrl(url);
> +}

How do you know that user has finished the authentication and the OAuth authorization can continue?

IMO it would be better to use a custom dialog with `QWebEngineView` rather than launching user's browser.

You should at least check if the call is successful and show an error message otherwise.

> tomboyserverauthenticatejob.cpp:72
> +{
> +
> +}

Should this do something? :-)

> tomboyserverauthenticatejob.cpp:77
> +{
> +    checkReplyError();
> +    if (error() != TomboyJobError::NoError)

Shouldn't you actually use the return value somehow?

> tomboyserverauthenticatejob.cpp:79
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move to the same line as the `if`

> tomboyserverauthenticatejob.cpp:86
> +    // Parse received data as JSON and get user-href
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +    QJsonObject jo = document.object();

`const QJsonDocument`

> tomboyserverauthenticatejob.cpp:87
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +    QJsonObject jo = document.object();
> +    mUserURL = jo["user-ref"].toObject()["api-ref"].toString();

`const QJsonObject`

> tomboyserverauthenticatejob.cpp:88
> +    QJsonObject jo = document.object();
> +    mUserURL = jo["user-ref"].toObject()["api-ref"].toString();
> +

Wrap bare string to `QLatin1String()`

> tomboyserverauthenticatejob.cpp:99
> +{
> +    checkReplyError();
> +    if (error() != TomboyJobError::NoError)

Shouldn't you actually use the returned value somehow?

> tomboyserverauthenticatejob.cpp:101
> +    if (error() != TomboyJobError::NoError)
> +    {
> +        setErrorText(mReply->errorString());

Move to the same line as the `if`

> tomboyserverauthenticatejob.cpp:108
> +    // Parse received data as JSON and get contentURL
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +    QJsonObject jo = document.object();

`const QJsonDocument`

> tomboyserverauthenticatejob.cpp:109
> +    QJsonDocument document = QJsonDocument::fromJson(mReply->readAll(), Q_NULLPTR);
> +    QJsonObject jo = document.object();
> +    mContentURL = jo["notes-ref"].toObject()["api-ref"].toString();

`const QJsonObject`

> tomboyserverauthenticatejob.cpp:110
> +    QJsonObject jo = document.object();
> +    mContentURL = jo["notes-ref"].toObject()["api-ref"].toString();
> +

Wrap bare strings into `QLatin1String()`

> tomboyserverauthenticatejob.h:1
> +#ifndef TOMBOYSERVERAUTHENTICATEJOB_H
> +#define TOMBOYSERVERAUTHENTICATEJOB_H

Missing license header

> tomboyserverauthenticatejob.h:14-17
> +    QString getRequestToken();
> +    QString getRequestTokenSecret();
> +    QString getContentUrl();
> +    QString getUserURL();

All methods `const`

REPOSITORY
  rKDEPIMRUNTIME KDE PIM Runtime

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

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

To: Stefan, #kde_pim, dvratil
Cc: dvratil, kde-pim, dvasin, winterz, smartins, vkrause, mlaurent, knauss
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/


More information about the kde-pim mailing list