Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 423813

Summary:

[CSS] Improvement of the CSS theme switching and applying the new stylesheet in runtime

Product:

[Eclipse Project] Platform

Reporter:

Daniel Rolka <daniel.rolka>

Component:

Assignee:

Daniel Rolka <daniel.rolka>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

daniel_megert, kolio_n, markus.kell.r, pelder.eclipse, pwebster

Version:

4.4

Target Milestone:

4.4 M5

Hardware:

OS:

Windows 7

Whiteboard:

Bug Depends on:

Bug Blocks:

424037, 426777

Attachments:

Description	Flags
Picture of error when switching theme	none

Description Daniel Rolka

2013-12-11 09:03:40 EST

Currently we switch the CSS theme for Workbench with the 'Window->Preferences->Appearance' preference page and after applying the new stylesheet we have to restart the Eclipse instance to get full effect.

At the first glance, I think, we are able to apply the new CSS stylesheet in fly, without restarting the Eclipse. However it requires some work:

First of all we have to slightly change the current approach that we use in the CSS engine when some style definition is not present in the CSS stylesheet or we reapply the style for Widget. In short, at this moment we get the styled widget's resources (Font, Color, Image or Cursor) and enhance it with the style definition.
Since after switching the CSS stylesheet we have to repopulate the CSS cache and dispose unused resources to avoid memory leaks then in some cases we can have Widgets with disposed resources and we will get the SWT.ERROR_GRAPHIC_DISPOSED exceptions (for instance execution of the Font.getFontData() or Image.getBounds() throws such exception when resource is disposed).
So to solve this issue we have to define some global defaults for the SWT resources and use it when particular CSS stylesheet doesn't have proper style or styled Widget contains disposed resources. We can modify the e4_basestyle.css file with it, but I think it would be better to have it somehow hardcoded in the Eclipse (I believe each Web browser follows this direction)

Second item that I've noticed is the GUI refreshing issue, but we should be able to fix it.

Does it make sense for you? If you see other issues that I'm not aware of please add proper comment to the bug.

If you think that it is a 'nice to have' thing, but we shouldn't waste the time for it, just mark the bug as invalid.

thanks,
Daniel

Comment 1 Daniel Rolka

2013-12-13 07:32:14 EST

Regarding the global defaults mentioned in the previous comment - we don't have to define any defaults. We can create it in the runtime before first styling of the Widgets based on the initial Widget resources (extra copy of resources that will be disposed during closing the Eclipse) and use it as the base. So when particular style definition is not present or Widget contains disposed resources then we will use the base instead of. 
Such approach doesn't introduce any regression in the CSS engine behavior and solves the basic issue that we have now with disposing the unused resources.

Daniel

Comment 2 Daniel Rolka

2013-12-13 07:42:01 EST

Just to make sure we are on the same page - extra copy of resources will contain more or less 4 items (font, font color, background color, cursor)

Daniel

Comment 3 Daniel Rolka

2013-12-13 08:15:47 EST

