Improved slider code review

UnitScan

By default, QSlider move his thumbtrack by a value belonging to the pageStep() prop on mouse click. To make thumbtrack jump directly at the mouse click point, we need to create a new class inherited by QSlider.

Header file (.h): http://www.unitscan.org/qimprovedslider-h
Source file (.cpp): http://www.unitscan.org/qimprovedslider-cpp

When thumbtrack jump directly at mouse click, onClick(int value) signal are fired: you can bind function through connect(widget, signal, class, slot) method.

This code works well, so is not a broken code. There is a way to improve this? Maybe trying to differentiate the direct jump from the slider handle dragging

Chris Kawa

Haven't actually run the code, but here's my review just by the looks of it:
The header:

It would be nice to provide the same set of constructors the base class has, i.e. add one with orientation parameter.
The only include you need in the header is for QSlider. Move all the others to the .cpp. For QMouseEvent it's enough to just put forward declaration in the header.
The whole point of include guards (the QIMPROVEDSLIDER_H macro) is that stuff doesn't get included twice. Move that to the top of your header and put includes under it.
onWHATEVER is a very unQtish naming convention. Signals mean something happened with the object, so instead of onClick() a better name would be clicked(). Make the signal const while you're at it, so you can emit it from const member functions if you need to.
Use override keyword when overriding (e.g. mousePressEvent). Saves you from typo errors misery.
If you don't need to do anything in the destructor don't implement it. There's no point to it.

The implementation:

Stealing focus in an enter event is a very unpolite thing to do. Imagine someone types something in a text input widget and just moves the mouse out of the way over your widget. Now he can't type. Or someone just passes over your widget without any intention to use it and you steal the focus anyway. Very user unfriendly.
Similarly setSliderDown in an enter event is a very weird thing to do. Imagine a button that pressed itself when you hover over it. that's just totally unexpected.
A "click" is a mouse press and release. You should not emit clicked signal from a press event. Only from the release event on the condition that the press has also taken place over your widget. A user can press a button over your widget and move away while holding it. That also should not emit a clicked signal.

UnitScan

@Chris-Kawa said in Improved slider code review:

A "click" is a mouse press and release. You should not emit clicked signal from a press event. Only from the release event on the condition that the press has also taken place over your widget. A user can press a button over your widget and move away while holding it. That also should not emit a clicked signal.

So I should take a look to http://doc.qt.io/qt-5/qslider.html#mouseReleaseEvent?

Chris Kawa

Yup, make the press and release work together.

UnitScan

.h

#include <QSlider>

#ifndef QIMPROVEDSLIDER_H
#define QIMPROVEDSLIDER_H

class QImprovedSlider : public QSlider
{
    Q_OBJECT
protected:
    void mousePressEvent(QMouseEvent *event) override;
    void mouseReleaseEvent(QMouseEvent *event) override;
public:
    explicit QImprovedSlider(QWidget *parent = 0);

public slots:

private:
private slots:

signals:
    void clicked(int value);

};

#endif // QIMPROVEDSLIDER_H

.cpp

void QImprovedSlider::mousePressEvent(QMouseEvent *event) {
    QStyleOptionSlider opt;
    initStyleOption(&opt);
    QRect sr = style()->subControlRect(QStyle::CC_Slider,
                                       &opt,
                                       QStyle::SC_SliderHandle,
                                       this);
    if (event->button() == Qt::LeftButton &&
            !sr.contains(event->pos())) {
        int newVal;
        if (orientation() == Qt::Vertical) {
            double halfHandleHeight = (0.5 * sr.height()) + 0.5;
            int adaptedPosY = height() - event->y();
            if (adaptedPosY < halfHandleHeight) {
                adaptedPosY = halfHandleHeight;
            }
            if (adaptedPosY > height() - halfHandleHeight) {
                adaptedPosY = height() - halfHandleHeight;
            }
            double newHeight = (height() - halfHandleHeight) - halfHandleHeight;
            double normalizedPosition = (adaptedPosY - halfHandleHeight)  / newHeight ;
            newVal = minimum() + (maximum()-minimum()) * normalizedPosition;
        }
        else {
            double halfHandleWidth = (0.5 * sr.width());
            int adaptedPosX = event->x();
            if (adaptedPosX < halfHandleWidth) {
                adaptedPosX = halfHandleWidth;
            }
            if (adaptedPosX > width() - halfHandleWidth) {
                adaptedPosX = width() - halfHandleWidth;
            }
            double newWidth = (width() - halfHandleWidth) - halfHandleWidth;
            double normalizedPosition = (adaptedPosX - halfHandleWidth)  / newWidth ;
            newVal = minimum() + ((maximum()-minimum()) * normalizedPosition);
        }
        if (invertedAppearance() == true) {
            this->setValue( maximum() - newVal );
            }
        else {
            this->setValue(newVal);
            }
        event->accept();
        //QSlider::mouseReleaseEvent(QMouseEvent::MouseButtonRelease);
    }
    else {
        QSlider::mousePressEvent(event);
    }
}

