Some tools like KGraphViewer interpret the "ON" nodes not having an
explicitly set fill colour as them being entirely black, which obscures
the text on them and looks funny. In fact, I thought they were off for
the longest time. Comparing to the output of the `dot` tool, I assume
they are supposed to be white.

Instead of speclawyering over who's in the wrong and must immediately
atone for their wickedness at the altar of RFC2119, just be explicit
about it, set the fillcolor to white, and nobody gets confused.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
This is somewhat "just thrown out there"; I noticed that not setting the
fill colour breaks KGraphViewer only *after* I thought this was just how
they were for several days. With this change, both dot and KGraphViewer
render it correctly, but I have no clue as to whether it's in the spirit
of the file format at all. I figure that if this saves some other poor
souls a bit of time and confusion, then it's worth it.
---
 tools/sound/dapm-graph | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/sound/dapm-graph b/tools/sound/dapm-graph
index f14bdfedee8f11507a6b7b04f6dd1847513e6da8..b6196ee5065a4e72069df663775518352d75d410 100755
--- a/tools/sound/dapm-graph
+++ b/tools/sound/dapm-graph
@@ -10,7 +10,7 @@ set -eu
 
 STYLE_COMPONENT_ON="color=dodgerblue;style=bold"
 STYLE_COMPONENT_OFF="color=gray40;style=filled;fillcolor=gray90"
-STYLE_NODE_ON="shape=box,style=bold,color=green4"
+STYLE_NODE_ON="shape=box,style=bold,color=green4,fillcolor=white"
 STYLE_NODE_OFF="shape=box,style=filled,color=gray30,fillcolor=gray95"
 
 # Print usage and exit

---
base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
change-id: 20250221-dapm-graph-node-colour-eb0011a45856

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

On Fri, 21 Feb 2025 21:39:32 +0100, Nicolas Frattaroli wrote:
> Some tools like KGraphViewer interpret the "ON" nodes not having an
> explicitly set fill colour as them being entirely black, which obscures
> the text on them and looks funny. In fact, I thought they were off for
> the longest time. Comparing to the output of the `dot` tool, I assume
> they are supposed to be white.
> 
> Instead of speclawyering over who's in the wrong and must immediately
> atone for their wickedness at the altar of RFC2119, just be explicit
> about it, set the fillcolor to white, and nobody gets confused.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: dapm-graph: set fill colour of turned on nodes
      commit: d31babd7e304d3b800d36ff74be6739405b985f2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Hello Nicolas,

+Cc: Mark Brown

On Fri, 21 Feb 2025 21:39:32 +0100
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> Some tools like KGraphViewer interpret the "ON" nodes not having an
> explicitly set fill colour as them being entirely black, which obscures
> the text on them and looks funny. In fact, I thought they were off for
> the longest time. Comparing to the output of the `dot` tool, I assume
> they are supposed to be white.
> 
> Instead of speclawyering over who's in the wrong and must immediately
> atone for their wickedness at the altar of RFC2119, just be explicit
> about it, set the fillcolor to white, and nobody gets confused.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> This is somewhat "just thrown out there"; I noticed that not setting the
> fill colour breaks KGraphViewer only *after* I thought this was just how
> they were for several days. With this change, both dot and KGraphViewer
> render it correctly, but I have no clue as to whether it's in the spirit
> of the file format at all. I figure that if this saves some other poor
> souls a bit of time and confusion, then it's worth it.

I confirm the issue with a plain installation of KGraphViewer (which I
didn't know at all before -- interesting tool).

So, let's have mercy on the poor souls:

Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

On Mon, Feb 24, 2025 at 04:33:11PM +0100, Luca Ceresoli wrote:
> Hello Nicolas,
> 
> +Cc: Mark Brown
> 
> On Fri, 21 Feb 2025 21:39:32 +0100
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > Some tools like KGraphViewer interpret the "ON" nodes not having an
> > explicitly set fill colour as them being entirely black, which obscures
> > the text on them and looks funny. In fact, I thought they were off for
> > the longest time. Comparing to the output of the `dot` tool, I assume
> > they are supposed to be white.

As documented in submitting-patches.rst please send patches to the 
maintainers for the code you would like to change.  The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.

On Monday, 24 February 2025 16:42:08 Central European Standard Time Mark Brown 
wrote:
> On Mon, Feb 24, 2025 at 04:33:11PM +0100, Luca Ceresoli wrote:
> > Hello Nicolas,
> > 
> > +Cc: Mark Brown
> > 
> > On Fri, 21 Feb 2025 21:39:32 +0100
> > 
> > Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> > > Some tools like KGraphViewer interpret the "ON" nodes not having an
> > > explicitly set fill colour as them being entirely black, which obscures
> > > the text on them and looks funny. In fact, I thought they were off for
> > > the longest time. Comparing to the output of the `dot` tool, I assume
> > > they are supposed to be white.
> 
> As documented in submitting-patches.rst please send patches to the
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.

Hi Mark,

I used b4 to submit the patch, which runs get-maintainer when I run b4 prep --
auto-to-cc. You can see that even running get_maintainer manually for this 
file, you're not one of the recipients:

 ./scripts/get_maintainer.pl -f tools/sound/dapm-graph
 Luca Ceresoli <luca.ceresoli@bootlin.com> (maintainer:SOUND - SOC LAYER / 
dapm-graph)
 linux-sound@vger.kernel.org (open list:SOUND - SOC LAYER / dapm-graph)
 linux-kernel@vger.kernel.org (open list)

It looks like you're not listed as a maintainer of this file or a parent tree 
in the MAINTAINERS file, which would explain the discrepancy.

Regards,
Nicolas Frattaroli

On Mon, Feb 24, 2025 at 05:09:36PM +0100, Nicolas Frattaroli wrote:
> On Monday, 24 February 2025 16:42:08 Central European Standard Time Mark Brown 
> wrote:
> > On Mon, Feb 24, 2025 at 04:33:11PM +0100, Luca Ceresoli wrote:

> > As documented in submitting-patches.rst please send patches to the
> > maintainers for the code you would like to change.  The normal kernel
> > workflow is that people apply patches from their inboxes, if they aren't
> > copied they are likely to not see the patch at all and it is much more
> > difficult to apply patches.

> I used b4 to submit the patch, which runs get-maintainer when I run b4 prep --
> auto-to-cc. You can see that even running get_maintainer manually for this 
> file, you're not one of the recipients:

That should probably be fixed, but in general note that you always need
to apply a bit of thought to the output of get_maintainer.pl rather than
just trust it blindly - it's prone to both false positives and false
negatives.