KSANECore in KDE review

[Harald Sitter]

On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich <a.stippich at 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