Add scan area / paper size configuration to the libksane ksanewidget and thereby to skanlite
Needs ReviewPublic

Authored by antonarnold on Oct 23 2019, 9:54 PM.

Details

Reviewers
sars
Group Reviewers
VDG
KDE Applications
Summary

BUG: 377599
BUG: 295395
BUG: 329149
BUG: 406911
BUG: 357759

The patch applies to libksane v19.04.3
Skanlite reference version is 2.1.0.1
Kubuntu 19.10
HP Officejet Pro 7740

UI and code style are adapted to the existing source code, do not expect perfection :-)

Diff Detail

Repository
R382 KSane Library
Lint
Lint Skipped
Unit
Unit Tests Skipped
antonarnold requested review of this revision.Oct 23 2019, 9:54 PM
antonarnold created this revision.
antonarnold edited the summary of this revision. (Show Details)Oct 23 2019, 9:59 PM
ndavis added a subscriber: ndavis.EditedOct 23 2019, 10:00 PM

If I understand this correctly, this doesn't add any new features, but it does add an additional method for configuring the scan area. Is that correct?

If I understand this correctly, this doesn't add any new features, but it does add an additional method for configuring the scan area. Is that correct?

The base version allows only manual scan area selection in the preview panel. That's useful for selecting just a cash register receipt or a polaroid image but rather inconvenient for standard paper sizes which needs to be approximated by the user or cropped manually afterwards.
The patch allows using pre defined paper size formats as A3, A4, Letter, Legal, etc. By gut feeling I'd call this an additional feature.
Also several bugs are addressed that annoyed me with my personal setup which has different scan input sources with different sizes and resolutions in one device.

sars added a comment.Oct 25 2019, 8:00 PM

Hi,

Thanks for your review request! :)

Can you split this review request into two separate requests. One for the scan area and one for the reordering of the options list (I'm not yet clear with what the reordering of the options list brings ;)

I have tried to keep the size options out of the UI because I was aiming at the photo scanning not the document scanning, but I guess a separate tab for scan area stuff does not hurt as long as it is possible to disable/hide.

src/ksanewidget.cpp
440

Please use KDE Frameworks coding style:

camelCase variables (except the m_ member prefix)
always use { } in if and for statements.
place the { on the same line ass the if/for statement except for multi-line if/for statements
You can read more at https://techbase.kde.org/Policies/Frameworks_Coding_Style

src/ksanewidget_p.cpp
534

Did you notice the code after "// calculate label widths"?

There I calculate the with of the longest label and use that for all the options so that we get aligned labels and controls.

src/options/ksaneoptcombo.cpp
334 ↗(On Diff #68642)

Before the final update we need to remove commented out debug code

In D24904#554068, @sars wrote:

Hi,

Thanks for your review request! :)

Can you split this review request into two separate requests. One for the scan area and one for the reordering of the options list (I'm not yet clear with what the reordering of the options list brings ;)

I have tried to keep the size options out of the UI because I was aiming at the photo scanning not the document scanning, but I guess a separate tab for scan area stuff does not hurt as long as it is possible to disable/hide.

Thanks for the review. I'll split and refactor and fine-tune the patch in the next weeks.
I'll also dive again in the reordering functions. I tried to address erratic behavior in my hardware setup:
My scanner has 2(/3) scan sources: flatbed and an automatic document feeder (which is actually represented as 2 sources: ADF + duplex to distinguish single side/both side scan).
The flatbed source allows high resolutions up to 1200DPI and color. The ADF only 300DPI and grayscale.
This is internally represented as SANE options with constraints. These are changing if I change the scan source.
I had several problems with inconsistent options/constraints. One was clearly caused by the order of the options, e.g. when skanlite sets all options and the source is not set to flatbed before resolution to 1200DPI, the option was discarded.
But also the internal SANE messages that report the need of a option/value reload does not work 100%.
I just dived into the scanning frameworks and libraries it's quite complex and there's also an additional HP specific library for my particular network scanner.
I'm not 100% certain if my initial problems are caused by the option order, bugs that I found later, or are caused by another abstraction layer.
I have a vague feeling it works better now, but I'll check against baseline.

sars added a comment.Oct 27 2019, 5:08 PM

Hmm yes, that order in which we restore the saved options could have a priority order. We could just check if the map contains scan source and execute that first. Then check for mode and resolution and then after that the rest.

So we could take a copy of the map (in setOptVals) and check if it contains a key, then set that value, remove the key/value from the map and then continue with the next in priority order. Then when the special handling is done execute the rest in the order they come in the map.

antonarnold edited the summary of this revision. (Show Details)

Cropped scan-area patch (removed option ordering from this diff)
Applied KDE Coding Style (manually and via astyle)
Updated UI element alignment

sars added a comment.Oct 29 2019, 6:28 PM

Generally good changes :)

Usually it is better to minimize the white-space changes so that reviewing the actual changes is easier. Then as a separate review do the white-space changes.

src/ksanewidget_p.cpp
369

Connections should not have spaces in the signal/slot strings

https://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures/

And even better convert to the new connect style which is much faster and has compile time checks.

519

is this variable rename intentional?

src/ksanewidget_p.h
52

There are some unneeded includes here... better to move as much header includes as possible to the cpp file

src/widgets/labeledcombo.h
91

one camelCase forgotten ;)

Thanks for the review, I'll revert the whitespace changes in the next revision.
I have some questions embedded in the inline code comments.
It would be great if you can provide some answers/information.

src/ksanewidget_p.cpp
369

Good to know, I just applied the astyle command in the coding style wiki article. But I see the linked shell script is more complex and seems to address this issue.
I'll revert all the astyle changes that do not apply to my own code changes

519

Yes that's actually another bug I fixed on the fly while applying the label width calculation code to the scan area tab.
The original code does the iteration over basic_layout twice.
How do you handle such minor bugfixes here? Put everything in a seperate feature branch with its own bug report, diff, review process?
It's clearly the most transparent and clean workflow, but also quite time consuming. Is there a guideline somewhere in which cases to be more/less strict?

src/ksanewidget_p.h
52

I'll take a look

src/widgets/labeledcombo.h
91

oops :-) but I use this opportunity to ask a question I already had in mind.
Is this a feasible way to pass the two overloaded signals?
More equivalent to the QComboBox signals would be to use overloaded signals too.
But in my opinion this may potentially cause much more follow up problems than introducing a distinct signal name.

