KDEReview for Kontrast

[Albert Astals Cid]

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")));

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

No you have not?

./src/contents/ui/FavoritePage.qml:8:import org.kde.kirigami 2.14 as Kirigami


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