Review Request: plasma-thunderbolt

[Daniel Vrátil]

On Wednesday, 15 May 2019 23:08:57 CEST Albert Astals Cid wrote:
> El dimecres, 15 de maig de 2019, a les 15:27:07 CEST, Daniel Vrátil va 
escriure:
> > Hi all,
> > 
> > plasma-thunderbolt is a new repo containing, you guessed it, Thunderbolt
> > KCM for Plasma. I initially submitted the code as a patch against
> > plasma-desktop [0], where it got reviewed, but it was ultimately decided
> > to better put it into a separate repository, since it's not just a KCM
> > but also a library and a KDED module. I have backported all the changes
> > from the Phabricator review back to the repository, so the code in the
> > repo is identical to the one in the Phab review (minus buildsystem
> > changes and a small build fix for clang).
> > 
> > However, since this is still a new code, it must formally pass through
> > kdereview before I can submit it into Plasma as a new module.
> > 
> > Thus I'd kindly ask you to take one more look at the codebase [1] and let
> > me know if there are any more issues to fix, or if we can proceed to
> > include this in the next Plasma release.
> 
> There's this cmake warning
> 
> CMake Warning at /usr/lib64/cmake/KF5Package/KF5PackageMacros.cmake:74
> (message): KPackage components should be specified in reverse domain
> notation. Appstream information won't be generated for kcm_bolt.
> Call Stack (most recent call first):
>   src/kcm/CMakeLists.txt:22 (kpackage_install_package)

I need some help with this - I looked into plasma-desktop, but all KCMs 
generate this warning and after renaming the package I just wasn't able to 
pair it with the library (which makes not sense to be called 
org.kde.plasma.thunderbolt.so) - so for now I'd just ignore the warning (we 
don't need Appstream data for KCM anyway, do we?).

> clazy complains about a few Q_PROPERTY that should ideally either have a
> NOTIFY signal or be marked as CONSTANT

I fixed the ones in the code, I can't fix the ones in the generated DBus 
interfaces and adaptors.


> it also complains about a few const slots that return values. Seems like
> what you want is to actually mark them as invokable/scriptable and not as
> slots?

Good point, fixed those.

Thanks for the review,
Dan

> 
> Cheers,
>   Albert
> 
> > Thanks,
> > - Dan
> > 
> > [0] https://phabricator.kde.org/D19011
> > [1] https://cgit.kde.org/plasma-thunderbolt.git


-- 
Daniel Vrátil
www.dvratil.cz | dvratil at kde.org
IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde)

GPG Key: 0x4D69557AECB13683
Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20190516/dec65d32/attachment.sig>