Bug 466075 - [CSS] Not possible to style the same preference node via IEclipsePreferences#id from different plug-ins
Summary: [CSS] Not possible to style the same preference node via IEclipsePreferences#...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 4.7 M7   Edit
Assignee: Lars Vogel CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 514312 (view as bug list)
Depends on:
Blocks: 458133 464997 502380
  Show dependency tree
 
Reported: 2015-05-01 09:32 EDT by Dawid Pakula CLA
Modified: 2017-05-01 16:46 EDT (History)
9 users (show)

See Also:


Attachments
Patch to CSSComputedStyleImpl (1.66 KB, application/octet-stream)
2015-06-11 07:49 EDT, Matthias Becker CLA
no flags Details
Patch to CSSValueListImpl (358 bytes, application/octet-stream)
2015-06-11 07:50 EDT, Matthias Becker CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dawid Pakula CLA 2015-05-01 09:32:20 EDT
If in two css files imported by org.eclipse.e4.ui.css.swt.theme extension point, same bundle ID will be used (IEclipsePreferences#bundle-id), only last will be imported properly.

Problem:
PDT keeps semantic highlighting preferences (own and registered by extension point), in own preference space. 

If adopter will want support dark theme and register own preferencestyle for same bundle ID, previous will be ignored. 


File 1:
IEclipsePreferences#org-eclipse-php-ui {
  preferences : 'original_preference=for_dark'
}
File 2:
IEclipsePreferences#org-eclipse-php-ui {
  preferences: 'additional_preferences'
}
Comment 1 Dawid Pakula CLA 2015-05-28 09:19:15 EDT
This is also problem on platform level. 

org.eclipse.ui.editor keep annotation colors in it's own space (org.eclipse.ui.editors.markerAnnotationSpecification extension point), so it's not possible to update via css.

Ideally if could be possible to override in this way:

IEclipsePreferences#org-eclipse-ui-editors {
  -preference-PHPReadOccurrenceIndicationColor : '72,72,72';
}
Comment 2 Matthias Becker CLA 2015-06-11 07:49:55 EDT
Created attachment 254327 [details]
Patch to CSSComputedStyleImpl
Comment 3 Matthias Becker CLA 2015-06-11 07:50:54 EDT
Created attachment 254328 [details]
Patch to CSSValueListImpl
Comment 4 Matthias Becker CLA 2015-06-11 08:04:23 EDT
I have the same issue here.
We contribute a colorDefinition via the org.eclipse.ui.themes extension point like this:

      <colorDefinition
            categoryId="com.acme.foo.ui.syntaxHighlight"
            id="com.acme.foo.ui.error"
            label="%ERROR_XMSG"
            value="255,0,0">
         <description>
            %ERROR_DESCRIPTION_XMSG
         </description>
      </colorDefinition>

To support the new dark theme we also provide a CSS via:
   <extension
         point="org.eclipse.e4.ui.css.swt.theme">
      <stylesheet
            uri="css/e4-dark_foo_syntaxhighlighting.css">
         <themeid
               refid="org.eclipse.e4.ui.css.theme.e4_dark">
         </themeid>
      </stylesheet>
   </extension>

The CSS contains:

IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'com.acme.foo.ui.error=251,114,98'
}

This does not work reliable. Eclipse also brings 
org.eclipse.ui.themes/css/dark/e4-dark_preferencestyle.css

that contains:

IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'ACTIVE_HYPERLINK_COLOR=138,201,242'
		'CONFLICTING_COLOR=240,15,66'
		'CONTENT_ASSIST_BACKGROUND_COLOR=52,57,61'
		'CONTENT_ASSIST_FOREGROUND_COLOR=238,238,238'
		'ERROR_COLOR=247,68,117'
		'HYPERLINK_COLOR=111,197,238'
		'INCOMING_COLOR=31,179,235'
		'OUTGOING_COLOR=238,238,238'
		'RESOLVED_COLOR=108,210,17'
}

We now have two CSS that contain preferences for org-eclipse-ui-workbench.
In this situation the last CSS wins as the implementation in CSSComputedStyleImpl simply removes already existing preferences and add new once. See:

	private void addCSSPropertyList(CSSPropertyList properties) {
		int length = properties.getLength();
		for (int i = 0; i < length; i++) {
			CSSProperty property = properties.item(i);
			super.removeProperty(property.getName());
			super.addProperty(property);
		}
	}

