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

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

[Mircea Cirjaliu]

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 <mircea.cirjaliu@yahoo.com>
---
 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);
+	}
 
 	bin = event->pos().x() - _bin0Offset();
 	getPlotInfo(event->pos(), &sd, &cpu, &pid);
@@ -291,17 +296,20 @@ void KsGLWidget::mouseReleaseEvent(QMouseEvent *event)
 		return;
 
 	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();
+}
+
 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,
-- 
2.43.0

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

[Yordan Karadzhov]

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 <mircea.cirjaliu@yahoo.com>
> ---
>   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,

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

[Mircea Cirjaliu]

Hi Yordan,

I tend to favor debuggability, expressiveness and readability in my coding, even if it makes for longer code
or seemingly unnecessary code constructs. I'll leave it to the compiler to optimize variable usage, I wanna
make it easy (for me or others) to set a breakpoint or understand what the code is doing.

On 27/10/2025 21:51, Yordan Karadzhov wrote:
> 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 <mircea.cirjaliu@yahoo.com>
>> ---
>>   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.

I needed a breakpoint there.

> 
>>         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.

Agree with you here, will fix.

> 
>>       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.

Layering functionality, i.e. mouse events call _rangeBoundXXX() functions, which instead act on _rubberBand.
For improved code readability, also having a properly named logical entry point aids in debugging.

> 
> 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,

Best,
Mircea

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

[Yordan Karadzhov]

Hi Mircea,

On 10/28/25 22:27, Mircea Cirjaliu wrote:
> Hi Yordan,
> 
> I tend to favor debuggability, expressiveness and readability in my coding, even if it makes for longer code
> or seemingly unnecessary code constructs. I'll leave it to the compiler to optimize variable usage, I wanna
> make it easy (for me or others) to set a breakpoint or understand what the code is doing.
> 
> On 27/10/2025 21:51, Yordan Karadzhov wrote:
>> 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 <mircea.cirjaliu@yahoo.com>
>>> ---
>>>    src/KsGLWidget.cpp | 40 +++++++++++++++++++++++++++-------------
>>>    src/KsGLWidget.hpp |  2 ++
>>>    2 files changed, 29 insertions(+), 13 deletions(-)
>>>

>>>    @@ -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.
> 
> I needed a breakpoint there.
>

I understand that this change helped you find the problem, but we are 
not merging debugging code upstream.


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

>>>    }
>>>    +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.
> 
> Layering functionality, i.e. mouse events call _rangeBoundXXX() functions, which instead act on _rubberBand.
> For improved code readability, also having a properly named logical entry point aids in debugging.
> 

Again, debugging cannot be a reason for pushing code upstream.

Best,
Yordan

>>
>> 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,
> 
> Best,
> Mircea