D13104: Protocol: QJsonObject for tracer.

[Daniel Vrátil]

dvratil added a comment.


  Sorry, on second thought, there are some bigger problems :(

INLINE COMMENTS

> protocol.cpp:207
> +    static_cast<const Akonadi::Protocol::ChangeNotification *>(this)->toJson(json); \
> +    static_cast<const Akonadi::Protocol::class *>(this)->toJson(json); \
>      } break;

Instead of manually chaining up like this (you are missing chain-up from `ChangeNotification` to `Command` btw), you could add this to the beginning of the generated `toJson` method:

  if (!parentClass.isEmpty()) {
      mImpl << "    " << parentClass << "::toJson(json);\n";
  }

Then you won't need all the special cases and it will work generically for any level of inheritance.

> protocol_p.h:158
>  
> -    QTextStream &toJson(QTextStream &stream) const;
> +    void toJson(QJsonObject &stream) const;
>  protected:

Hmm, technically this should be `virtual`, shouldn't it? But I don't want to introduce virtual methods into Protocol objects, that's why the debugString() is done in that slightly cumbersome way of having Protocol::debugString() which calls the free `QDebug operator<<(QDebug &, ....)` functions. Maybe we could have the same for JSON, just have a free `QJsonObject &operator<<(QJsonObject &obj, ....)` free function for each Protocol class and make it a friend, so it can access its members and introduce Protocol::debugJson() method that calls the respective operator as well. What do you think?

> cppgenerator.cpp:667
> +        } else if (prop->type() == QStringLiteral("uint")) {
> +            mImpl << "    json[QStringLiteral(\"" << prop->name() << "\")] = (int)" <<  prop->mVariableName() << ";/* "<<  prop->type() << " */\n";
>          } else if (TypeHelper::isNumericType(prop->type())) {

`static_cast<int>` instead of C cast.

> cppgenerator.cpp:697
> +                        "        QJsonObject jsonObject;\n"
> +                        "        for ( auto key : " << prop->mVariableName() << ".keys() ) {\n"
> +                        "            jsonObject[QString::fromUtf8(key)] = QString::fromUtf8(" << prop->mVariableName() << ".value(key));\n"

This needs to construct the keys list and then do a value lookup for each key in the loop, use an iterator instead, please.

> cppgenerator.cpp:702
> +                        "    }\n";
> +            } else if (prop->type() == QStringLiteral("ModifySubscriptionCommand::ModifiedParts") ||
> +                       prop->type() == QStringLiteral("ModifyTagCommand::ModifiedParts") ||

This kinda sucks as it needs to be changed whenever a new QFlags are introduced :/  Can't compiler figure out the cast to int itself?

We can probably try to figure out some QMetaType or SFINAE workaround later so we could merge this with the `else` below. Not a big problem right now.

> scope.cpp:318
>  {
> -    stream << "{\"ID\": " << id << ", \"RemoteID\": \"" << remoteId << "\"}";
> -    return stream;
> -}
> -
> -QTextStream &Scope::toJson(QTextStream& stream) const
> -{
> -    stream << "{";
>      switch (scope()) {
>      case Scope::Uid:

I think the best way to serialize this to JSON would be:

  {
     "type": [one of "UID", "RID", "GID", "HRID", "invalid"],
     "value": [respective set serialized]
  }

This way you avoid the awkward  `{ "invalid": 0 }` serialization for invalid scope

> scope.cpp:341
>      default:
> -        stream << "None";
> +        json[QStringLiteral("inavlid")] = scope();
>      }

Typo: `inavlid`

REPOSITORY
  R165 Akonadi

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

To: knauss, dvratil
Cc: kde-pim, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20180526/61c922b6/attachment.html>