Community
Participate
Working Groups
Created attachment 268152 [details] error (with header) See attached screenshot. The Properties View in Oxygen is not readable in the dark theme.
New Gerrit change created: https://git.eclipse.org/r/96470
I just uploaded a very first implementation. Up to now only the TabbedPropertyTitle is considered. Styling of TabbedPropertyList is not yet implemented. Can somebody who is familiar with this topic tell me if I am going into the right direction? Should we make it possible to style the different colors IFormColors.H_GRADIENT_START, IFormColors.H_GRADIENT_END and IFormColors.H_BOTTOM_KEYLINE1, IFormColors.H_BOTTOM_KEYLINE2 independently? Why does the setting of the foreground of the CLabel show no effect (label stays white instead of the light blue as in form headings)?
(In reply to Matthias Becker from comment #2) > Why does the setting of the foreground of the CLabel show no effect (label > stays white instead of the light blue as in form headings)? I just figured this out. Styling of CLabels is already handles by CSSPropertyTextSWTHandler. So I only needed to add: TabbedPropertyTitle > CLabel{ color: #9AC9D8; } to the CSS.
From a feature perspective my change is finished. Tested on my MacBook. In addition I will test on windows. Can someone test on the other platforms? Can someone pls. do a code review for this change? I have added multipe reInit*Color-Methods to https://git.eclipse.org/r/#/c/96470/4/bundles/org.eclipse.ui.views.properties.tabbed/src/org/eclipse/ui/internal/views/properties/tabbed/view/TabbedPropertyList.java and https://git.eclipse.org/r/#/c/96470/4/bundles/org.eclipse.ui.views.properties.tabbed/src/org/eclipse/ui/internal/views/properties/tabbed/view/TabbedPropertyTitle.java Is this ok to add these public boiler-blade methods. Has anybody a smarter idea? Do we still have a chance to add this in the Oxygen release?
(In reply to Matthias Becker from comment #4) > Do we still have a chance to add this in the Oxygen release? Definitely possible for 4.7.1. If you need it for 4.7 please send an email to the PMC mailing list (https://dev.eclipse.org/mailman/listinfo/eclipse-pmc) asking for permission to merge this. Please add add some tests for this feature in your review.
(In reply to Lars Vogel from comment #5) > (In reply to Matthias Becker from comment #4) > Please add add some tests for this feature in your review. What kind of test do you have in mind? Test for my changes in the TabbedProperties* classes or tests of the complete theming round trip? Can you point me to similar existing tests?
(In reply to Matthias Becker from comment #6) > (In reply to Lars Vogel from comment #5) > > (In reply to Matthias Becker from comment #4) > > Please add add some tests for this feature in your review. > What kind of test do you have in mind? Test for my changes in the > TabbedProperties* > classes or tests of the complete theming round trip? Can you point me to > similar existing tests? Plug-in is org.eclipse.e4.ui.tests.css.swt. An example for a recently added test is ExpandableCompositeTest. For example, we test also the reset (which is called during theme switching), which I think is at the moment still missing in your code.
Created attachment 268237 [details] Screenshot showing the error
Created attachment 268238 [details] Screenshot showing the error (when hovering over not active tab)
Created attachment 268239 [details] Screenshot showing the error (when scrolling is required)
Created attachment 268240 [details] With fix included
Created attachment 268241 [details] With fix included (when hovering over not active tab)
Created attachment 268242 [details] With fix included (when scrolling is required)
This can easily tested with the tabbed properties view example from https://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/examples/org.eclipse.ui.examples.views.properties.tabbed.article The tabbed properties view is visible in the resource perspective. More details about the example in https://www.eclipse.org/articles/Article-Properties-View/properties-view.html
(In reply to Lars Vogel from comment #7) > (In reply to Matthias Becker from comment #6) > > (In reply to Lars Vogel from comment #5) > > > (In reply to Matthias Becker from comment #4) > > > Please add add some tests for this feature in your review. > > What kind of test do you have in mind? Test for my changes in the > > TabbedProperties* > > classes or tests of the complete theming round trip? Can you point me to > > similar existing tests? > > Plug-in is org.eclipse.e4.ui.tests.css.swt. An example for a recently added > test is ExpandableCompositeTest. For example, we test also the reset (which > is called during theme switching), which I think is at the moment still > missing in your code. Ok got it. Should I put the test into the org.eclipse.ui.tests.views.properties.tabbed bundle or into the org.eclipse.e4.ui.tests.css.swt bundle?
(In reply to Lars Vogel from comment #5) > (In reply to Matthias Becker from comment #4) > Please add add some tests for this feature in your review. Done with patchset 5. Do I have to add the new testclasses into a test suite somewhere? Is there still something "formal" missing in my change?
(In reply to Matthias Becker from comment #16) > Is there still something "formal" missing in my change? No as far as I can see. Please ask PMC for approval.
Created attachment 268259 [details] With fix included (with header)
Do I still have to do something so that it get's merged?
(In reply to Matthias Becker from comment #19) > Do I still have to do something so that it get's merged? I plan to give it a good test today and merge it, if I find no issues.
Gerrit change https://git.eclipse.org/r/96470 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f271d558e1b6e5d6ae2aa058d23487d5244078ab
Thank you Matthias.
Thank YOU Lars for helping with the approval process details and testing
(In reply to Matthias Becker from comment #23) > Thank YOU Lars for helping with the approval process details and testing No worries. We do have a test failure for the newly added tests. Opened Bug 516697 to handle that.
This causes test failure in org.eclipse.jdt.text.tests: http://download.eclipse.org/eclipse/downloads/drops4/I20170515-2000/testresults/html/org.eclipse.jdt.text.tests_ep47I-unit-cen64-gtk2_linux.gtk.x86_64_8.0.html testPluginsNotLoaded Wrong bundles loaded: - org.eclipse.ui.views.properties.tabbed junit.framework.AssertionFailedError: Wrong bundles loaded: - org.eclipse.ui.views.properties.tabbed at org.eclipse.jdt.text.tests.PluginsNotLoadedTest.testPluginsNotLoaded(PluginsNotLoadedTest.java:284) ...
New Gerrit change created: https://git.eclipse.org/r/97208
(In reply to Noopur Gupta from comment #25) > This causes test failure in org.eclipse.jdt.text.tests: > http://download.eclipse.org/eclipse/downloads/drops4/I20170515-2000/ > testresults/html/org.eclipse.jdt.text.tests_ep47I-unit-cen64-gtk2_linux.gtk. > x86_64_8.0.html > > testPluginsNotLoaded > > Wrong bundles loaded: - org.eclipse.ui.views.properties.tabbed > > junit.framework.AssertionFailedError: Wrong bundles loaded: > - org.eclipse.ui.views.properties.tabbed > > at > org.eclipse.jdt.text.tests.PluginsNotLoadedTest. > testPluginsNotLoaded(PluginsNotLoadedTest.java:284) > ... see my gerrit change in jdt.ui for this
(In reply to Matthias Becker from comment #27) > (In reply to Noopur Gupta from comment #25) > > This causes test failure in org.eclipse.jdt.text.tests: > > http://download.eclipse.org/eclipse/downloads/drops4/I20170515-2000/ > > testresults/html/org.eclipse.jdt.text.tests_ep47I-unit-cen64-gtk2_linux.gtk. > > x86_64_8.0.html > > > > testPluginsNotLoaded > > > > Wrong bundles loaded: - org.eclipse.ui.views.properties.tabbed > > > > junit.framework.AssertionFailedError: Wrong bundles loaded: > > - org.eclipse.ui.views.properties.tabbed > > > > at > > org.eclipse.jdt.text.tests.PluginsNotLoadedTest. > > testPluginsNotLoaded(PluginsNotLoadedTest.java:284) > > ... > > see my gerrit change in jdt.ui for this Disabling the test to fix the problem is not really a fix. The test and many other clients in the world do not make use of the properties view and hence loading it upfront is a no go.
(In reply to Matthias Becker from comment #27) > (In reply to Noopur Gupta from comment #25) > > This causes test failure in org.eclipse.jdt.text.tests: > > http://download.eclipse.org/eclipse/downloads/drops4/I20170515-2000/ > > testresults/html/org.eclipse.jdt.text.tests_ep47I-unit-cen64-gtk2_linux.gtk. > > x86_64_8.0.html > > > > testPluginsNotLoaded > > > > Wrong bundles loaded: - org.eclipse.ui.views.properties.tabbed > > > > junit.framework.AssertionFailedError: Wrong bundles loaded: > > - org.eclipse.ui.views.properties.tabbed > > > > at > > org.eclipse.jdt.text.tests.PluginsNotLoadedTest. > > testPluginsNotLoaded(PluginsNotLoadedTest.java:284) > > ... > > see my gerrit change in jdt.ui for this Can you explain why disabling the jdt.ui test for org.eclipse.ui.views.properties.tabbed is required now?
(In reply to Noopur Gupta from comment #29) > Can you explain why disabling the jdt.ui test for > org.eclipse.ui.views.properties.tabbed is required now? This is because: In the css file for the dark theme we now have some entries for the new properties. At startup the "PartRenderingEngine" calls "initializeStyling" which reads all the extensions for the org.eclipse.e4.ui.css.core.propertyHandler extension point (in the constructor of RegistryCSSPropertyHandlerProvider) and this calls createExecutableExtension() of the ConfigurationElementHanle. And this loads class org.eclipse.ui.internal.views.properties.tabbed.css.TabbedPropertyTitleCssPropertyHandler. So this is an issue that got not introduces by my change - it made it only visible.
(In reply to Matthias Becker from comment #30) > (In reply to Noopur Gupta from comment #29) > > Can you explain why disabling the jdt.ui test for > > org.eclipse.ui.views.properties.tabbed is required now? > > This is because: > In the css file for the dark theme we now have some entries for the new > properties. > At startup the "PartRenderingEngine" calls "initializeStyling" which reads > all the extensions for the org.eclipse.e4.ui.css.core.propertyHandler > extension point (in the constructor of RegistryCSSPropertyHandlerProvider) > and this calls > createExecutableExtension() of the ConfigurationElementHanle. And this loads > class > org.eclipse.ui.internal.views.properties.tabbed.css.TabbedPropertyTitleCssPropertyHandler. > > > So this is an issue that got not introduces by my change - it made it only > visible. The PMC approval was based on the assertions that no side-effect will happen for non-dark theme clients. This is obviously not true. Those things should be loaded lazy. Introducing code that leads to additional plug-in loading during RC1 is a -1 for me. I'll raise this again during the PMC call.
(In reply to Dani Megert from comment #31) > The PMC approval was based on the assertions that no side-effect will happen > for non-dark theme clients. This is obviously not true. Those things should > be loaded lazy. I looked at the code and to me it will be a major effort for avoid eager activation of the plug-in. Nothing we can do for 4.7.0. > Introducing code that leads to additional plug-in loading during RC1 is a -1 > for me. I'll raise this again during the PMC call. I'm also unhappy about additional activation, that could slow done the start of Eclipse. +0 for this. That means I have no strong objections if the rest of the PMC decides to revert this and move it to a later release). Matthias could use a feature patch for 4.7.0. I also think we should replicate the JDT test for platform UI to avoid such things for the future. I will open a bug for this.
(In reply to Lars Vogel from comment #32) > (In reply to Dani Megert from comment #31) > > The PMC approval was based on the assertions that no side-effect will happen > > for non-dark theme clients. This is obviously not true. Those things should > > be loaded lazy. > > I looked at the code and to me it will be a major effort for avoid eager > activation of the plug-in. Nothing we can do for 4.7.0. Yes, definitely not something for 4.7. It's very good it revealed this weakness. Imagine we would load every plug-in that contributes e.g. an action! > > Introducing code that leads to additional plug-in loading during RC1 is a -1 > > for me. I'll raise this again during the PMC call. > > I'm also unhappy about additional activation, that could slow done the start > of Eclipse. Especially since our contributors spent a lot of time to reduce the startup time. > I also think we should replicate the JDT test for platform UI to avoid such > things for the future. I will open a bug for this. A big +1!
The PMC decided that the activation of the plug-in so late in the release cycle it to risky. IMHO we should reconsider this for 4.7.1. If we can remove the activator (see Bug 516738) the need for activation would be removed. Retargeting for 4.7.1. Revert will follow shortly.
New Gerrit change created: https://git.eclipse.org/r/97259
Gerrit change https://git.eclipse.org/r/97259 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8d5c4c7bdc6deb2ce688d5003df17f3485b5cd56
(In reply to Eclipse Genie from comment #35) > New Gerrit change created: https://git.eclipse.org/r/97259 Can we at least keep this in the CSS: TabbedPropertyTitle > CLabel{ color: #9AC9D8; } I does make the titlebar readable and no new impl. was neede for this.
(In reply to Lars Vogel from comment #32) > Matthias could use a feature patch for 4.7.0. What is a feature patch?
(In reply to Matthias Becker from comment #37) > TabbedPropertyTitle > CLabel{ > color: #9AC9D8; > } > Fine for me, this a good polish item for RC1. Please open new bug, add a Gerrit. IIRC the deadline for RC1 changes is tomorrow.
(In reply to Matthias Becker from comment #39) > What is a feature patch? See http://www.vogella.com/tutorials/EclipsePatching/article.html
(In reply to Lars Vogel from comment #40) > (In reply to Matthias Becker from comment #37) > > > TabbedPropertyTitle > CLabel{ > > color: #9AC9D8; > > } > > > > Fine for me, this a good polish item for RC1. Please open new bug, add a > Gerrit. IIRC the deadline for RC1 changes is tomorrow. See Bug 516744 for this.
New Gerrit change created: https://git.eclipse.org/r/97337
(In reply to Eclipse Genie from comment #43) > New Gerrit change created: https://git.eclipse.org/r/97337 This is my proposal for the delayed creation of the property handlers. Manual tests showed that with this change the bundle that contributes the property handler is not activated at startup. The activation is delayed to the point in time when property handler is needed. In the properties view example this means it only activates the org.eclipse.ui.views.properties.tabbed bundle when the properties view is open and when the dark theme is active. Using the dark theme without using the properties view does not activate the org.eclipse.ui.views.properties.tabbed bundle.
New Gerrit change created: https://git.eclipse.org/r/97429
(In reply to Eclipse Genie from comment #45) > New Gerrit change created: https://git.eclipse.org/r/97429 Noopur, if I run the PluginsNotLoadedTest with or without this change, I always get "Wrong bundles loaded - org.eclipse.search". I'm only running PluginsNotLoadedTest. Does it maybe depend on another test?
Created attachment 268929 [details] PluginsNotLoadedTest - Screenshot (In reply to Lars Vogel from comment #46) > (In reply to Eclipse Genie from comment #45) > > New Gerrit change created: https://git.eclipse.org/r/97429 > > Noopur, if I run the PluginsNotLoadedTest with or without this change, I > always get "Wrong bundles loaded - org.eclipse.search". I'm only running > PluginsNotLoadedTest. Does it maybe depend on another test? It does not fail when I run it. Please try in a new workspace with only the required plug-ins (see attached screenshot).
I am lost in the various changes. Sorry. Is may change (to enable styling) part of the master again?
(In reply to Matthias Becker from comment #48) > I am lost in the various changes. Sorry. > Is may change (to enable styling) part of the master again? All good on your side, I plan to merge this soon (after trying once more to run the JDT unit test).
(In reply to Noopur Gupta from comment #47) > It does not fail when I run it. Please try in a new workspace with only the > required plug-ins (see attached screenshot). Thanks for the detailed tip. This works. I basically had to remove all the installed components from the runtime.
Gerrit change https://git.eclipse.org/r/97429 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=907961ad1c05cf80188ce46b86fc2bfa8b98ea28
Thanks, Matthias, I validated that with the change in Bug 516738 the JDT UI test PluginsNotLoadedTest is not failing anymore locally for me. Thanks to Noopur for the help with the test. I leave this bug open to downport it together with Bug 516738. @Matthias, please validate the change in tomorrow's I-Build, see email list platform-releng-dev@eclipse.org for the next download link, we do currently do not update our download page.
(In reply to Lars Vogel from comment #52) > @Matthias, please validate the change in tomorrow's I-Build Done. Tabbed Properties View is correctly styled there. Do I have to do something for the downport?
New Gerrit change created: https://git.eclipse.org/r/100468
(In reply to Matthias Becker from comment #53) > (In reply to Lars Vogel from comment #52) > > @Matthias, please validate the change in tomorrow's I-Build > > Done. Tabbed Properties View is correctly styled there. > > Do I have to do something for the downport? I have prepared cherry-picks to R4_7_maintenance locally for this bug and for bug 516738. But I cannot push them. I get the following error message: prohibited by Gerrit Branch refs/heads/R4_7_maintenance: You are not allowed to perform this operation. To push into this reference you need 'Push' rights. User: mbeckerg51 Is this branch still closed?
New Gerrit change created: https://git.eclipse.org/r/100679
New Gerrit change created: https://git.eclipse.org/r/100681
New Gerrit change created: https://git.eclipse.org/r/100688
Gerrit change https://git.eclipse.org/r/100688 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=86e1ce5e7d561471d92757a2f6510c433af08c6d
Gerrit change https://git.eclipse.org/r/100681 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0db8b78835077f16b96586fa7d979fc6439e937a
Thank you, Matthias for your contributions.
The fix forgot to export the package. This could have be seen noticed due to the the warning on the manifest file. Fixed in master with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4b839f47dabf7de4b76d4224a4fe0999122e5bf8 Fixed in R4_7_maintenance with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ba49de0d18f35f57ee67bc785845a616adc9eb60&h=R4_7_maintenance
(In reply to Dani Megert from comment #62) > The fix forgot to export the package. This could have be seen noticed due to > the the warning on the manifest file. > > Fixed in master with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=4b839f47dabf7de4b76d4224a4fe0999122e5bf8 > > Fixed in R4_7_maintenance with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=ba49de0d18f35f57ee67bc785845a616adc9eb60&h=R4_7_maintenance Thanks Dani for fixing this.
New Gerrit change created: https://git.eclipse.org/r/101934
Gerrit change https://git.eclipse.org/r/101934 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=db6a352f061ce874849e3e7ce98a4b4e7e057a3c
New Gerrit change created: https://git.eclipse.org/r/102787
Gerrit change https://git.eclipse.org/r/102787 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=52e14cdb3d0dff7d7f8d70fab624d1103dbe7f18
