Improved slider code review
-
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-cppWhen 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
-
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. ForQMouseEvent
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 ofonClick()
a better name would beclicked()
. Make the signalconst
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 aclicked
signal.
-
@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 aclicked
signal.
So I should take a look to http://doc.qt.io/qt-5/qslider.html#mouseReleaseEvent?
- A "click" is a mouse press and release. You should not emit
-
Yup, make the press and release work together.
-
.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); }
-
setSliderDown(true)
means the handle is drawn as pressed. It should go in the press event. Calling it withtrue
andfalse
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;
-
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); }
-
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 withsetSliderDown
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.