code-coverage-api-plugin
code-coverage-api-plugin copied to clipboard
Fix for #449 - Enhanced tree map visualization
In order to make the tree map easier to read, I changed the following:
- the tree map is shown on the whole screen now
- the border color saturation is alternating, following the echarts documentation, which says that an alternating high color saturation is a good option for distinguishing different sections.
Examples:
before:
after:
before:
after:
Codecov Report
Merging #455 (a57e861) into master (cad5c71) will increase coverage by
0.01%
. The diff coverage is80.83%
.
@@ Coverage Diff @@
## master #455 +/- ##
============================================
+ Coverage 72.78% 72.79% +0.01%
- Complexity 1014 1025 +11
============================================
Files 87 88 +1
Lines 3770 3816 +46
Branches 437 439 +2
============================================
+ Hits 2744 2778 +34
- Misses 878 891 +13
+ Partials 148 147 -1
Impacted Files | Coverage Δ | |
---|---|---|
...rage/model/visualization/colorization/ColorId.java | 100.00% <ø> (ø) |
|
...ins/plugins/coverage/model/CoverageTableModel.java | 9.16% <11.11%> (-0.15%) |
:arrow_down: |
...kins/plugins/coverage/model/CoverageViewModel.java | 57.65% <31.25%> (-4.48%) |
:arrow_down: |
...ugins/coverage/model/ChangeCoverageTableModel.java | 9.37% <33.33%> (ø) |
|
...s/coverage/model/IndirectCoverageChangesTable.java | 10.34% <33.33%> (ø) |
|
...odel/visualization/colorization/ColorProvider.java | 100.00% <100.00%> (ø) |
|
...sualization/colorization/ColorProviderFactory.java | 100.00% <100.00%> (ø) |
|
...isualization/colorization/CoverageChangeLevel.java | 100.00% <100.00%> (ø) |
|
...alization/colorization/CoverageColorJenkinsId.java | 100.00% <100.00%> (ø) |
|
...sualization/colorization/CoverageColorPalette.java | 100.00% <100.00%> (ø) |
|
... and 7 more |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
The problem with that approach is, that you then need to scroll on small displays. I found no way yet to always use the whole visible screen only for the trees. Maybe the problem is that the side panel is too large.
Okay, well I do not have a small screen and when I try it by resizing the window, it fills just the visible screen due to the resize event, but this might behave differently on a real small screen.
Nonetheless, I think in most cases h-100
is useful, even if a few users have to scroll.
I guess not that many developer use a small laptop screen all the time and even if it is the case, a bigger tree map allows that more file names are displays and the scrolling is limited.
What do you think about that?
In total I think the greens look better, but the reds not.
I totally agree - I tried some different approaches and found a better solution. Also, I recognized that the package colors are generated by the echarts API, but these should be set dependend on the coverage. Furthermore, the alpha of the currently used colors are problematic in the case of the map, so I removed these. Therefore, I locally changed some values including the echarts plugin and the result can be shown above - I updated the "after" examples. The new approach shows the coverage of each node exactly and different levels are clearly separable.
I am not available for two weeks now, so I would update the code afterwards - but maybe you can already give me some feedback on the new approach?
The problem with that approach is, that you then need to scroll on small displays. I found no way yet to always use the whole visible screen only for the trees. Maybe the problem is that the side panel is too large.
Okay, well I do not have a small screen and when I try it by resizing the window, it fills just the visible screen due to the resize event, but this might behave differently on a real small screen. Nonetheless, I think in most cases
h-100
is useful, even if a few users have to scroll. I guess not that many developer use a small laptop screen all the time and even if it is the case, a bigger tree map allows that more file names are displays and the scrolling is limited. What do you think about that?
I would rather prefer to create different views for small and large screens. This shouldn't be too hard with Bootstraps classes. See https://github.com/jenkinsci/code-coverage-api-plugin/blob/master/plugin/src/main/resources/coverage/coverage-table.jelly#L14 for an example
(See https://getbootstrap.com/docs/5.2/layout/grid/ for a documentation of all responsive classes)
In total I think the greens look better, but the reds not.
I totally agree - I tried some different approaches and found a better solution. Also, I recognized that the package colors are generated by the echarts API, but these should be set dependend on the coverage. Furthermore, the alpha of the currently used colors are problematic in the case of the map, so I removed these. Therefore, I locally changed some values including the echarts plugin and the result can be shown above - I updated the "after" examples. The new approach shows the coverage of each node exactly and different levels are clearly separable.
I am not available for two weeks now, so I would update the code afterwards - but maybe you can already give me some feedback on the new approach?
The new colors look better now. I wonder if we should use the red, orange and green colors of Jenkins as well here? https://weekly.ci.jenkins.io/design-library/Colors/
I would rather prefer to create different views for small and large screens.
So you mean we can add h-100
for large screens only in order to use the full screen here but also support small screens without h-100
?
I wonder if we should use the red, orange and green colors of Jenkins as well here?
Yes of course, so you mean to change the currently used colors in CoverageColorPalette
and insert the RGB codes of the Jenkins colors manually? Or is there a possibility to load the library colors dynamically as for example RGB values in Java?
I would rather prefer to create different views for small and large screens.
So you mean we can add
h-100
for large screens only in order to use the full screen here but also support small screens withouth-100
?
Yes, that shouldn't be too hard to implement.
I wonder if we should use the red, orange and green colors of Jenkins as well here?
Yes of course, so you mean to change the currently used colors in
CoverageColorPalette
and insert the RGB codes of the Jenkins colors manually? Or is there a possibility to load the library colors dynamically as for example RGB values in Java?
Getting them dynamically should work for the charts if we change the proxy methods:
So rather than using
viewProxy.getOverview(function (t) {
createOverview(t.responseObject(), 'coverage-overview');
});
we could use
viewProxy.getOverview(colors, function (t) {
createOverview(t.responseObject(), 'coverage-overview');
});
and fill colors
with a JSON object, that contains the important styles. Something like:
{
green: '#00ff00',
orange: '#00ff00',
red: '#00ff00'
}
(See https://weekly.ci.jenkins.io/design-library/Colors/ for the available colors).
You can get the colors then via:
function getTextColor() {
return getComputedStyle(document.body).getPropertyValue('--text-color') || '#333';
}
Then the colors automatically will be adapted to the selected theme.
Yes, that shouldn't be too hard to implement.
I agree, this looks like a good solution.
Getting them dynamically should work for the charts if we change the proxy methods:
Thanks for the explanation. As I understand this, the color JSON has to be set manually? In this case, I prefer using an Enum since this allows using these colors within the whole plugin and it is easier to integrate. More specific what I wanted to know is if for example this color JSON can be requested or if I can call a Java API to request these color hex codes within the Java code of the coverage plugin? I am not really familiar of how the design library works...
Yes, that shouldn't be too hard to implement.
I agree, this looks like a good solution.
Getting them dynamically should work for the charts if we change the proxy methods:
Thanks for the explanation. As I understand this, the color JSON has to be set manually? In this case, I prefer using an Enum since this allows using these colors within the whole plugin and it is easier to integrate. More specific what I wanted to know is if for example this color JSON can be requested or if I can call a Java API to request these color hex codes within the Java code of the coverage plugin? I am not really familiar of how the design library works...
It works the other was round: there is no way for the server to call the client (and there is no API on the client side). You need to wrap all information in an existing Ajax call from the client to the server:
In Jenkins processing is started on the client where the user selects the HTTP address. This is mapped on the server side into a jelly view. This jelly view is then rendered as HTML and send to the client that will render it. In this view we can invoke an associated JS file and call back to the server using Ajax. I.e., when the client currently starts a request to get the coverage data (the chart is empty in the jelly view) the client can provide an additional parameter (that is the reason that I am proposing a JSON object on the JS side, where we can put everything into what we want. I use the same thing in the trend chart configuration). This JSON can be interpreted on the server side to extract the colors. An enum will not work as it is constant on the server side, but actually each client might have its own colors. Maybe we can setup a call to discuss this part in more detail?
Yes, this would be perfect! I sent you an email.
@uhafner I finished the color rework. The Jenkins colors from the Design Library are now in use and for the table and the tree map dynamically loaded. The fallback colors correspond to the Jenkins colors aswell. Currently, some HSL colors are provided in the dark mode - these cases are not handled yet and the fallback is used. The size of the tree map is like before in order to work on all screen sizes.
Nice to hear and thanks for the review! Then, everything should be ready to be released. :)