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.
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.
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
Can you re-add the BUG: keywords to the description for the bugs that this patch fixes? Thanks!
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) |
reverted whitespace changes, fixed headers, filed and linked bug, moving seperate bugs to own diff
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 |
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 |
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 :) |
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. |
@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?