DevHeads.net

Review Request: plasma-thunderbolt

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.

Thanks,
- Dan

[0] <a href="https://phabricator.kde.org/D19011" title="https://phabricator.kde.org/D19011">https://phabricator.kde.org/D19011</a>
[1] <a href="https://cgit.kde.org/plasma-thunderbolt.git" title="https://cgit.kde.org/plasma-thunderbolt.git">https://cgit.kde.org/plasma-thunderbolt.git</a>

Comments

Re: Review Request: plasma-thunderbolt

By Jonathan Riddell at 05/21/2019 - 04:15

On Wed, May 15, 2019 at 03:27:07PM +0200, Daniel Vrátil wrote:
Installing the bolt package on Debian/Ubuntu/KDE neon I have this file
/usr/lib/x86_64-linux-gnu/boltd
But it is not found by FindBolt.cmake

I need to add this to the search path ${LIB_INSTALL_DIR}

Looks great but I don't have anything Thunderbolt so it's just a white box :)

Jonathan

Re: Review Request: plasma-thunderbolt

By Albert Astals Cid at 05/15/2019 - 17:08

El dimecres, 15 de maig de 2019, a les 15:27:07 CEST, Daniel Vrátil va escriure:
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)

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

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?

Cheers,
Albert

Re: Review Request: plasma-thunderbolt

By Daniel =?ISO-88... at 05/16/2019 - 05:12

On Wednesday, 15 May 2019 23:08:57 CEST Albert Astals Cid wrote:
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?).

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

Good point, fixed those.

Thanks for the review,
Dan

Re: Review Request: plasma-thunderbolt

By Nate Graham at 05/15/2019 - 09:34

+1 from me; the original was good and this looks good too.

One minor thing that I don't think should be a blocker: Could we copy
FindBolt.cmake into ECM with an eye towards not needing it here in a
future release?

Nate

On 5/15/19 7:27 AM, Daniel Vrátil wrote:

Re: Review Request: plasma-thunderbolt

By Daniel =?ISO-88... at 05/16/2019 - 04:19

On Wednesday, 15 May 2019 15:34:58 CEST Nate Graham wrote:
I think having FindBolt in ECM makes sense if there's a chance that some other
components will use it. Right now I don't think anyone else is going to look
for the Bolt daemon (as a runtime dependency).

Dan

Re: Review Request: plasma-thunderbolt

By Friedrich W. H.... at 05/15/2019 - 09:55

Am Mittwoch, 15. Mai 2019, 15:27:07 CEST schrieb Daniel Vrátil:
Pushed some small fixes to toplevel CMakeLists.txt

Other things seen on quick look at code (also not tested runtime):
* kded misses a Messages.sh file.
* no COPYRIGHT license files in the repo
* kde_enable_exceptions() duplicated a few times, perhaps only do in subdirs
where needed or use of kde_target_enable_exceptions() if fitting
* libkbolt being a private library could be reflected in the libname, also get
install(TARGETS kbolt ${KDE_INSTALL_TARGETS_DEFAULT_ARGS} LIBRARY
NAMELINK_SKIP)

Cheers
Friedrich

Re: Review Request: plasma-thunderbolt

By Daniel =?ISO-88... at 05/16/2019 - 04:57

On Wednesday, 15 May 2019 15:55:01 CEST Friedrich W. H. Kossebau wrote:
All fixed, thanks for the review (and the fixes)!

Dan