Le lundi, août 3, 2020 11:12 PM, Albert Astals Cid <aa...@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 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"))); 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