[Kde-pim] [Differential] [Commented On] D2038: A new akonadi resource type implementing the Tomboy REST API

[krake (Kevin Krammer)]

krake added a comment.


  Just a couple of recommendations :)

INLINE COMMENTS

> configdialog.cpp:31
> +    // Create window
> +    setWindowTitle(i18n("Select a tomboy server"));
> +    QWidget *mainWidget = new QWidget(this);

Since "Tomboy" is a product name it make sense to use the same capitalzation as the official name.

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

Since this seems to be user visible text it should be passed through a translation function.
In such a case it also helps translators if string composition is done via placeholders instead of string concatenation.
See https://techbase.kde.org/Development/Tutorials/Localization/i18n_Krazy#Placeholders_and_Arguments

> configdialog.h:2
> +/*
> +    Copyright (c) 2016 Stefan Stäglich
> +

Not sure if it is required but it is customary to have name + email for each author

> tomboycollectionsdownloadjob.cpp:26
> +#include "debug.h"
> +#include "tomboycollectionsdownloadjob.h"
> +

A recommendation (not a requirement) is to have the header for the class itself as the first include:
https://community.kde.org/Policies/Library_Code_Policy#Include_order

As a result include order is often done from most specific to most low level, e.g.

own header -> project headers -> PIM headers -> KDE Framework headers -> Qt Headers -> system headers

> tomboynotesresource.cpp:71
> +{
> +    qCDebug(log_tomboynotesresource) << "Retriving collections started";
> +

typo: Retrieving :)

> tomboynotesresource.cpp:311
> +
> +bool TomboyNotesResource::configurationNotValid()
> +{

could this method be const?

> tomboynotesresource.desktop:3
> +Name=Akonadi TomboyNotes Resource
> +Comment=Resource for tomboy compatible notes server
> +Type=AkonadiResource

I would also start Tomboy with a capital T here

> tomboynotesresource.h:24
> +
> +#include <QtNetwork>
> +#include <AkonadiAgentBase/ResourceBase>

Also just as a general recommendation: use specific includes instead of module includes
A module inculdes will result in all headers for the module to be included, making the compiler parse dozends or not hundert of classes that it then doesn't need.

Module includes are convenient for prototyping, but most often not a good idea for production code

> tomboynotesresource.h:26
> +#include <AkonadiAgentBase/ResourceBase>
> +#include <KIO/AccessManager>
> +#include "o1tomboy.h"

theoretically you could go even further here and use a forward declaration since the AccessManager type is only used by pointer here (the include would then move to the cpp file).

The include is fine here since the context here is a single application, but in a library you would want to have each header with as few dependencies on other headers as possible.
See https://community.kde.org/Policies/Library_Code_Policy#Avoid_including_other_headers_in_headers

> tomboyserverauthenticatejob.cpp:71
> +    setError(TomboyJobError::PermanentError);
> +    setErrorText(i18n("Authorization has been failed! It could be an SSL error!"));
> +    emitResult();

"has failed"

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: krake, mlaurent, knauss, dvratil, kde-pim, dvasin, winterz, smartins, vkrause
_______________________________________________
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/