diff mbox series

[v2,5/8] kernel-shark-qt: Create "Apply filter XX" checkboxes in KsUtils

Message ID 20181214125212.9637-6-ykaradzhov@vmware.com (mailing list archive)
State Accepted
Headers show
Series More modifications toward KS 1.0 | expand

Commit Message

Yordan Karadzhov Dec. 14, 2018, 12:52 p.m. UTC
The code responsible for the creation of the "Apply filters to Graph"
and "Apply filters to List" checkboxes (showing in the "Filtering" menu),
has been moved outside of the KsMainWindow class and is now available
in KsUtils. This is done because we want to have the same checkboxes
available in the KsQuickContextMenu.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsMainWindow.cpp | 39 +++++++++++++---------------
 kernel-shark-qt/src/KsMainWindow.hpp |  6 ++---
 kernel-shark-qt/src/KsUtils.cpp      | 25 ++++++++++++++++++
 kernel-shark-qt/src/KsUtils.hpp      |  2 ++
 4 files changed, 47 insertions(+), 25 deletions(-)

Comments

Steven Rostedt Dec. 14, 2018, 5:01 p.m. UTC | #1
On Fri, 14 Dec 2018 12:52:39 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> The code responsible for the creation of the "Apply filters to Graph"
> and "Apply filters to List" checkboxes (showing in the "Filtering" menu),
> has been moved outside of the KsMainWindow class and is now available
> in KsUtils. This is done because we want to have the same checkboxes
> available in the KsQuickContextMenu.
> 

I applied this patch and it shows the "Hide CPU [x]". But it's a bit
flaky. The first time I tried it, it didn't do anything. Then after
starting kernelshark again, I was able to have it hide the CPU, but
when I clicked on the "Filter" menu on the top toolbar, the filter went
away.

Another thing, the "Apply filters to Graph" checkbox is a bit
confusing. What does it actually mean? The filters are applied by
default to the graph, correct? But the checkbox isn't set.

Also, does it make sense to have that "Apply filters to graph" checkbox
in the Graph menu. It would make more sense if it said "Apply filters
to table".

I think I'll apply this series except for this patch and the last one
(5 and 8).

-- Steve
Yordan Karadzhov Dec. 17, 2018, 5:44 p.m. UTC | #2
On 14.12.18 г. 19:01 ч., Steven Rostedt wrote:
> On Fri, 14 Dec 2018 12:52:39 +0000
> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> 
>> The code responsible for the creation of the "Apply filters to Graph"
>> and "Apply filters to List" checkboxes (showing in the "Filtering" menu),
>> has been moved outside of the KsMainWindow class and is now available
>> in KsUtils. This is done because we want to have the same checkboxes
>> available in the KsQuickContextMenu.
>>
> 
> I applied this patch and it shows the "Hide CPU [x]". But it's a bit
> flaky. The first time I tried it, it didn't do anything. Then after
> starting kernelshark again, I was able to have it hide the CPU, but
> when I clicked on the "Filter" menu on the top toolbar, the filter went
> away.

Interesting, I have an idea why this may happen, but I am not sure.


> 
> Another thing, the "Apply filters to Graph" checkbox is a bit
> confusing. What does it actually mean? The filters are applied by
> default to the graph, correct? But the checkbox isn't set.
> 
> Also, does it make sense to have that "Apply filters to graph" checkbox
> in the Graph menu. It would make more sense if it said "Apply filters
> to table".

You are right. There is a typo in the name of the checkbox. But it does 
the correct thing.

I will fix the two problems and will resend the patch.

Thanks a lot!
Yordan


> 
> I think I'll apply this series except for this patch and the last one
> (5 and 8).
> 
> -- Steve
>
diff mbox series

Patch

diff --git a/kernel-shark-qt/src/KsMainWindow.cpp b/kernel-shark-qt/src/KsMainWindow.cpp
index 7213c01..142cd1f 100644
--- a/kernel-shark-qt/src/KsMainWindow.cpp
+++ b/kernel-shark-qt/src/KsMainWindow.cpp
@@ -49,9 +49,7 @@  KsMainWindow::KsMainWindow(QWidget *parent)
   _quitAction("Quit", this),
   _importFilterAction("Import Filter", this),
   _exportFilterAction("Export Filter", this),
-  _graphFilterSyncAction(this),
   _graphFilterSyncCBox(nullptr),
-  _listFilterSyncAction(this),
   _listFilterSyncCBox(nullptr),
   _showEventsAction("Show events", this),
   _showTasksAction("Show tasks", this),
@@ -292,22 +290,13 @@  void KsMainWindow::_createMenus()
 
 	/* Filter menu */
 	filter = menuBar()->addMenu("Filter");
+
+	connect(filter, 		&QMenu::aboutToShow,
+		this,			&KsMainWindow::_updateFilterMenu);
+
 	filter->addAction(&_importFilterAction);
 	filter->addAction(&_exportFilterAction);
 
