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

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 <[email protected]>
>>> ---
>>>    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