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

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

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