I "hacked" in a solution for this issue (see my attachments p1.diff and p2.diff) that merge the two ValueLists in such a case. As CSSValueListImpl did not provide a method to add new values after object construction I simply added a "addAll" method. I alos had some issue with a CSSException thrown in CSSPropertyPaddingSWTHandler#applyCSSPropertyPadding see:

if(length < 2 || length > 4) {
  throw new CSSException("Invalid padding property list length");
}

Because of this I added the   "preferences".equals(name)   check.


I now that this is not the final solution, but it can function as a proof-of-concept.
Anyway with this change my coloring in works fine for the dark theme.
Comment 5 Lars Vogel CLA 2015-06-11 08:12:27 EDT
(In reply to Matthias Becker from comment #2)
> Created attachment 254327 [details]
> Patch to CSSComputedStyleImpl

Thanks Matthias. Can you please provide the fix as Gerrit review? https://wiki.eclipse.org/Platform_UI/How_to_Contribute#Creating_a_Gerrit_review_or_a_patch
Comment 6 Eclipse Genie CLA 2015-06-11 08:52:42 EDT
New Gerrit change created: https://git.eclipse.org/r/50004
Comment 7 Matthias Becker CLA 2015-07-28 07:29:47 EDT
Hi everybody,

any progress on this topic?

Regards,
Matthias
Comment 8 Brian de Alwis CLA 2015-07-30 23:33:51 EDT
The issue with this approach is that it violates how CSS normally works and would bind us to a particular implementation (i.e., one that supported this kind of behaviour)

I'm wondering if there might be a different approach such as incorporating a CSS processor like SASS or LESS.  I haven't yet looked at them in light of supporting this use case.
Comment 9 Matthias Becker CLA 2015-07-31 02:02:05 EDT
Dear Brian,

my patch is just a "hack" to verify the error cause.
So what is the "normal way" CSS works?

We have to find a solution that allows different plugin to contribute font definitions like this:

IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'com.acme.foo.ui.error=251,114,98'
}

IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'ACTIVE_HYPERLINK_COLOR=138,201,242'
		'CONFLICTING_COLOR=240,15,66'
		'CONTENT_ASSIST_BACKGROUND_COLOR=52,57,61'
		'CONTENT_ASSIST_FOREGROUND_COLOR=238,238,238'
		'ERROR_COLOR=247,68,117'
		'HYPERLINK_COLOR=111,197,238'
		'INCOMING_COLOR=31,179,235'
		'OUTGOING_COLOR=238,238,238'
		'RESOLVED_COLOR=108,210,17'
}

an both are considered.

