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 ©) {
^
&
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