KDEReview for Kontrast

[Carl Schwan]

Le lundi, août 3, 2020 11:12 PM, Albert Astals Cid <aacid at kde.org> a écrit :

> El dilluns, 3 d’agost de 2020, a les 4:26:25 CEST, Carl Schwan va escriure:
>
> > 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.
>
> I'm 93% sure the file should be kontrast.svg given you're doing
> QApplication::setWindowIcon(QIcon::fromTheme(QStringLiteral("kontrast")));

I updated my setWindowIcon call now and the icon now works.

>
> > > The # of the colors is cut for me https://i.imgur.com/1GC2sEU.png

I fixed this behavior of the TextField and also added a maximumLength attribute
so that the text can't be more than 7 characters long.

I also now make sure that the text color of the field is always visible.

https://invent.kde.org/accessibility/kontrast/-/commit/2d235189603c87ead8d03cc0bef9d1933716552d

> > > 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

I now also added the KAboutData::setupCommandLine call and --help and --author work.

> > > 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

There is now a basic help page adding information about the contrast range:
https://invent.kde.org/accessibility/kontrast/-/merge_requests/4

> >
> > > 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.
>
> No you have not?
>
> ./src/contents/ui/FavoritePage.qml:8:import org.kde.kirigami 2.14 as Kirigami

Sorry it looks like I forgot to commit the change :(
https://invent.kde.org/accessibility/kontrast/-/commit/d73003f36d71ec036a7d612eb3487ea3801bd6c1
>
> > 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.
>
> No need, i was just curious about the waste of horizontal space :)
>
> Cheers,
> Albert
>
> > > Cheers,
> > > Albert
> > >
> > > > Thanks
> > > > Carl