Graph color fixes

[hjoliver]

Address #2965
... (may or may not close the issue, depending)

1. consistent use of desktop theme foreground and background colors:

• always use desktop theme foreground for default node boundary, edge, and font color
• always use desktop theme background for default node fill color
• (if users set non-default colors for some attributes, it is up to them to manage contrast with any remaining default colors still being used).

2. fix default node attributes

• now it has exactly the same effect as styling root (and can be overridden by styling root).

[hjoliver]

Example: no vis. settings except for orange3 default node fill color.

Light theme:

Switch to dark theme, then refresh view:

[hjoliver]

Note I've chosen a mid-toned non-default fill color in the example above, so that it works well enough with the default desktop foreground color in both light and dark themes.... if I chose a darker fill color then I might also need to override the font color if using a light desktop theme, and so on.

[hjoliver]

Example of default node attributes:

[visualization]
   default node attributes = "color=red", "style=filled", "fillcolor=orange3", "fontcolor=blue"

On this branch (correct):

On 7.8.x (wrong):

On 7.8.x we are overriding the user default node attributes with the theme values; here I am setting theme defaults first, then user defaults (then specific user node attributes).

I have checked that this doesn't break cylc graph -n (inheritance graph) and cylc gui graph view.

[hjoliver]

Assigning @matthewrmshin as reviewer too, since he has been involved in the issue.

[matthewrmshin]

(Unit test failing - a glance suggests that it is related to this change.)

[matthewrmshin]

... (may or may not close the issue, depending)

I'll suggest closing #2965 with this - unless we expect to have the same issue with OS desktop themes in the web UI.

[hjoliver]

(Not surprisingly, some graph-related tests broken by this change ... I'll fix today, I hope)

[hjoliver]

... (may or may not close the issue, depending)

I'll suggest closing #2965 with this - unless we expect to have the same issue with OS desktop themes in the web UI.

My "depending" comment was aimed at @sadielbartholomew confirming that this fully fixes the issue she reported (the symptoms didn't seem to quite line up...)

[sadielbartholomew]

Sorry for the radio silence @hjoliver, I've been preparing for annual review & then our internal User Update seminar which has taken all my time with urgency. Thanks for sorting this. I'll get it reviewed on Wednesday as priority.

[sadielbartholomew]

Thanks for investigating & finding a solution to most of the problems, Hilary, & sorry for the delayed review.

I'm happy with your approach, as outlined in #2965 (comment), & have tested this for a range of [visualization] settings. Everything works as described, except one aspect, namely you seem to have, in fixing other facets, made unreadable the case where (default) nodes are simply set to be filled, without any customisation on colourings that would I agree be too much work to practicably account for, e.g. the suite.rc having simply (for the styling):

[visualization]
    default node attributes = "style=filled"

where I, at least, see (with the equivalent issue but inverted for a light theme):

this branch	master branch

To make clear, this is not a custom fill colour, just the default one, so I don't think intended to bypass it as "non-standard" as per your comment:

if a user sets a non-default fill color, it is their responsibility to also set a font color that contrasts with their custom fill color.

From experience in seeing the graphing displayed by users here, it seems common for users to set nodes to be filled by default, so I think the above needs fixing. I thought I might as well investigate to find an amendment to fix it myself since it seemed simple (actually, it was more tricky & time-consuming than I thought, so I slightly regret attempting it, but I got there I think 😁). If you make this change, then the issue seems to be resolved, seemingly without introducing issues elsewhere:

diff --git a/lib/cylc/graphing.py b/lib/cylc/graphing.py
index 15ca6af8c..5b0a3695e 100644
--- a/lib/cylc/graphing.py
+++ b/lib/cylc/graphing.py
@@ -278,7 +278,7 @@ class CGraphPlain(pygraphviz.AGraph):
         """
 
         # Transparent graph bg - let the desktop theme bg shine through.
-        self.graph_attr['bgcolor'] = '#ffffff00'
+        self.graph_attr['bgcolor'] = bgcolor
         self.graph_attr['color'] = fgcolor
         self.graph_attr['fontcolor'] = fgcolor
 
@@ -360,6 +360,11 @@ class CGraph(CGraphPlain):
             attr, value = [val.strip() for val in item.split('=', 1)]
             attrs[attr] = value
             node.attr[attr] = value
+        # For "style=filled", ensure labels are readable (inverse lightness)
+        if node.attr['style'] == 'filled' and not (
+            any(attribute.startswith('fontcolor') for
+                attribute in self.vizconfig['default node attributes'])):
+            node.attr['fontcolor'] = self.graph_attr['bgcolor']
         if node.attr['style'] != 'filled' and (
                 'color' in attrs and 'fontcolor' not in attrs):
             node.attr['fontcolor'] = node.attr['color']

I then get:

default node attributes	dark theme	light theme (after refreshing)
None i.e. unfilled default
style=filled

[hjoliver]

I've applied your suggested change @sadielbartholomew - seems to do the trick, thanks 👎

[hjoliver]

(Still some graphing tests to fix, I expect.)

[hjoliver]

Tests passed. Should be good to go now @sadielbartholomew

[sadielbartholomew]

Excellent, thanks for sorting this out Hilary. This should be sufficient to close #2965, so I will close it tentatively (if users report similar problems in future we could always re-open it, but I know we want to focus on our new UI ▶️, so unless it's urgent I would leave the old GUI styling alone now regardless).

• Happy with the approach;
• Feedback (Graph color fixes #3047 (review)) addressed;
• Tests pass (Travis CI &) locally, with changes to expected values in the test being appropriate;
• Graph styling is as it should be with manual testing over a reasonable phase space of possible [visualization] settings for cylc graph with gcylc graph style unaffected.