Differential D15316

Port KolourPaint away from deprecated KDoubleNumInput
AbandonedPublic
Actions

Authored by yurchor on Sep 6 2018, 3:03 PM.

Details

Reviewers
aacid
mkoller
Group Reviewers
KDE Applications
Summary

KDoubleNumInput is deprecated in KF5, so it should be ported to get rid of KDELIBS4SUPPORT.

Test Plan

Press Ctrl+M and choose "Histogram Equalizer" or "Hue, Saturation, Value" option from the top-level drop-down list. The behavior should be similar to the unpatched case.

Diff Detail

Repository
R374 KolourPaint
Lint
Lint Skipped
Unit
Unit Tests Skipped
yurchor requested review of this revision.Sep 6 2018, 3:03 PM
yurchor created this revision.
aacid added inline comments.Sep 23 2018, 8:45 PM
widgets/imagelib/effects/kpEffectWidgetBase.h
44

Do you need these two classes to do double<->int conversion in the signals, right?

A bit sad :/

Anyway, don't call them QSomething, that's reserved for Qt, call them KpSomething

yurchor marked an inline comment as done.Sep 24 2018, 5:07 AM

Renamed according to KpXXXX scheme.

yurchor updated this revision to Diff 42214.Sep 24 2018, 5:12 AM

Get rid of "Q" at the beginning of the class names.

aacid added inline comments.Sep 24 2018, 9:59 AM
widgets/imagelib/effects/kpEffectHSVWidget.cpp
60

Something is "not working the same" in the Hue slider, with the old code, when i move the mouse wheel up/down it changes in 1.08 increments, with the new code it changes in 0,03 increments, which make it kind of hard to use since it barely moves.

Can you have a look at that?

yurchor marked an inline comment as done.Sep 24 2018, 1:54 PM
yurchor added inline comments.
widgets/imagelib/effects/kpEffectHSVWidget.cpp
60

Actually, I was thinking this will be a funny feature (rough tuning with dragging the slider and finetuning with the wheel-scrolling) but it can be done to mimic the good old behavior.

yurchor updated this revision to Diff 42251.Sep 24 2018, 2:08 PM
yurchor marked an inline comment as done.

Mimic the old single step value for the the hue slider.

aacid added a comment.Oct 12 2018, 4:17 PM

With the new code i get 0,10 increments in Hue instead 0,03 but still far away from the 1,08 i get with the old version

Can you reproduce what the same i get?

yurchor added a comment.Oct 12 2018, 4:49 PM
In D15316#341983, @aacid wrote:

With the new code i get 0,10 increments in Hue instead 0,03 but still far away from the 1,08 i get with the old version

Can you reproduce what the same i get?

Sure. Sorry, I did not notice somehow. It is hard to make this magic 1.08 step, but 1.00 step can be easily implemented (see the next iteration of the patch)

yurchor updated this revision to Diff 43482.Oct 12 2018, 4:51 PM

Make the scroll step equal to 1.00 (close to the old good 1.08).

aacid added a comment.Oct 12 2018, 5:03 PM

You fixed one and broke the other :D

Now Saturation is on 0.10 instead of 0.03 it was before.

Can they not have independent stepping?

yurchor added a comment.Oct 12 2018, 5:07 PM
In D15316#342016, @aacid wrote:

You fixed one and broke the other :D

Now Saturation is on 0.10 instead of 0.03 it was before.

Can they not have independent stepping?

Yes. Sorry again.

yurchor updated this revision to Diff 43488.Oct 12 2018, 5:49 PM

Different scales for (-180, 180) and (0, 1) ranges.

aacid added a comment.Oct 15 2018, 9:02 PM

I feel like having two classes of each is not the best of the ideas, since it seems the only difference in one you use 10 and in the other 100 in notifyValueChanged.

I'm going to propose two solutions:

  • Ideally try to get that 10/100 value calculated from the range, step values, something like (making it totally up range/0.1) or similar
  • If that doesn't seem to work out, make that 10, 100 a parameter you pass either in the constructor or as a setter, it'll be much simpler than having two classes that share 99% of the code.

