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

[dvratil (Daniel Vrátil)]

dvratil added a comment.


  Some minor coding style issues that slipped through, otherwise I think the code is OK.
  
  I'm however worried about the "o2" library. It does not install anything - the binary or the headers and it builds only static library, not a shared one. Packagers will hate us having to package it manually or will simply decide to ignore the dependency and will not ship the resource at all (speaking as a packager, I'd just ignore the dependency).
  
  I see two reasonable options here: you could bundle the o2 library (which is what you did originally), but bundled libraries are hard to keep in sync with upstream and are generally an annoyance for developers as well as packagers. Other option is to borrow only the relevant parts from the o2 library, strip them down to what you actually need and build your own OAuth1 implementation directly into the resource.

INLINE COMMENTS

> CMakeLists.txt:88
>  
> +find_package(Qt5WebEngineWidgets REQUIRED)
> +

Normally you would append WebEngineWidgets to the list above, but since it's only needed to build your resource, it should be moved below your `find_package(o2)` and made conditional:

  if (o2_FOUND)
     find_package(Qt5 OPTIONAL_COMPONENTS WebEngineWidgets)
  endif ()

> CMakeLists.txt:140
> +find_package(o2 OPTIONAL)
>  
>  add_subdirectory(resources)

Please add

  set_package_properties(o2 PROPERTIES
      DESCRIPTION "OAuth1 and OAuth2 library for Qt. Needed to build the Tomboy resource."
      URL "https://github.com/pipacs/o2"
      TYPE OPTIONAL)

To make it easier for packagers to figure out what they need to package.

> Findo2.cmake:11
> +find_package(PkgConfig)
> +pkg_check_modules(PC_O2 QUIET o2)
> +set(O2_DEFINITIONS ${PC_O2_CFLAGS_OTHER})

Are you sure this works? The library does not install any pkgconfig files (does not install anything for that matter), so I doubt this finds anything...

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

`QLatin1String`

> tomboynotesresource.cpp:243
> +    }
> +    else
> +    {

`} else {`

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

Move to the same line as the `if`

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/