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