Regards,
Matthias
Comment 10 Brian de Alwis CLA 2015-08-07 16:00:56 EDT
(In reply to Matthias Becker from comment #9)
> So what is the "normal way" CSS works?

The behaviour we're seeing (that the most suitable one, according to specificity) is the normal way.  It just doesn't match what you're trying to do :-/

CSS wasn't designed for this kind of extensibility as it wasn't necessary for the web: the web wanted a way to override previous settings.  We (Eclipse/E4) didn't really anticipate your kind of use for the 'preferences' property mechanism.


Since these preferences content is static, I could see adding support for 'properties' to include a spec to an extension id.  The CSS might look like:

    preferences: theme('XXX');

And that could load all .properties files found in this bundle's themes/XXX.  Fragments could contribute their own files under this directory.

(Just throwing this out there for discussion; I haven't really thought this through.)
Comment 11 Matthias Becker CLA 2015-08-10 03:33:08 EDT
(In reply to Brian de Alwis from comment #10)
 
> CSS wasn't designed for this kind of extensibility as it wasn't necessary
> for the web: the web wanted a way to override previous settings.  We
> (Eclipse/E4) didn't really anticipate your kind of use for the 'preferences'
> property mechanism.

But that's the way how support for the dark theme needs to be implemented. 
I don't find our implementation that "exotic" out product defines color via the org.eclipse.ui.themes extension point (this shows up under "Colors and Fonts" in the preference page). I think this is the standard way of defining colors that can be configured by the user.

And now we "simply" want to support the new darks theme of eclipse. So if "CSS wasn't designed for this" we need another way to support the dark theme.
Comment 12 Lars Vogel CLA 2015-08-11 06:17:55 EDT
(In reply to Brian de Alwis from comment #8)
> The issue with this approach is that it violates how CSS normally works and
> would bind us to a particular implementation (i.e., one that supported this
> kind of behaviour)

AFAIK CSS does support adding properties to existing styles. For example I think the following is legal in CSS:

h3.title {
  color: #333;
  font-size: 16px;
}

and another CSS stylesheet or rule might add the following:

h3.title {
  font-style: italic;
}

I must admit that I have never read any CSS specification but AFAIK this is widely in use.
Comment 13 Lars Vogel CLA 2015-08-11 06:25:10 EDT
Also CascadeTest from org.eclipse.e4.ui.tests.css.core demonstrates overriding values from one CSS by another CSS file.
Comment 14 Dawid Pakula CLA 2015-08-11 06:27:05 EDT
(In reply to Lars Vogel from comment #12)
> (In reply to Brian de Alwis from comment #8)
> > The issue with this approach is that it violates how CSS normally works and
> > would bind us to a particular implementation (i.e., one that supported this
> > kind of behaviour)
> 
> AFAIK CSS does support adding properties to existing styles. For example I
> think the following is legal in CSS:

This is why CSS have font and font-* properties. "font"| will simply override all previous "font" and "font-*" usages. "Font-*" override only one element from "font"

I requested ability to provide additional settings (for example for eclipse annotation colors, colors and fonts), via:

IEclipsePreferences#plugin-id {
 preference-my-preference-id: "";
}

"preference-" is off course just a prefix. "-" should be replaced by ".".
Comment 15 Lars Vogel CLA 2015-08-11 06:33:14 EDT
(In reply to Dawid Pakula from comment #14)
> This is why CSS have font and font-* properties. "font"| will simply
> override all previous "font" and "font-*" usages. "Font-*" override only one
> element from "font"

I think this comment does not apply. AFAIK the following also works in CSS

h3.title {
  color: #333;
  font-size: 16px;
}

and another CSS stylesheet or rule might add the following:

h3.title {
  color: red;
}
Comment 16 Dawid Pakula CLA 2015-08-11 07:21:20 EDT
(In reply to Lars Vogel from comment #15)
> (In reply to Dawid Pakula from comment #14)
> > This is why CSS have font and font-* properties. "font"| will simply
> > override all previous "font" and "font-*" usages. "Font-*" override only one
> > element from "font"
> 
> I think this comment does not apply. AFAIK the following also works in CSS

Why?

Depend on loading order you will receive color red or color #333 + font-size in h3.title scope. Color isn't part of font-* .

Current e4 css implementation works in same way.
Comment 17 Matthias Becker CLA 2015-08-11 08:08:37 EDT
So the issue is that the key in all the CSS files is "preferences"
and the value (from the CSS point of view) are the name/value pairs for the preferences

Example:

A)
IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'com.acme.foo.ui.error=251,114,98'
}

B)
IEclipsePreferences#org-eclipse-ui-workbench {
	preferences:
		'ACTIVE_HYPERLINK_COLOR=138,201,242'
		'CONFLICTING_COLOR=240,15,66'
}

and depending on the load order I either get the values from A or from B.

So couldn't we change this to: 
A)
IEclipsePreferences#org-eclipse-ui-workbench {
	preferences#com.acme.foo.ui.error="251,114,98'
}

B)
IEclipsePreferences#org-eclipse-ui-workbench {
	preferences#ACTIVE_HYPERLINK_COLOR='138,201,242'
        preferences#CONFLICTING_COLOR='240,15,66'
}

In this case the key of the prefrence would be part of the key in the CSS file.
Comment 18 Brian de Alwis CLA 2015-08-11 10:28:58 EDT
(In reply to Matthias Becker from comment #17)
> So the issue is that the key in all the CSS files is "preferences"
> and the value (from the CSS point of view) are the name/value pairs for the
> preferences

That's right.

> So couldn't we change this to: 
> A)
> IEclipsePreferences#org-eclipse-ui-workbench {
> 	preferences#com.acme.foo.ui.error="251,114,98'
> }
> 
> B)
> IEclipsePreferences#org-eclipse-ui-workbench {
> 	preferences#ACTIVE_HYPERLINK_COLOR='138,201,242'
>         preferences#CONFLICTING_COLOR='240,15,66'
> }
> 
> In this case the key of the prefrence would be part of the key in the CSS
> file.

