GUI: use GTK theme colors. #2842

hjoliver · 2018-11-07T06:47:01Z

Play better with the system GTK theme. (Test with a dark theme) (This has been a long time coming).

Removes the quirky background colors in log viewer and start dialogs [which override theme colors]
Use GTK theme background in the graph view. [graphviz uses a white canvas by default]

hjoliver · 2018-11-07T09:47:15Z

Left - master; right - this branch. The green log viewer border is just weird. The white graph background is hard on the eyes, and violates the theme choice.

hjoliver · 2018-11-07T09:49:25Z

(Motivation: I tend to use a dark desktop theme these days, and GUI demos look better if theme-compliant).

kinow · 2018-11-07T10:00:05Z

Me gusta! I think we should allow theming in the new Web GUI too. At least a business-like, a dark theme, and a high contrast would be good options (all following the accessibility guidelines, especially related to (colour)blindness)

oliver-sanders · 2018-11-07T10:19:37Z

I think we should allow theming in the new Web GUI too

Easy!

a dark theme, and a high contrast would be good options

Yes and yes!

The green log viewer border is just weird

I think this is a feature we can quietly loose in the web GUI.

sadielbartholomew · 2018-11-07T10:59:20Z

Brilliant @hjoliver!

I'm also one of the many who prefer (strongly) a theme with a dark background for most applications (if only GitHub had in-built themes; I have to resort to using a plugin to go 'dark' at home, which I can't use on my work desktop). This could easily be a popular feature.

At least a business-like, a dark theme, and a high contrast would be good options

I agree with @kinow, for any GUI (new or old) we should aim for a small number of distinct themes to include these. With too much potential for customisation, users could get distracted (or at least, I know I would as a casual user) & it could disorient us with respect to user support.

oliver-sanders · 2018-11-07T11:18:03Z

The challenge with this (and other cylc8) user preference setting is how to store it.

At the moment user preferences are picked up so long as the GUI is run on a host with a shared file system, for these cases users need only configure preferences (e.g. gcylc.rc) once. However, with the new framework:

Browser cookies (obviously not portable across browsers)
Some centralised "preferences service" (not portable across cylc suite "services" e.g. local "out of the box", research suite service, production suite service)

wxtim · 2018-11-07T12:14:16Z

Me gusta! I think we should allow theming in the new Web GUI too. At least a business-like, a dark theme, and a high contrast would be good options (all following the accessibility guidelines, especially related to (colour)blindness)

Perhaps the testing regimen for the new GUI should include some accessibility checks.

oliver-sanders · 2018-11-07T12:48:19Z

Bumping this conversation to #1873 to avoid convoluting this PR.

include some accessibility checks

Auto / semi-auto checks do indeed exist, there are many different things to check for (multiple types of colour blindness, issues with contrast and saturation, short-sightedness, etc)

sadielbartholomew · 2018-11-07T12:57:56Z

Bumping this conversation to #1873 to avoid convoluting this PR.

Good idea, #1873 is the natural home, but I'm now thinking that #1873 should be partitioned (via separate issues or maybe tags?) by topic sooner rather than later, otherwise relevant information will be hard to find. Is someone on this or could I provide suggestions so we can then get sub-topics agreed? [posting this here rather than on the other PR purely to help it get seen by most of us]

lib/cylc/gui/updater_graph.py

kinow · 2018-11-07T22:03:37Z

@sadielbartholomew

should be partitioned (via separate issues or maybe tags?)

+1 would be nice to have some labels/tags, and/or use github projects. I'm keeping track of some requirements/issue via a personal project, but would be happy to start using projects under something like cylc/cylc.

hjoliver · 2018-11-08T03:45:50Z

@kinow - I'm keen to start using GitHub Projects, just haven't found the time to get started yet. Happy for others to have a crack at it while I'm away though.

sadielbartholomew · 2018-11-08T15:02:35Z

(Sorry for hijacking this unrelated PR. Also, if this is a bit heavy-weight I can move this to email etc.)

@kinow I've just had a look at your project as linked & it looks really cool 😎.

I'm keen to start using GitHub Projects, just haven't found the time to get started yet

I've never thought to explore the other features GitHub offers, but from @kinow's well-organised example I am thinking Projects could be a very useful tool for management of our intensive ~2 year plan.

