RKWard is in kdereview - again

[Albert Astals Cid]

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.

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
>