code-coverage-api-plugin icon indicating copy to clipboard operation
code-coverage-api-plugin copied to clipboard

Fix for #449 - Enhanced tree map visualization

Open fo-code opened this issue 2 years ago • 3 comments

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: example1_before after: example_mixed

before: example2_before after: example_green

fo-code avatar Aug 21 '22 18:08 fo-code

Codecov Report

Merging #455 (a57e861) into master (cad5c71) will increase coverage by 0.01%. The diff coverage is 80.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

codecov[bot] avatar Aug 21 '22 18:08 codecov[bot]

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?

fo-code avatar Aug 26 '22 13:08 fo-code

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/

uhafner avatar Aug 28 '22 16:08 uhafner

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?

fo-code avatar Sep 11 '22 14:09 fo-code

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?

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.

uhafner avatar Sep 11 '22 22:09 uhafner

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...

fo-code avatar Sep 12 '22 11:09 fo-code

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?

uhafner avatar Sep 12 '22 20:09 uhafner

Yes, this would be perfect! I sent you an email.

fo-code avatar Sep 13 '22 08:09 fo-code

@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.

fo-code avatar Sep 30 '22 14:09 fo-code

Nice to hear and thanks for the review! Then, everything should be ready to be released. :)

fo-code avatar Oct 03 '22 07:10 fo-code