Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfe6252
JENKINS-8958 Code coverage color scheme not suited for color blind users
cizezsy Mar 8, 2018
c0d3644
JENKINS-8958 Code coverage color scheme not suited for color blind users
cizezsy Mar 11, 2018
76846bd
remove useless constants
cizezsy Mar 17, 2018
5ee28ba
fix documentation comment tag error
cizezsy Mar 17, 2018
63ba458
make graph of Trend more user friendly, color blind mode now include …
cizezsy Mar 17, 2018
7a93266
unfold import statement
cizezsy Mar 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion src/main/java/hudson/plugins/cobertura/CoberturaPublisher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,8 @@ public FilePath[] invoke(File f, VirtualChannel channel) throws IOException, Int
@Symbol("cobertura")
public static final class DescriptorImpl extends BuildStepDescriptor<Publisher> {

private boolean colorBlindMode;

CoverageMetric[] metrics = {
CoverageMetric.PACKAGES,
CoverageMetric.FILES,
Expand All @@ -889,6 +891,23 @@ public static final class DescriptorImpl extends BuildStepDescriptor<Publisher>
CoverageMetric.LINE,
CoverageMetric.CONDITIONAL,};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jeffpearce jeffpearce Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

screenshot 2018-03-15 07 36 24

super(clazz);
load();
}

/**
* load global config from disk
*/
public DescriptorImpl() {
super();
load();
}


/**
* This human readable name is used in the configuration screen.
*/
Expand Down Expand Up @@ -930,7 +949,7 @@ public List<CoberturaPublisherTarget> getTargets(CoberturaPublisher instance) {
*/
@Override
public boolean configure(StaplerRequest req, JSONObject formData) throws FormException {
req.bindParameters(this, "cobertura.");
req.bindJSON(this, formData);
save();
return super.configure(req, formData);
}
Expand Down Expand Up @@ -964,6 +983,25 @@ public CoberturaPublisher newInstance(StaplerRequest req, JSONObject formData) t
public boolean isApplicable(Class<? extends AbstractProject> jobType) {
return true;
}

/**
* Setter for property 'colorBlindMode'.
*
* @param colorBlindMode Value to set for property 'colorBlindMode'.
*/
@DataBoundSetter
public void setColorBlindMode(boolean colorBlindMode) {
this.colorBlindMode = colorBlindMode;
}

/**
* Getter for property 'colorBlindMode'.
*
* @return Value for property 'colorBlindMode'.
*/
public boolean getColorBlindMode() {
return colorBlindMode;
}
}

private static List<CoberturaPublisherTarget> bindTargetsFromForm(JSONObject formData) {
Expand Down
30 changes: 21 additions & 9 deletions src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
package hudson.plugins.cobertura.targets;

import hudson.model.AbstractBuild;
import hudson.model.Api;
import hudson.model.Item;
import hudson.model.Run;
import hudson.plugins.cobertura.BuildUtils;
import hudson.plugins.cobertura.Chartable;
import hudson.plugins.cobertura.CoberturaBuildAction;
import hudson.plugins.cobertura.CoverageChart;
import hudson.plugins.cobertura.Ratio;
import hudson.model.*;
import hudson.plugins.cobertura.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import hudson.util.Graph;
import hudson.util.TextFile;

Expand All @@ -29,6 +22,7 @@
import java.util.TreeMap;
import java.util.TreeSet;

import jenkins.model.Jenkins;
import org.jfree.chart.JFreeChart;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
Expand All @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Generated
*/
Expand Down Expand Up @@ -504,4 +501,19 @@ public Map<String, CoveragePaint> getPaintedSources() {
public Api getApi() {
return new Api(this);
}

/**
*
* @return css file name according to the config of color blind mode
*/
public String getCssFileName() {
Descriptor descriptor = Jenkins.getInstance().getDescriptor(CoberturaPublisher.class);
if(descriptor instanceof CoberturaPublisher.DescriptorImpl) {
CoberturaPublisher.DescriptorImpl d = (CoberturaPublisher.DescriptorImpl) descriptor;
if(d.getColorBlindMode()) {
return COLOR_BLIND_CSS_FILE;
}
}
return DEFAULT_CSS_FILE;
}
}
10 changes: 10 additions & 0 deletions src/main/resources/hudson/plugins/cobertura/CoberturaPublisher/global.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">

<f:section title="${%Cobertura}">
<f:entry title="${%Color Blind Mode}" field="cobertura.colorBlindMode">
<f:checkbox checked="${instance.colorBlindMode}"/>
</f:entry>
</f:section>

</j:jelly>
2 changes: 1 addition & 1 deletion src/main/resources/hudson/plugins/cobertura/targets/CoverageResult/index.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<l:layout xmlns:cobertura="/hudson/plugins/cobertura/tags" css="/plugin/cobertura/css/style.css" norefresh="true">
<l:layout xmlns:cobertura="/hudson/plugins/cobertura/tags" css="/plugin/cobertura/css/${it.getCssFileName()}" norefresh="true">
<st:include it="${it.owner}" page="sidepanel.jelly"/>
<l:main-panel>
<h1>${%Code Coverage}</h1>
Expand Down
123 changes: 123 additions & 0 deletions src/main/webapp/css/style-color-blind.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
table.source {
Copy link
Contributor

@jeffpearce jeffpearce Mar 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

border-style: solid;
border-color: #bbb;
border-spacing: 0;
border-collapse: collapse;
border-width: 1px;
width: 100%;
}

table.source td {
border-style: solid;
border-color: #bbb;
border-spacing: 0;
border-collapse: collapse;
border-width: 0px 0px 0px 0px;
padding-left: 0.2em;
padding-right: 0.2em;
font-family: monospace;
}

table.source .text {
margin-top: -1px;
}

table.source td.line {
text-align: right;
border-width: 0px 1px 0px 0px;
}

table.source td.hits {
text-align: right;
border-width: 0px 1px 0px 0px;
}

table.source th {
padding-left: 0.5em;
font-weight: bold;
font-family: serif;
background-color: #f0f0f0;
border-width: 1px 1px 1px 1px;
text-align: left;
}

table.source tr.coverFull {
background-color: #dfd;
}

table.source tr.coverPart {
background-color: #ffd;
}

table.source tr.coverPart td.hits, table.source tr.coverNone td.hits {
font-weight: bold;
}

table.source tr.coverNone {
background-color: #fdd;
}

table.source tr.noCover {
background-color: #fff;
}

div.percentgraph {
background-color: #f0e442;
border: #808080 1px solid;
height: 1.3em;
margin: 0px;
padding: 0px;
width: 100px;
}

div.percentgraph div.greenbar {
background-color: #0072b2;
height: 1.3em;
margin: 0px;
padding: 0px;
}

div.percentgraph div.na {
background-color: #eaeaea;
height: 1.3em;
margin: 0px;
padding: 0px;
}

div.percentgraph span.text {
display: block;
text-align: center;
position: absolute;
width: 100px;
color: #000
}

table.percentgraph {
border: 0px;
font-size: 130%;
margin: 0px;
margin-left: 0px;
margin-right: 0px;
padding: 0px;
text-align: left;
}

table.percentgraph tr.percentgraph {
border: 0px;
margin: 0px;
padding: 0px;
}

table.percentgraph td.percentgraph {
border: 0px;
margin: 0px;
padding: 0px;
padding-left: 4px;
}

table.percentgraph td.data {
text-align: right;
}
.center {
text-align: center;
}