Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [eclipse-pmc] Approval for Bug 516114 to add support for styling of the tabbed properties view

The contribution is tested on Win10, Suse Linux and Mac.

 

From: <eclipse-pmc-bounces@xxxxxxxxxxx> on behalf of "Oberhuber, Martin" <Martin.Oberhuber@xxxxxxxxxxxxx>
Reply-To: "eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
Date: Thursday, 11. May 2017 at 18:12
To: "eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
Subject: Re: [eclipse-pmc] Approval for Bug 516114 to add support for styling of the tabbed properties view

 

Hi Dani, all -

 

Thanks very much for your diligent checking, and asking hard questions.

 

I am deeply convinced that maintaining the very high quality standards of the Eclipse Platform is key. Maintaining this requires sticking to plans and saying “no” to things even if they look compelling. On the other hand, there is genuine interest and the contribution looks solid to me from what I have seen (though I don’t really know the domain of e4/CSS so have to trust Lars’ statements regarding the risk and possible implications).

 

I’ve had a quick look, and we are talking about 17 files (some of them new) with approx. 380 LOC code plus approx. 180 LOC tests. To me, this looks like a complexity that is manageable. Given Lars’ explanations I am +1 for this change – would have been better to get into M7 candidate to make the test pass. As it appears to be *new* functionality affecting the dark theme only I am OK if submitter is testing on all arches. I’ll probably switch my own dev. Env to dark theme once I get hold of a build that includes this.

 

Bottomline, +1 from me but this is *not* a precedent for sneaking stuff in that late and I’ll rather say no next time.

 

Thanks,

Martin

--

Martin Oberhuber, SMTS / Product Owner – Development Tools, Wind River

 

From: <eclipse-pmc-bounces@xxxxxxxxxxx> on behalf of "daniel_megert@xxxxxxxxxx" <daniel_megert@xxxxxxxxxx>
Reply-To: "eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
Date: Thursday 11 May 2017 at 15:25
To: "eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
Subject: Re: [eclipse-pmc] Approval for Bug 516114 to add support for styling of the tabbed properties view

 

Thanks for the details. For me it's still quite some new code for RC1.

I will abstain from the vote and like to hear what other PMC members say.

Dani



From:        Lars Vogel <lars.vogel@xxxxxxxxxxx>
To:        eclipse-pmc@xxxxxxxxxxx
Date:        10.05.2017 18:43
Subject:        Re: [eclipse-pmc] Approval for Bug 516114 to add support for styling of the tabbed properties view
Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx





On Wed, May 10, 2017 at 6:26 PM, Daniel Megert <daniel_megert@xxxxxxxxxx> wrote:
> A lot of stuff is called by the CSS engine ;-)

Indeed. :-)

> Which CSS property? Where do I see this restriction in the code and that it
> is restricted to the Dark theme?

The new properties are in the change. See
https://git.eclipse.org/r/#/c/96470/6/bundles/org.eclipse.ui.views.properties.tabbed/plugin.xml

If the CSS engine finds such a property for a registered CSS element
(in most cases widgets, but we also have other handlers for example
for preferences), it will call the adapter to adapt the widget to the
CSS engine. The registered handler will afterwards be called. A
handler is also registered via the plugin.xml.

In Matthias case we have two handlers,
https://git.eclipse.org/r/#/c/96470/6/bundles/org.eclipse.ui.views.properties.tabbed/src/org/eclipse/ui/internal/views/properties/tabbed/css/TabbedPropertyListCssPropertyHandler.java
and
https://git.eclipse.org/r/#/c/96470/6/bundles/org.eclipse.ui.views.properties.tabbed/src/org/eclipse/ui/internal/views/properties/tabbed/css/TabbedPropertyTitleCssPropertyHandler.java

Based on your concerns I asked Matthias via the Gerrit to add a test
that if the CSS property is not set that the default colors are not
changed.

Best regards, Lars