On that note, @cylc/core, I've been looking into ways we can work as efficiently & cooperatively as possible towards our short-term future roadmap, i.e. thinking about ideas relating to the 'Goals' section of the Cylc Development Workshop Agenda. Juggling the software development life-cycle for the Python3 port, web GUI & new architecture will be tricky, especially without a fully co-located dev team.

I strongly believe investing a bit (or even quite a lot) of overhead in discussing how to streamline our collective working processes would pay off in the long-term in terms of time & effort saved. I am happy to lead on discussing this, but only if people would be receptive to uptake of small methodology changes.

Any thoughts on the best place to outline these ideas? I would simply collate them to outline at the Workshop itself, but since we will have so much to discuss otherwise I feel there is a large risk this will get side-lined. So ideally I would want to pre-register them in text somewhere they can be openly viewed. However, a GitHub Issue does not seem appropriate as it is not relating to the codebase itself.
So I am thinking perhaps an email (with attached document) or a Wiki post on the cylc repo? Or should I perhaps try to investigate this with the Metomi team individually as one of the co-located teams, & then the issue is (essentially) reduced to that of keeping on track / communicating as well as possible MO <--> NIWA?

hjoliver · 2018-11-08T17:53:21Z

How about forking the new cylc/admin repo and raising a PR with a new .md file, and a link to it in the workshop agenda?

hjoliver · 2018-11-08T21:07:07Z

Since this got a generally positive reception, and is a pretty trivial change, I've assigned reviewers and moved it to next-release.

kinow · 2018-11-09T00:46:46Z

Tested on XFCE, Ubuntu LTS, setting style to Adwaita-dark.

Here's master:

And here's after switching to the code in this pull request:

Looks much much better. Actually, without this change, cylc gui is not usable on XFCE with a dark theme. Great fix.

What I liked about the code changes, is that it is mainly removal. Few lines added 😀 Only minor nit-pick was the comment left with "Turn off white canvas border area". I think that can be removed later?

Will leave it to @sadielbartholomew to finish the review.

sadielbartholomew · 2018-11-09T11:33:48Z

forking the new cylc/admin repo and raising a PR with a new .md file, and a link to it in the workshop agenda

Great idea @hjoliver I will do exactly that.

Just about to review this PR itself & so a merge should hopefully be imminent. I'm excited as the screenshots above look incredible 😍.

sadielbartholomew

Summary:

