Re: RKWard is in kdereview - again
Hi, I recently used RKWard for my Master Thesis, cool project! A couple of observations: - CommitPolicy.txt still mentions Phabricator, that should point to Gitlab instead - CommitPolicy.txt mentions Ubuntu Trusty as base for requirements, that is *ancient* by now, maybe 20.04 would be a more reasonable base? - The repo doesn't have Gitlab CI, that should be added - Optional/nice-to-have: We tend to use SPDX indentifiers for license headers these days, maybe think about converting the existing headers - You are using QtScript, which is deprecated. What's your plan for that? - I see you are distributing stable Windows releases, but there are no stable build jobs for that on binary-factory.kde.org - Consider adding links to the Windows releases to the appstream metadata. That way they show up on apps.kde.org/rkward. See https://invent.kde.org/utilities/kate/-/merge_requests/667 for an example. - On Windows we tend to use the Breeze QStyle since that looks and works quite a bit better than the "native" style. See https://invent.kde.org/utilities/kate/-/blob/master/kate/main.cpp#L110 - Consider adding a color scheme selector (KColorSchemeManager) to the menu. On Windows that gives you dark mode support for free (this only really works nicely when using Breeze QStyle). See https://invent.kde.org/utilities/kate/-/blob/master/kate/katemainwindow.cpp#L235 - On Windows the title bar shows a generic icon instead of the RKWard icon - Consider publishing RKWard on the Microsoft Store - Consider publshing RKWard on Flathub Feel free to reach out for questions/help with any of this. Cheers Nico
Re: RKWard is in kdereview - again
On Saturday, 26 March 2022 21:34:06 CEST Thomas Friedrichsmeier wrote: > 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 used productively on Linux/BSD, Mac, and Windows. Congratulations! RKWard has been packaged on FreeBSD for a long time already (although it's only at version 0.7.1, not the latest release -- cc'ing the FreeBSD maintainer there). On the FreeBSD side there's only two patches in packaging; one missing include (not needed with current git) and a CMake thingy that I don't quite understand (about gfortran). I do notice that there's a FindR.cmake in Cantor, and one in RKWard, and they are somewhat different: perhaps some coordination to make them both do the same (and the right) things? [ade] signature.asc Description: This is a digitally signed message part.
Re: RKWard is in kdereview - again
Hi, On Mon, 28 Mar 2022 15:39:55 +0200 Nicolas Fella wrote: > I recently used RKWard for my Master Thesis, cool project! thanks! > A couple of observations: > > - CommitPolicy.txt still mentions Phabricator, that should point to > Gitlab instead Done. > - CommitPolicy.txt mentions Ubuntu Trusty as base for requirements, > that is *ancient* by now, maybe 20.04 would be a more reasonable base? Done. > - The repo doesn't have Gitlab CI, that should be added Yes. Help (or some pointers) on that would be appreciated, though. > - Optional/nice-to-have: We tend to use SPDX indentifiers for license > headers these days, maybe think about converting the existing headers Deferred to the issue list, for now: https://invent.kde.org/education/rkward/-/issues/8 > - You are using QtScript, which is deprecated. What's your plan for > that? I'm aware we'll have to to port to QJSEngine for KF6. I hope to start looking into that, soon. https://invent.kde.org/education/rkward/-/issues/6 > - I see you are distributing stable Windows releases, but there are no > stable build jobs for that on binary-factory.kde.org Yes, that's rather hackish. Last I asked, I was told to get out of playground, before we could have separate stable builds (or was that stable translations, one of these two, anyway). So kdereview is actually part of the plan to get there. > - Consider adding links to the Windows releases to the appstream > metadata. That way they show up on apps.kde.org/rkward. See > https://invent.kde.org/utilities/kate/-/merge_requests/667 for an > example. Thanks for the pointer. Done. > - On Windows we tend to use the Breeze QStyle since that looks and > works quite a bit better than the "native" style. See > https://invent.kde.org/utilities/kate/-/blob/master/kate/main.cpp#L110 > > - Consider adding a color scheme selector (KColorSchemeManager) to the > menu. On Windows that gives you dark mode support for free (this only > really works nicely when using Breeze QStyle). See > https://invent.kde.org/utilities/kate/-/blob/master/kate/katemainwindow.cpp#L235 > > - On Windows the title bar shows a generic icon instead of the RKWard > icon Will look into these, soon. > - Consider publishing RKWard on the Microsoft Store > > - Consider publshing RKWard on Flathub Make sense. First step (for the latter) will be to create a flatpak, of course. > Feel free to reach out for questions/help with any of this. Creating a flatpak is one of the things I shied away from looking into too closely, so far. It does not look to complicated from a first glance (yet I suppose we will have to include R, too), but I sure wouldn't mind a volunteer stepping up... Thanks for all the pointers! Thomas pgpsQJJOkYbgE.pgp Description: OpenPGP digital signature
Re: RKWard is in kdereview - again
On 3/28/22 16:53, Adriaan de Groot wrote: On Saturday, 26 March 2022 21:34:06 CEST Thomas Friedrichsmeier wrote: 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 used productively on Linux/BSD, Mac, and Windows. Congratulations! RKWard has been packaged on FreeBSD for a long time already (although it's only at version 0.7.1, not the latest release -- cc'ing the FreeBSD maintainer there). On the FreeBSD side there's only two patches in packaging; one missing include (not needed with current git) and a CMake thingy that I don't quite understand (about gfortran). I do notice that there's a FindR.cmake in Cantor, and one in RKWard, and they are somewhat different: perhaps some coordination to make them both do the same (and the right) things? [ade] Talking about FreeBSD: I started adding Gitlab CI and the FreeBSD build fails: https://invent.kde.org/education/rkward/-/jobs/274861, presumably due to having a different tar implementation than Linux. I'd appreciate if someone could comment/look into that Cheers Nico
Re: RKWard is in kdereview - again
On Mon, 28 Mar 2022 00:09:38 +0200 Albert Astals Cid wrote: [...] > Results of running clang-tidy with some of my favorite options > attached. Thanks! > The first group [bugprone-integer-division] seems an actual bug since >double RKGraphicsDeviceFrontendTransmitter::lwdscale = 72/96; > means lwdscale value is 0 Fixed. > The second group [bugprone-parent-virtual-call] is worth looking at, > may be what you want or may not. None of these caused real misbehavior, but in none of the cases there was a terribly good reason for writing them, this way, either. Changed. > 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 Fixed. One was actually a (minor / optimization) bug, where the bool was meant to be an int. > 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?. Will do, but deferred for a quiet hour: https://invent.kde.org/education/rkward/-/issues/9 Regards Thomas pgp9ORnxDk8LR.pgp Description: OpenPGP digital signature
Re: RKWard is in kdereview - again
On Mon, 28 Mar 2022 17:09:44 +0200 Nicolas Fella wrote: [...] > Talking about FreeBSD: I started adding Gitlab CI and the FreeBSD > build fails: https://invent.kde.org/education/rkward/-/jobs/274861, > presumably due to having a different tar implementation than Linux. Sweet! > I'd appreciate if someone could comment/look into that Ouch, that issue, again. The option can safely be stripped, but was added to make builds reproducible ( https://invent.kde.org/education/rkward/-/merge_requests/6). I've limited that to "Linux" systems, now. Regards Thomas pgpEPK2w38lyW.pgp Description: OpenPGP digital signature
Re: RKWard is in kdereview - again
On Monday, 28 March 2022 17:39:17 CEST Thomas Friedrichsmeier wrote: > On Mon, 28 Mar 2022 17:09:44 +0200 > Nicolas Fella wrote: > [...] > > > Talking about FreeBSD: I started adding Gitlab CI and the FreeBSD > > build fails: https://invent.kde.org/education/rkward/-/jobs/274861, > > presumably due to having a different tar implementation than Linux. > > Sweet! > > > I'd appreciate if someone could comment/look into that > > Ouch, that issue, again. The option can safely be stripped, but was > added to make builds reproducible ( > https://invent.kde.org/education/rkward/-/merge_requests/6). I've > limited that to "Linux" systems, now. Relevant bits are a bug report asking for the same in ECM: https://bugs.kde.org/show_bug.cgi?id=443532 The corresponding MR: https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/238 and associated commentary. Stealing from ECM is encouraged :) [ade] signature.asc Description: This is a digitally signed message part.
