Community
Participate
Working Groups
(From bug 433605 comment #4) The Dark theme overrides all Java editor syntax coloring color changes on restart. Changes to other font attributes are properly saved and retained. On the Java > Editor > Syntax Coloring preference page, "Restore Defaults" doesn't restore the defaults of the current theme, but the platform defaults. After a restart, the Dark theme default colors are applied again (by the first part of this bug). JDT doesn't intend to provide theme-specific default syntax colors if that means the user loses the ability to configure syntax colors.
This is not specific for the dark theme, but would apply for any custom theme which uses the new property API. Not sure how critical this is hence I copy in Paul and Daniel.
Daniel, could you please give us a little description of the current state of priority of prefs applied? *OS Defaults *Contributed from Theme *Set by the user PW
(In reply to Markus Keller from comment #0) > > JDT doesn't intend to provide theme-specific default syntax colors if that > means the user loses the ability to configure syntax colors. Since this targets the dark theme only and is completely new and opt in, I would still recommend JDT providing it's contribution to this theme. The 80% case will be users that switch to the dark theme and then customize some of the colors (Daniel, I assume this works correctly)? If there's some bug that occurss between Restore defaults and a restart that causes some unexpected pref changes, that can be addressed as well. If the dark theme will prevent users from customizing the regular platform themes, that's a problem though. PW
(In reply to Paul Webster from comment #3) > Since this targets the dark theme only and is completely new and opt in, I > would still recommend JDT providing it's contribution to this theme. The > 80% case will be users that switch to the dark theme and then customize some > of the colors (Daniel, I assume this works correctly)? Unfortunately, this 80% case is exactly the first scenario that doesn't work. Users can customize syntax colors, but all their customization work is lost on restart. As long as JDT doesn't contribute "dark theme" defaults, user customizations are properly persisted across restarts. There also seems to be a very special scenario for syntax colors that have been customized *before* I switch to the Dark theme: Those are not overridden by the JDT color overrides -- and further changes to these syntax colors are properly kept across restarts.
OK, this is the usecase I expect to work: 1. user fires up Luna 2. user switches to the dark theme and gets a dark editor (JDT, CDT, whatever) 3. user tweaks some of the prefs 4. the user restarts 5. the users sees the dark-themed editor with their tweaks. PW
(In reply to Paul Webster from comment #5) + 1. And two more steps to verify (works fine right now, and it's important that it stays like this): 6. user switches back to Classic or E4 theme and restarts 7. "Restore Defaults" restores standard colors; new user tweaks are persisted "Restore Defaults" in the dark theme is not a priority problem.
(In reply to Paul Webster from comment #2) > Daniel, could you please give us a little description of the current state > of priority of prefs applied? > > *OS Defaults > *Contributed from Theme > *Set by the user > > PW 1) OS Defaults 2) Set by the user 3) Contributed from Theme All properties that we override are stored in the special 'overriddenByCSS' property and reverted after switching the theme or closing the Workbench The 'Restore Defaults' is not currently supported by the feature Daniel
I forgot to mention in my last comment - there is the refreshing issue after switching the CSS theme - Bug 433765. Maybe it is the cause of the issue here Daniel
(In reply to Daniel Rolka from comment #7) > (In reply to Paul Webster from comment #2) > > Daniel, could you please give us a little description of the current state > > of priority of prefs applied? > > > > *OS Defaults > > *Contributed from Theme > > *Set by the user > > > > PW > > 1) OS Defaults > 2) Set by the user > 3) Contributed from Theme That order is wrong. The user needs to have the last word, so it needs to be in the order Paul wrote above.
What about the case where the user sets a few defaults in the java editor, then switches to the Dark theme? We don't want them to be in a mostly dark editor except for a few things (certain keywords) that might disappear entirely. But for me that means switching themes can override user prefs, not staying within the same theme. PW
(In reply to Paul Webster from comment #10) The user prefs can be set to default (removed from the pref store) at the time when the theme is switched. But after the theme switch, user preferences need to have higher priority than theme customizations.
(In reply to Markus Keller from comment #11) > (In reply to Paul Webster from comment #10) > The user prefs can be set to default (removed from the pref store) at the > time when the theme is switched. But after the theme switch, user > preferences need to have higher priority than theme customizations. OK, that makes sense and I think covers both use cases. PW
This is a must fix for the dark theme to be adopted.
(In reply to Markus Keller from comment #9) > (In reply to Daniel Rolka from comment #7) > > (In reply to Paul Webster from comment #2) > > > Daniel, could you please give us a little description of the current state > > > of priority of prefs applied? > > > > > > *OS Defaults > > > *Contributed from Theme > > > *Set by the user > > > > > > PW > > > > 1) OS Defaults > > 2) Set by the user > > 3) Contributed from Theme > > That order is wrong. The user needs to have the last word, so it needs to be > in the order Paul wrote above. Sorry I was wrong it works according to Paul's list Daniel
(In reply to Paul Webster from comment #10) > But for me that means switching themes can override user prefs, not staying > within the same theme. > > PW When the user customizes the preference with the preference dialog, it gets stored in the preference store. The entry in the preference store informs the CSS engine that it is the user's customization and we have to skip it during processing the CSS rules. When we remove the preference from the store (return to the System default), we expose the preference for the CSS engine for overriding. Daniel
(In reply to Markus Keller from comment #11) > (In reply to Paul Webster from comment #10) > The user prefs can be set to default (removed from the pref store) at the > time when the theme is switched. But after the theme switch, user > preferences need to have higher priority than theme customizations. Do you want to consider the reseting to default as the user's customization? if so, we have to save the default value in the preference store in order to mark it for the CSS engine. Without any entry in the preference store we don't know whether it is the customization or the OS default and we override it when proper CSS rule is present Daniel
Let me remind the Bug 433765 - please re-open the current editor after switching the Dark theme to the other one and check if the considered issue still occurs Daniel
(In reply to Daniel Rolka from comment #17) > Let me remind the Bug 433765 - please re-open the current editor after > switching the Dark theme to the other one and check if the considered issue > still occurs This bug is about restarting Eclipse, not about missing updates after switching themes. See comment 5 for steps to reproduce. The problem seems to be that users prefs are dumped on restart when the dark theme is active. But they should only be dumped once when the dark theme gets activated.
(In reply to Paul Webster from comment #5) > OK, this is the usecase I expect to work: > > 1. user fires up Luna > 2. user switches to the dark theme and gets a dark editor (JDT, CDT, > whatever) > 3. user tweaks some of the prefs > 4. the user restarts > 5. the users sees the dark-themed editor with their tweaks. > > PW I've prepared the patch for the issue - https://git.eclipse.org/r/#/c/26590/ Daniel
Here is my test script, with https://git.eclipse.org/r/#/c/26006/ applied, no fix and a fresh workspace: 1) create a project with some java classes 2) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to some other color, like Green and apply. Colors change. 3) Preferences>General>Appearance and switch to the Dark theme Problem #1: the Class names are still Green, I expected them to be set via the dark theme. Closing and opening the editor, restarting had no effect. 4) Preferences>Java>Editor>Syntax Coloring ... Restore Defaults Problem #2: switched the Class name to black, would hope it would pick up the Dark theme. Fixed by a restart. 5) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to some other color, like Green and apply. Colors change. 6) restart. Problem #3: Color goes back to the Dark theme default. 7) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to some other color, like Green and apply. Colors change. 8) Preferences>General>Appearance and switch to GTK Possible problem #4: This time it set the color to black. That might be reasonable. I'm most concerned about Problem #1 and Problem #3. PW
Tested with the fix https://git.eclipse.org/r/#/c/26590/ 1) create a project with some java classes 2) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to some other color, like Green and apply. Colors change. 3) Preferences>General>Appearance and switch to the Dark theme Problem #1: the Class names are still Green. Still a problem. 4) Preferences>Java>Editor>Syntax Coloring ... Restore Defaults Problem #2: switched the Class name to black. Fixed by a restart. 5) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to some other color, like Green and apply. Colors change. 6) restart. Problem #3: fixed 7) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. not necessary. 8) Preferences>General>Appearance and switch to GTK This time it set the color to black. That might be reasonable.
OK, with the fix for Problem #3 my original usecase works as I expected. Markus, is that enough for Luna, or should we address Problem #1 as well? Fixing Problem #1 would give us a clean slate for trying out the dark theme. PW
(In reply to Paul Webster from comment #21) > Tested with the fix https://git.eclipse.org/r/#/c/26590/ > > 1) create a project with some java classes > > 2) Preferences>Java>Editor>Syntax Coloring ... Java>Classes. Change it to > some other color, like Green and apply. Colors change. > > 3) Preferences>General>Appearance and switch to the Dark theme > > Problem #1: the Class names are still Green. Still a problem. > When you modify some syntax color values with the preference dialog it will be skipped during applying the CSS rules as the user's customization. You have to either reset to default value with the proper button or remove proper entry from the preference file in order to expose the value again to the CSS engine (in other words - remove the customization) Do you want to modify the current behavior and apply the CSS rules for the preferences customized by user as well? Daniel
(In reply to Daniel Rolka from comment #23) > > Do you want to modify the current behavior and apply the CSS rules for the > preferences customized by user as well? We want comment #10 and comment #11. That switching themes on the appearance pref page can override preferences set through CSS. But in general, user prefs override CSS prefs which override defaults. PW
(In reply to Paul Webster from comment #24) > (In reply to Daniel Rolka from comment #23) > > > > Do you want to modify the current behavior and apply the CSS rules for the > > preferences customized by user as well? > > We want comment #10 and comment #11. That switching themes on the > appearance pref page can override preferences set through CSS. > > But in general, user prefs override CSS prefs which override defaults. > > PW > comment #11: The user's customization has got the higher priority than the CSS definitions. When we are going to treat the 'reset to default' as the user's customization we can't remove it from the preference store, but store the preference with the default value. In reverse, we are not able to figure out that it is the customization of the preference. > comment #10: It seems to be bigger change, do you want to implement it in the 4.4 or we can postpone it till 4.5? I think the current functionality covers most use cases. All mentioned modifications are some kind of extensions according to me and I think it can be done in the Mars Daniel
(In reply to Daniel Rolka from comment #25) > > comment #11: > > The user's customization has got the higher priority than the CSS > definitions. When we are going to treat the 'reset to default' as the user's > customization we can't remove it from the preference store, but store the > preference with the default value. In reverse, we are not able to figure out > that it is the customization of the preference. I'm not sure I understand this. The request is to (in effect) reset any user changes in Prefs that come in via CSS during a theme switch. There's no requirement to remember what those old user values were for when you switch themes again. PW
(In reply to Daniel Rolka from comment #19) > I've prepared the patch for the issue - https://git.eclipse.org/r/#/c/26590/ OK, this seems to fix the most pressing issues. Restore Defaults doesn't work, and right after switching to the Dark theme, it sometimes takes almost a minute until all syntax color changes trickled through to the Java editor. (In reply to Paul Webster from comment #22) > Markus, is that enough for Luna, or should we address Problem #1 as well? > Fixing Problem #1 would give us a clean slate for trying out the dark theme. This patch is enough for me to release bug 433605. Clearing all user colors on theme switch has advantages and disadvantages, and we can sort this out later. I don't understand enough of the CSS code to give a valuable review+, but if Paul also thinks it's fine, then I'd release this for RC2.
(In reply to Markus Keller from comment #27) > This patch is enough for me to release bug 433605. Clearing all user colors > on theme switch has advantages and disadvantages, and we can sort this out > later. > > I don't understand enough of the CSS code to give a valuable review+, but if > Paul also thinks it's fine, then I'd release this for RC2. I think it's fine. Daniel, we can release this for RC2, and we'll create bugs for the remaining issue. PW
Brian, could you please review the code in https://git.eclipse.org/r/#/c/26590/ ? PW
(In reply to Paul Webster from comment #28) > (In reply to Markus Keller from comment #27) > > This patch is enough for me to release bug 433605. Clearing all user colors > > on theme switch has advantages and disadvantages, and we can sort this out > > later. > > > > I don't understand enough of the CSS code to give a valuable review+, but if > > Paul also thinks it's fine, then I'd release this for RC2. > > I think it's fine. Daniel, we can release this for RC2, and we'll create > bugs for the remaining issue. > > PW It sounds good for me thanks, Daniel
(In reply to Paul Webster from comment #26) > (In reply to Daniel Rolka from comment #25) > > > comment #11: > > > > The user's customization has got the higher priority than the CSS > > definitions. When we are going to treat the 'reset to default' as the user's > > customization we can't remove it from the preference store, but store the > > preference with the default value. In reverse, we are not able to figure out > > that it is the customization of the preference. > > I'm not sure I understand this. The request is to (in effect) reset any > user changes in Prefs that come in via CSS during a theme switch. There's > no requirement to remember what those old user values were for when you > switch themes again. > > PW I see it now, sorry I misinterpreted the comment #11. Since we have the workaround for that (the user has to 'reset to default' the preference in order to expose it again for overriding via the CSS) I would consider it also as the separate bug for the Mars Daniel
Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=06103efe547cb81c0c3e818e73a429f424d40442 PW
Daniel, please open the additional bugs for Mars. PW
With this fix the syntax coloring in JDT does not work anymore. If I revert the commit the changes introduced with Bug 433605 work again.
(In reply to Lars Vogel from comment #34) > With this fix the syntax coloring in JDT does not work anymore. If I revert > the commit the changes introduced with Bug 433605 work again. See bug 435488.
caused by bug 435488 PW
(In reply to Paul Webster from comment #33) > Daniel, please open the additional bugs for Mars. > > PW OK, I've opened the Bug 435512 that I believe covers the comment #10 and the comment #11 Daniel
This does not work. 1. start with I20140522-1330 2. paste this into 'Package Explorer': public class C {} 3. switch to Dark theme and restart 4. decide to change the background color on the 'Text Editors' preference page ==> does not work! 5. maybe restart help? ==> no, does not work NOTE: If this is not fixed, JDT will remove the Dark theme support. I've reopened bug 433605 to keep track of this.
(In reply to Dani Megert from comment #38) > This does not work. > > 1. start with I20140522-1330 > 2. paste this into 'Package Explorer': public class C {} > 3. switch to Dark theme and restart > 4. decide to change the background color on the 'Text Editors' preference > page I tried with java syntax highlighting and the Classes preference, and that worked OK (change took, survived restart). This is probably not the same problem. It looks like that Text Editor pref is being overridden by org.eclipse.ui.themes/css/dark/e4-dark_basestyle.css (not really related to this pref, but to the dark theme itself). ColorDefinition#org-eclipse-ui-workbench-INACTIVE_CODE_EDITOR_BG_COLOR { color: #262626; label: 'Inactive, code editor background color'; } ColorDefinition#org-eclipse-ui-workbench-ACTIVE_CODE_EDITOR_BG_COLOR { color: #202020; label: 'Active, code editor background color'; } And used in e4-dark_partstyle.css #org-eclipse-e4-ui-compatibility-editor Canvas, #org-eclipse-e4-ui-compatibility-editor Canvas > *, /* Workaround for CDT folding column SWT-BUG (styles aren't inherited) */ #org-eclipse-e4-ui-compatibility-editor Canvas > * > * { /*background-color: #262626;*/ background-color: '#org-eclipse-ui-workbench-INACTIVE_CODE_EDITOR_BG_COLOR'; /* SWT-BUG: background-color rule for LineNumberRulerColumn is ignored */ } .MPartStack.active #org-eclipse-e4-ui-compatibility-editor Canvas, .MPartStack.active #org-eclipse-e4-ui-compatibility-editor Canvas > *, /* Workaround for CDT folding column SWT-BUG (styles aren't inherited) */ .MPartStack.active #org-eclipse-e4-ui-compatibility-editor Canvas > * > * { /*background-color: #202020;*/ background-color: '#org-eclipse-ui-workbench-ACTIVE_CODE_EDITOR_BG_COLOR'; /* SWT-BUG: background-color rule for LineNumberRulerColumn is ignored */ } i.e. reverting the JDT dark theme will have no effect on that preference not working in the dark theme. PW
On an unrelated note, the editor background color can be changed on Preferences>General>Appearance>Color and Fonts>Other defined by CSS :-) PW
Created attachment 243414 [details] Nothing to select (In reply to Paul Webster from comment #40) > On an unrelated note, the editor background color can be changed on > Preferences>General>Appearance>Color and Fonts>Other defined by CSS :-) > > PW Not sure what's funny (:-) here. I don't see anything that I could change here.
(In reply to Dani Megert from comment #41) > Created attachment 243414 [details] > Nothing to select I think you have to be in the dark theme to see them. PW
I've confirmed they are available when the dark theme is set. Obviously not what you want when modifying the text preference. But they're related to the definition of the dark theme itself, not to theme preferences overriding user preferences. PW
I've opened Bug 435574 for the dark theme overriding the text editor's background preference.
Verified that comment #21 works and problem #3 is gone. So switch to the dark theme and then change one of the java syntax prefs, and that change takes effect and remains after a restart. In 4.4.0.I20140522-1330 PW
.
(In reply to Paul Webster from comment #44) > I've opened Bug 435574 for the dark theme overriding the text editor's > background preference. Yes, looks like that's a separate/new issue.
