* [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band [not found] <5c7a78ce-b1f6-4396-982a-8b884242d8cc.ref@yahoo.com> @ 2025-10-20 19:00 ` Mircea Cirjaliu 2025-10-27 20:51 ` Yordan Karadzhov 0 siblings, 1 reply; 4+ messages in thread From: Mircea Cirjaliu @ 2025-10-20 19:00 UTC (permalink / raw) To: linux-trace-devel; +Cc: y.karadz 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band 2025-10-20 19:00 ` [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band Mircea Cirjaliu @ 2025-10-27 20:51 ` Yordan Karadzhov 2025-10-28 20:27 ` Mircea Cirjaliu 0 siblings, 1 reply; 4+ messages in thread From: Yordan Karadzhov @ 2025-10-27 20:51 UTC (permalink / raw) To: Mircea Cirjaliu, linux-trace-devel 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, ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band 2025-10-27 20:51 ` Yordan Karadzhov @ 2025-10-28 20:27 ` Mircea Cirjaliu 2025-10-30 19:01 ` Yordan Karadzhov 0 siblings, 1 reply; 4+ messages in thread From: Mircea Cirjaliu @ 2025-10-28 20:27 UTC (permalink / raw) To: Yordan Karadzhov; +Cc: linux-trace-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band 2025-10-28 20:27 ` Mircea Cirjaliu @ 2025-10-30 19:01 ` Yordan Karadzhov 0 siblings, 0 replies; 4+ messages in thread From: Yordan Karadzhov @ 2025-10-30 19:01 UTC (permalink / raw) To: Mircea Cirjaliu; +Cc: linux-trace-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-30 19:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5c7a78ce-b1f6-4396-982a-8b884242d8cc.ref@yahoo.com>
2025-10-20 19:00 ` [PATCH] kernelshark: fix bug in the behavior of ksglwidget rubber band Mircea Cirjaliu
2025-10-27 20:51 ` Yordan Karadzhov
2025-10-28 20:27 ` Mircea Cirjaliu
2025-10-30 19:01 ` Yordan Karadzhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).