KSANECore in KDE review

[Alexander Stippich]

On Sunday, 27 March 2022 21:28:38 CEST Harald Sitter wrote:
> 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

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