(In reply to Daniel Rolka from comment #2)
> Just to make sure we are on the same page - extra copy of resources will
> contain more or less 4 items (font, font color, background color, cursor)
> 
> Daniel

Let's just take the OS defaults as the resource base (Device.getSystemColor/Device.getSystemFont) and update it with the provided style definition

Daniel

Comment 4 Paul Webster

2013-12-20 17:02:09 EST

OK, some comments regarding resource usage by the CSS engine and 3.x themes.

- The CSS engine uses org.eclipse.e4.ui.css.swt.resources.SWTResourcesRegistry to keep track of SWT resources. Because of how the engine works (applying rules one widget at a time) it expects SWT resources to live as long as the engine itself

- The 3.x theme API feeds RGB and FontData into the JFace ColorRegistry and FontRegistry. Changing a name->RGB or name->FontData sends out a property change event.

- The 3.x theme API don't expect the fonts or colors to go away for the lifecycle of the workbench theme manager.

- in CSS the CSS engine uses org.eclipse.e4.ui.css.core.resources.CSSResourcesHelpers.getCSSFontPropertiesKey(CSS2FontProperties) to create a font property key out of the various CSS attributes like font-family, size, style, weight, etc.

- org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine.convert(CSSValue, Object, Object) has to take the CSS key and return a real Font or Color.

- FontDescriptor and ColorDescriptor can be used to create ref-counted fonts using LocalResourceManager backed by JFaceResources.getResources().

- unless we can somehow match the descriptor with the widget, we can't destroy it before setting a new one.

- maybe plugging in just a FontRegistry/ColorRegistry (String->FontData, String->RGB) would be enough to prevent Fonts from being leaked/us getting Widget is disposed exceptions.

- Another option is translating keys into FontDescriptors, and using resource manager.create/destroy to get each Font/Color. They'll be refcounted that way, but we need to keep a map of widgets->*Descriptor so we can destroy them through the resource manager.

Comment 5 Daniel Rolka

2014-01-08 06:51:44 EST

The patch proposal has been pushed to Gerrit - https://git.eclipse.org/r/#/c/20372/

We still need to restart the Workbench to get full effect of switching the CSS, but currently we can freely dispose unused SWT resources (and the user won't see the memory usage pick after switching the CSS theme)

Daniel

Comment 6 Paul Elder

2014-01-10 13:55:05 EST

(In reply to Daniel Rolka from comment #5)
> The patch proposal has been pushed to Gerrit -
> https://git.eclipse.org/r/#/c/20372/
> 
> We still need to restart the Workbench to get full effect of switching the
> CSS, but currently we can freely dispose unused SWT resources (and the user
> won't see the memory usage pick after switching the CSS theme)
> 
> Daniel

Patch generally look OK - I could not find any obvious resource leaks! 

However, the code in org.eclipse.e4.ui.css.swt.dom.CTabFolderElement.reset() should use a pattern other than reflection in order to reset CTabRendering instances. I suggest defining an appropriate interface in org.eclipse.e4.ui.css.swt, and have CTabRendering implement it.

Comment 7 Paul Elder

2014-01-10 15:08:51 EST

Daniel. I'm seeing the following NPE when I switch the theme back to the Default Theme. Looks like ControlElement.reset() may need to be re-examined.

java.lang.NullPointerException
	at org.eclipse.swt.ole.win32.OleControlSite.setBackground(OleControlSite.java:906)
	at org.eclipse.e4.ui.css.swt.dom.ControlElement.reset(ControlElement.java:146)
	at org.eclipse.e4.ui.css.swt.engine.AbstractCSSSWTEngineImpl.reset(AbstractCSSSWTEngineImpl.java:91)
	at org.eclipse.e4.ui.css.swt.internal.theme.ThemeEngine.setTheme(ThemeEngine.java:401)
	at org.eclipse.e4.ui.css.swt.internal.theme.ThemeEngine.setTheme(ThemeEngine.java:382)
	at org.eclipse.ui.internal.dialogs.ViewsPreferencePage$2.selectionChanged(ViewsPreferencePage.java:98)
	at org.eclipse.jface.viewers.Viewer$2.run(Viewer.java:164)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:49)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:176)
	at org.eclipse.jface.viewers.Viewer.fireSelectionChanged(Viewer.java:162)
	at org.eclipse.jface.viewers.StructuredViewer.updateSelection(StructuredViewer.java:2201)
	at org.eclipse.jface.viewers.StructuredViewer.handleSelect(StructuredViewer.java:1218)
	at org.eclipse.jface.viewers.StructuredViewer$4.widgetSelected(StructuredViewer.java:1249)
	at org.eclipse.jface.util.OpenStrategy.fireSelectionEvent(OpenStrategy.java:242)
	at org.eclipse.jface.util.OpenStrategy.access$4(OpenStrategy.java:236)
	at org.eclipse.jface.util.OpenStrategy$1.handleEvent(OpenStrategy.java:406)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4351)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1061)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4170)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3759)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:827)
	at org.eclipse.jface.window.Window.open(Window.java:803)
	at org.eclipse.ui.internal.dialogs.WorkbenchPreferenceDialog.open(WorkbenchPreferenceDialog.java:215)
	at org.eclipse.ui.internal.OpenPreferencesAction.run(OpenPreferencesAction.java:65)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:499)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:588)
	at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:505)
	at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:415)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4351)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1061)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4170)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3759)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run(PartRenderingEngine.java:1122)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1006)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:146)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:612)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:565)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:125)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:80)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:372)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:226)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:88)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:55)
	at java.lang.reflect.Method.invoke(Method.java:613)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:636)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:591)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1450)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1426)

Comment 8 Daniel Rolka

2014-01-13 06:19:52 EST

