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 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
