Bug 341482

Comment 1 Pete Robbins 2011-03-31 09:39:49 EDT

Created attachment 192270 [details]
patch to make overview chart respond to system colour settings

Note this patches org.eclipse.mat.chart and org.eclipse.mat.chhart.ui

Comment 2 Pete Robbins 2011-03-31 09:40:55 EDT

ChartBuilder sets all backgrounds explicitly to white.

To fix this we can add the background and foreground colours to the ChartBuilder.create method and change the calls to this to pass in the desired colours. 

For the Overview pane this is derived from the current SWT settings.

For the report charts (via HtmlPieChartRender) we do not know where these are being viewed so the defult black on white can be used until we can figure out a way to render the charts using the current viewable settings.

The attached patch:
1) updates the ChartBuilder.create method to accept the background/foreground colours.
2) Chartbuilder.create uses the passed in colours rather than hard-coded white
3) PieChartPane is updated to pass in the current SWT background/foreground colours to Chartbuilder.create
4) HtmlPieChartRendere is updated to pass in the default black/white
5) LabelRenderScript adn StorageUnitRenderScript are updated to remove the hardcoded setting of background to white.

This patch allows the overview pane to respond to the users choice of colours.

NOTE: This only takes effect when the user opens the Overview AFTER having set high-contrast mode, ie it does not currently detect the change and re-render the charts.

Comment 3 Krum Tsvetkov 2011-04-08 08:58:49 EDT

Thank you for the patch!
I have just submitted the patch from bug 341498. After this, the "Apply Patch" tool has problems with the PieChartPane changes.
Could you please "merge" the latest changes and attach again the patch.

Comment 4 Pete Robbins 2011-04-08 09:10:42 EDT

Created attachment 192825 [details]
merged patch

Here is a merged patch vs revision 1096

I confirmn this patch is 100% written by myself, I am authorized to contribute it and it is submitted under the epl

Comment 5 Pete Robbins 2011-04-08 09:14:45 EDT

apologies ... that is the wrong patch please discard

Comment 6 Pete Robbins 2011-04-08 09:15:53 EDT

Created attachment 192826 [details]
mat.chart patch vs revision 1096

Comment 7 Krum Tsvetkov 2011-04-08 09:31:26 EDT

Hi Pete,

the first patch had also some changes to org/eclipse/mat/ui/internal/chart/PieChartPane.java, and the last one deson't have them.
Is this fine?

Comment 8 Pete Robbins 2011-04-08 09:46:22 EDT

Created attachment 192831 [details]
PieChartPane patch

I'm clearly not having a good day!

Here is the missing patch to PieChartPane.

As before I confirm these are written by myyself, I have permission to contribute the code and they are submitted under EPL.

Comment 9 Krum Tsvetkov 2011-04-12 07:12:23 EDT

Thanks for the reworked patch! I finally found some time to play with it and it works very good.
I have one question (more for my personal understanding) - in ChartBuilder on lines 110, 119, 121, 123 you set the background to 
ColorDefinitionImpl.create(background.getRed(), background.getBlue(), background.getRed()), i.e. R,B,R from background.
What does this lead to? My naive thinking is that it should be R,G,B. 

BTW I found it very interesting what one sees while running in high contrast mode (I had never tried this before). I figured out some further (hopefully minor) problems, e.g. in List Objects we add a decorator to the icon, which is a small arrow indicating the direction of the reference. This arrow is black and is invisible in high contrast mode, unless the line is selected... I'll file a separate bug for this and see if I can fix it. 

Please just give me your comment about the question above. I'm going to submit the patch.

Comment 10 Pete Robbins 2011-04-12 09:17:22 EDT

(In reply to comment #9)
> Thanks for the reworked patch! I finally found some time to play with it and it
> works very good.
> I have one question (more for my personal understanding) - in ChartBuilder on
> lines 110, 119, 121, 123 you set the background to 
> ColorDefinitionImpl.create(background.getRed(), background.getBlue(),
> background.getRed()), i.e. R,B,R from background.
> What does this lead to? My naive thinking is that it should be R,G,B. 
> 

Your naive thinking is correct. It should be RGB. I blame copy&paste and auto-complete! Please change the code for the last paramater to be background.getBlue(). In the tests of high contrast black and white are the same for each RGB value, ie 255,255,255 or 0,0,0 which is why I didn't spot this in testing.

Cheers,

Comment 11 Krum Tsvetkov 2011-04-13 02:52:58 EDT

I did the modifications from your last comment and submitted the patch.
I adjusted the copyright headers in the files and set the iplog flag for the last two patches.
I am closing the message now. You could probably re-test with the latest state from the repostitory if the probem is solved now. In case of further problems just reopen the bug.
Thanks for doing the accessibility testing and sending the patches!