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