Skanpage moved to kdereview
Albert Astals Cid
aacid at kde.org
Mon Aug 9 20:21:19 BST 2021
El dilluns, 9 d’agost de 2021, a les 17:34:52 (CEST), Harald Sitter va escriure:
> it works amazingly well. good job! I mostly only have some nitpicky stuff
>
> - since the code base is really close to
> complete reuse coverage, it might be nice to push it over the finishing
> line and then `reuse lint` it to not have it fall behind again
> - your cmake code is GPL but policy-wise it should be BSD-ish. any
> chance this can be relicensed?
> - for stylistic consistency with other repos you should probably use
> https://cmake.org/cmake/help/v3.16/module/FeatureSummary.html
> - testconfig.h.in -> I think you want
> https://doc.qt.io/qt-5/qtest.html#QFINDTESTDATA ;)
> - getUnitString -> can that maybe use
> https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html
> - the scanner options window is not high enough by default on my
> system (with Noto Sans 10pt as font)
> - the options really ought to use an apply/cancel pattern. applying
> changes on the fly feels very strange for a KDE app
> - something is wonky with the "Scan area size" label in the options
> window. it appears to be constructed from the string "Scan area size"
> and the string ":". they should be one string though. indeed the other
> options are one string :shrug:
> - not really a fan of the comboboxes and long options label in the
> toolbar. it feels very crowded and with long scanner names the window
> easily takes up more than half my screen width at minimum size
> - I'm not sure if that is the code's fault (it certainly looks
> correct) but in french the documentlist.qml bottom label reads `1
> page` when there are no pages
The bug is in the code, you can not assume that the "singular form" is for n == 1.
In french the singular form is used for 1 and 0, so it needs to be %1 page and not "1 page".
It's tricky :/
Cheers,
Albert
>
> HS
>
More information about the kde-core-devel
mailing list