Bug 510228

Comment 1 Lars Vogel 2017-01-11 01:45:33 EST

Ralf, you worked recently in forms. Could you investigate?

Comment 2 Ralf Petter 2017-01-11 02:38:50 EST

Ok i will try to reproduce the problem in my installation and then fix it.

Comment 3 Lars Vogel 2017-01-11 02:57:43 EST

(In reply to Ralf Petter from comment #2)
> Ok i will try to reproduce the problem in my installation and then fix it.

To reproduce:

1.) Open the Git staging view and the MANIFEST.MF editor
2.) Go to preferences and switch in Appearance to the Dark theme

-> view and editor look strange. Resizing the window fixes part of that.

Comment 4 Ralf Petter 2017-01-11 11:29:26 EST

The problem is not the setBackground method, but the setTitleBarBackground and setTitelBarBorderColor methods. This two methods set only the colors in a Hashmap titleColors, but the do not trigger a recreation of the backgroundimage of the section header. 

In the constructor of the section is a resize listener added to the section control which triggers an update of the backgroundimage. That's the reason why the dark theme colors are applied on resize. 

The easiest solution will be to extract the update code from the listener to a private method and then call this code from the listener and from the setters of titlebarbackground and titlebarbordercolor. @Lars if you agree, i can build and upload a patch for this.

Some performance considerations:

In my opinion it is a performance problem, smilar to the one i fixed in FormHeading that the backgroundimage is recreated many times during a resize operation. In my opinion the update of the image is not needed during a resize operation. I will investigate and file another bug for this problem.

The CCSEngine updates also the setters very often so i think the controls should have a mechanism to detect whether the same values are already set and if yes the should return without an update to improve performance. I think this is the main reason why the CSSEngine slows down many things in the ui from eclipse. Maybe another solution will be to enable the CCSEngine to query a value before the value is set to decide if a change is needed? But i think the better way is to handle this on the widget level, because the widget knows best which setters are expensive to update and which are not.

Comment 5 Lars Vogel 2017-01-11 12:19:42 EST

(In reply to Ralf Petter from comment #4)
> @Lars if you agree, i
> can build and upload a patch for this.

+1

> Some performance considerations:
> 

> But i think the better way is to handle this on the widget level, because
> the widget knows best which setters are expensive to update and which are
> not.

+1. I think if the new value is identical to the old value, the setter can simply return and avoid executing additional computation.

Comment 6 Ralf Petter 2017-01-20 03:52:57 EST

Moved to M6. I have already a working patch, but the patch needs more testing. And there is not much time left before m5 will be released. I will push a WIP version of the patch to Gerrit today in the afternoon, so that anyone interested can test my patch.

Comment 7 Ralf Petter 2017-01-22 12:31:46 EST

Created attachment 266398 [details]
Shows screenshots after theme switching before and after the fix.

I have uploaded a first version woriking version to fix this bug. I have added a new JUnit Testclass for Section which i have cloned from ExpandableCompositeTest. Then i have added two new tests to check for the correct behaviour. Please be aware that the testBackgroundImageCaching test will only be succesfull when the fix from Bug 510826 has been applied. I am open for suggestions how to improve this fix, because i think that some things in the fix feels a little bit like a hack.

Comment 8 Ralf Petter 2017-01-22 12:35:31 EST

New Gerrit change created: https://git.eclipse.org/r/89296/

Comment 9 Lars Vogel 2017-03-03 03:49:16 EST

Mass move. Please move back to M6, if necessary

Comment 10 Lars Vogel 2020-05-19 05:50:25 EDT

Marcus, can you take over?