(In reply to Paul Elder from comment #7)
> Daniel. I'm seeing the following NPE when I switch the theme back to the
> Default Theme. Looks like ControlElement.reset() may need to be re-examined.
> 
> java.lang.NullPointerException
> 	at
> org.eclipse.swt.ole.win32.OleControlSite.setBackground(OleControlSite.java:
> 906)
> 	at

OK, I was able to recreate the issue. It is connected to the org.eclipse.swt.browser.WebSite widget that is the Win 32 specific one. The widget extends the OleClientSite class that handles improperly the null color value (lines with: new Variant(color.handle)). The best place to handle the issue would be the SWT component, however I will try to bypass it on our site to unblock the patch.

Daniel

Comment 9 Paul Elder

2014-01-13 10:48:35 EST

Change https://git.eclipse.org/r/#/c/20372/

released to master as:

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=946c6d54de1d199bb882124673f4ede122a00f89

Comment 10 Dani Megert

2014-01-15 07:12:24 EST

(In reply to Paul Elder from comment #9)
> Change https://git.eclipse.org/r/#/c/20372/
> 
> released to master as:
> 
> https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=946c6d54de1d199bb882124673f4ede122a00f89

This seems to destroy/loose the Eclipse application icon:

1. install N20140114-2000
2. start new workspace
3. open Window > Preferences > General > Appearance
4. change the theme to 'Windows 7 Classic'
==> Eclipse application (top, left) is replaced by some other icon

NOTE: Restarting Eclipse fixes the problem.


And there's another issues which might be related to this fix or to the fix from bug 423704: with the above steps I sometimes (more often when launching a target out of my development workspace) get an error dialog saying 'Argument not valid' (see attached screenshot) and the following exception:

java version "1.7.0_07"
Java(TM) SE Runtime Environment (build 1.7.0_07-b10)
Java HotSpot(TM) Client VM (build 23.3-b01, mixed mode)

Configuration location:
    file:/C:/eclipse/workspaces/HEAD/.metadata/.plugins/org.eclipse.pde.core/eoe (Sun)/
Configuration file:
    file:/C:/eclipse/workspaces/HEAD/.metadata/.plugins/org.eclipse.pde.core/eoe (Sun)/config.ini loaded
Install location:
    file:/C:/eclipse/drops/eclipse-SDK-I20140114-0800-win32/
Framework located:
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi/
Loading extension: reference:file:C:/eclipse/workspaces/HEAD/org.eclipse.osgi.compatibility.state
	eclipse.properties not found
Framework classpath:
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi/org.eclipse.osgi_3.10.0.v20140113-1519.jar
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi.compatibility.state/org.eclipse.osgi.compatibility.state_1.0.0.v20140110-1728.jar
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi/
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi/
    file:/C:/eclipse/workspaces/HEAD/org.eclipse.osgi.compatibility.state/
Splash location:
    C:\eclipse\workspaces\HEAD\Git\eclipse.platform\platform\org.eclipse.platform\splash.bmp
Debug options:
    file:/C:/eclipse/drops/eclipse-SDK-N20140114-2000-win32-x86_64-hacked/.options not found
Time to load bundles: 65
Starting application: 6619
osgi> !SESSION 2014-01-15 13:00:48.280 -----------------------------------------------
eclipse.buildId=4.4.0.I20140114-0800
java.version=1.7.0_07
java.vendor=Oracle Corporation
BootLoader constants: OS=win32, ARCH=x86, WS=win32, NL=en_US
Framework arguments:  -product org.eclipse.sdk.ide -keyring c:\eclipse\.keyring
Command-line arguments:  -product org.eclipse.sdk.ide -data C:\eclipse\workspaces\HEAD/../eoe -dev file:C:/eclipse/workspaces/HEAD/.metadata/.plugins/org.eclipse.pde.core/eoe (Sun)/dev.properties -os win32 -ws win32 -arch x86 -debug -console -consolelog -clean -keyring c:\eclipse\.keyring

!ENTRY org.eclipse.ui 4 4 2014-01-15 13:00:58.521
!MESSAGE Conflicting handlers for org.eclipse.ui.window.splitEditor: {org.eclipse.ui.internal.SplitHandler} vs {org.eclipse.ui.internal.SplitHandler@178d17b}
Application Started: 15651

!ENTRY org.eclipse.jface 4 2 2014-01-15 13:01:11.015
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jface".
!STACK 0
java.lang.IllegalArgumentException: Argument not valid
	at org.eclipse.swt.SWT.error(SWT.java:4400)
	at org.eclipse.swt.SWT.error(SWT.java:4334)
	at org.eclipse.swt.SWT.error(SWT.java:4305)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:476)
	at org.eclipse.swt.widgets.ToolItem.setImage(ToolItem.java:706)
	at org.eclipse.e4.ui.css.swt.dom.ToolItemElement.reset(ToolItemElement.java:75)
	at org.eclipse.e4.ui.css.swt.engine.AbstractCSSSWTEngineImpl.reset(AbstractCSSSWTEngineImpl.java:91)
	at org.eclipse.e4.ui.css.swt.internal.theme.ThemeEngine.setTheme(ThemeEngine.java:401)
	at org.eclipse.e4.ui.css.swt.internal.theme.ThemeEngine.setTheme(ThemeEngine.java:382)
	at org.eclipse.ui.internal.dialogs.ViewsPreferencePage$2.selectionChanged(ViewsPreferencePage.java:98)
	at org.eclipse.jface.viewers.Viewer$2.run(Viewer.java:164)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:49)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:176)
	at org.eclipse.jface.viewers.Viewer.fireSelectionChanged(Viewer.java:162)
	at org.eclipse.jface.viewers.StructuredViewer.updateSelection(StructuredViewer.java:2201)
	at org.eclipse.jface.viewers.StructuredViewer.handleSelect(StructuredViewer.java:1218)
	at org.eclipse.jface.viewers.StructuredViewer$4.widgetSelected(StructuredViewer.java:1249)
	at org.eclipse.jface.util.OpenStrategy.fireSelectionEvent(OpenStrategy.java:242)
	at org.eclipse.jface.util.OpenStrategy.access$4(OpenStrategy.java:236)
	at org.eclipse.jface.util.OpenStrategy$1.handleEvent(OpenStrategy.java:406)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4351)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1061)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4170)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3759)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:827)
	at org.eclipse.jface.window.Window.open(Window.java:803)
	at org.eclipse.ui.internal.dialogs.WorkbenchPreferenceDialog.open(WorkbenchPreferenceDialog.java:215)
	at org.eclipse.ui.internal.OpenPreferencesAction.run(OpenPreferencesAction.java:65)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:499)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:588)
	at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:505)
	at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:415)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4351)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1061)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4170)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3759)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run(PartRenderingEngine.java:1122)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1006)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:146)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:612)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:565)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:125)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:80)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:372)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:226)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:601)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:636)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:591)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1450)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1426)

