KSANECore in KDE review

[Gilles Caulier]

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.stippich at gmx.net> 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 <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
>
>
>