Re: Review Request: plasma-thunderbolt
On Wed, May 15, 2019 at 03:27:07PM +0200, Daniel Vrátil wrote:
> 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.
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
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 | dvra...@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 signature.asc Description: This is a digitally signed message part.
Re: Review Request: plasma-thunderbolt
On Wednesday, 15 May 2019 15:55:01 CEST Friedrich W. H. Kossebau wrote:
> Am Mittwoch, 15. Mai 2019, 15:27:07 CEST schrieb Daniel Vrátil:
> > 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.
>
> 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)
All fixed, thanks for the review (and the fixes)!
Dan
>
> Cheers
> Friedrich
--
Daniel Vrátil
www.dvratil.cz | dvra...@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
signature.asc
Description: This is a digitally signed message part.
Re: Review Request: plasma-thunderbolt
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) 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 > > Thanks, > - Dan > > [0] https://phabricator.kde.org/D19011 > [1] https://cgit.kde.org/plasma-thunderbolt.git > >
Re: Review Request: plasma-thunderbolt
In data mercoledì 15 maggio 2019 15:27:07 CEST, Daniel Vrátil ha scritto: > 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. The repository at this point is missing either a LICENSE or a COPYING file with the correct license (there is none). -- Luca Beltrame GPG key ID: A29D259B signature.asc Description: This is a digitally signed message part.
Re: Review Request: plasma-thunderbolt
Am Mittwoch, 15. Mai 2019, 15:27:07 CEST schrieb Daniel Vrátil:
> 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.
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
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] https://phabricator.kde.org/D19011 [1] https://cgit.kde.org/plasma-thunderbolt.git -- Daniel Vrátil www.dvratil.cz | dvra...@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 signature.asc Description: This is a digitally signed message part.
