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.stipp...@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.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 > > >