JENKINS-8958 Code coverage color scheme not suited for color blind users #95

cizezsy · 2018-03-08T11:45:25Z

Add global config which can enable colorblind mode.

I use a Chrome extension which simulates the website as a color vision impaired person would see. And the following is the results.
Normal see it:

Red-blind see it:

Green-blind see it:

Blue-blind see it:

jeffpearce · 2018-03-10T13:41:53Z

src/main/webapp/css/style-color-blind.css

@@ -0,0 +1,123 @@
+table.source {


My concern with duplicating the entire css file is that anyone wanting to change the layout would have to change in two places (and might miss this one). Looking at the layout control https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/layout/layout.jelly#L46-L49 I see the css parameter is deprecated.

This suggests a slightly different approach, which would be to move the css path into a <script> tag inside of an <l:style> control. Then we could provide a color-blind css file that has just the overrides (presumably the colors). For regular mode, just import the base stylesheet, and for color-blind mode import the base stylesheet, then import the color-blind stylesheet. This has the advantage that we could easily provide more than one color-blind mode in the future should we want to.

Oh, You are right. Thanks for your advice :)

jeffpearce · 2018-03-14T14:50:51Z

src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java

@@ -47,6 +41,9 @@
 @ExportedBean(defaultVisibility = 2)
 public class CoverageResult implements Serializable, Chartable {

+    private static final String DEFAULT_CSS_FILE = "style.css";
+    private static final String COLOR_BLIND_CSS_FILE = "style-color-blind.css";


I don't think these two constants are being used anymore

oleg-nenashev

Just minor comments

oleg-nenashev · 2018-03-14T20:13:31Z

src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java

+     *
+     * @return whether enable color blind mode in global config
+     */
+    public boolean isEnableColorBlindMode() {


Nice2 have: @since TODO for new APIs

oleg-nenashev · 2018-03-14T20:14:31Z

src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java

-import hudson.plugins.cobertura.CoverageChart;
-import hudson.plugins.cobertura.Ratio;
+import hudson.model.*;
+import hudson.plugins.cobertura.*;


It is generally preferable to avoid it in the production code, because there is a risk of ambiguity if one of the packages introduces a competing class.

oleg-nenashev · 2018-03-14T20:15:15Z

src/main/java/hudson/plugins/cobertura/CoberturaPublisher.java

+        /**
+         * load global config from disk
+         */
+        public DescriptorImpl(Class<? extends Publisher> clazz) {


Is it actually used anywhere?

The comment of Descriptor said Descriptor can persist data just by storing them in fields. However, it is the responsibility of the derived type to properly invoke save() and load().
So I call save() in configure() to persist global configuration. And the comment of load() said The constructor of the derived class must call this method. (If we do that in the base class, the derived class won't get a chance to set default values.). So I follow the comment to implemented its two constructors and call load method.

It provides a config setting on the Jenkins global settings page:

oleg-nenashev · 2018-03-15T23:05:42Z

Ideally it would make sense to have a Jenkins-wide colorblind mode. There were some discussions about that in PRs, but we do not have API for that so far. OTOH it could be an intetesting project idea, e.g. for GSoC next year

…

On Mar 15, 2018 15:38, "Jeff Pearce" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/hudson/plugins/cobertura/CoberturaPublisher.java <#95 (comment)> : > @@ -889,6 +891,23 @@ public ParseReportCallable(String reportFilePath) { CoverageMetric.LINE, CoverageMetric.CONDITIONAL,}; + /** + * load global config from disk + */ + public DescriptorImpl(Class<? extends Publisher> clazz) { It provides a config setting on the plugin page: [image: screenshot 2018-03-15 07 36 24] <https://user-images.githubusercontent.com/1479145/37470052-e2d9b21a-2823-11e8-9249-77325ac490e6.png> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoP_UW97p-sXksmKCVwgI00-HNVoXks5tenz-gaJpZM4SijvE> .

jeffpearce · 2018-03-15T23:07:48Z

That was my initial thought as well. I could see doing the work in Cobertura now, and converting it to use the global setting later. Do you think that's worthwhile, or should we close this one?

jeffpearce · 2018-03-15T23:08:35Z

I guess the global config might have specific colors though, so it's not clear it would be a 1:1 translation to this code

steven-foster · 2018-03-17T01:06:52Z

Maybe a User Property instead of global? or both

jeffpearce · 2018-03-17T01:50:18Z

@steven-foster can you elaborate? I'm not sure what the difference is between User and global property

cizezsy · 2018-03-17T02:09:30Z

@jeffpearce Should I extended color-blind mode more than just in coverage bar but also in trend image and so on?

jeffpearce · 2018-03-17T02:11:07Z

That makes sense to me

cizezsy · 2018-03-17T02:15:43Z

OK, the trend image and source line mark are also unsuitable for color vision impaired person. I think I should change them all.

jeffpearce · 2018-03-17T02:16:21Z

👍 Thanks again for the hard work on this plugin

cizezsy · 2018-03-17T07:04:31Z

@jeffpearce I add more diversity to the origin chart lines. So not just for the color-blind people but for normal people can differentiate it easier.

Origin:

Now
Normal see it:

Red-blind see it:

Green-blind see it:

Blue-blind see it:

what do you think of it?

…source code mark

steven-foster · 2018-03-17T12:08:48Z

@jeffpearce a UserProperty would allow each user to configure it for themselves, rather than a global configuration which would change it for everybody using the Jenkins instance. It's just a suggestion, there might be a reason not to do it this way :)

jeffpearce · 2018-04-10T17:22:04Z

@steven-foster - that would be good. I could see this as a next step, and UserProperty down the road, so it could be configured per-user

jeffpearce · 2018-04-10T17:23:06Z

@cizezsy - I'm sorry, this fell off my radar. Can you remind me where we're at? Is this ready for another round of review?

cizezsy · 2018-04-13T02:28:04Z

@jeffpearce My fault, I do not say it clearly. I have changed the line shape of trend image so that it will be more friendly to the color-blind user. But, the original image is not clear also for normal users. So I think we should not add this change just for colorblind mode but for all users. Maybe I should start another pull request like "improve trend image" to do this thing?

jeffpearce · 2022-05-17T20:36:01Z

@cizezsy - do you think this PR is still relevant?

cizezsy · 2022-05-18T05:56:59Z

@jeffpearce I think this PR can be closed...

JENKINS-8958 Code coverage color scheme not suited for color blind users

cfe6252

jeffpearce reviewed Mar 10, 2018

View reviewed changes

JENKINS-8958 Code coverage color scheme not suited for color blind users

c0d3644

jeffpearce reviewed Mar 14, 2018

View reviewed changes

oleg-nenashev reviewed Mar 14, 2018

View reviewed changes

remove useless constants

76846bd

fix documentation comment tag error

5ee28ba

cizezsy added 2 commits March 17, 2018 15:29

make graph of Trend more user friendly, color blind mode now include …

63ba458

…source code mark

unfold import statement

7a93266

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-8958 Code coverage color scheme not suited for color blind users #95

JENKINS-8958 Code coverage color scheme not suited for color blind users #95

cizezsy commented Mar 8, 2018 •

edited

Loading

jeffpearce Mar 10, 2018 •

edited

Loading

cizezsy Mar 11, 2018

jeffpearce Mar 14, 2018

oleg-nenashev left a comment

oleg-nenashev Mar 14, 2018

oleg-nenashev Mar 14, 2018

oleg-nenashev Mar 14, 2018

cizezsy Mar 15, 2018

jeffpearce Mar 15, 2018 •

edited

Loading

oleg-nenashev commented Mar 15, 2018 via email

jeffpearce commented Mar 15, 2018

jeffpearce commented Mar 15, 2018

steven-foster commented Mar 17, 2018

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018 •

edited

Loading

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018

steven-foster commented Mar 17, 2018

jeffpearce commented Apr 10, 2018

jeffpearce commented Apr 10, 2018

cizezsy commented Apr 13, 2018 •

edited

Loading

jeffpearce commented May 17, 2022

cizezsy commented May 18, 2022

JENKINS-8958 Code coverage color scheme not suited for color blind users #95

Are you sure you want to change the base?

JENKINS-8958 Code coverage color scheme not suited for color blind users #95

Conversation

cizezsy commented Mar 8, 2018 • edited Loading

jeffpearce Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

cizezsy Mar 11, 2018

Choose a reason for hiding this comment

jeffpearce Mar 14, 2018

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Mar 14, 2018

Choose a reason for hiding this comment

oleg-nenashev Mar 14, 2018

Choose a reason for hiding this comment

oleg-nenashev Mar 14, 2018

Choose a reason for hiding this comment

cizezsy Mar 15, 2018

Choose a reason for hiding this comment

jeffpearce Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

oleg-nenashev commented Mar 15, 2018 via email

jeffpearce commented Mar 15, 2018

jeffpearce commented Mar 15, 2018

steven-foster commented Mar 17, 2018

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018 • edited Loading

jeffpearce commented Mar 17, 2018

cizezsy commented Mar 17, 2018

steven-foster commented Mar 17, 2018

jeffpearce commented Apr 10, 2018

jeffpearce commented Apr 10, 2018

cizezsy commented Apr 13, 2018 • edited Loading

jeffpearce commented May 17, 2022

cizezsy commented May 18, 2022

cizezsy commented Mar 8, 2018 •

edited

Loading

jeffpearce Mar 10, 2018 •

edited

Loading

jeffpearce Mar 15, 2018 •

edited

Loading

cizezsy commented Mar 17, 2018 •

edited

Loading

cizezsy commented Apr 13, 2018 •

edited

Loading