Re: RKWard is in kdereview - again
El dilluns, 28 de març de 2022, a les 0:09:38 (CEST), Albert Astals Cid va escriure: > El dissabte, 26 de març de 2022, a les 21:34:06 (CEST), Thomas > Friedrichsmeier va escriure: > > Hi! > > > > KDE.org has been our home for a 7.5(!) years, now (after over a > > decade on sourceforge), but we still haven't left playground... After a > > lot of procrastination on that matter, a previous review failed due to > > lack of time on my part. Sorry! Now, finally, I'd like to ask you to > > start reviewing RKWard for inclusion into exragear / education, once > > more. > > > > RKWard is an easy to use and easily extensible IDE/GUI for R (a > > powerful language and environment for statistical computing, if you > > did not know it, yet). It aims to combine the power of the > > R-language with the ease of use of commercial statistics tools. > > > > RKWard is used productively on Linux/BSD, Mac, and Windows. > > > > > > > > Here's a summary of the critical comments we got in the previous round > > of review (https://marc.info/?l=kde-core-devel=153883367600690=2): > > > > Albert Astals Cid remarked > > (https://marc.info/?l=kde-core-devel=153912336114603=2): > > > > > the i18n folder seems like from the past and something you should not > > > need if in kde infrastructure. Could you delete it? > > > > We cannot easily get rid of this, entirely, as we are shipping > > (non-compiled) plugins that keep their message catalogs in relative > > paths, and thus we have to install .mo files to custom locations. Pino > > Toscano has helped to bring this more in line with standard KDE.org > > processes (https://invent.kde.org/education/rkward/-/merge_requests/2). > > > > > Also I'd suggest you compile enabling > > > -DECM_ENABLE_SANITIZERS="address;undefined" > > > > Thanks for the hint, did not know that switch. One effect of this is a > > lot of false positive "runtime errors", when casting a half-destroyed > > QObject* to its original (derived) type, in order to remove it from > > containers (e.g. a QHash of KActionCollection()s). Is there an elegant > > way around this? > > > > > There's a few memory leaks (reported at exit) that you may want to > > > have a look. > > > > I fixed a lot of that, but didn't work out all of them. > > > > > And there's also a few undefined behaviour warnings on exit, you've > > > them marked as "known" things but it'd be good if you could find a way > > > to fix them. > > > > Not quite sure what you were referring to, esp. what I may have marked > > as known behavior. I did fix several quirks at exit since this comment > > (and I keep introducing new ones, all the time). > > > > > Your help menu is for some reason missing the Change Language option, > > > i tried to do it a quick fix but could not, i would appreciate if you > > > could find a way to only define the extra actions and not all of them > > > (like we do for example in okular). > > > > I've added the switch application language option (in Settings rather > > than Help). Our help menu is perhaps more customized than meets the eye > > on first glance. For instance, we have an app-integrated help system > > instead of external handbook (at least as the primary documentation), > > and our bug report dialog is beefed up to include a lot of extra > > information by default (mostly importantly: version of R). I guess it > > would be possible to hack KHelpMenu this way, but at least at some > > point in the past it seemed cleaner to implement "from scratch" (but > > using the default actions, where applicable). > > > > > > > > Comments by Jonathan Riddel > > (https://marc.info/?l=kde-core-devel=153916691226377=2): > > > > > It installs two desktop files which creates duplicate menu entries > > > /usr/share/applications/org.kde.rkward-open.desktop > > > /usr/share/applications/org.kde.rkward.desktop > > > > Completed following suggestions by Thomas Baumgart and Meik Michalke: > > https://marc.info/?l=kde-core-devel=153942961310071=2 > > > > > The .desktop files call it a "GUI for R" which is not a great > > > description, everything in the menu is a GUI. I recommend "R > > > Statistical Programming" or "IDE for R" maybe. > > > > Renamed to Statistics with R, following Meik's suggestion > > (https://marc.info/?l=kde-core-devel=153942595709279=2). > > > > > I tidied up the files with the icon licence as they could easily be > > > lost. > > > > Done by Jonathan. > > > > > It depends on WebKit which is not supported, could this be ported to > > > WebEngine? > > > > Meanwhile, RKWard can be compiled to use QtWebEngine (and this is the > > default), but support for webkit has not been dropped, because MinGW is > > an important platform for us. > > > > > It's uncommon having debian/ packaging directly in the source and > > > there's also debian-official/ which could get confusing and > > > out-of-sync and messy. I recommend moving them to another archive. > > > Storing the packaging in KDE neon Git would be cool as
Re: RKWard is in kdereview - again
El dissabte, 26 de març de 2022, a les 21:34:06 (CEST), Thomas Friedrichsmeier va escriure: > Hi! > > KDE.org has been our home for a 7.5(!) years, now (after over a > decade on sourceforge), but we still haven't left playground... After a > lot of procrastination on that matter, a previous review failed due to > lack of time on my part. Sorry! Now, finally, I'd like to ask you to > start reviewing RKWard for inclusion into exragear / education, once > more. > > RKWard is an easy to use and easily extensible IDE/GUI for R (a > powerful language and environment for statistical computing, if you > did not know it, yet). It aims to combine the power of the > R-language with the ease of use of commercial statistics tools. > > RKWard is used productively on Linux/BSD, Mac, and Windows. > > > > Here's a summary of the critical comments we got in the previous round > of review (https://marc.info/?l=kde-core-devel=153883367600690=2): > > Albert Astals Cid remarked > (https://marc.info/?l=kde-core-devel=153912336114603=2): > > > the i18n folder seems like from the past and something you should not > > need if in kde infrastructure. Could you delete it? > > We cannot easily get rid of this, entirely, as we are shipping > (non-compiled) plugins that keep their message catalogs in relative > paths, and thus we have to install .mo files to custom locations. Pino > Toscano has helped to bring this more in line with standard KDE.org > processes (https://invent.kde.org/education/rkward/-/merge_requests/2). > > > Also I'd suggest you compile enabling > > -DECM_ENABLE_SANITIZERS="address;undefined" > > Thanks for the hint, did not know that switch. One effect of this is a > lot of false positive "runtime errors", when casting a half-destroyed > QObject* to its original (derived) type, in order to remove it from > containers (e.g. a QHash of KActionCollection()s). Is there an elegant > way around this? > > > There's a few memory leaks (reported at exit) that you may want to > > have a look. > > I fixed a lot of that, but didn't work out all of them. > > > And there's also a few undefined behaviour warnings on exit, you've > > them marked as "known" things but it'd be good if you could find a way > > to fix them. > > Not quite sure what you were referring to, esp. what I may have marked > as known behavior. I did fix several quirks at exit since this comment > (and I keep introducing new ones, all the time). > > > Your help menu is for some reason missing the Change Language option, > > i tried to do it a quick fix but could not, i would appreciate if you > > could find a way to only define the extra actions and not all of them > > (like we do for example in okular). > > I've added the switch application language option (in Settings rather > than Help). Our help menu is perhaps more customized than meets the eye > on first glance. For instance, we have an app-integrated help system > instead of external handbook (at least as the primary documentation), > and our bug report dialog is beefed up to include a lot of extra > information by default (mostly importantly: version of R). I guess it > would be possible to hack KHelpMenu this way, but at least at some > point in the past it seemed cleaner to implement "from scratch" (but > using the default actions, where applicable). > > > > Comments by Jonathan Riddel > (https://marc.info/?l=kde-core-devel=153916691226377=2): > > > It installs two desktop files which creates duplicate menu entries > > /usr/share/applications/org.kde.rkward-open.desktop > > /usr/share/applications/org.kde.rkward.desktop > > Completed following suggestions by Thomas Baumgart and Meik Michalke: > https://marc.info/?l=kde-core-devel=153942961310071=2 > > > The .desktop files call it a "GUI for R" which is not a great > > description, everything in the menu is a GUI. I recommend "R > > Statistical Programming" or "IDE for R" maybe. > > Renamed to Statistics with R, following Meik's suggestion > (https://marc.info/?l=kde-core-devel=153942595709279=2). > > > I tidied up the files with the icon licence as they could easily be > > lost. > > Done by Jonathan. > > > It depends on WebKit which is not supported, could this be ported to > > WebEngine? > > Meanwhile, RKWard can be compiled to use QtWebEngine (and this is the > default), but support for webkit has not been dropped, because MinGW is > an important platform for us. > > > It's uncommon having debian/ packaging directly in the source and > > there's also debian-official/ which could get confusing and > > out-of-sync and messy. I recommend moving them to another archive. > > Storing the packaging in KDE neon Git would be cool as we already > > have packaging for all the rest of KDE software. > > Meanwhile packaging has been taken over by the debian-qt-kde team > (thank you so much!). Jonathan himself added RKWard to neon. We still > have a (basic) debian packaging to support nightly builds in our Ubuntu > PPAs (which fill
Re: KSANECore in KDE review
El diumenge, 27 de març de 2022, a les 18:29:18 (CEST), Alexander Stippich va escriure: > 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 :) Ran it through clang-tidy + my preferred options, came with a few suggestions, nothing earth shattering but i think it makes sense, if you don't, that's fine, attached the file with the results, feel free to ask if you don't understand what some warnings say The only other question i have is if you think that it may make sense to hide the members of DeviceInfo behind a d-pointer in case it ever needs more and that would be an ABI change (not sure which kind of ABI promises you want to have for this library). Ok, I lied, I have another question, what's the goal for the library? independently released? KDE Gear? KDE Frameworks? Cheers, Albert > > 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 > > Best regards, > Alex > > > > ksanecore/src/coreinterface.h:113:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreInterface(QObject *parent = nullptr); ^ explicit ksanecore/src/scanthread.h:43:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] ScanThread(SANE_Handle handle); ^ explicit ksanecore/src/coreinterface_p.h:35:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreInterfacePrivate(CoreInterface *parent); ^ explicit ksanecore/src/coreoption.h:73:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] CoreOption(QObject *parent = nullptr); ^ explicit ksanecore/src/internaloption.h:22:5: error: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] InternalOption(BaseOption *option, QObject *parent = nullptr); ^ explicit ksanecore/src/coreinterface.cpp:35:26: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors] : QObject(parent), d(std::unique_ptr(new CoreInterfacePrivate(this))) ^~~ ~~ std::make_unique ksanecore/src/coreoption.cpp:16:62: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors] CoreOption::CoreOption(QObject *parent) : QObject(parent), d(std::unique_ptr(new OptionPrivate())) ^~~ ~~~ std::make_unique ksanecore/src/coreinterface.cpp:133:98: error: the parameter 'userName' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString , QString userName, QString password) ^ const & ksanecore/src/coreinterface.cpp:133:116: error: the parameter 'password' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString , QString userName, QString password) ^
Re: KSANECore in KDE review
On Sun, Mar 27, 2022 at 6:29 PM Alexander Stippich 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
KSANECore in KDE review
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 Best regards, Alex
