Skanpage moved to kdereview

Harald Sitter sitter at kde.org
Mon Aug 9 16:34:52 BST 2021


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

HS


More information about the kde-core-devel mailing list