Re: [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band

Hi Mircea,

Looks like this is indeed a bug, however it is hard to reproduce. I had 
to do a lot of right mouse button clicks before seeing this misbehavior 
to show up.

I will take the fix, but the patch itself heeds some additional work. 
See my comments in code below.

On 10/20/25 22:00, Mircea Cirjaliu wrote:
> Accidentally pressing right mouse button while dragging
> the range will cause the rubber band to behave abnormally.
> Improved the logic behind range dragging to account for this case.
> The state of the rubber band will be reset on right click.
> 
> Signed-off-by: Mircea Cirjaliu <[email protected]>
> ---
>   src/KsGLWidget.cpp | 40 +++++++++++++++++++++++++++-------------
>   src/KsGLWidget.hpp |  2 ++
>   2 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/src/KsGLWidget.cpp b/src/KsGLWidget.cpp
> index 7f2001c..a2c1fc8 100644
> --- a/src/KsGLWidget.cpp
> +++ b/src/KsGLWidget.cpp
> @@ -190,6 +190,9 @@ void KsGLWidget::mousePressEvent(QMouseEvent *event)
>   	if (event->button() == Qt::LeftButton) {
>   		_posMousePress = _posInRange(event->pos().x());
>   		_rangeBoundInit(_posMousePress);
> +	} else if (event->button() == Qt::RightButton) {
> +		if (_rubberBand.isVisible())
> +			_rangeBoundCancel();
>   	}
>   }
>   
> @@ -262,8 +265,10 @@ void KsGLWidget::mouseMoveEvent(QMouseEvent *event)
>   	if (isEmpty())
>   		return;
>   
> -	if (_rubberBand.isVisible())
> -		_rangeBoundStretched(_posInRange(event->pos().x()));
> +	if (_rubberBand.isVisible()) {
> +		size_t posMouseRel = _posInRange(event->pos().x());
> +		_rangeBoundStretched(posMouseRel);
> +	}

I don't see the point in this change.

>   
>   	bin = event->pos().x() - _bin0Offset();
>   	getPlotInfo(event->pos(), &sd, &cpu, &pid);
> @@ -291,17 +296,20 @@ void KsGLWidget::mouseReleaseEvent(QMouseEvent *event)
>   		return;
>   

No need to pile if on top of another if here. Just start the function with

	if (isEmpty() || !_rubberBand.isVisible())
		return;

The rest may stay unchanged.

>   	if (event->button() == Qt::LeftButton) {
> -		size_t posMouseRel = _posInRange(event->pos().x());
> -		int min, max;
> -		if (_posMousePress < posMouseRel) {
> -			min = _posMousePress - _bin0Offset();
> -			max = posMouseRel - _bin0Offset();
> -		} else {
> -			max = _posMousePress - _bin0Offset();
> -			min = posMouseRel - _bin0Offset();
> -		}
> +		if (_rubberBand.isVisible()) {
> +			size_t posMouseRel = _posInRange(event->pos().x());
> +
> +			int min, max;
> +			if (_posMousePress < posMouseRel) {
> +				min = _posMousePress - _bin0Offset();
> +				max = posMouseRel - _bin0Offset();
> +			} else {
> +				max = _posMousePress - _bin0Offset();
> +				min = posMouseRel - _bin0Offset();
> +			}
>   
> -		_rangeChanged(min, max);
> +			_rangeChanged(min, max);
> +		}
>   	}
>   }
>   
> @@ -1132,7 +1140,7 @@ void KsGLWidget::_rangeChanged(int binMin, int binMax)
>   	/* The rubber band is no longer needed. Make it invisible. */
>   	_rubberBand.hide();
>   
> -	if ( (binMax - binMin) < 4) {
> +	if ((binMax - binMin) < 4) {
>   		/* Most likely this is an accidental click. Do nothing. */
>   		return;
>   	}
> @@ -1182,6 +1190,12 @@ void KsGLWidget::_rangeChanged(int binMin, int binMax)
>   	}
>   }
>   
> +void KsGLWidget::_rangeBoundCancel()
> +{
> +	/* The rubber band is no longer needed. Make it invisible. */
> +	_rubberBand.hide();
> +}
> +
What is the point in defining a function that does nothing but calling 
another function.

Thanks,
Yordan

>   int KsGLWidget::_posInRange(int x)
>   {
>   	int posX;
> diff --git a/src/KsGLWidget.hpp b/src/KsGLWidget.hpp
> index cafc70b..8fcac55 100644
> --- a/src/KsGLWidget.hpp
> +++ b/src/KsGLWidget.hpp
> @@ -315,6 +315,8 @@ private:
>   
>   	void _rangeChanged(int binMin, int binMax);
>   
> +	void _rangeBoundCancel();
> +
>   	bool _findAndSelect(QMouseEvent *event);
>   
>   	bool _find(int bin, int sd, int cpu, int pid,