On Sunday, 27 March 2022 21:28:38 CEST Harald Sitter wrote: > On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich <a.stipp...@gmx.net> 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