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 >