Community
Participate
Working Groups
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 */ }
This patch avoid to override the preference and uses the color directly in the CSS. https://git.eclipse.org/r/27148
Created attachment 243434 [details] Font colors in the Dark theme Some JDT keywords in the Dark theme are colored with black (at least on my Windows 7), see attachment. I think from the user's point of view it would be better to have it white or whitish Daniel
(In reply to Daniel Rolka from comment #2) > Some JDT keywords in the Dark theme are colored with black (at least on my > Windows 7), see attachment. I think from the user's point of view it would > be better to have it white or whitish That should be covered by Bug 433605, I think.
I have seen in the past two + review flags, but AFAIK one + review from another committer is sufficient, hence I commit the change.
Fixed with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6e4f37735f061e2bc80a90f8aa4aa0655943f502
(In reply to Lars Vogel from comment #5) > Fixed with > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=6e4f37735f061e2bc80a90f8aa4aa0655943f502 Lars, please revert until the proper approvals are in place. http://www.eclipse.org/eclipse/development/plans/freeze_plan_4_4.php#FixPassAfterRC2 ==> Two additional committers and a component lead must +1 your bug report (3 people who are not the one making the change) Plus, you need to announce your plan to fix a bug to platform-releng-dev@eclipse.org mailing list.
Dark theme uses two color variation to highlight each active/inactive view, the editor background pref defines only one color and in current CSS engine there is no way to assign a second color derived from the first, for this reason two new ColorDefinition are created that override the Text Editor for #org-eclipse-e4-ui-compatibility-editor and that can be modified by the user in 'Appearance > Colors&Fonts > Other defined by CSS'. If there is already an API to read the pref values in CSS, possible workaround are: 1) In the stylesheet, bind the Text Editor bg pref value (org.eclipse.ui.editors/AbstractTextEditor.Color.Background) instead of org-eclipse-ui-workbench-ACTIVE_CODE_EDITOR_BG_COLOR and leave the Inactive key as it is now; 2) In the stylesheet, remove the Active/Inactive behaviour from the code editor and use always one state/color bound to Text Editor pref value (the downside is that it become harder to distinguish when the code editor is active or not); 3) Add some lines of Java code in Dark theme bundle that derive and assign the Inactive color value starting from the Active color value for the editor; (In reply to Lars Vogel from comment #1) > This patch avoid to override the preference and uses the color directly in > the CSS. > > https://git.eclipse.org/r/27148 Lars, Daniel this patch does not solve the issue, it overrides the Text Editor pref value (because it remains unbound in CSS) and it removes the possibility for the user to change the editor background color.
(In reply to Dani Megert from comment #6) > (In reply to Lars Vogel from comment #5) > > Fixed with > > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=6e4f37735f061e2bc80a90f8aa4aa0655943f502 > > Lars, please revert until the proper approvals are in place. Reverted with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8a74269b13407741627900ea6cb766b4ccfcc438 > http://www.eclipse.org/eclipse/development/plans/freeze_plan_4_4. > php#FixPassAfterRC2 > ==> > Two additional committers and a component lead must +1 your bug report (3 > people who are not the one making the change) > > Plus, you need to announce your plan to fix a bug to > platform-releng-dev@eclipse.org mailing list. Thanks, didn't know that.
(In reply to Andrea Guarinoni from comment #7) > Lars, Daniel this patch does not solve the issue, it overrides the Text > Editor pref value (because it remains unbound in CSS) and it removes the > possibility for the user to change the editor background color. Can you upload a Gerrit review?
Some additional side notes: * the LineNumberRulerColumn widget is hard-coded to use as background the value of the AbstractTextEditor.Color.Background key; * Deleting the CSS styles for #org-eclipse-e4-ui-compatibility-editor will trigger the engine to use automatically the org.eclipse.ui.editors/AbstractTextEditor.Color.Background key's value but in some editors like eg. CDT this may generate artifacts because there are other widgets contained into the Text Editor's Canvas that init a bright value for their background-color that are overridden by the deleted CSS styles. (In reply to Daniel Rolka from comment #2) > Created attachment 243434 [details] > Font colors in the Dark theme > > Some JDT keywords in the Dark theme are colored with black (at least on my > Windows 7), see attachment. I think from the user's point of view it would > be better to have it white or whitish > > Daniel This is definitely an issue about JDT preferences keys that are not loaded as expected (colors remain the same as standard bright themes).
(In reply to Andrea Guarinoni from comment #10) > This is definitely an issue about JDT preferences keys that are not loaded > as expected (colors remain the same as standard bright themes). IIRC correctly the CSS engine can currently not load from JAR files in requires that the plug-in is extracted (Eclipse-BundleShape: dir in MANIFEST.MF). Maybe that is the issue here?
(In reply to Lars Vogel from comment #9) > (In reply to Andrea Guarinoni from comment #7) > > Lars, Daniel this patch does not solve the issue, it overrides the Text > > Editor pref value (because it remains unbound in CSS) and it removes the > > possibility for the user to change the editor background color. > > Can you upload a Gerrit review? I'm not sure that currently there is a better solution to this issue than the one already adopted.. :(
(In reply to Andrea Guarinoni from comment #12) > (In reply to Lars Vogel from comment #9) > > (In reply to Andrea Guarinoni from comment #7) > > > Lars, Daniel this patch does not solve the issue, it overrides the Text > > > Editor pref value (because it remains unbound in CSS) and it removes the > > > possibility for the user to change the editor background color. > > > > Can you upload a Gerrit review? > > I'm not sure that currently there is a better solution to this issue than > the one already adopted.. :( Since we are quite late in the 4.4 release I think we can go with the current patch and prepare sth better for Mars Daniel
(In reply to Daniel Rolka from comment #13) > Since we are quite late in the 4.4 release I think we can go with the > current patch and prepare sth better for Mars Here is a new review which reverts the reverted commit, i.e., should contain the original changes: https://git.eclipse.org/r/#/c/27163/
(In reply to Lars Vogel from comment #11) > > IIRC correctly the CSS engine can currently not load from JAR files in > requires that the plug-in is extracted (Eclipse-BundleShape: dir in > MANIFEST.MF). Maybe that is the issue here? The CSS engine can read from plugins that are jars as well as directories, so this should have no effect. PW
(In reply to Daniel Rolka from comment #13) > (In reply to Andrea Guarinoni from comment #12) > > (In reply to Lars Vogel from comment #9) > > > (In reply to Andrea Guarinoni from comment #7) > > > > Lars, Daniel this patch does not solve the issue, it overrides the Text > > > > Editor pref value (because it remains unbound in CSS) and it removes the > > > > possibility for the user to change the editor background color. > > > > > > Can you upload a Gerrit review? > > > > I'm not sure that currently there is a better solution to this issue than > > the one already adopted.. :( > > Since we are quite late in the 4.4 release I think we can go with the > current patch and prepare sth better for Mars > > Daniel Unfortunately the original patch doesn't solve the issue - the 'AbstractTextEditor.Color.Background' preference is still overridden by some CSS rule. Daniel
The new patch proposal for the issue: https://git.eclipse.org/r/#/c/27178/ Daniel
The new Gerrit's review link: https://git.eclipse.org/r/#/c/27415/ Daniel
(In reply to Daniel Rolka from comment #18) > The new Gerrit's review link: https://git.eclipse.org/r/#/c/27415/ > > Daniel Review got two +2
Component lead +1 from me. Submitted with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d5ed7013b4e2841659551b4f0e09eb718d7b588f
(In reply to Dani Megert from comment #20) > Component lead +1 from me. I thought Paul is our platform.ui component lead? Or does this permission system scale up, and the level above can also give +1? (Note: I know you more than qualified to give +1 but I would like to understand the whole system)
(In reply to Lars Vogel from comment #21) > (In reply to Dani Megert from comment #20) > > Component lead +1 from me. > > I thought Paul is our platform.ui component lead? Or does this permission > system scale up, and the level above can also give +1? > > (Note: I know you more than qualified to give +1 but I would like to > understand the whole system) Please read comment 6 again carefully ;-)
(In reply to Dani Megert from comment #22) > Please read comment 6 again carefully ;-) Got it, thanks.
Verified in I20140528-2000.