Those are not valid property names, and out underlying implementation does not support catch-all properties like that.

I don't see how we can use a pure CSS solution here, and hence my 'theme(XXX)' idea above (which I think really would have to be 'url(theme:XXX)' to be compliant).
Comment 19 Matthias Becker CLA 2015-08-31 11:39:36 EDT
Any progress on this. Regarding the architecture / CSS-Compliance topic I cannot help. All I can do is to outline the use-case and the issue.
This currently really inhibits that we add support for the dark theme in our plugins.
Comment 20 Matthias Becker CLA 2015-09-21 11:19:16 EDT
Sorry that I have to "annoy" again.

How can we make some progress to a final solution here?
Comment 21 Timo Kinnunen CLA 2015-09-27 19:39:21 EDT
(In reply to comment #18)
> I don't see how we can use a pure CSS solution here, and hence my 'theme(XXX)'
> idea above (which I think really would have to be 'url(theme:XXX)' to be
> compliant).

This should be workable. Simply write:

preferences > org-eclipse-ui-workbench > ACTIVE_HYPERLINK_COLOR { color: rgb(138,201,242) }
preferences > org-eclipse-ui-workbench > CONFLICTING_COLOR { color: rgb(240,15,66) }
and
preferences > org-eclipse-ui-workbench > com-acme-foo-ui-error { color: rgb(251,114,98) }

instead of:

IEclipsePreferences#org-eclipse-ui-workbench {
 preferences:
  'ACTIVE_HYPERLINK_COLOR=138,201,242'
  'CONFLICTING_COLOR=240,15,66'
}

IEclipsePreferences#org-eclipse-ui-workbench {
 preferences:
  'com.acme.foo.ui.error=251,114,98'
}
Comment 22 Matthias Becker CLA 2015-09-28 02:39:40 EDT
(In reply to Timo Kinnunen from comment #21)
> (In reply to comment #18)
> > I don't see how we can use a pure CSS solution here, and hence my 'theme(XXX)'
> > idea above (which I think really would have to be 'url(theme:XXX)' to be
> > compliant).
> 
> This should be workable. Simply write:
> 
> preferences > org-eclipse-ui-workbench > ACTIVE_HYPERLINK_COLOR { color:
> rgb(138,201,242) }
> preferences > org-eclipse-ui-workbench > CONFLICTING_COLOR { color:
> rgb(240,15,66) }
> and
> preferences > org-eclipse-ui-workbench > com-acme-foo-ui-error { color:
> rgb(251,114,98) }
> 
> instead of:
> 
> IEclipsePreferences#org-eclipse-ui-workbench {
>  preferences:
>   'ACTIVE_HYPERLINK_COLOR=138,201,242'
>   'CONFLICTING_COLOR=240,15,66'
> }
> 
> IEclipsePreferences#org-eclipse-ui-workbench {
>  preferences:
>   'com.acme.foo.ui.error=251,114,98'
> }

Is this a proposal for a solution to be implemented or should this already work with the current implementation?
Comment 23 Timo Kinnunen CLA 2015-09-28 15:51:53 EDT
No, not even close to either one. Rather it's one way of reorganizing the preferences-setting-custom-CSS that Eclipse uses to make it easily extendable using normal CSS. Hopefully it's enough to demonstrate that pure CSS is expressive enough for this use-case.
Comment 24 Matthias Becker CLA 2015-09-29 02:58:44 EDT
@Brian de Alwis: What's your opinion to the proposal in comment 21?
Comment 25 Brian de Alwis CLA 2016-03-15 10:24:50 EDT
Oh dear, I missed these last notes :-(

I think Timo's approach is a great idea.  The E4 CSS theme management supports multiple CSS engines, and one could be created to monitor the preference nodes.  You can see an example of setting up a separate CSS Engine at 

https://git.eclipse.org/c/e4/org.eclipse.e4.ui.git/tree/bundles/org.eclipse.e4.ui.css.workbench/

This creates a CSS engine to allow tweaking the E4 application model.  The nice thing about this approach is that it doesn't require new API.

Normally CSS selectors are of the form 'WIDGET#ID.CLASS'.  Timo's example might be better expressed as

.preferences > #org-eclipse-ui-workbench > #ACTIVE_HYPERLINK_COLOR { color:
rgb(138,201,242); }

