Community
Participate
Working Groups
The Juno M1 EPP packages didn't specify any CSS theme in their product definition. The result worked ok, but it looked quite ugly. There were some thick border lines on the toolbar and thinner lines elsewhere. We need to understand what is happening in the default case where the product doesn't specify a CSS file. For compatibility we should try to do something reasonable in this case because it will be common when people first start porting their products to 4.x.
If no themeId is given: a) as a branding property (key=cssTheme) b) as a command line option (key=cssTheme) c) as a System.getProperty (key=cssTheme) Code found in E4Application#createE4Workbench The theme system is not initialized (so no css is applied at all) Code is found in PartRenderingEngine#initializeStyling.
(In reply to comment #1) > If no themeId is given.. The theme system is not initialized That is consistent with the behaviour we were seeing in the EPP packages. It sounds like it needs a fourth fallback - a "default default style" or something like that for the case where nothing is specified. This will be a common case when people start to try running their applications on Eclipse 4.
I think it is the task of the compat layer to set an id. Looking at the code i think the best place would be in the Workbench-Class around 531: -------8<------- E4Workbench e4Workbench = e4app.createE4Workbench(getApplicationContext(),display); IEclipseContext workbenchContext = e4Workbench.getContext(); workbenchContext.set(Display.class, display); if( workbenchContext.get(E4Application.THEME_ID) == null ) { workbenchContext.set(E4Application.THEME_ID, "org.eclipse.e4.ui.css.theme.e4_default"); workbenchContext.set(E4Workbench.CSS_RESOURCE_URI_ARG, "platform:/plugin/org.eclipse.platform/images/"); } -------8<------- One thing I'm not sure why it is done this way is that the resource-uri needs to be specified like this because they can be contributed through the theme extension point. Maybe Bogdan knows why?
Created attachment 202106 [details] Patch System properties are used as a fallback so this is the safest solution (we are already set the LegacyIDE.e4xmi)
ping, can we release this patch?
Bogdan / John, if this makes sense (it appears to to me) I'd be happy to apply the patch...
The approach sounds reasonable, but I wonder if we should only set the system property if it is still undefined at that point. Perhaps someone is attempting to use a system property from their config.ini or similar to specify a different theme.
Not sure I understand. If someone wants to specify one then this defect doesn't come into effect does it ? I'm presuming that through whatever mechanism we parse / get the over-riding values we'd just overwrite the ones that Tom's patch place into the model (i.e. the values in the model are a 'fail safe' mechanism, specifying *some* default e4 supplied CSS in all cases).
His patch does System.setProperty on E4Application.THEME_ID. If that system property had been previously set to something else (for example in config.ini), then his patch code is slamming it back to the default setting. I'm just suggesting checking if a value is already set before overwriting it: if (System.getProperty(E4Application.THEME_ID)== null) { System.setProperty(E4Application.THEME_ID, ...); }
(In reply to comment #9) > His patch does System.setProperty on E4Application.THEME_ID. If that system > property had been previously set to something else (for example in config.ini), > then his patch code is slamming it back to the default setting. I'm just > suggesting checking if a value is already set before overwriting it: > > if (System.getProperty(E4Application.THEME_ID)== null) { > System.setProperty(E4Application.THEME_ID, ...); > } Ok that looks fine but we then should add the same check for xmi which is pushed in the line above
we should fix this for 3.x apps running on Eclipse 4.x
Defered to 4.3
One of the adopter product was hit by this bug because it didn't have "cssTheme" property specified in product definition. The outcome was that Window > General > Appearance page was not initialized properly and error dialog with NPE was shown. Can we please fix this bug to problems with migration to 4.x?
(In reply to comment #13) > One of the adopter products was hit by this bug because it didn't have > "cssTheme" property specified in product definition. The outcome was that > Window > General > Appearance page was not initialized properly and error > dialog with NPE was shown. Can we please fix this bug to problems with > migration to 4.x? Should be: Can we please fix this bug to *avoid* problems with migration to 4.x?
Created attachment 233622 [details] NPE during activation
Daniel, could you please take a look at this bug as well? PW
The same NPE reported also in bug 381845.
Do we have the final version of this functionality? Should I use the modified version of Tom's patch to handle the config.ini file or prepare sth from scratch? Daniel
(In reply to Daniel Rolka from comment #18) > Do we have the final version of this functionality? Should I use the > modified version of Tom's patch to handle the config.ini file or prepare sth > from scratch? I'd start with comment #3 and see about setting a useful default if we the correct information isn't set. PW
Gerrit review link: https://git.eclipse.org/r/#/c/16908/
Released the Gerrit patch with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=5aacec6526f95c6da600acbf109b9b35af9348b0 In an RCP app [1] that includes the Appearance pref page, I don't see an error and the Appearance combo is disabled. A larger IDE based app the appearance page should just default (um) to the default e4 theme. Is this the behaviour we're looking for? PW [1] Add the pref action in the action bar advisor: prefsAction = ActionFactory.PREFERENCES.create(window); register(prefsAction); helpMenu.add(prefsAction); Add the preference page: <page name="%PreferencePages.Views" category="org.eclipse.ui.preferencePages.Workbench" class="org.eclipse.ui.ExtensionFactory:appearancePreferencePage" id="org.eclipse.ui.preferencePages.Views"> <keywordReference id="org.eclipse.ui.ide.appearance"/> <keywordReference id="org.eclipse.ui.ide.colorlabels"/> <keywordReference id="org.eclipse.ui.ide.themes"/> <keywordReference id="org.eclipse.ui.ide.tabs"/> <keywordReference id="org.eclipse.ui.ide.apearancepage"/> <keywordReference id="org.eclipse.ui.ide.animations"> </keywordReference> </page>
(In reply to Paul Webster from comment #21) > A larger IDE based app the appearance page should just default (um) to the > default e4 theme. > > Is this the behaviour we're looking for? Hi, with this patch I cannot set a theme via property "applicationCSS" any longer. It now always uses the IThemeEngine in PartRenderingEngine#initializeStyling() because "cssTheme" is never null. M.
(In reply to Paul Webster from comment #21) > Released the Gerrit patch with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=5aacec6526f95c6da600acbf109b9b35af9348b0 Daniel, you either have to update your fix so org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.initializeStyling(Display, IEclipseContext) works correctly (maybe use cssTheme if it is set but not the default, else check the applicationCSS, else use the cssTheme(default) if that fails) or we'll have to revert this fix and pursue another one. PW
(In reply to Paul Webster from comment #23) > (In reply to Paul Webster from comment #21) > > Released the Gerrit patch with > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=5aacec6526f95c6da600acbf109b9b35af9348b0 > > Daniel, you either have to update your fix so > org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine. > initializeStyling(Display, IEclipseContext) works correctly (maybe use > cssTheme if it is set but not the default, else check the applicationCSS, > else use the cssTheme(default) if that fails) or we'll have to revert this > fix and pursue another one. > > PW I'm going to prepare the new fix handling the issue. However if the previous one is a blocker please revert it, Daniel
(In reply to Daniel Rolka from comment #24) > I'm going to prepare the new fix handling the issue. However if the previous > one is a blocker please revert it, It is not a blocker for me. M.
(In reply to Markus Kuppe from comment #22) > (In reply to Paul Webster from comment #21) > > A larger IDE based app the appearance page should just default (um) to the > > default e4 theme. > > > > Is this the behaviour we're looking for? > > Hi, > > with this patch I cannot set a theme via property "applicationCSS" any > longer. It now always uses the IThemeEngine in > PartRenderingEngine#initializeStyling() because "cssTheme" is never null. > > M. The patch for the issue will be published using the Bug 426572, Daniel
*** Bug 381845 has been marked as a duplicate of this bug. ***
*** Bug 385572 has been marked as a duplicate of this bug. ***
I think this is fixed with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7bc3b56214e9389e8f776ee27705af402e93db94
If cssTheme is not specified at startup time, then the IThemeEngine is null if I want to use it later. I think we should register IThemeEngine in any case, if cssTheme is specified or not.
Released as: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4e361ecb178e9ec96c79b9e3c8a92de65842bf2c Daniel
Verified in the build: I20140428-2000 Daniel
