Skanpage moved to kdereview

[Alexander Stippich]

On Montag, 9. August 2021 17:34:52 CEST Harald Sitter wrote:
> it works amazingly well. good job! I mostly only have some nitpicky stuff
>

Thanks!

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

Done.

> - your cmake code is GPL but policy-wise it should be BSD-ish. any
> chance this can be relicensed?

I actually missed that there is a separate policy for cmake. The files have
been relicensed.

> - for stylistic consistency with other repos you should probably use
> https://cmake.org/cmake/help/v3.16/module/FeatureSummary.

Done.

> - testconfig.h.in -> I think you want
> https://doc.qt.io/qt-5/qtest.html#QFINDTESTDATA ;)

Done.

> - getUnitString -> can that maybe use
> https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html

What do you have in mind here? getUnitString just matches the enum of libksane
to a unit suffix string, it does not format the values in any way.

> - the scanner options window is not high enough by default on my
> system (with Noto Sans 10pt as font)

This works if there are only a few options available, imho not so much when
there are quite a few options. But I guess one could implement a maximum
height up to which the window is scaled. For now, I implemented that the size
of the options window at least gets remembered. But I will look at it again.

> - the options really ought to use an apply/cancel pattern. applying
> changes on the fly feels very strange for a KDE app

This has been implemented just now.

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

Hmm, how does this look? I don't see a difference.
There actually is a subtle difference between how this string and the others
are obtained: Scan area size is a custom option of libksane and is translated
there, whereas the others are coming from SANE directly. However, they are all
appended the same way with "%1:" in Skanpage.

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

There has been a discussion already to move the scanner name to the window
title like it is done in Skanlite. This would save some space. Also, the
comboboxes are currently not scaled to their content width. Then there would
also be some possible savings.
Personally I find the quick access that the comboboxes provide for the most
important options very useful.

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

Fixed.

>
> HS

Thanks for the feedback!
Alex