RKWard is in kdereview - again

Albert Astals Cid aacid at kde.org
Mon Mar 28 00:28:14 BST 2022


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&m=153883367600690&w=2):
> > 
> > Albert Astals Cid remarked
> > (https://marc.info/?l=kde-core-devel&m=153912336114603&w=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&m=153916691226377&w=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&m=153942961310071&w=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&m=153942595709279&w=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 a somewhat different gap than Neon), but this is now
> > kept in a separate repository.
> > 
> > 
> > 
> > Jonathan commented in a separate mail
> > (https://marc.info/?l=kde-core-devel&m=154118912915065&w=2)
> > 
> > > There's no appstream metainfo file nor product-screenshot
> > > https://community.kde.org/Guidelines_and_HOWTOs/AppStream
> > 
> > Done: Contributed by Meik and Yuri
> > 
> > 
> > 
> > Thanks to all for your feedback last time (I left out feedback that did
> > no criticize anything, but I hope I have not left out anything
> > substantial in my summary)!
> > 
> > Looking forward to your comments.
> 
> Results of running clang-tidy with some of my favorite options attached.

Now with infinity percent more attachments.

> 
> The first group [bugprone-integer-division] seems an actual bug since 
>    double RKGraphicsDeviceFrontendTransmitter::lwdscale = 72/96;
> means lwdscale value is 0
> 
> 
> The second group [bugprone-parent-virtual-call] is worth looking at, may be what you want or may not.
> 
> 
> The third group [modernize-use-bool-literals] is totally just for our monkey brains, the compiler doesn't care, so if you don't care either it's fine to not change it
> 
> 
> The fourth group [performance-unnecessary-value-param] is just about adding a few const & to copy less things, mostly it's Qt things that are cheap to copy, so you're not going to get much performance, but the const & is faster anyway, so why not do it?.
> 
> 
> Cheers,
>   Albert
> 
> > 
> > Thomas
> > 
> 
> 
> 
> 
> 

-------------- next part --------------
rkward/rbackend/rkwarddevice/rkgraphicsdevice_frontendtransmitter.cpp:38:56: warning: result of integer division used in a floating point context; possible loss of precision [bugprone-integer-division]
double RKGraphicsDeviceFrontendTransmitter::lwdscale = 72/96;
                                                       ^
rkward/rbackend/rkwarddevice/rkgraphicsdevice_frontendtransmitter.cpp:268:54: warning: result of integer division used in a floating point context; possible loss of precision [bugprone-integer-division]
                                        RKGraphicsDeviceFrontendTransmitter::lwdscale = desktop->physicalDpiX () / 96;   // taken from devX11.c
                                                                                        ^





rkward/core/renvironmentobject.cpp:156:8: warning: qualified name 'RObject::updateStructure' refers to a member overridden in subclass; did you mean 'RContainerObject'? [bugprone-parent-virtual-call]
                if (!RObject::updateStructure (new_data)) return false;
                     ^~~~~~~~~
                     RContainerObject::

rkward/plugin/rkoptionset.cpp:884:9: warning: qualified name 'QAbstractItemModel::flags' refers to a member overridden in subclass; did you mean 'QAbstractTableModel'? [bugprone-parent-virtual-call]
        return QAbstractItemModel::flags (index) | Qt::ItemIsDragEnabled | Qt::ItemIsDropEnabled;
               ^~~~~~~~~~~~~~~~~~~~
               QAbstractTableModel::

rkward/core/rkpseudoobjects.cpp:150:17: warning: qualified name 'RObject::getObjectDescription' refers to a member overridden in subclass; did you mean 'REnvironmentObject'? [bugprone-parent-virtual-call]
        QString desc = RObject::getObjectDescription ();
                       ^~~~~~~~~
                       REnvironmentObject::

rkward/settings/rksettingsmoduleplugins.cpp:486:24: warning: qualified name 'QAbstractItemModel::flags' refers to a member overridden in subclass; did you mean 'QAbstractTableModel'? [bugprone-parent-virtual-call]
        Qt::ItemFlags flags = QAbstractItemModel::flags (index);
                              ^~~~~~~~~~~~~~~~~~~~
                              QAbstractTableModel::





rkward/scriptbackends/qtscriptbackend.cpp:201:9: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
        while (1) {
               ^
               true
rkward/scriptbackends/qtscriptbackend.cpp:299:9: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
        while (1) {
               ^
               true

rkward/core/rkrownames.cpp:142:20: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
        bool from_index = 0;
                          ^
                          false

rkward/rbackend/rksessionvars.cpp:80:10: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
                while (1) {
                       ^
                       true

rkward/rbackend/rkrbackend.cpp:277:10: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
                while (1) {
                       ^
                       true






rkward/agents/rksaveagent.cpp:38:36: warning: the parameter 'url' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
bool checkOverwriteWorkspace (QUrl url, QWidget *parent) {
                                   ^
                              const  &

rkward/misc/rkcommonfunctions.cpp:238:30: warning: the const qualified parameter 'tip' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
        void setTips (const QString tip, QWidget *first, QWidget *second, QWidget *third) {
                                    ^
                                   &

rkward/scriptbackends/qtscriptbackend.cpp:332:56: warning: the parameter 'scriptfile' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
        bool loadCommonScript (QScriptEngine* engine, QString scriptfile) {
                                                              ^
                                                      const  &

rkward/core/robjectlist.cpp:220:55: warning: the const qualified parameter 'namespace_names' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void RObjectList::updateNamespaces (const QStringList namespace_names) {
                                                      ^
                                                     &

rkward/windows/rkcommandeditorwindow.cpp:104:75: warning: the const qualified parameter '_url' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
RKCommandEditorWindow::RKCommandEditorWindow (QWidget *parent, const QUrl _url, const QString& encoding, int flags) : RKMDIWindow (parent, RKMDIWindow::CommandEditorWindow) {
                                                                          ^
                                                                         &

rkward/windows/rkcommandeditorwindow.cpp:1144:60: warning: the const qualified parameter 'r_command' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
QString RKCommandHighlighter::commandToHTML (const QString r_command, HighlightingMode mode) {
                                                           ^
                                                          &

rkward/plugin/rkcomponentmap.cpp:176:93: warning: the const qualified parameter 'group' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
RKComponentGUIXML::Group *findOrCreateGroup (RKComponentGUIXML::Menu *parent, const QString group) {
                                                                                            ^
                                                                                           &

rkward/plugin/rkcomponentmap.cpp:207:107: warning: the const qualified parameter 'after_group' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void insertGroup (RKComponentGUIXML::Menu *parent, const QString &group_id, bool separated, const QString after_group) {
                                                                                                          ^
                                                                                                         &

rkward/plugin/rkcomponentmap.cpp:234:99: warning: the const qualified parameter 'in_group' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void insertEntry (RKComponentGUIXML::Menu *parent, RKComponentGUIXML::Entry *entry, const QString in_group) {
                                                                                                  ^
                                                                                                 &

rkward/plugin/rkcomponentmap.cpp:245:83: warning: the const qualified parameter 'id' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
RKComponentGUIXML::Menu *findMenu (RKComponentGUIXML::Menu *parent, const QString id) {
                                                                                  ^
                                                                                 &

rkward/plugin/rkcomponentmap.cpp:260:101: warning: the const qualified parameter 'description' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
int RKComponentGUIXML::addEntries (RKComponentGUIXML::Menu *menu, XMLHelper &xml, const QDomElement description, const QString& cnamespace) {
                                                                                                    ^
                                                                                                   &

rkward/plugin/rkcomponentmap.cpp:547:73: warning: the const qualified parameter 'message' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void RKPluginMapParseResult::addAndPrintError (int level, const QString message) {
                                                                        ^
                                                                       &

rkward/settings/rksettingsmodule.cpp:88:141: warning: the parameter 'setter' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
QComboBox* RKConfigBase::makeDropDownHelper(const LabelList &entries, RKSettingsModuleWidget* module, int initial, std::function<void(int)> setter) {
                                                                                                                                            ^
                                                                                                                   const                   &

rkward/settings/rksettings.cpp:162:55: warning: the const qualified parameter 'url' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
        connect(l, &QLabel::linkActivated, [=](const QString url) { RKWorkplace::mainWorkplace()->openAnyUrl(QUrl(url)); });
                                                             ^
                                                            &

rkward/windows/rkhtmlwindow.cpp:1245:52: warning: the parameter 'path' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
QString RKHelpRenderer::componentPathToId (QString path) {
                                                   ^
                                           const  &

rkward/main.cpp:114:38: warning: the const qualified parameter 'appname' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
QString findExeAtPath (const QString appname, const QString &path) {
                                     ^
                                    &

rkward/main.cpp:174:52: warning: the parameter 'message' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
QString resolveRSpecOrFail (QString input, QString message) {
                                                   ^
                                           const  &

rkward/rkconsole.cpp:216:44: warning: the parameter 'name' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void RKConsole::triggerEditAction (QString name) {
                                           ^
                                   const  &

rkward/core/rkpseudoobjects.cpp:52:82: warning: the const qualified parameter 'name' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
RKNamespaceObject::RKNamespaceObject (REnvironmentObject* package, const QString name) : REnvironmentObject (package, name.isNull () ? "NAMESPACE" : name) {
                                                                                 ^
                                                                                &

rkward/settings/rksettingsmoduleplugins.cpp:117:125: warning: the const qualified parameter 'new_list' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
RKSettingsModulePlugins::PluginMapList RKSettingsModulePlugins::setPluginMaps (const RKSettingsModulePlugins::PluginMapList new_list) {
                                                                                                                            ^
                                                                                                                           &

rkward/plugin/rkstandardcomponent.cpp:540:62: warning: the const qualified parameter 'id' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
QDomElement RKComponentBuilder::doElementCopy (const QString id, XMLHelper &xml, const QDomElement &copy) {
                                                             ^
                                                            &

rkward/rbackend/rkrbackend.cpp:663:36: warning: the parameter 'files' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void REditFilesHelper (QStringList files, QStringList titles, QString wtitle, RBackendRequest::RCallbackType edit, bool delete_files, bool prompt) {
                                   ^
                       const      &

rkward/rbackend/rkrbackend.cpp:663:55: warning: the parameter 'titles' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void REditFilesHelper (QStringList files, QStringList titles, QString wtitle, RBackendRequest::RCallbackType edit, bool delete_files, bool prompt) {
                                                      ^
                                          const      &

rkward/rbackend/rkrbackend.cpp:663:71: warning: the parameter 'wtitle' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void REditFilesHelper (QStringList files, QStringList titles, QString wtitle, RBackendRequest::RCallbackType edit, bool delete_files, bool prompt) {
                                                                      ^
                                                              const  &

rkward/rbackend/rkrbackend.cpp:736:29: warning: the parameter 'caption' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                            ^
                    const  &

rkward/rbackend/rkrbackend.cpp:736:46: warning: the parameter 'message' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                                             ^
                                     const  &

rkward/rbackend/rkrbackend.cpp:736:63: warning: the parameter 'button_yes' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                                                              ^
                                                      const  &

rkward/rbackend/rkrbackend.cpp:736:83: warning: the parameter 'button_no' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                                                                                  ^
                                                                          const  &

rkward/rbackend/rkrbackend.cpp:736:102: warning: the parameter 'button_cancel' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                                                                                                     ^
                                                                                             const  &

rkward/rbackend/rkrbackend.cpp:736:125: warning: the parameter 'default_button' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int doDialogHelper (QString caption, QString message, QString button_yes, QString button_no, QString button_cancel, QString default_button, bool wait) {
                                                                                                                            ^
                                                                                                                    const  &

rkward/rbackend/rkrbackend.cpp:902:23: warning: the parameter 'callstring' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void doError (QString callstring) {
                      ^
              const  &

rkward/windows/rkworkplace.cpp:810:65: warning: the const qualified parameter 'old_base' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
QUrl checkAdjustRestoredUrl (const QString &_url, const QString old_base) {
                                                                ^
                                                               &

rkward/windows/rkworkplace.cpp:913:80: warning: the parameter 'spec' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
RKMDIWindow* restoreDocumentWindowInternal (RKWorkplace* wp, ItemSpecification spec, const QString &base) {
                                                                               ^
                                                             const            &

rkward/windows/rkworkplace.cpp:1075:41: warning: the const qualified parameter 'windows' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
        void update (const QList<RKMDIWindow*> windows) {
                                               ^
                                              &

rkward/misc/rkoutputdirectory.cpp:42:75: warning: the const qualified parameter 'prefix' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void listDirectoryState(const QString& _dir, QString *list, const QString prefix) {
                                                                          ^
                                                                         &


More information about the kde-core-devel mailing list