where the top-level might also be an IEclipsePreferences with class 'preferences' and perhaps id 'instance' / 'configuration' / 'default' / 'installation' for the different scopes?
Comment 26 Dawid Pakula CLA 2016-03-15 10:54:00 EDT
Unfortunately preference names often use dots (for example org.eclipse.jdt.core.compiler.compliance). So maybe rather than only:

IEclipsePreferences#overridden-bundle {
  preferences: 'additional_preferences'
}

Allow also:
IEclipsePreferences#overridden-bundle #my-bundle {
  preferences: 'additional_preferences'
}

and later append all IEclipsePreferences#overridden-bundle #* into overridden bundle.
Comment 27 Brian de Alwis CLA 2016-03-15 11:22:39 EDT
Dawid, the problem is that 'preferences' is a single property and under CSS the most applicable version wins.  We don't want to have to do our processing to reconcile the total set of properties.  The advantage of Timo's approach is that it exposes the individual font and colour properties/preferences as individual CSS properties, and so the normal CSS rules apply to determine a winner.
Comment 28 Matthias Becker CLA 2016-03-16 03:42:42 EDT
(In reply to Brian de Alwis from comment #27)
> Dawid, the problem is that 'preferences' is a single property and under CSS
> the most applicable version wins.  We don't want to have to do our
> processing to reconcile the total set of properties.  The advantage of
> Timo's approach is that it exposes the individual font and colour
> properties/preferences as individual CSS properties, and so the normal CSS
> rules apply to determine a winner.

That sounds good. Let me know as soon you have first implementation for that. I can help with testing with my use-case.
Comment 29 Brian de Alwis CLA 2016-03-16 09:25:14 EDT
I have no cycles for this at the moment.  Marking as 'helpwanted'.
Comment 30 Lars Vogel CLA 2016-04-20 12:06:15 EDT
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
Comment 31 Matthias Becker CLA 2016-08-29 04:57:22 EDT
Hey Guys,

are there any plans to fix this?

Regards,
Matthias
Comment 32 Lars Vogel CLA 2016-09-28 04:17:42 EDT
Note: EGit allows uses IEclipsePreferences#org-eclipse-ui-workbench and is overriding the platform styling from e4-dark_preferences.css for this node.
Comment 33 Lars Vogel CLA 2016-09-28 04:35:33 EDT
(In reply to Matthias Becker from comment #31)
> are there any plans to fix this?

Yes. Are you available to help with the implementation of Comment 25? I would suggest not to create another CSS engine but to add it to the existing one.
Comment 34 Matthias Becker CLA 2016-09-28 06:37:58 EDT
(In reply to Lars Vogel from comment #33)
> Yes. Are you available to help with the implementation of Comment 25? I
> would suggest not to create another CSS engine but to add it to the existing
> one.

Sorry, Lars: I don't have the knowledge for this topic. So I cannot help here.
Comment 35 Lars Vogel CLA 2016-09-29 10:06:22 EDT
(In reply to Matthias Becker from comment #34)
> Sorry, Lars: I don't have the knowledge for this topic. 

I try to have a look.
Comment 36 Matthias Becker CLA 2016-12-30 07:16:31 EST
I just though about the proposal of writing:
  .preferences > #org-eclipse-ui-workbench > #ACTIVE_HYPERLINK_COLOR { 
    color : rgb(138,201,242); 
  }
  .preferences > #org-eclipse-ui-workbench > #CONFLICTING_COLOR { 
    color : rgb(240,15,66); 
  }
instead of 
  IEclipsePreferences#org-eclipse-ui-workbench {
    preferences:
      'ACTIVE_HYPERLINK_COLOR=138,201,242'
      'CONFLICTING_COLOR=240,15,66'
  }
and I have some questions:

You are adding the "artifical" property "color" so that the selectors do not clash like "IEclipsePreferences#org-eclipse-ui-workbench" did. Correct?
But it's not always a color definition. e4-dark_preferencestyle.css in bundle org.eclipse.ui.themes for example contains 'AbstractTextEditor.Color.Background.SystemDefault=false'
Would we then distinguish the different "types". Wouldn't it be easier to just have always the same property name that is very generic e.g. like "preference-value"?
And why are you putting the value into "rgb( )". I today's format this not present. To sum it up I would propose:
  .preferences > #org-eclipse-ui-workbench > #ACTIVE_HYPERLINK_COLOR { 
    preference-value : 138,201,242; 
  }
  .preferences > #org-eclipse-ui-workbench > #CONFLICTING_COLOR { 
    preference-value : 240,15,66; 
  }

