Review Request: Allow slider to be exponential to the spin box on KDoubleNumInput

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/582/#review914
-----------------------------------------------------------


coool! i've long wanted this for various places in the code. nice to see it actually coming! some comments follow, though nothing fatal imho.


trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment604>

    singleStep so it's more like QSpinBox, QAbastractSlider, QSeekSlider, etc.?



trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment605>

    setSingleStep so it's more like QSpinBox, QAbastractSlider, QSeekSlider, etc.?



trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment606>

    default arg is odd here since it requires one to look at the apidox when its used in code to see the defaults:
    
    setSliderEnabled();
    
    vs
    
    setSliderEnabled(true);
    setSliderEanbled(false);
    
    it's not worth the 4-5 extra characters and also in line with the rest of our api :)
    
    also, a getter: bool isSliderEnbled() const?



trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment607>

    -> singleStep as noted above?



trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment608>

    -> setSingleStep as noted above?



trunk/KDE/kdelibs/kdeui/widgets/knuminput.h
<http://reviewboard.kde.org/r/582/#comment610>

    should this be using qreal instead of double?
    
    as for naming, perhaps including the word "Ratio" would make it a bit clearer? setExponentialRatio(qreal)?
    
    the apidox should probably also note what the default is and some examples of what provided values mean (e.g. 1.0 == the same)
    
    yes, mildly obvious kind of stuff but the kind of stuff people ask ;)



trunk/KDE/kdelibs/kdeui/widgets/knuminput.cpp
<http://reviewboard.kde.org/r/582/#comment609>

    should be in the KDoubleNumInputPrivate ctor init list with the other dptr members



trunk/KDE/kdelibs/kdeui/widgets/knuminput.cpp
<http://reviewboard.kde.org/r/582/#comment611>

    should check to make sure d->exponent is > 0 here to avoid possible div by zero? either that or else check for zero passed into the setter for d->exponent


- Aaron


On 2009-04-13 12:26:29, Cyrille Berger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/582/
> -----------------------------------------------------------
> 
> (Updated 2009-04-13 12:26:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Can't seem to be able to update patches on reviewboard :( so creating a new request.
> 
> The main point if this patch is to allow to have an "exponential" slider for KDoubleNumInput. It's usefull when you have an input where the main interesting range is small values, while supporting a big range. For instance, in Krita, when we set the radius of the brush we have a range of 0.0 to 9999.0, but most of the time the user work in the range of 0.0-100.0, but since the slider is linear to 0.0 to 9999.0, the most usefull range is only 10% of the slider which makes the slider very unusuable, hence the idea of an exponential slider, where we could make sure the range 0.0-100.0 use half the slider.
> 
> 
> Extra goodies, add a few Q_PROPERTY for enabling slider in designer. And it also bring KDoubleNumInput API closer to KIntegerNumInput (unfortunately I can't go further to have the same setRange function as in KIntegerNumInput since it would break ABI)
> 
> In this new patch, I have also added step/setStep (and Q_PROPERTY) to change the steps without calling setRange. In the Qt API, it's called setSingleStep, but in knuminput it was refered as "step", not sure if I shouldn't call the new function (and its property) singleStep (and possibly adjust the naming in the other function parameters, when possible) to be closer to Qt's API.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/widgets/knuminput.h 952354 
>   trunk/KDE/kdelibs/kdeui/widgets/knuminput.cpp 952354 
> 
> Diff: http://reviewboard.kde.org/r/582/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cyrille
> 
>