Message-ID: <1352946038.18025.87.camel@gandalf.local.home>
Subject: Re: [PATCH 2/4] kernel-shark: Allow unsetting of all CPUs in filter
From: Steven Rostedt <rostedt@goodmis.org>
To: David Sharp <dhsharp@google.com>
Cc: linux-kernel@vger.kernel.org, Vaibhav Nagarnaik <vnagarnaik@google.com>
Date: Wed, 14 Nov 2012 21:20:38 -0500
In-Reply-To: <1352944274-21699-2-git-send-email-dhsharp@google.com>
References: <1352944274-21699-1-git-send-email-dhsharp@google.com>
	 <1352944274-21699-2-git-send-email-dhsharp@google.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2303
Lines: 63

On Wed, 2012-11-14 at 17:51 -0800, David Sharp wrote:
> From: Vaibhav Nagarnaik <vnagarnaik@google.com>
> 
> The button "All CPUs" in CPU filter dialog allows setting of all CPUs
> but doesn't clear CPUs when it is unchecked. Make sure that when the
> "All CPUs" button is unchecked, all the CPUs get unchecked.
> 
> Tested: In kernelshark, go to Filter->list CPUs and uncheck "All CPUs"
> button. All the CPUs should be unchecked at that point.

This was actually done purposely, as I thought it may be used as a way
to change to a mode to select individual CPUs.

But I'm fine with having it do this feature, except...

Now when I apply this patch, if I go to deselect a CPU, it unselects the
"All CPUs" and causes all CPUs to be unselected.

Need to make it so it only unselects all CPUs if it was directly
unselected. Not when its done by deselecting a single CPU.

So, I'm going to NAK this patch, but if you can fix it so that
deselecting the "All CPUs" directly has this feature, but I can still
deselect a single CPU without it causing all other CPUs to be
deselected, then I'm fine with the change.

-- Steve

> 
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
> Signed-off-by: David Sharp <dhsharp@google.com>
> ---
>  trace-filter.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/trace-filter.c b/trace-filter.c
> index c657a18..89cf032 100644
> --- a/trace-filter.c
> +++ b/trace-filter.c
> @@ -1723,12 +1723,10 @@ void cpu_toggle(gpointer data, GtkWidget *widget)
>  
>  	if (strcmp(label, CPU_ALL_CPUS_STR) == 0) {
>  		cpu_helper->allcpus = active;
> -		if (active) {
> -			/* enable all toggles */
> -			for (cpu = 0; cpu < cpu_helper->cpus; cpu++)
> -				gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(cpu_helper->buttons[cpu]),
> -							     TRUE);
> -		}
> +		/* enable/disable all toggles */
> +		for (cpu = 0; cpu < cpu_helper->cpus; cpu++)
> +			gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(cpu_helper->buttons[cpu]),
> +						     active ? TRUE : FALSE);
>  		return;
>  	}
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/