>
> Dani
>
>
>
> From:        Lars Vogel <lars.vogel@xxxxxxxxxxx>
> To:        eclipse-pmc@xxxxxxxxxxx
> Date:        10.05.2017 18:13
> Subject:        Re: [eclipse-pmc] Approval for Bug 516114 to add support for
> styling of the tabbed properties view
> Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx
> ________________________________
>
>
>
> These new internal methods are only called by the CSS engine -> no
> influence on the other themes if the CSS property is not set.
>
>
>
>
>
> On Wed, May 10, 2017 at 6:10 PM, Daniel Megert <daniel_megert@xxxxxxxxxx>
> wrote:
>> Please explain which changes e.g. in TabbedPropertyList.java only affect
>> the
>> dark theme? And also in all the other Java classes.
>>
>> Dani
>>
>>
>>
>> From:        Lars Vogel <lars.vogel@xxxxxxxxxxx>
>> To:        eclipse-pmc@xxxxxxxxxxx
>> Date:        10.05.2017 18:05
>> Subject:        Re: [eclipse-pmc] Approval for Bug 516114 to add support
>> for
>> styling of the tabbed properties view
>> Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx
>> ________________________________
>>
>>
>>
>> On Wed, May 10, 2017 at 6:00 PM, Daniel Megert <daniel_megert@xxxxxxxxxx>
>> wrote:
>>>> Dani, the patch will only influence the dark theme.
>>>
>>> I strongly disagree. None of the code changes I quickly looked at are
>>> restricted to a specific theme.
>>
>> Yes, they are. They way this works in the CSS engine is via the CSS
>> file. See that in
https://git.eclipse.org/r/#/c/96470/onlythedark
>
>>
>> theme css has changed
>> (bundles/org.eclipse.ui.themes/css/dark/e4-dark_globalstyle.css). All
>> other css files are unchanged.
>>
>>
>>
>>>
>>> Dani
>>>
>>>
>>>
>>> From:        Lars Vogel <lars.vogel@xxxxxxxxxxx>
>>> To:        eclipse-pmc@xxxxxxxxxxx
>>> Date:        10.05.2017 17:52
>>> Subject:        Re: [eclipse-pmc] Approval for Bug 516114 to add support
>>> for
>>> styling of the tabbed properties view
>>>
>>> Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx
>>> ________________________________
>>>
>>>
>>>
>>> Dani, the patch will only influence the dark theme. The CSS engine is
>>> implemented in a way that it only applies a CSS handler  if a CSS
>>> property
>>> is set. The patch from Matthias define styling only for the dark theme.
>>>
>>> Verification has been done on Windows and Linux. The CSS engine does rely
>>> on
>>> setters of the individual SWT widgets, so any giving limitation for SWT
>>> widgets will also apply for the CSS engine.
>>>
>>> Matthias, do you have access to a Mac to validate the changes on a Mac?
>>>
>>>> To me the change looks too big for RC1 and I'd vote to move it to 4.7.1.
>>>
>>> I think this moves it to 4.7.1 unless Dani changes his mind.
>>>
>>> Best regards, Lars
>>>
>>>
>>> Am 10.05.2017 5:21 nachm. schrieb "Daniel Megert"
>>> <daniel_megert@xxxxxxxxxx>:
>>> Did you verify the change on all platforms and all themes?
>>>
>>> To me the change looks too big for RC1 and I'd vote to move it to 4.7.1.
>>>
>>> Dani
>>>
>>>
>>>
>>> From:        "Becker, Matthias" <ma.becker@xxxxxxx>
>>> To:        "eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
>>> Date:        10.05.2017 08:58
>>> Subject:        [eclipse-pmc] Approval for Bug 516114 to add support for
>>> styling of the tabbed properties view
>>> Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx
>>> ________________________________
>>>
>>>
>>>
>>> Dear PMC,
>>>
>>> I would like to ask for PMC permissions for “Bug 516114- [CSS] [Dark] Add
>>> support for styling of TabbedProperties in Properties View (and apply to
>>> dark layout)”.
>>>
>>> This fix is need to provide the CSS styling implementation for the tabbed
>>> properties view and to provide the css for the dark theme. The tabbed
>>> properties view is used in the “Resource” perspective and also in SAPs
>>> “ABAP
>>> Development Tools” (
>>
>
https://tools.hana.ondemand.com/#abap). As the “ABAP
>
>>
>>> Development Tools” use the tabbed properties view extensively (I think
>>> other
>>> products do as well) our Team currently cannot provide support for the
>>> dark
>>> theme.I really would like to have this in Oxygen so that Eclipse can
>>> deliver
>>> a relative decent dark theme with Oxygen. In addition, it’s my plan to
>>> provide the first support of the dark theme in the “ABAP Development
>>> Tools”
>>> in our Oxygen-based version.
>>>
>>> The implementation took the implementation of “Bug 465148- [CSS] [Dark]
>>> Add
>>> support for styling Forms (and apply to dark layout)” as a blueprint. The
>>> change also contains Unit tests. The changes are limited to the
>>> org.eclipse.ui.views.properties.tabbed bundle. Hence the risk should be
>>> low.
>>> If we do not deliver it we have an inconsistency in our UI.
>>>
>>> This is my first code contribution to the eclipse platform ui project. Up
>>> to
>>> now I only have contributed a HiDPI Icons to various eclipse projects.
>>> This
>>> change still needs the final code review by the Platform UI team. Lars
>>> Vogel
>>> is willing to do a detailed code review after PMC approval.
>>>
>>> Best regards,
>>> Matthias Becker
>>> (SAP SE)_______________________________________________
>>> eclipse-pmc mailing list
>>> eclipse-pmc@xxxxxxxxxxx
>>> To change your delivery options, retrieve your password, or unsubscribe
>>> from
>>> this list, visit
>>>
>>
>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>
>>
>>>
>>>
>>>
>>> _______________________________________________
>>> eclipse-pmc mailing list
>>> eclipse-pmc@xxxxxxxxxxx
>>> To change your delivery options, retrieve your password, or unsubscribe
>>> from
>>> this list, visit
>>>
>>>
>>>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc_______________________________________________
>>> eclipse-pmc mailing list
>>> eclipse-pmc@xxxxxxxxxxx
>>> To change your delivery options, retrieve your password, or unsubscribe
>>> from
>>> this list, visit
>>>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>>>
>>>
>>>
>>> _______________________________________________
>>> eclipse-pmc mailing list
>>> eclipse-pmc@xxxxxxxxxxx
>>> To change your delivery options, retrieve your password, or unsubscribe
>>> from
>>> this list, visit
>>>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>>
>>
>>
>> --
>> Eclipse Platform UI and e4 project co-lead
>> CEO vogella GmbH
>>
>> Haindaalwisch 17a, 22395 Hamburg
>> Amtsgericht Hamburg: HRB 127058
>> Geschäftsführer: Lars Vogel, Jennifer Nerlich de Vogel
>> USt-IdNr.: DE284122352
>> Fax (040) 5247 6322, Email: lars.vogel@xxxxxxxxxxx, Web:
>>
http://www.vogella.com
>> _______________________________________________
>> eclipse-pmc mailing list
>> eclipse-pmc@xxxxxxxxxxx
>> To change your delivery options, retrieve your password, or unsubscribe
>> from
>> this list, visit
>>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>>
>>
>>
>> _______________________________________________
>> eclipse-pmc mailing list
>> eclipse-pmc@xxxxxxxxxxx
>> To change your delivery options, retrieve your password, or unsubscribe
>> from
>> this list, visit
>>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>
>
>
> --
> Eclipse Platform UI and e4 project co-lead
> CEO vogella GmbH
>
> Haindaalwisch 17a, 22395 Hamburg
> Amtsgericht Hamburg: HRB 127058
> Geschäftsführer: Lars Vogel, Jennifer Nerlich de Vogel
> USt-IdNr.: DE284122352
> Fax (040) 5247 6322, Email: lars.vogel@xxxxxxxxxxx, Web:
>
http://www.vogella.com
> _______________________________________________
> eclipse-pmc mailing list
> eclipse-pmc@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from
> this list, visit
>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc
>
>
>
> _______________________________________________
> eclipse-pmc mailing list
> eclipse-pmc@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from
> this list, visit
>
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc



--
Eclipse Platform UI and e4 project co-lead
CEO vogella GmbH

Haindaalwisch 17a, 22395 Hamburg
Amtsgericht Hamburg: HRB 127058
Geschäftsführer: Lars Vogel, Jennifer Nerlich de Vogel
USt-IdNr.: DE284122352
Fax (040) 5247 6322, Email: lars.vogel@xxxxxxxxxxx, Web:
http://www.vogella.com
_______________________________________________
eclipse-pmc mailing list
eclipse-pmc@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipse-pmc





Back to the top