Does it make sense to you?

yurchor added a comment.Oct 17 2018, 11:58 AM
In D15316#343728, @aacid wrote:

I feel like having two classes of each is not the best of the ideas, since it seems the only difference in one you use 10 and in the other 100 in notifyValueChanged.

I'm going to propose two solutions:

  • Ideally try to get that 10/100 value calculated from the range, step values, something like (making it totally up range/0.1) or similar
  • If that doesn't seem to work out, make that 10, 100 a parameter you pass either in the constructor or as a setter, it'll be much simpler than having two classes that share 99% of the code.

    Does it make sense to you?

Sure. The first way seems to be more reasonable.

yurchor updated this revision to Diff 43785.Oct 17 2018, 12:00 PM

Calculate slider parameters from the range of input values.

aacid accepted this revision.Oct 17 2018, 9:23 PM
This revision is now accepted and ready to land.Oct 17 2018, 9:23 PM
cfeck added a reviewer: mkoller.Oct 17 2018, 10:32 PM
cfeck added a subscriber: cfeck.

Adding maintainer, because I have seen him reverting previous (unreviewed) attempts.

mkoller requested changes to this revision.Oct 20 2018, 2:59 PM

other than ma comments, I'm ok with it.

widgets/imagelib/effects/kpEffectHSVWidget.cpp
57

passing a range of -180..180 in ctor and setting a range of -100..100 in the next line seems odd.

64

same comment as above. Why passing 2 different ranges ?

widgets/imagelib/effects/kpEffectWidgetBase.h
73

There is no advantage in using const & for a double: const double &minimumValue
simply use double minimumValue

This revision now requires changes to proceed.Oct 20 2018, 2:59 PM
yurchor marked an inline comment as done.Oct 20 2018, 3:26 PM
yurchor added inline comments.
widgets/imagelib/effects/kpEffectHSVWidget.cpp
57

It appears that creating a connection between two different widgets one of which understands integers and both of them do not know about deficiencies of each other only is a tricky thing. To make the appearance for the different ranges similar I decided to make the ranges equal to [-100, 100].

Should I put it into the header? May it be that some inline lambdas can be useful? May it be that some comments on this will be enough?

I just do not know. Now it's a bit cryptic and ugly indeed but kind of working.

yurchor updated this revision to Diff 43974.Oct 20 2018, 3:27 PM

Avoid using const pointers.

yurchor marked 2 inline comments as done.Oct 20 2018, 3:47 PM

Sorry, I have misread the comment. :)

yurchor updated this revision to Diff 43979.Oct 20 2018, 3:48 PM

Remove ranges from the constructors.

mkoller added a comment.Oct 20 2018, 4:09 PM

I suggest you create ONE new widget which contains a slider and a spinbox, and you pass the range it shall use.
Now it's very confusing.

widgets/imagelib/effects/kpEffectHSVWidget.cpp
21

HUE has -180 .. +180.
Why is this here -100..+100 ?

yurchor abandoned this revision.Oct 20 2018, 4:13 PM
In D15316#346363, @mkoller wrote:

I suggest you create ONE new widget which contains a slider and a spinbox, and you pass the range it shall use.
Now it's very confusing.

Ok. I will try, but not now.

Revision Contents
Changeset List

Diff 43979

View Options

dialogs/imagelib/transforms/kpTransformResizeScaleDialog.cpp

Loading...
View Options

widgets/imagelib/effects/kpEffectHSVWidget.h

Loading...
View Options

widgets/imagelib/effects/kpEffectHSVWidget.cpp

Loading...
View Options

widgets/imagelib/effects/kpEffectToneEnhanceWidget.h

Loading...
View Options

widgets/imagelib/effects/kpEffectToneEnhanceWidget.cpp

Loading...
View Options

widgets/imagelib/effects/kpEffectWidgetBase.h

Loading...