Unfortunately this may yet need some tweaking. @hjoliver (or @kinow if Hilary's conference means he cannot answer soon): I'm not sure whether we want to have it as-is in the urgent next release & then fix any minor issues for the release after, as this does work & is still a major improvement? I am registering a 'tentative' approval informally so you can merge if you see fit, but the issue below should be addressed (an approach approved by the MO team, as we are all too busy to try to fix any of these issues today).

Feedback & issue:

I tested with 3 themes on my RHEL 6.10 & GNOME 2.28.2 environment:

the default 'System' theme ('light' not 'dark'), as a control;
'high contrast inverse', a sadly horribly ugly theme but the only provided theme with a dark background as standard;
my own custom theme, 'dark' but otherwise not stylised ('Styler' theme with dark background).

In short, everything looks good except for in Graph View where there is inconsistency with the view background & a different background appearing behind the entire graph.

Illustrative cropped screenshots (all from the GUI launched via this PR branch):

For the dark themes:

(Same context) for the light theme:

Particularly wacky output for a non-singular graph with my custom theme:

Also at one point I got a STDERR message saying Warning: #003 is not a known color., which I did not get on master, but I can't see immediately how this would originate from the code change (git grep "#003" in the Cylc repo does not produce any results).

oliver-sanders · 2018-11-12T11:50:27Z

I managed to remove the graph background and fix the #003 error reported by @sadielbartholomew with the simple diff:

diff --git a/lib/cylc/gui/updater_graph.py b/lib/cylc/gui/updater_graph.py
index 70a473b4e..c0bce95f9 100644
--- a/lib/cylc/gui/updater_graph.py
+++ b/lib/cylc/gui/updater_graph.py
@@ -237,7 +237,7 @@ class GraphUpdater(threading.Thread):
 
     def update_xdot(self, no_zoom=False):
         colors = getattr(self.xdot.widget.style, 'bg', None)
-        self.graphw.graph_attr['bgcolor'] = colors[gtk.STATE_NORMAL]
+        self.graphw.graph_attr['bgcolor'] = '#ffffff00'  # transparent background
         self.xdot.set_dotcode(self.graphw.to_string(), no_zoom=no_zoom)
         if self.first_update:
             self.xdot.widget.zoom_to_fit()

The #003 error is presumably because graphviz is expecting colors in two character per field hex format?

This works fine but by removing the white background we risk making the graph unreadable on a dark background because the text and arrows are coloured black.

This diff puts the text in the inverse color to the background, It's a hack but makes the point:

diff --git a/lib/cylc/gui/updater_graph.py b/lib/cylc/gui/updater_graph.py
index 70a473b4e..68aa88071 100644
--- a/lib/cylc/gui/updater_graph.py
+++ b/lib/cylc/gui/updater_graph.py
@@ -237,8 +237,18 @@ class GraphUpdater(threading.Thread):
 
     def update_xdot(self, no_zoom=False):
         colors = getattr(self.xdot.widget.style, 'bg', None)
-        self.graphw.graph_attr['bgcolor'] = colors[gtk.STATE_NORMAL]
-        self.xdot.set_dotcode(self.graphw.to_string(), no_zoom=no_zoom)
+
+        self.graphw.graph_attr['bgcolor'] = '#ffffff00'  # transparent background
+        graph = self.graphw.to_string()
+        tcolor = colors[gtk.STATE_NORMAL]
+        new_color = '#' + ''.join('%02x' % (rgb * 255) for rgb in (
+            1 - tcolor.red_float,
+            1 - tcolor.green_float,
+            1 - tcolor.blue_float,
+        ))
+        graph = graph.replace('black', '"%s"' % new_color)
+
+        self.xdot.set_dotcode(graph, no_zoom=no_zoom)
         if self.first_update:
             self.xdot.widget.zoom_to_fit()
             self.first_update = False

Alternatively we could set a threshold for which to flip the default color from black to white:

if sum(tcolor.red_float, tcolor.green_float, tcolor.blue_float) < 1.5:
    # dark theme
    graph = graph.replace('black', '#ffffff')  # need a better way to do this!
else:
    # light theme

hjoliver · 2018-11-13T19:31:56Z

Thanks @oliver-sanders, transparent graph background certainly seems a better way than attempting to retrieve the theme background color (where it might be hard to know which variant to use).

Not sure what to do about edge, border, and font colours. Maybe we should just allow manual styling of those (like for cylc graph) and document that that might be needed if you use a dark theme. Although your threshold idea might be good enough.

matthewrmshin · 2018-11-13T20:35:05Z

Searching the phrase how to tell if a color is light or dark on Google yields some very interesting results.

matthewrmshin · 2018-11-13T20:40:06Z

(Or can we do colour ^ 0x00ffffff to choose the inverted colour for font, border, etc?)

hjoliver · 2018-11-22T01:25:54Z

Test battery is now un-broken. Inheritance graphs now use plain theme fg/bg colors. Tweaking the graph comparison tests was almost a nightmare (look at the new line count!) ... but saved by vim macros.

Maybe that's the last major tweak of the old GUI, before it retires forever!

sadielbartholomew · 2018-11-23T10:46:32Z

I'm just running my local test battery as a final confirmation that everything is up to scratch & having a look through the code itself in the meantime. Hopefully we are good to go!

oliver-sanders · 2018-11-23T11:47:24Z

Looking good now. There is one remaining issue which is with the default Cylc colour theme which hardcodes black text even for unfilled nodes which can result in black text on a black background.

As the other built-in themes use filled nodes for all states they should be unaffected.

This diff outlines one potential way of getting around this:

diff --git a/etc/gcylc-themes.rc b/etc/gcylc-themes.rc
index 030d2bb92..575968b21 100644
--- a/etc/gcylc-themes.rc
+++ b/etc/gcylc-themes.rc
@@ -19,16 +19,16 @@
         # * The 'retry' states are hollow (pre-running) orange (almost red)
         #   as a failure has occurred but we're doing something about it.
         # * The 'waiting' state is light blue.
-        defaults  = "color=#bbbbbb", "style=unfilled", "fontcolor=black"
+        defaults  = "color=#bbbbbb", "style=unfilled"
         waiting   = "color=#88c6ff"
         held      = "color=#fe83ff"
         queued    = "color=#dcd901"
         ready     = "color=#a08f49"
         expired   = "color=#000000"
-        submitted = "color=#a1cf25", "style=filled"
+        submitted = "color=#a1cf25", "style=filled", "fontcolor=black"
         submit-failed = "color=#ff007e", "style=filled", "fontcolor=white"
-        running   = "color=#00c140", "style=filled"
-        succeeded = "color=#ada5a5", "style=filled"
+        running   = "color=#00c140", "style=filled", "fontcolor=black"
+        succeeded = "color=#ada5a5", "style=filled", "fontcolor=black"
         failed    = "color=#ff0000", "style=filled", "fontcolor=white"
         retrying  = "color=#ff7e00"
         submit-retrying  = "color=#fa7e00"
diff --git a/lib/cylc/gui/updater_graph.py b/lib/cylc/gui/updater_graph.py
index 5e44c1bc5..46a8aa5fc 100644
--- a/lib/cylc/gui/updater_graph.py
+++ b/lib/cylc/gui/updater_graph.py
@@ -260,17 +260,24 @@ class GraphUpdater(threading.Thread):
             state = self.state_summary[id_]['state']
         else:
             state = self.fam_state_summary[id_]['state']
+
+        # required theme attributes
         try:
             node.attr['style'] = 'bold,' + self.theme[state]['style']
             node.attr['fillcolor'] = self.theme[state]['color']
             node.attr['color'] = self.theme[state]['color']
-            node.attr['fontcolor'] = self.theme[state]['fontcolor']
         except KeyError:
             # unknown state
             node.attr['style'] = 'unfilled'
             node.attr['color'] = 'black'
             node.attr['fontcolor'] = 'black'
 
+        # optional theme attributes
+        theme = self.theme.get(state, {})
+        for attr in ['fontcolor']:
+            if attr in theme:
+                node.attr[attr] = theme[attr]
+
         if shape:
             node.attr['shape'] = shape

sadielbartholomew

(My local test battery passes, so re-approving after new changes, especially to tests, though the issue pointed out by Oliver issue will need to addressed.)

lib/cylc/graphing.py

hjoliver · 2018-11-24T18:23:26Z

Fixed the default theme as per @oliver-sanders suggestion, and tested:

hjoliver · 2018-11-24T18:24:10Z

Should be good to merge now.

Works for gcylc graph view and "cylc graph".

hjoliver · 2018-11-24T18:48:34Z

(Rebased on to master, for code coverage comparison with base commit).

hjoliver · 2018-11-24T21:02:40Z

Patch coverage only 17%, better do some unit tests...

hjoliver · 2018-11-25T21:33:53Z

OK, I added some unit tests, enough to get the patch coverage up to 76%. Would rather not spend more time on this now, before the workshop!

sadielbartholomew

Final re-approve: sanity checked, unit test is sensible & the remaining issue:

one remaining issue ... the default Cylc colour theme which hardcodes black text even for unfilled nodes which can result in black text on a black background.

is now fixed. For reference I get with the 'default' 'State Icon Theme':

"Hello darkness my old friend!"

matthewrmshin

Looks OK on my desktop (GNOME Shell 3.28.3 on RHEL7.6) - with normal, dark, high contract, and inverse high contrast themes. All look OK.

hjoliver added this to the soon milestone Nov 7, 2018

hjoliver self-assigned this Nov 7, 2018

hjoliver mentioned this pull request Nov 7, 2018

GTK GUIs: ugly in RHEL7 Gnome Shell metomi/rose#2249

Open

hjoliver commented Nov 7, 2018

View reviewed changes

lib/cylc/gui/updater_graph.py Show resolved Hide resolved

hjoliver requested review from kinow and sadielbartholomew November 8, 2018 21:06

hjoliver modified the milestones: soon, next release Nov 8, 2018

kinow approved these changes Nov 9, 2018

View reviewed changes

sadielbartholomew previously approved these changes Nov 9, 2018

View reviewed changes

cylc deleted a comment Nov 22, 2018

sadielbartholomew approved these changes Nov 23, 2018

View reviewed changes

lib/cylc/graphing.py Outdated Show resolved Hide resolved

hjoliver added 7 commits November 25, 2018 07:47

GUI: use GTK theme colors.

6792661

Theme bg threshold.

d2b7dfd

With desktop theme bg and fg.

b59f19e

Works for gcylc graph view and "cylc graph".

Tweaks and tidies.

c955c81

Fix graph tests.

948323c

Address a minor TODO.

021039c

Address feedback: default theme.

5c203f1

hjoliver force-pushed the gui-gtk-theme branch from 5cf44f4 to 5c203f1 Compare November 24, 2018 18:47

hjoliver added 3 commits November 26, 2018 07:46

Add unit tests for graphing module.

2b36b6c

Add missing test file.

a07c406

pep8

50c74b4

cylc deleted a comment Nov 25, 2018

hjoliver added 2 commits November 26, 2018 09:46

style, again

c995c71

style again_2

46dc393

cylc deleted a comment from codecov bot Nov 25, 2018

sadielbartholomew approved these changes Nov 26, 2018

View reviewed changes

matthewrmshin approved these changes Nov 26, 2018

View reviewed changes

matthewrmshin merged commit d5e8d13 into cylc:master Nov 26, 2018

hjoliver deleted the gui-gtk-theme branch December 12, 2018 00:58

sadielbartholomew mentioned this pull request Feb 21, 2019

cylc graph (readability): node labels always white #2965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUI: use GTK theme colors. #2842

GUI: use GTK theme colors. #2842

hjoliver commented Nov 7, 2018 •

edited

Loading

hjoliver commented Nov 7, 2018

hjoliver commented Nov 7, 2018

kinow commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018 •

edited

Loading

sadielbartholomew commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018

wxtim commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018 •

edited

Loading

sadielbartholomew commented Nov 7, 2018 •

edited

Loading

kinow commented Nov 7, 2018

hjoliver commented Nov 8, 2018 •

edited

Loading

sadielbartholomew commented Nov 8, 2018 •

edited

Loading

hjoliver commented Nov 8, 2018

hjoliver commented Nov 8, 2018

kinow commented Nov 9, 2018

sadielbartholomew commented Nov 9, 2018

sadielbartholomew left a comment •

edited

Loading

oliver-sanders commented Nov 12, 2018 •

edited

Loading

hjoliver commented Nov 13, 2018

matthewrmshin commented Nov 13, 2018

matthewrmshin commented Nov 13, 2018

hjoliver commented Nov 22, 2018 •

edited

Loading

sadielbartholomew commented Nov 23, 2018

oliver-sanders commented Nov 23, 2018

sadielbartholomew left a comment •

edited

Loading

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 25, 2018

sadielbartholomew left a comment

matthewrmshin left a comment

GUI: use GTK theme colors. #2842

GUI: use GTK theme colors. #2842

Conversation

hjoliver commented Nov 7, 2018 • edited Loading

hjoliver commented Nov 7, 2018

hjoliver commented Nov 7, 2018

kinow commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018 • edited Loading

sadielbartholomew commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018

wxtim commented Nov 7, 2018

oliver-sanders commented Nov 7, 2018 • edited Loading

sadielbartholomew commented Nov 7, 2018 • edited Loading

kinow commented Nov 7, 2018

hjoliver commented Nov 8, 2018 • edited Loading

sadielbartholomew commented Nov 8, 2018 • edited Loading

hjoliver commented Nov 8, 2018

hjoliver commented Nov 8, 2018

kinow commented Nov 9, 2018

sadielbartholomew commented Nov 9, 2018

sadielbartholomew left a comment • edited Loading

Choose a reason for hiding this comment

Summary:

Feedback & issue:

Illustrative cropped screenshots (all from the GUI launched via this PR branch):

oliver-sanders commented Nov 12, 2018 • edited Loading

hjoliver commented Nov 13, 2018

matthewrmshin commented Nov 13, 2018

matthewrmshin commented Nov 13, 2018

hjoliver commented Nov 22, 2018 • edited Loading

sadielbartholomew commented Nov 23, 2018

oliver-sanders commented Nov 23, 2018

sadielbartholomew left a comment • edited Loading

Choose a reason for hiding this comment

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 24, 2018

hjoliver commented Nov 25, 2018

sadielbartholomew left a comment

Choose a reason for hiding this comment

matthewrmshin left a comment

Choose a reason for hiding this comment

hjoliver commented Nov 7, 2018 •

edited

Loading

oliver-sanders commented Nov 7, 2018 •

edited

Loading

oliver-sanders commented Nov 7, 2018 •

edited

Loading

sadielbartholomew commented Nov 7, 2018 •

edited

Loading

hjoliver commented Nov 8, 2018 •

edited

Loading

sadielbartholomew commented Nov 8, 2018 •

edited

Loading

sadielbartholomew left a comment •

edited

Loading

oliver-sanders commented Nov 12, 2018 •

edited

Loading

hjoliver commented Nov 22, 2018 •

edited

Loading

sadielbartholomew left a comment •

edited

Loading