What do you mean with "where the top-level might also be an IEclipsePreferences with class 'preferences' and perhaps id 'instance' / 'configuration' / 'default' / 'installation' for the different scopes?"?
I simply do not understand that.

What about compatibility. Would we simple re-write the affected CSS files in the various bundles to the new "format" or would we have to ensure that both today's and the new format are working?

What about the additional CSS Engine. Why are you proposing to create an additional one? Today's implementation also handles all the different kinds with one CSS engine.
Is this related to the change of the "format" in the CSS file?
Comment 37 Lars Vogel CLA 2017-03-03 03:49:00 EST
Mass move. Please move back to M6, if necessary
Comment 38 Lars Vogel CLA 2017-03-29 14:44:24 EDT
Short analysis:

Preferences are handled by EclipsePreferenceProvider for the EclipsePreferenceElement. The handler is EclipsePreferenceHandler. 

EclipsePreferenceHandler is currently trivial, we should try to enhance it to support the > notation as suggested in this bug.
Comment 39 Lars Vogel CLA 2017-03-30 10:01:23 EDT
(In reply to Lars Vogel from comment #38)
> Short analysis:
> 
> Preferences are handled by EclipsePreferenceProvider for the
> EclipsePreferenceElement. The handler is EclipsePreferenceHandler. 
> 
> EclipsePreferenceHandler is currently trivial, we should try to enhance it
> to support the > notation as suggested in this bug.

One alternative would be to allow pseudo elements for preferences, e.g., 

IEclipsePreferences#org-eclipse-ui-editors:eclipse-platform.ui {
}

This should make the selector unique. In preference handler could simply ignore the pseudo element and still apply the rules.

WDYT?
Comment 40 Eclipse Genie CLA 2017-03-30 10:20:55 EDT
New Gerrit change created: https://git.eclipse.org/r/94149
Comment 41 Lars Vogel CLA 2017-03-30 10:23:02 EDT
(In reply to Eclipse Genie from comment #40)
> New Gerrit change created: https://git.eclipse.org/r/94149

Matthias, can you test this change? If you have conflicting preference nodes you need to give at least on of them a pseudo attribute.
Comment 42 Lars Vogel CLA 2017-03-30 10:23:58 EDT
*** Bug 514312 has been marked as a duplicate of this bug. ***
Comment 44 Lars Vogel CLA 2017-03-31 02:00:44 EDT
Sorry for the long processing time of this. The fix was surprisingly easy. We now allow pseudo selectors for preferences. This way the CSS engine will not override the node. So instead of 

IEclipsePreferences#org-eclipse-ui-workbench {
 preferences:
   'key1=value1'
   /* more values */
}

you should use:


IEclipsePreferences#org-eclipse-ui-workbench:a-selector {
 preferences:
   'key1=value1'
   /* more values */
}

I suggest to use the bundle name as selector (keep in mind to replace "." with "-"), e.g.:

IEclipsePreferences#org-eclipse-ui-workbench:org-eclipse-ui-themes { /* pseudo attribute added to allow contributions without replacing this node, see Bug 466075 */
	preferences:
        'key1=value1'
   /* more values */
}