sars added inline comments.Oct 30 2019, 9:24 PM
src/ksanewidget_p.cpp
369

Thanks :)

At work we have git clang-format in use. That is also an alternative :) it can work on only the changed code., but it does need some setting up. There was actually just added a clang-format file added to ECM (Ectra Cmake Modules in KF5 for a more official KDE Frameworks style.

519

I don't know of a guideline, it is just up to each project. If the patches are small it is easier to review and generally quicker to merge.

Personally, if the changes are small I work directly in master and just use the web-interface to post the "git diff" and then push when done.

In this particular, case we could have the "small fixes" in one review, the "size options" in one and the re-ordering of options in a third one. Then later if we need to check why something was done, we have clear separation in the commits.

src/widgets/labeledcombo.h
91

It is a mater of taste, but in deed it is simpler to have distinct signal names in stead of overloads. Overloads need special handling to connect with the new connect style.

sars added a comment.Sun, Nov 17, 9:25 AM

Ping

I hope I did not scare you off with my comments. These fixes are really welcome!

I had some really bad weeks, I guess these fixes will have to wait 1 or 2 more weeks.

sars added a comment.Sun, Nov 24, 4:56 PM

That is absolutely fine!

I'm just making sure that we get them in at some point. The bugs have been there a long time so a couple of more weeks is no problem. It will mean they will not make it for the 19.12 release tho.

If you want, I can work on the bug-fix patches, but it is up to you :)

sars added a comment.Sun, Nov 24, 4:59 PM

Scratch that 19.12 comment, the 19.12 feature freeze was already the 14th... The bug fixes can go in as they are bug fixes ;) so no hurry.

reverted whitespace changes, fixed headers, moving seperate bugs to own diff

camel case fix

sars added a comment.Thu, Dec 5, 8:09 PM

I have been a bit reluctant to add the predefined page sizes, but maybe it is time to rethink it...

I have a couple of requests tho.

I want the "manual selection" to be in sync with this new selection and the "enable manual selection" should not be needed.

When the size is changed manually, the combo-box and the sliders should be updated. If the new selection does not match a predefined page size, there should be a "custom" entry in the combo-box.

Since these are options that all printers will have, we could put the tab as second in stead of third.

Tell me if you want me to do some of the coding. Otherwise I'll wait for you to do it :)

src/ksanewidget_p.cpp
432

One could investigate QPrinter::pageLayout() to get the default page size for the system :)

434

Would it be an idea to use QPageSize for filling this map?

467

How do we handle portrait mode if the page size goes outside of the scanable area?

Would it be better to just add portrait and landscape variants of the page sizes and drop those that do not fit in the scan area?

I have been a bit reluctant to add the predefined page sizes, but maybe it is time to rethink it...

I can only talk for myself but I need this feature really often, both private and professional.

I have a couple of requests tho.
I want the "manual selection" to be in sync with this new selection and the "enable manual selection" should not be needed.
When the size is changed manually, the combo-box and the sliders should be updated. If the new selection does not match a predefined page size, there should be a "custom" entry in the combo-box.
Since these are options that all printers will have, we could put the tab as second in stead of third.

Yes I agree, this would provide a much more responsive and modern UI. It could even be reduced to a single combo box without the sliders and placed into the first tab.

Tell me if you want me to do some of the coding. Otherwise I'll wait for you to do it :)

I think you have some really good ideas and have a broader knowledge over the Qt framework, feel free to realize your own visions.
I'll take a deeper look into the option reordering issue since this is hardware depending.

src/ksanewidget_p.cpp
432

Mhh that might work though asking some (possibly non existent) printer isn't a clean solution.
But maybe an answer lies within the code.

There is a linux locale extension that provides this information but I didn't find its equivalent in QLocale.
$locale -k LC_PAPER
height=297
width=210
paper-codeset="UTF-8"

434

Of course good idea. I didn't realize that this class exsists.

467

That sounds reasonable. The variants have to be updated though if the scanner options change.
If only valid page sizes are offered there should be also a plausiblity check for the offset/width silders (and derived maximum values).