void QImprovedSlider::mouseReleaseEvent(QMouseEvent *event) {
    //press and release work together?
    setSliderDown(true);
    emit clicked(value());
    setSliderDown(false);
}

Chris Kawa

setSliderDown(true) means the handle is drawn as pressed. It should go in the press event. Calling it with true and false in the same function is pointless, as there is no event loop processing between them so no drawing.
You should also call the base implementation (QSlider::mouseReleaseEvent(event)) in your override, because you might have just removed some built-in functionality.

Other nitpicks:

as I mentioned earlier: includes go under the include guards:

#ifndef QIMPROVEDSLIDER_H
#define QIMPROVEDSLIDER_H
#include <QSlider>

These do nothing if you don't put methods under them so just remove them:

public slots:
private:
private slots:

As mentioned before, make the signal const. It's not mandatory but a good practice:

signals:
    void clicked(int value) const;

UnitScan

void QImprovedSlider::mousePressEvent(QMouseEvent *event) {
    QStyleOptionSlider opt;
    initStyleOption(&opt);
    QRect sr = style()->subControlRect(QStyle::CC_Slider,
                                       &opt,
                                       QStyle::SC_SliderHandle,
                                       this);
    if (event->button() == Qt::LeftButton &&
            !sr.contains(event->pos())) {
        int newVal;
        if (orientation() == Qt::Vertical) {
            double halfHandleHeight = (0.5 * sr.height()) + 0.5;
            int adaptedPosY = height() - event->y();
            if (adaptedPosY < halfHandleHeight) {
                adaptedPosY = halfHandleHeight;
            }
            if (adaptedPosY > height() - halfHandleHeight) {
                adaptedPosY = height() - halfHandleHeight;
            }
            double newHeight = (height() - halfHandleHeight) - halfHandleHeight;
            double normalizedPosition = (adaptedPosY - halfHandleHeight)  / newHeight ;
            newVal = minimum() + (maximum()-minimum()) * normalizedPosition;
        }
        else {
            double halfHandleWidth = (0.5 * sr.width());
            int adaptedPosX = event->x();
            if (adaptedPosX < halfHandleWidth) {
                adaptedPosX = halfHandleWidth;
            }
            if (adaptedPosX > width() - halfHandleWidth) {
                adaptedPosX = width() - halfHandleWidth;
            }
            double newWidth = (width() - halfHandleWidth) - halfHandleWidth;
            double normalizedPosition = (adaptedPosX - halfHandleWidth)  / newWidth ;
            newVal = minimum() + ((maximum()-minimum()) * normalizedPosition);
        }
        if (invertedAppearance() == true) {
            this->setValue( maximum() - newVal );
        }
        else {
            this->setValue(newVal);
        }
        event->accept();
        setSliderDown(true);
    }
    else {
        QSlider::mousePressEvent(event);
    }
}

void QImprovedSlider::mouseReleaseEvent(QMouseEvent *event) {
    if (isSliderDown()) { emit clicked(value()); }
    setSliderDown(false);
    QSlider::mouseReleaseEvent(event);
}

Chris Kawa

Right, so now I've actually run the code and it doesn't behave really well.
When you press and hold the mouse it doesn't really highligh the handle and you can't drag it around unless you release and press it again. That's not good.
All in all I would stop trying to control the slider state manually with setSliderDown and just let the default handlers do their work i.e.:

void QImprovedSlider::mouseReleaseEvent(QMouseEvent *event) {
    if (isSliderDown()) //just take care of emitting signal and leave the rest to default event handler
        emit clicked(value());

    QSlider::mouseReleaseEvent(event);
}

same in the press event:

    ...
        else {
            this->setValue(newVal);
        }
        //event->accept(); //don't try to control the state, just move to new position
        //setSliderDown(true);
    }
    //else { //let the default handler do its work always
    QSlider::mousePressEvent(event);
    //}
}

This seems to behave a lot nicer i.e. highlights correctly and lets you drag the handle immediately after the press without releasing first.