Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

From: Slavomir Kaslev <slavomir.kaslev@gmail.com>
To: Yordan Karadzhov <ykaradzhov@vmware.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	"linux-trace-devel@vger.kernel.org" 
	<linux-trace-devel@vger.kernel.org>
Subject: Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry
Date: Fri, 19 Apr 2019 11:07:55 +0300	[thread overview]
Message-ID: <CAE0o1Nvw_Ef-XSAVzMHmfa-3+gBQzutSfTj_mc-FsJ41GQFXkA@mail.gmail.com> (raw)
In-Reply-To: <b0021d24-65c0-7e97-6c23-a13732bc7760@vmware.com>

On Fri, Apr 19, 2019 at 10:46 AM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
>
>
> On 19.04.19 г. 10:29 ч., Slavomir Kaslev wrote:
> > On Thu, Apr 18, 2019 at 6:23 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> >>
> >> Markers can point to entries that are filtered out. When switching
> >> the marker it may happen that new Active Marker points to an entry
> >> that is filtered. In this case no actions must be taken and a warning
> >> message for the user must be displayed.
> >>
> >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> >> ---
> >>   kernel-shark/src/KsTraceViewer.cpp | 27 +++++++++++++++++----------
> >>   kernel-shark/src/KsTraceViewer.hpp |  2 ++
> >>   2 files changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
> >> index 04a38b8..85da64f 100644
> >> --- a/kernel-shark/src/KsTraceViewer.cpp
> >> +++ b/kernel-shark/src/KsTraceViewer.cpp
> >> @@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
> >>     _graphFollowsCheckBox(this),
> >>     _graphFollows(true),
> >>     _mState(nullptr),
> >> -  _data(nullptr)
> >> +  _data(nullptr),
> >> +  _em(this)
> >>   {
> >>          this->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Maximum);
> >>
> >> @@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
> >>                  QModelIndex index =
> >>                          _proxyModel.mapFromSource(_model.index(row, 0));
> >>
> >> -               /*
> >> -                * The row of the active marker will be colored according to
> >> -                * the assigned property of the current state of the Dual
> >> -                * marker. Auto-scrolling is temporarily disabled because we
> >> -                * do not want to scroll to the position of the marker yet.
> >> -                */
> >> -               _view.setAutoScroll(false);
> >> -               _view.selectRow(index.row());
> >> -               _view.setAutoScroll(true);
> >> +               if (index.isValid()) {
> >> +                       /*
> >> +                        * The row of the active marker will be colored according to
> >> +                        * the assigned property of the current state of the Dual
> >> +                        * marker. Auto-scrolling is temporarily disabled because we
> >> +                        * do not want to scroll to the position of the marker yet.
> >> +                        */
> >> +                       _view.setAutoScroll(false);
> >> +                       _view.selectRow(index.row());
> >> +                       _view.setAutoScroll(true);
> >> +               } else {
> >> +                       _view.clearSelection();
> >> +                       QString err("The marker's entry is filtered out.");
> >> +                       _em.showMessage(err);
> >
> > When is this error message going to be cleared? I don't think it
> > changes atm unless a new error comes along.
>
> The message will be displayed in a small widget (QErrorMessage). This
> widget has an "OK" button and a "Show this message again" checkbox.
>
> To clear the message you can either press "OK" or close the widget.
> If you uncheck the "Show this message again" checkbox, no message will
> be shown next time when the error occurs.

That makes sense. In that case, the patch lgtm

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

-- Slavi