This revision became obsolete with D26351

There are a lot of unit conversions in the diff so I tried to run 'LC_ALL=en_US.en_US ./skanlite'
That doesn't look valid

@sars ah you are right I thought it is supposed to be a fixed size layout but it is not as it can be seen in the before picture already.

In D26351#590487, @sars wrote:

@antonarnold Good bug catch! All the scan sources I have here, have the same size so I did never reach that code path...

As I always reset the size to full scan area the default size selection should be "Custom"

In a later PR, we could leave the size as it is, if it fits.

For another PR it could also be possible to go through the sizes in the combobox when the scan size is manually changed and set the size that matches, but I'm not sure it is worth the hassle ;)

@sars That's a really elegant solution. For me the gui is fine. Simple enough and functional. If hiding is necessary something like a collapsable accordeon view might be modern, but I'm no ui design expert.
I found one bug when I switch from a scan source with a big page size to a smaller scan source the combo box is updated and a valid item is selected but the width/height is not set correctly.
Actually I just found out that changing the scan source always resets x,y,w,h to 0,0,max,max

Now uses correct localized number conversion

I'll change it and check directly if everything behaves correctly. Shouldn't take long.

In D25052#575136, @aacid wrote:

@antonarnold We will need your email address for landing with proper attribution, should we use the one you used to register your identity.kde.org account?

I don't have write access to the git repostiory if that's what you're referring to. Nor is some commit action in phabricator available to me.

In D25588#569248, @sars wrote:

BTW should this also be done to to other options setting numbers?

I stripped down the diff to the minimum necessary code changes.

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.

camel case fix

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

removed unneeded header include that I forget

reverted whitespace changes, fixed headers, filed and linked bug, moving seperate bugs to own diff

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

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.

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

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.

In D24904#553042, @ndavis wrote:

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?