-	auto lamMakeCBAction = [&](QWidgetAction *action, QString name)
-	{
-		QWidget  *containerWidget = new QWidget(filter);
-		containerWidget->setLayout(new QHBoxLayout());
-		containerWidget->layout()->setContentsMargins(FONT_WIDTH, FONT_HEIGHT/5,
-							      FONT_WIDTH, FONT_HEIGHT/5);
-		QCheckBox *checkBox = new QCheckBox(name, filter);
-		checkBox->setChecked(true);
-		containerWidget->layout()->addWidget(checkBox);
-		action->setDefaultWidget(containerWidget);
-		return checkBox;
-	};
-
 	/*
 	 * Set the default filter mask. Filter will apply to both View and
 	 * Graph.
@@ -317,20 +306,20 @@  void KsMainWindow::_createMenus()
 
 	kshark_ctx->filter_mask |= KS_EVENT_VIEW_FILTER_MASK;
 
-	_graphFilterSyncCBox = lamMakeCBAction(&_graphFilterSyncAction,
-					       "Apply filters to Graph");
+	_graphFilterSyncCBox =
+		KsUtils::addCheckBoxToMenu(filter, "Apply filters to Graph");
+	_graphFilterSyncCBox->setChecked(true);
 
 	connect(_graphFilterSyncCBox,	&QCheckBox::stateChanged,
 		this,			&KsMainWindow::_graphFilterSync);
 
-	_listFilterSyncCBox = lamMakeCBAction(&_listFilterSyncAction,
-					      "Apply filters to List");
+	_listFilterSyncCBox =
+		KsUtils::addCheckBoxToMenu(filter, "Apply filters to List");
+	_listFilterSyncCBox->setChecked(true);
 
 	connect(_listFilterSyncCBox,	&QCheckBox::stateChanged,
 		this,			&KsMainWindow::_listFilterSync);
 
-	filter->addAction(&_graphFilterSyncAction);
-	filter->addAction(&_listFilterSyncAction);
 	filter->addAction(&_showEventsAction);
 	filter->addAction(&_showTasksAction);
 	filter->addAction(&_hideTasksAction);
@@ -446,6 +435,14 @@  void KsMainWindow::_filterSyncCBoxUpdate(kshark_context *kshark_ctx)
 		_graphFilterSyncCBox->setChecked(false);
 }
 
+void KsMainWindow::_updateFilterMenu()
+{
+	kshark_context *kshark_ctx(nullptr);
+
+	if (kshark_instance(&kshark_ctx))
+		_filterSyncCBoxUpdate(kshark_ctx);
+}
+
 void KsMainWindow::_importFilter()
 {
 	kshark_context *kshark_ctx(nullptr);
diff --git a/kernel-shark-qt/src/KsMainWindow.hpp b/kernel-shark-qt/src/KsMainWindow.hpp
index 72f7059..301acc9 100644
--- a/kernel-shark-qt/src/KsMainWindow.hpp
+++ b/kernel-shark-qt/src/KsMainWindow.hpp
@@ -110,12 +110,8 @@  private:
 
 	QAction		_exportFilterAction;
 
-	QWidgetAction	_graphFilterSyncAction;
-
 	QCheckBox	*_graphFilterSyncCBox;
 
-	QWidgetAction	_listFilterSyncAction;
-
 	QCheckBox	*_listFilterSyncCBox;
 
 	QAction		_showEventsAction;
@@ -222,6 +218,8 @@  private:
 
 	void _deselect();
 
+	void _updateFilterMenu();
+
 	void _filterSyncCBoxUpdate(kshark_context *kshark_ctx);
 
 private slots:
diff --git a/kernel-shark-qt/src/KsUtils.cpp b/kernel-shark-qt/src/KsUtils.cpp
index 2ebbae3..0298010 100644
--- a/kernel-shark-qt/src/KsUtils.cpp
+++ b/kernel-shark-qt/src/KsUtils.cpp
@@ -95,6 +95,31 @@  void graphFilterSync(bool state)
 	}
 }
 
+
+/**
+ * @brief Add a checkbox to a menu.
+ *
+ * @param menu: Input location for the menu object, to which the checkbox will be added.
+ * @param name: The name of the checkbox.
+ *
+ * @returns The checkbox object;
+ */
+QCheckBox *addCheckBoxToMenu(QMenu *menu, QString name)
+{
+	QWidget  *containerWidget = new QWidget(menu);
+	containerWidget->setLayout(new QHBoxLayout());
+	containerWidget->layout()->setContentsMargins(FONT_WIDTH, FONT_HEIGHT/5,
+						      FONT_WIDTH, FONT_HEIGHT/5);
+	QCheckBox *checkBox = new QCheckBox(name, menu);
+	containerWidget->layout()->addWidget(checkBox);
+
+	QWidgetAction *action = new QWidgetAction(menu);
+	action->setDefaultWidget(containerWidget);
+	menu->addAction(action);
+
+	return checkBox;
+}
+
 /**
  * @brief Simple CPU matching function to be user for data collections.
  *
diff --git a/kernel-shark-qt/src/KsUtils.hpp b/kernel-shark-qt/src/KsUtils.hpp
index 052cc71..cb95b4f 100644
--- a/kernel-shark-qt/src/KsUtils.hpp
+++ b/kernel-shark-qt/src/KsUtils.hpp
@@ -93,6 +93,8 @@  void listFilterSync(bool state);
 
 void graphFilterSync(bool state);
 
+QCheckBox *addCheckBoxToMenu(QMenu *menu, QString name);
+
 /** @brief Convert the timestamp of the trace record into a string showing
  *	   the time in seconds.
  *