| sars |
| VDG | |
| KDE Applications |
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 :-)
| Lint Skipped |
| Unit Tests Skipped |
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.
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) | |
| 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 |
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.
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.
Cropped scan-area patch (removed option ordering from this diff)
Applied KDE Coding Style (manually and via astyle)
Updated UI element alignment
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. | |
| 519 | Yes that's actually another bug I fixed on the fly while applying the label width calculation code to the scan area tab. | |
| 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. | |
| 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. | |
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.
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 :)
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.
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. There is a linux locale extension that provides this information but I didn't find its equivalent in QLocale. | |
| 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. | |