Hi Darkstone, I would prefer 1A) which is to revert the change, prove that Type Selectors work, and change the documentation for SkinnableComponent to guide people to use Type Selectors. Then overriding styleName should not be needed, and folks will build components following the same pattern as the rest of the SDK.
Thoughts? -Alex On 12/17/14, 12:53 PM, "DarkStone" <darkst...@163.com> wrote: >Hi Alex, > >I got what you saying, I understand why you don't agree to the changes, >you've got your point, I think you are right. > >But that won't prevent others from doing it in the wrong ways. > >There is a paragraph in SkinnableComponent online help about implementing >the constructor: > >"Use the constructor to set the initial values of class properties. For >example, you can set default values for properties and styles, or >initialize data structures, such as Arrays. You can also set the >skinClass style to the name of your skin class." >http://help.adobe.com/en_US/flex/using/WS460ee381960520ad-2811830c121e9107 >ecb-7ff9.html#WS460ee381960520ad-2811830c121e9107ecb-7ff7 > >It encourages developers to write setStyle("skinClass", MyDefaultSkin) to >the constructor, if they want to set default skin for it. > >But doing so, set styleName on the MXML won't change the skin: ><ns:MyComponent styleName="otherSkin"/> > >This will cause confusion, because built in Spark components like Button, >CheckBox, they all have their default skins, and if developers want to >use a different skin, they do this: ><s:Button styleName="myButtonSkin"/> > >The confusion is: > ><s:Button styleName="myButtonSkin"/> is working, the skin changes to >MyButtonSkin. > ><ns:MyComponent styleName="otherSkin"/> is not working, the skin remains >default. > >They would be wondering why, and would assume this is a bug. > >So, the way I see it, there are two choices: > >1. Don't treat this as a bug, revert the changes I made, and update the >help docs about SkinnableComponent, tell developers they have to override >the styleName setter function by themselves, in order to make this case >work. > >2. Treat this as a bug, apply the changes I made. > >Both are OK for me, I still prefer the 2nd one, but I fully respect other >committers and PMC members' opinions. > > >DarkStone >2014-12-18 > > >在 2014-12-18 03:28:49，"Alex Harui" <aha...@adobe.com> 写道： >>Hi Darkstone, >> >>Think of the general case. Let’s say that you also call: >> >>myComponent.setStyle(“fontWeight", “bold"); >> >>myComponent.setStyle(“color", #ff0000); >> >> >>When you later call: >> >>myComponent.styleName = “otherSkin” and you have >> >>.otherskin { >> skinClass : MyOtherSkin; >> fontWeight : “normal”; >> color : #00FF00; >>} >> >>Why should skinClass be given special treatment? There is an order to >>the >>CSS lookup. Styles set on an instance override other styles specified in >>other selectors. I don’t think we should change that order, and I don’t >>think we should try to figure out what instance styles to clear. It >>isn’t >>really clear to me that folks will always want those styles cleared. >> >>It sounds like what your friend really wants is to specify a Type >>Selector >>for MyComponent and specify the default skin there instead of calling >>setStyle on each instance. Then styles set that have higher priority >>like >>styleName and instance styles will override with the intended result. >> >>CSS is expected to work a certain way. I don’t think we should change >>it, >>and I think your friend should be specifying a Type Selector. >> >>HTH, >>-Alex >> >> >>On 12/17/14, 10:06 AM, "DarkStone" <darkst...@163.com> wrote: >> >>>Hi Alex, >>> >>>Actually, this bug is reported to me by a Flex Developer, who is a >>>friend >>>of mine. >>> >>>The demand is, he wants to set a default skin class for his component >>>(which extends SkinnableComponent), so inside the constructor of his >>>component, he writes this: >>>setStyle("skinClass", MyDefaultSkin); >>>super(); >>> >>>Then, he wants other people to apply a different skin if they want to, >>>other people would write this: >>><ns:MyComponent styleName="otherSkin"/> >>> >>>I think his demand is very common, now the problem is, by doing so, the >>>skin won't change. >>> >>>This bug only affects on custom SkinnableComponent, but it doesn't >>>affect >>>on any built in Flex Components like Spark Button, CheckBox, etc.. >>> >>>That's why I think it is a bug : ) >>> >>> >>>DarkStone >>>2014-12-18 >>> >>> >>>At 2014-12-18 01:30:23, "Alex Harui" <aha...@adobe.com> wrote: >>>>Hi Darkstone, >>>> >>>>By this description, this is not a bug. Any style set via setStyle >>>>overrides styles in CSS selectors. The more common case is someone >>>>does >>>>want to always have myComponent use SkinA regardless of underlying >>>>changes >>>>to the selectors, like when style modules are loaded. If you want an >>>>overridden style to revert to using the styles of the selectors, then >>>>you >>>>have to call clearStyle in your code. >>>> >>>>-Alex >>>> >>>> >>>>On 12/17/14, 2:05 AM, "DarkStone" <darkst...@163.com> wrote: >>>> >>>>>Hi Alex, >>>>> >>>>>I've located the problem: >>>>> >>>>>Step 1. Using MySkinnableComponent.setStyle("skinClass", SkinA) to >>>>>apply >>>>>an initial skin is ok. >>>>> >>>>>Step 2. Then using MySkinnableComponent.styleName = "skinB" won't >>>>>work, >>>>>the skin won't change. >>>>> >>>>>When [Step 2] happens, inside the validateSkinChange() function of >>>>>SkinnableComponent.as (line 440-443): >>>>> >>>>>newSkinClass = getStyle("skinClass"); >>>>>skipReload = newSkinClass && getQualifiedClassName(newSkinClass) == >>>>>getQualifiedClassName(_skin); >>>>> >>>>>The getStyle("skinClass") returns the old skin (SkinA), which should >>>>>be >>>>>the new skin (SkinB). >>>>> >>>>>As a result, the newSkinClass && getQualifiedClassName(newSkinClass) >>>>>== >>>>>getQualifiedClassName(_skin) returns true, which should be false. >>>>> >>>>>That's why the SkinnableComponent won't change the skin. >>>>> >>>>> >>>>> >>>>>I know how to fix this now. >>>>> >>>>>No need to override the styleName setter function in >>>>>SkinnableComponent.as. >>>>> >>>>>Just add a line clearStyle("skinClass") to styleChanged() function in >>>>>SkinnableComponent.as (add at line 552): >>>>> >>>>>override public function styleChanged(styleProp:String):void >>>>>{ >>>>> var allStyles:Boolean = styleProp == null || styleProp == >>>>>"styleName"; >>>>> >>>>> if (allStyles || styleProp == "skinClass" || styleProp == >>>>>"skinFactory") >>>>> { >>>>> clearStyle("skinClass"); //This will fix the bug >>>>> skinChanged = true; >>>>> invalidateProperties(); >>>>> >>>>> if (styleProp == "skinFactory") >>>>> skinFactoryStyleChanged = true; >>>>> } >>>>> >>>>> super.styleChanged(styleProp); >>>>>} >>>>> >>>>>clearStyle("skinClass"); //This will fix the bug >>>>>That's the only line needs to be added, in order to fix this bug. >>>>> >>>>> >>>>>I've already updated the JIRA for this >>>>>https://issues.apache.org/jira/browse/FLEX-34689 >>>>> >>>>> >>>>>DarkStone >>>>>2014-12-17 >>>>> >>>>> >>>>>在 2014-12-17 14:12:52，"Alex Harui" <aha...@adobe.com> 写道： >>>>>> >>>>>> >>>>>>On 12/16/14, 9:36 PM, "DarkStone" <darkst...@163.com> wrote: >>>>>> >>>>>>>Hi Alex, >>>>>>> >>>>>>>Why this needs to be fixed is simple: >>>>>>> >>>>>>>1. Using MySkinnableComponent.setStyle("skinClass", SkinA) to apply >>>>>>>an >>>>>>>initial skin is ok. >>>>>>> >>>>>>>2. Then using MySkinnableComponent.styleName = "skinB" won't work, >>>>>>>the >>>>>>>skin won't change. >>>>>>> >>>>>>>The styleChanged() function in SkinnableComponent doesn't handle >>>>>>>this >>>>>>>scenario correctly, that's why we need to override the styleName >>>>>>>setter >>>>>>>function to fix this. >>>>>> >>>>>>I’m wondering why the styleChanged() function can’t be changed to >>>>>>handle >>>>>>this scenario correctly. There are other ways to change skinClass, >>>>>>like >>>>>>setting it directly in a CSSStyleDeclaration or by loading a Style >>>>>>module. >>>>>> I would expect them all to call styleChanged() and the right thing >>>>>>to >>>>>>happen. >>>>>> >>>>>>Thanks, >>>>>>-Alex >>>>>> >>>> >>