Comment 11 Dani Megert

2014-01-15 07:13:34 EST

Created attachment 239001 [details]
Picture of error when switching theme

Comment 12 Daniel Rolka

2014-01-15 10:10:49 EST

I've made quick look at the code and I can confirm that both issues are connected to the previous patch for the current bug (the part of patch with replacing of images with the default ones is not working correctly)

I'm going to prepare the additional patch for the issue

Daniel

Comment 13 Dani Megert

2014-01-15 10:13:17 EST

(In reply to Daniel Rolka from comment #12)
> I've made quick look at the code and I can confirm that both issues are
> connected to the previous patch for the current bug (the part of patch with
> replacing of images with the default ones is not working correctly)
> 
> I'm going to prepare the additional patch for the issue

Please also check that the part tab icons and the fonts are correct. I've seen those being wrong as well.

Comment 14 Daniel Rolka

2014-01-16 07:44:57 EST

OK, I've prepared the improvement for the previous patch that handles both issues mentioned above: https://git.eclipse.org/r/#/c/20706/

As I wrote, the main goal of the change is to clear the CSS cache and dispose all SWT resources contained inside one before switching to the new CSS theme. The user has to restart the Eclipse instance anyway to take the full effect of the change.

Daniel

Comment 15 Daniel Rolka

2014-01-21 06:01:06 EST

Released as: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2e0e17f76c55abcab0e3a30bb4d72a65314df2b6

Daniel

Comment 16 Daniel Rolka

2014-01-21 06:21:38 EST

Verified in the build: eclipse-SDK-I20140120-2000

Daniel

Comment 17 Lars Vogel

2014-02-19 06:12:47 EST

*** Bug 410795 has been marked as a duplicate of this bug. ***