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

[dvratil (Daniel Vrátil)]

dvratil added a comment.


  You have to add
  
  add_subdirectory(tomboyresource)
  
  to resources/CMakeLists.txt, otherwise the code will never be built. I would argue that you should make the resource optional and only build it if the "o2" dependency is available.

INLINE COMMENTS

> CMakeLists.txt:41
> +target_link_libraries(akonadi_tomboynotes_resource
> +    o2
> +    Qt5::DBus

You are linking against the "o2" target, but it's not being located anywhere. You have to add find_package(o2 OPTIONAL) to the root CMakeLists.txt. And since the o2 project does not seem to install any CMake or pkgconfig files, you'll have to write your own Findo2.cmake and add it to cmake/modules. You can look on some of the files there to get an idea how to write the Findo2.cmake file.

> configdialog.cpp:68
> +{
> +    if ( ui->kcfg_ServerURL->text() != mSettings->serverURL() ) {
> +        mSettings->setRequestToken(QString::null);

No spaces inside "(   )"

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

Space after `if`

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

Move `{` on the same line as the `if`

> dvratil wrote in tomboyitemuploadjob.cpp:65
> - The boundary string  feels rather arbitrary - where is that coming from?
> - wrap in `QStringLiteral()`

Missed one string :)

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/