Re: KSANECore in KDE review
On Saturday, 2 April 2022 11:28:11 CEST Gilles Caulier wrote: > Hi, > > Just a question about Qt6 support in KSane* API. Do you plan this kind > of support in a near future? As i can see libksane is not yet ported. > > Best regards > > Gilles Caulier Hi, I turned on Qt6 CI for KSANECore and it works just fine. I don't know in what state the rest of libksane is in for Qt6. I will have a look when libksane uses KSANECore, so hopefully this will be sorted out during the KDE Gear 22.08 release cycle. Best regards, Alex > Le sam. 2 avr. 2022 à 11:26, Alexander Stippich a écrit : > > On Sunday, 27 March 2022 21:28:38 CEST Harald Sitter wrote: > > > On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich > > > > wrote: > > > > Hello everyone, > > > > > > > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > > > > before, but as a short summary: > > > > KSANECore is a Qt interface to the SANE scanner library. It is stripped > > > > out of > > > > > > the KSaneWidget of libksane without any QWidget dependency. It is > > > > currently > > > > > > located inside the libksane repository as KSaneCore and basically just > > > > copied > > > > > > into the new repository. > > > > > > > > Due to breaking API anyway, the code was cleaned up, better named as well > > > > as > > > > > > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > > > > KSaneWidget of libksane will remain ABI compatible. > > > > > > > > I don't know if it is strictly required to move the new repo with already > > > > existing code through KDE review, but I guessed it is better to be on the > > > > safe > > > > > > side :) > > > > > > > > The plan is to switch libksane and Skanpage over to the new library during > > > > the > > > > > > KDE Gear 22.08 release cycle. The adaptions are located at > > > > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > > > > and > > > > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit > > > > > > amazing! > > > > > > for everyone's convenience here's the url to the lib ;) > > > https://invent.kde.org/libraries/ksanecore > > > > Thanks! Could have thought about this myself... > > > > > some comments from scrolling through the code: > > > > > > you may want to reconsider how stringEnumTranslation works > > > https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md > > > either use q_global_static, or - IMHO neater - move it into the > > > function loadDeviceOptions as function local static since we don't > > > need it outside that function anyway. > > > > > > CoreInterface, CoreInterfacePrivate, InternalOption, ScanThread and > > > CoreOption should have their constructors marked explicit > > > > > > the typedefs in coreoption.h are super old school maybe modernize them? ;) > > > > > > some of the API documentation still refers to libksane, should that be > > > > changed? > > > > All done. > > > > > on a similar note, you still use the KSane namespace and include > > > guards. It may make sense to rename them KSaneCore and KSANECORE > > > respectively? for consistency if nothing else > > > > KSane was chosen deliberately here. The plan is to have a KSane::Core... and > > the KSane::Widget in the future. libksane would become KSaneWidget later > > during the Qt 6 transition, to keep ABI compatibility for now. > > > > > if you havent considered this: you might want to use the clang-format > > > rules from ECM to join the common formatting we have (and apply that > > > via commit hooks) > > > > Not done yet, but I will look into it. > > > > > I would argue that reloadDevicesList and getOption(const OptionName > > > optionEnum); should have their const dropped. the const there doesn't > > > impact the signature and as such is confusing from an API POV > > > > > > on a matter of less code noise I would use std::make_unique when > > > creating new unique ptrs instead of manually passing a raw pointer to > > > the ctor. > > > > > > in CoreInterfacePrivate m_batchMode + Delay are uninitialized in the > > > ctor. please initialize them nullptr > > > > > > since you have Qt6 ifdefs you may want to enable qt6 CI as well > > > > > > the shebang line in your Messages.sh appears to have lost its position > > > and is no longer at the top of the file > > > > All fixed. > > > > > you appear to have an autotests dir and cmakelists but no actual tests? :O > > > > That is reserved for the future when I find the time to actually write the > > tests :) > > > > Thanks for the review! > > > > Best regards, > > Alex
Re: KSANECore in KDE review
On Sunday, 27 March 2022 23:17:52 CEST Albert Astals Cid wrote: > El diumenge, 27 de març de 2022, a les 18:29:18 (CEST), Alexander Stippich va escriure: > > Hello everyone, > > > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > > before, but as a short summary: > > KSANECore is a Qt interface to the SANE scanner library. It is stripped out of > > the KSaneWidget of libksane without any QWidget dependency. It is currently > > located inside the libksane repository as KSaneCore and basically just copied > > into the new repository. > > > > Due to breaking API anyway, the code was cleaned up, better named as well as > > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > > KSaneWidget of libksane will remain ABI compatible. > > > > I don't know if it is strictly required to move the new repo with already > > existing code through KDE review, but I guessed it is better to be on the safe > > side :) > > Ran it through clang-tidy + my preferred options, came with a few suggestions, nothing earth shattering but i think it makes sense, if you don't, > that's fine, attached the file with the results, feel free to ask if you > don't understand what some warnings say Thanks, everything has been implemented. > The only other question i have is if you think that it may make sense to hide the members of DeviceInfo behind a d-pointer in case it ever needs > more and that would be an ABI change (not sure which kind of ABI promises you want to have for this library). Haven't really thought about that. It is pretty much tied to the information provided by SANE. And the SANE API has not changed in decades, if it ever will. So it hasn't caused any issues during the last years for libksane either, I think. But it may be worthwhile to wrap it in a class so that it is possible to just switch to a new SANE API version in case there will be one. I'm going to have a look, since I'm striving for ABI compatibility similar to libksane's widget interface. > > Ok, I lied, I have another question, what's the goal for the library? independently released? KDE Gear? KDE Frameworks? I would like to submit it to KDE Gear as soon as possible to get the transition to the new library done and such that it is released in conjunction with everything related to scanning (Skanlite, Skanpage, libksane). That said, I think it could become a KDE Framework in the future, but for example autotests are still missing. Thanks and best regards, Alex > Cheers, > Albert > > > The plan is to switch libksane and Skanpage over to the new library during the > > KDE Gear 22.08 release cycle. The adaptions are located at > > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > > and > > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit > > > > Best regards, > > Alex
Re: KSANECore in KDE review
Hi, Just a question about Qt6 support in KSane* API. Do you plan this kind of support in a near future? As i can see libksane is not yet ported. Best regards Gilles Caulier Le sam. 2 avr. 2022 à 11:26, Alexander Stippich a écrit : > > On Sunday, 27 March 2022 21:28:38 CEST Harald Sitter wrote: > > On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich > wrote: > > > Hello everyone, > > > > > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > > > before, but as a short summary: > > > KSANECore is a Qt interface to the SANE scanner library. It is stripped > out of > > > the KSaneWidget of libksane without any QWidget dependency. It is > currently > > > located inside the libksane repository as KSaneCore and basically just > copied > > > into the new repository. > > > > > > Due to breaking API anyway, the code was cleaned up, better named as well > as > > > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > > > KSaneWidget of libksane will remain ABI compatible. > > > > > > I don't know if it is strictly required to move the new repo with already > > > existing code through KDE review, but I guessed it is better to be on the > safe > > > side :) > > > > > > The plan is to switch libksane and Skanpage over to the new library during > the > > > KDE Gear 22.08 release cycle. The adaptions are located at > > > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > > > and > > > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit > > > > amazing! > > > > for everyone's convenience here's the url to the lib ;) > > https://invent.kde.org/libraries/ksanecore > > Thanks! Could have thought about this myself... > > > some comments from scrolling through the code: > > > > you may want to reconsider how stringEnumTranslation works > > https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md > > either use q_global_static, or - IMHO neater - move it into the > > function loadDeviceOptions as function local static since we don't > > need it outside that function anyway. > > > > CoreInterface, CoreInterfacePrivate, InternalOption, ScanThread and > > CoreOption should have their constructors marked explicit > > > > the typedefs in coreoption.h are super old school maybe modernize them? ;) > > > > some of the API documentation still refers to libksane, should that be > changed? > > All done. > > > on a similar note, you still use the KSane namespace and include > > guards. It may make sense to rename them KSaneCore and KSANECORE > > respectively? for consistency if nothing else > > KSane was chosen deliberately here. The plan is to have a KSane::Core... and > the KSane::Widget in the future. libksane would become KSaneWidget later > during the Qt 6 transition, to keep ABI compatibility for now. > > > if you havent considered this: you might want to use the clang-format > > rules from ECM to join the common formatting we have (and apply that > > via commit hooks) > > Not done yet, but I will look into it. > > > I would argue that reloadDevicesList and getOption(const OptionName > > optionEnum); should have their const dropped. the const there doesn't > > impact the signature and as such is confusing from an API POV > > > > on a matter of less code noise I would use std::make_unique when > > creating new unique ptrs instead of manually passing a raw pointer to > > the ctor. > > > > in CoreInterfacePrivate m_batchMode + Delay are uninitialized in the > > ctor. please initialize them nullptr > > > > since you have Qt6 ifdefs you may want to enable qt6 CI as well > > > > the shebang line in your Messages.sh appears to have lost its position > > and is no longer at the top of the file > > All fixed. > > > you appear to have an autotests dir and cmakelists but no actual tests? :O > > That is reserved for the future when I find the time to actually write the > tests :) > > Thanks for the review! > > Best regards, > Alex > > >
Re: KSANECore in KDE review
On Sunday, 27 March 2022 21:28:38 CEST Harald Sitter wrote: > On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich wrote: > > Hello everyone, > > > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > > before, but as a short summary: > > KSANECore is a Qt interface to the SANE scanner library. It is stripped out of > > the KSaneWidget of libksane without any QWidget dependency. It is currently > > located inside the libksane repository as KSaneCore and basically just copied > > into the new repository. > > > > Due to breaking API anyway, the code was cleaned up, better named as well as > > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > > KSaneWidget of libksane will remain ABI compatible. > > > > I don't know if it is strictly required to move the new repo with already > > existing code through KDE review, but I guessed it is better to be on the safe > > side :) > > > > The plan is to switch libksane and Skanpage over to the new library during the > > KDE Gear 22.08 release cycle. The adaptions are located at > > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > > and > > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit > > amazing! > > for everyone's convenience here's the url to the lib ;) > https://invent.kde.org/libraries/ksanecore Thanks! Could have thought about this myself... > some comments from scrolling through the code: > > you may want to reconsider how stringEnumTranslation works > https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md > either use q_global_static, or - IMHO neater - move it into the > function loadDeviceOptions as function local static since we don't > need it outside that function anyway. > > CoreInterface, CoreInterfacePrivate, InternalOption, ScanThread and > CoreOption should have their constructors marked explicit > > the typedefs in coreoption.h are super old school maybe modernize them? ;) > > some of the API documentation still refers to libksane, should that be changed? All done. > on a similar note, you still use the KSane namespace and include > guards. It may make sense to rename them KSaneCore and KSANECORE > respectively? for consistency if nothing else KSane was chosen deliberately here. The plan is to have a KSane::Core... and the KSane::Widget in the future. libksane would become KSaneWidget later during the Qt 6 transition, to keep ABI compatibility for now. > if you havent considered this: you might want to use the clang-format > rules from ECM to join the common formatting we have (and apply that > via commit hooks) Not done yet, but I will look into it. > I would argue that reloadDevicesList and getOption(const OptionName > optionEnum); should have their const dropped. the const there doesn't > impact the signature and as such is confusing from an API POV > > on a matter of less code noise I would use std::make_unique when > creating new unique ptrs instead of manually passing a raw pointer to > the ctor. > > in CoreInterfacePrivate m_batchMode + Delay are uninitialized in the > ctor. please initialize them nullptr > > since you have Qt6 ifdefs you may want to enable qt6 CI as well > > the shebang line in your Messages.sh appears to have lost its position > and is no longer at the top of the file All fixed. > you appear to have an autotests dir and cmakelists but no actual tests? :O That is reserved for the future when I find the time to actually write the tests :) Thanks for the review! Best regards, Alex
Re: KSANECore in KDE review
El diumenge, 27 de març de 2022, a les 18:29:18 (CEST), Alexander Stippich va escriure: > Hello everyone, > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > before, but as a short summary: > KSANECore is a Qt interface to the SANE scanner library. It is stripped out > of > the KSaneWidget of libksane without any QWidget dependency. It is currently > located inside the libksane repository as KSaneCore and basically just copied > into the new repository. > > Due to breaking API anyway, the code was cleaned up, better named as well as > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > KSaneWidget of libksane will remain ABI compatible. > > I don't know if it is strictly required to move the new repo with already > existing code through KDE review, but I guessed it is better to be on the > safe > side :) Ran it through clang-tidy + my preferred options, came with a few suggestions, nothing earth shattering but i think it makes sense, if you don't, that's fine, attached the file with the results, feel free to ask if you don't understand what some warnings say The only other question i have is if you think that it may make sense to hide the members of DeviceInfo behind a d-pointer in case it ever needs more and that would be an ABI change (not sure which kind of ABI promises you want to have for this library). Ok, I lied, I have another question, what's the goal for the library? independently released? KDE Gear? KDE Frameworks? Cheers, Albert > > The plan is to switch libksane and Skanpage over to the new library during > the > KDE Gear 22.08 release cycle. The adaptions are located at > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > and > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit > > Best regards, > Alex > > > > ksanecore/src/coreinterface.h:113:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreInterface(QObject *parent = nullptr); ^ explicit ksanecore/src/scanthread.h:43:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] ScanThread(SANE_Handle handle); ^ explicit ksanecore/src/coreinterface_p.h:35:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreInterfacePrivate(CoreInterface *parent); ^ explicit ksanecore/src/coreoption.h:73:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreOption(QObject *parent = nullptr); ^ explicit ksanecore/src/internaloption.h:22:5: error: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] InternalOption(BaseOption *option, QObject *parent = nullptr); ^ explicit ksanecore/src/coreinterface.cpp:35:26: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors] : QObject(parent), d(std::unique_ptr(new CoreInterfacePrivate(this))) ^~~ ~~ std::make_unique ksanecore/src/coreoption.cpp:16:62: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors] CoreOption::CoreOption(QObject *parent) : QObject(parent), d(std::unique_ptr(new OptionPrivate())) ^~~ ~~~ std::make_unique ksanecore/src/coreinterface.cpp:133:98: error: the parameter 'userName' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString , QString userName, QString password) ^ const & ksanecore/src/coreinterface.cpp:133:116: error: the parameter 'password' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString , QString userName, QString password) ^
Re: KSANECore in KDE review
On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich wrote: > > Hello everyone, > > KSANECore is now in KDE review. Kåre and I mentioned it in previous emails > before, but as a short summary: > KSANECore is a Qt interface to the SANE scanner library. It is stripped out of > the KSaneWidget of libksane without any QWidget dependency. It is currently > located inside the libksane repository as KSaneCore and basically just copied > into the new repository. > > Due to breaking API anyway, the code was cleaned up, better named as well as > smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. > KSaneWidget of libksane will remain ABI compatible. > > I don't know if it is strictly required to move the new repo with already > existing code through KDE review, but I guessed it is better to be on the safe > side :) > > The plan is to switch libksane and Skanpage over to the new library during the > KDE Gear 22.08 release cycle. The adaptions are located at > https://invent.kde.org/astippich/skanpage/-/commits/ksanecore > and > https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit amazing! for everyone's convenience here's the url to the lib ;) https://invent.kde.org/libraries/ksanecore some comments from scrolling through the code: you may want to reconsider how stringEnumTranslation works https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md either use q_global_static, or - IMHO neater - move it into the function loadDeviceOptions as function local static since we don't need it outside that function anyway. CoreInterface, CoreInterfacePrivate, InternalOption, ScanThread and CoreOption should have their constructors marked explicit the typedefs in coreoption.h are super old school maybe modernize them? ;) some of the API documentation still refers to libksane, should that be changed? on a similar note, you still use the KSane namespace and include guards. It may make sense to rename them KSaneCore and KSANECORE respectively? for consistency if nothing else if you havent considered this: you might want to use the clang-format rules from ECM to join the common formatting we have (and apply that via commit hooks) I would argue that reloadDevicesList and getOption(const OptionName optionEnum); should have their const dropped. the const there doesn't impact the signature and as such is confusing from an API POV on a matter of less code noise I would use std::make_unique when creating new unique ptrs instead of manually passing a raw pointer to the ctor. in CoreInterfacePrivate m_batchMode + Delay are uninitialized in the ctor. please initialize them nullptr since you have Qt6 ifdefs you may want to enable qt6 CI as well the shebang line in your Messages.sh appears to have lost its position and is no longer at the top of the file you appear to have an autotests dir and cmakelists but no actual tests? :O
KSANECore in KDE review
Hello everyone, KSANECore is now in KDE review. Kåre and I mentioned it in previous emails before, but as a short summary: KSANECore is a Qt interface to the SANE scanner library. It is stripped out of the KSaneWidget of libksane without any QWidget dependency. It is currently located inside the libksane repository as KSaneCore and basically just copied into the new repository. Due to breaking API anyway, the code was cleaned up, better named as well as smaller API fixes made on top. Also, KSANECore is fully REUSE compliant. KSaneWidget of libksane will remain ABI compatible. I don't know if it is strictly required to move the new repo with already existing code through KDE review, but I guessed it is better to be on the safe side :) The plan is to switch libksane and Skanpage over to the new library during the KDE Gear 22.08 release cycle. The adaptions are located at https://invent.kde.org/astippich/skanpage/-/commits/ksanecore and https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit Best regards, Alex
