KDEReview for Kontrast

[Carl Schwan]

Le dimanche, août 2, 2020 6:20 PM, Albert Astals Cid <aacid at kde.org> a écrit :

> El dijous, 30 de juliol de 2020, a les 11:16:25 CEST, Carl Schwan va escriure:
>
> > Hi,
> > I would like to move Kontrast, a contrast checker application, to KDEReview Kontrast can check if two colors pass the WCAG 2.0 specification and save some user's favorite color combinations.
> > Some screenshots of the application and a design review from the VSG is available here: https://invent.kde.org/accessibility/kontrast/-/issues/1
> > From a code point of view, the application is very simple, but I still would appreciate a general code review on it (it's my first Qt app written from scratch). The code is available here: https://invent.kde.org/accessibility/kontrast
> > I don't plan to add new features and would like after the KDEReview, to release a first version of the application, and then move it to the release service so that the application gets regularly translations improvement.

Hi Albert,

Thanks a lot for the review

> You don't have an icon, which is not optimal [actually i see you have an icon in invent.k.o so the hard part of drawing it seems to be done :)]

I added the icon and I hope I installed it to the correct location: https://invent.kde.org/accessibility/kontrast/-/commit/8a008c1387c0234d5e1d537bdd331007d7b1ff07. It was already stored in breeze-icons but I guess it is better to also have the app icon in the application so that it is displayed on other DEs.

>
> The # of the colors is cut for me https://i.imgur.com/1GC2sEU.png
>
> Missing i18n:
> ./src/contents/ui/MainPage.qml:28: title: "Please choose a color"

Fixed now

>
> Would be great if you had the typical --help --author, etc.
> See QCommandLineParser and KAboutData::setupCommandLine
>
> Would a documentation of the ranges make sense?
> i.e. something that has the ranges and the descriptions you put for each of the ranges in one place? Something like a "Help" page.

Great ideas, I put them on my TODO list. https://invent.kde.org/accessibility/kontrast/-/issues

>
> Could only test part of the app since you're requiring unreleased Kirigami 2.14
> Which probably means your
> set(KF5_MIN_VERSION "5.70.0")
> should be changed to
> set(KF5_MIN_VERSION "5.73.0")

I have now changed the kirigami dependency to require an older Kirigami version, since
I wasn't using any new Kirigami feature anyway. But I would still recommend using Kontrast
with the latest Kirigami version since the new version comes with a few Accessibility
improvements ;)

>
> Out of curiosity any reason you decided to go with
> auto SavedColorModel::refresh() -> void
> instead of
> void SavedColorModel::refresh()
> ?

This code was contributed by Carson Black and I have no strong preferences for the coding style
of the methods. I guess changing it back to the traditional style could make sense to follow
the general KDE coding style.


>
> Cheers,
> Albert
>
> > Thanks
> > Carl