Optimize support of scanners with different image sources or duplex unit
ClosedPublic

Authored by antonarnold on Oct 29 2019, 6:10 PM.

Details

Summary

BUG: 414600

The initial changeset of D24904 has been split into two seperate revisions.
This one addresses various bugs which may result in inconsistent scanner options.

Diff Detail

Repository
R382 KSane Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
antonarnold requested review of this revision.Oct 29 2019, 6:10 PM
antonarnold created this revision.

Can you re-add the BUG: keywords to the description for the bugs that this patch fixes? Thanks!

sars added a comment.Oct 29 2019, 7:14 PM

Isn't the change in setOptVals() the only change needed for fixing the scan option order problem?

Looking forward to the next iteration :)

src/ksanewidget.cpp
106

no spaces in the signal/slot signatures

471

Is this a bug-fix or a mistake?

747

This could be simplified:

if (map.contains(foo)) {

if (setValue(foo, map[foo])) {
    ret++;
}
map.remove(foo);

}

936

looks like these are the same white-space changes as in the other review. This will lead to merge conflicts.

src/ksanewidget_p.cpp
1167

signal/slot signature without spaces...

src/options/ksaneoptcombo.cpp
334

maybe just remove the whole commented code and the comment.

352

This fix could have been a separate commit and review. (it has nothing to do with the reorganization of the restoration of the options)

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

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

antonarnold added inline comments.Thu, Nov 28, 12:22 PM
src/ksanewidget.cpp
471

this is part of the bugfix that options become inconsistent

747

I assume you mean setOptValue? I'd rather avoid calling it since it does some additional stuff besides setting the option.

removed unneeded header include that I forget

sars added inline comments.Tue, Dec 3, 6:35 PM
src/ksanewidget.cpp
432

Why do we need to rearrange the m_optList? Isn't it enough that we restore the options in the right order?

748

I try to avoid assignments in the if() statement as it is harder to read and the risk of mistakes is higher. But this is a bit of taste issue ;)

src/ksanewidget_p.cpp
501

The other "Try to ..." statements try to change the default "Color mode", 8bits/color and 600DPI.

This reads the source and writes the same value back.... Why? Do you have a buggy back-end that somehow requires this? In that case I think it should be fixed in the back-end.

985

Good addition :)

src/options/ksaneoptcombo.cpp
352

Still holds (could be in the bug-fix commit), but we can let it in here anyways :D

antonarnold updated this revision to Diff 71143.Mon, Dec 9, 9:36 PM

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

antonarnold added inline comments.Mon, Dec 9, 9:41 PM
src/ksanewidget.cpp
748

I'm not a fan of this, too. But it's the same style as in setOptValue so I wouldn't start to change it here.

src/options/ksaneoptcombo.cpp
352

The initial code that your comment referred to has already been moved to https://phabricator.kde.org/D25588

sars accepted this revision.Tue, Dec 10, 10:04 AM

Thanks for these fixes :)

Please tell me if you do not have commit access, so that I can commit them for you.

src/ksanewidget.cpp
761

I was about to click "Accept Revision" when this hit my eye... The logic is reversed...

But then again this seems to be a fix to a bug :)

This revision is now accepted and ready to land.Tue, Dec 10, 10:04 AM

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.

src/ksanewidget.cpp
761

Yes this fixes the return value to actually represent the number of written options.

aacid added a subscriber: aacid.Tue, Dec 10, 9:47 PM

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

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

@aacid that's fine

This revision was automatically updated to reflect the committed changes.