Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

Hi, 
> Hello,
>
> Nicolò Balzarotti <anothersms@gmail.com> writes:
>
>> I just noticed that nlohmann-json-cpp is deprecated for json-modern-cxx,
>> fixed it in the two patches that were using it.
>
> Thank you for the patches.
>

thanks for your review.

> Unfortunately, I cannot build nheko because of a missing lmdbxx input.
>
lmdbxx was patch #3, but I created some noise sending replying to
guix-patches and creating multiple issues.  I'm sending it again with
other fixes applied.

> Some comments follow.
>
>> +           (lambda _
>> +             (substitute* "CMakeLists.txt"
>> +               (("add_test\\(BasicConnectivity") "# add_test")
>> +               (("add_test\\(ClientAPI") "# add_test")
>> +               (("add_test\\(MediaAPI") "# add_test")
>> +               (("add_test\\(Encryption") "# add_test"))
>
> Nitpick: I suggest to use a single regexp for these.

Sure, done.  I'm not much confident with substitute* yet.

>
>> +    (inputs
>> +     `(("boost" ,boost)
>> +       ("libolm" ,libolm)
>> +       ("libsodium" ,libsodium)
>> +       ("openssl" ,openssl)
>> +       ("json-modern-cxx" ,json-modern-cxx)
>> +       ("spdlog" ,spdlog)
>> +       ("zlib" ,zlib)))
>
> Could you re-order inputs alphabetically?

Done

>
>> +    (description "@code{mtxclient} is a C++ library that implements client API
>> +for the Matrix protocol.  It's built on to of @code{Boost.Asio}.")
>
> Nitpick: "It's" -> "It is".

Done
>> +    (license license:expat)))
>> +
>>  (define-public quaternion
>>    (package
>>      (name "quaternion")
>> @@ -1795,8 +1849,8 @@ QMatrixClient project.")
>>       (origin
>>         (method git-fetch)
>>         (uri (git-reference
>> -              (url "https://github.com/QMatrixClient/Quaternion")
>> -              (commit version)))
>> +             (url "https://github.com/QMatrixClient/Quaternion")
>> +             (commit version)))
>
> This change is unrelated to the patch. Could you remove it?
I'm sorry
>
>> +    (inputs
>> +     `(("boost" ,boost)
>> +       ("cmark" ,cmark)
>> +       ("libolm" ,libolm)
>> +       ("lmdb" ,lmdb)
>> +       ("lmdbxx" ,lmdbxx)
>
> What is that?
See previous comment

>> +       ("mtxclient" ,mtxclient)
>> +       ("openssl" ,openssl)
>> +       ("json-modern-cxx" ,json-modern-cxx)
>> +       ("qtbase" ,qtbase)
>> +       ("qtsvg" ,qtsvg)
>> +       ("qtmultimedia" ,qtmultimedia)
>> +       ("spdlog" ,spdlog)
>> +       ("tweeny" ,tweeny)
>> +       ("zlib" ,zlib)))
>> +    (native-inputs
>> +     `(("pkg-config" ,pkg-config)
>> +       ("qtlinguist" ,qttools)))
>
> Isn't it a bit confusing?
I copied it from
./gnu/packages/lxqt.scm:1332:          ("qtlinguist" ,qttools)))
./gnu/packages/sync.scm:193:       ("qtlinguist" ,qttools)))
./gnu/packages/music.scm:279:         ("qtlinguist" ,qttools)))
./gnu/packages/music.scm:4128:       ("qtlinguist" ,qttools)))

I can change it if needed, but I found other instances searching for
"qtlinguist" as cmake complained about it.  If those instances were
"qttools" I would have not found it.

>
>> +    (build-system qt-build-system)
>
> Nitpick: usually, build-system is above inputs and arguments.
>
>> +    (home-page "https://github.com/Nheko-Reborn/nheko")
>> +    (synopsis "Desktop client for Matrix using Qt and C++14")
>> +    (description "@code{Nheko} want to provide a native desktop app for the
>> +Matrix protocol that feels more like a mainstream chat app and less like an IRC
>> +client.
>
> "that feels more..." sounds link marketing buzz. Maybe we could remove it.
>
>> +Most of the features you would expect from a chat application are missing right
>> +now but we are getting close to a more feature complete client.
>
> I'm not sure this part is warranted either.
Removed and rephrased

>
>> Specifically
>> +there is support for:
>> +@itemize
>> +@item E2E encryption (text messages only: attachments are currently sent unencrypted).
>> +@item User registration.
>> +@item Creating, joining & leaving rooms.
>> +@item Sending & receiving invites.
>> +@item Sending & receiving files and emoji.
>> +@item Typing notifications.
>> +@item Username auto-completion.
>> +@item Message & mention notifications.
>> +@item Redacting messages.
>> +@item Read receipts.
>> +@item Basic communities support.
>> +@item Room switcher (@key{ctrl-K}).
>> +@item Light, Dark & System themes.
>> +@end itemize\n")
>
> No need for the final newline.

I wasn't sure, as there are dozens of instances with the newline after

rep -ri --line-number  '@end itemize[^\\n]'  | wc -l
181

grep -ri --line-number  '@end itemize\\n'  | wc -l
277


I removed it, but if there's a standard, it would be better if it was
respected (as for "new" contributors it's difficult to understand the
right way just by looking at existing packages)