I have adjusted the preference nodes in Platform UI, so even if extender do not do this, they will not override the platform preference styling nodes. Overriding individual values still works.
Comment 45 Matthias Becker CLA 2017-03-31 05:29:57 EDT
(In reply to Lars Vogel from comment #41)
> (In reply to Eclipse Genie from comment #40)
> > New Gerrit change created: https://git.eclipse.org/r/94149
> 
> Matthias, can you test this change? If you have conflicting preference nodes
> you need to give at least on of them a pseudo attribute.

I will try to find some time to test it.
Comment 46 Matthias Becker CLA 2017-03-31 08:39:46 EDT
(In reply to Lars Vogel from comment #41)
> (In reply to Eclipse Genie from comment #40)
> > New Gerrit change created: https://git.eclipse.org/r/94149
> 
> Matthias, can you test this change? If you have conflicting preference nodes
> you need to give at least on of them a pseudo attribute.

Your change is very minimal. Would the following be ok to test:
- Take a neon target platform
- Apply your code changes
- Add my css overriding specific to our plugins

Or are there other changes in Oxygen that are needed to make this work correctly?
Comment 47 Dawid Pakula CLA 2017-04-02 12:26:07 EDT
This not solving a problem, preferences from pseudo classes aren't merged. I'm testing on build I20170401-2000

my css:
IEclipsePreferences#org-eclipse-ui-editors:org-eclipse-php-ui {
  preferences: 
        'PHPReadOccurrenceIndicationColor=27,98,145'
        'PHPWriteOccurrenceIndicationColor=27,98,145'
        'overrideIndicatorColorForPHP=78,120,117'
}

result: all editors have white background in dark theme.
Comment 48 Eclipse Genie CLA 2017-04-02 13:12:16 EDT
New Gerrit change created: https://git.eclipse.org/r/94271
Comment 49 Matthias Becker CLA 2017-04-03 02:16:47 EDT
(In reply to Dawid Pakula from comment #47)
> This not solving a problem, preferences from pseudo classes aren't merged.
> I'm testing on build I20170401-2000
> 
> my css:
> IEclipsePreferences#org-eclipse-ui-editors:org-eclipse-php-ui {
>   preferences: 
>         'PHPReadOccurrenceIndicationColor=27,98,145'
>         'PHPWriteOccurrenceIndicationColor=27,98,145'
>         'overrideIndicatorColorForPHP=78,120,117'
> }
> 
> result: all editors have white background in dark theme.

I did see the same issue on my side. All editor have white background (other color-redefinition also where not handled correctly).

With Dawids additional change in https://git.eclipse.org/r/94271 I don't see issues here on my MacBook. Editor background is dark es expected also the other color-redefintions (e.g. text editor selection background) are considered correctly.
Comment 51 Lars Vogel CLA 2017-04-03 14:10:16 EDT
(In reply to Eclipse Genie from comment #48)
> New Gerrit change created: https://git.eclipse.org/r/94271

Thanks Dawid for the fix and Matthias for testing.
Comment 52 Eclipse Genie CLA 2017-04-03 14:44:58 EDT
New Gerrit change created: https://git.eclipse.org/r/94325
Comment 54 Dawid Pakula CLA 2017-04-03 16:08:13 EDT
Would be good to inform adopters on mailing list about this feature.
Comment 55 Lars Vogel CLA 2017-04-03 16:12:50 EDT
(In reply to Dawid Pakula from comment #54)
> Would be good to inform adopters on mailing list about this feature.

+1 Could you do this David, as you provide the final patch for this?
Comment 56 Matthias Becker CLA 2017-04-04 01:51:48 EDT
Thank you Lars and Dawid for fixing this bug.
Comment 57 Lars Vogel CLA 2017-04-04 02:07:13 EDT
(In reply to Matthias Becker from comment #56)
> Thank you Lars and Dawid for fixing this bug.

In retro-perspective, is was a super easy fix. Sorry for the long time to fix that bug. Would be cool, if you could share at some point the improved ABAP development workbench dark theme with me.
Comment 58 Matthias Becker CLA 2017-04-04 02:13:18 EDT
(In reply to Lars Vogel from comment #57)
> (In reply to Matthias Becker from comment #56)
> > Thank you Lars and Dawid for fixing this bug.
> 
> In retro-perspective, is was a super easy fix. Sorry for the long time to
> fix that bug. Would be cool, if you could share at some point the improved
> ABAP development workbench dark theme with me.

I am working on this as a "pet project". I WILL share it as soon as a first version is available.

Btw: Is it possible to contribute a plugin/fragment only for given platform release?
The reason for my question: The ABAP development tools always have to support the most current and the previous Eclipse platform release (currently Mars / Neon). As soon as Oxygen is available we will support Neon / Oxygen. But as contributing to the dark theme under Neon will break it I am searching for a simple solution to only install this when installed on an Oxygen platform but not when installed on Neon.
Comment 59 Lars Vogel CLA 2017-04-04 02:52:25 EDT
(In reply to Matthias Becker from comment #58)

> Btw: Is it possible to contribute a plugin/fragment only for given platform
> release?

You should be able to define a the minimum version to the 4.7 version of the org.eclipse.e4.ui.css.swt plug-in. This way it will not resolve on earlier releases but on 4.7.
Comment 60 Dawid Pakula CLA 2017-04-04 07:53:46 EDT
(In reply to Matthias Becker from comment #58)
> The reason for my question: The ABAP development tools always have to
> support the most current and the previous Eclipse platform release
> (currently Mars / Neon). As soon as Oxygen is available we will support Neon
> / Oxygen. But as contributing to the dark theme under Neon will break it I
> am searching for a simple solution to only install this when installed on an
> Oxygen platform but not when installed on Neon.

I made simple check and Neon ignore preferences with pseudo classes. So on Neon dark theme will be buggy as before and on Oxygen will work correctly. For PDT project is enough, I hope for your also ;)
Comment 61 Matthias Becker CLA 2017-04-19 10:32:30 EDT
(In reply to Dawid Pakula from comment #60)
> (In reply to Matthias Becker from comment #58)
> > The reason for my question: The ABAP development tools always have to
> > support the most current and the previous Eclipse platform release
> > (currently Mars / Neon). As soon as Oxygen is available we will support Neon
> > / Oxygen. But as contributing to the dark theme under Neon will break it I
> > am searching for a simple solution to only install this when installed on an
> > Oxygen platform but not when installed on Neon.
> 
> I made simple check and Neon ignore preferences with pseudo classes. So on
> Neon dark theme will be buggy as before and on Oxygen will work correctly.
> For PDT project is enough, I hope for your also ;)

Where in the code can I check this? Dawid, can you give me a hint?
Comment 62 Dawid Pakula CLA 2017-04-19 10:53:17 EDT
(In reply to Matthias Becker from comment #61)
> Where in the code can I check this? Dawid, can you give me a hint?

See this: https://git.eclipse.org/c/gerrit/pdt/org.eclipse.pdt.git/tree/plugins/org.eclipse.php.ui/resources/css/dark/preferencestyle.css#n53

Work fine with recent Oxygen SDK integration build.
Comment 63 Matthias Becker CLA 2017-04-19 11:03:14 EDT
(In reply to Dawid Pakula from comment #62)
> (In reply to Matthias Becker from comment #61)
> > Where in the code can I check this? Dawid, can you give me a hint?
> 
> See this:
> https://git.eclipse.org/c/gerrit/pdt/org.eclipse.pdt.git/tree/plugins/org.
> eclipse.php.ui/resources/css/dark/preferencestyle.css#n53
> 
> Work fine with recent Oxygen SDK integration build.

Sorry, I was not precise with my question. I wanted to know the place in the code in neon that ignores preferences with pseudo classes.
Comment 64 Dawid Pakula CLA 2017-04-19 11:06:13 EDT
Neon ignore IEclipsePreferences#plugin-id css elements if have any pseudo class by design. You don't need any code.

In my case, Neon ignore this css IEclipsePreferences#org-eclipse-ui-editors:org-eclipse-php-ui {}
Comment 65 Leo Ufimtsev CLA 2017-05-01 16:27:45 EDT
SUMMARY of comments 1-64
########################

I ran into this issue and to summarize the fix for the next reader:

# PROBLEM:
if you have two plugins with preference definitions, the last one overrides the 
previous definitions instead of adding items:

PluginA:
IEclipsePreferences#org-eclipse-ui-editors {
	preferences:'colorA=255,0,0'
}

PluginB: (denfinition overrides and colorA is lost)
IEclipsePreferences#org-eclipse-ui-editors {
	preferences: 'colorB=0,0,0'
}

# SOLUTION:
Add a pseudo attribe, which is the id of your bundle, making it unique.
E.g ":org.PLUGIN-ID"

IEclipsePreferences#org-eclipse-ui-editors:org.PLUGIN-A {
	preferences:'colorA=255,0,0'
}

IEclipsePreferences#org-eclipse-ui-editors:org.PLUGIN-B {
	preferences: 'colorB=0,0,0'
}

Example patch that utilizes this:
https://git.eclipse.org/r/#/c/94149/4/bundles/org.eclipse.ui.themes/css/dark/e4-dark_preferencestyle.css


(Thank you for everyone involved in this fix).
Comment 66 Lars Vogel CLA 2017-05-01 16:46:05 EDT
Leo, IIRC you cannot use "." In the pseudo selector. Please all dots with "-".