overiding automagical get/set methods and private vs public

bbito

This question is related to a pull request I'm trying to put together: http://forums.enyojs.com/discussion/2300/onyx-slider-sliding-past-limits.
I want to 'borrow' the getValue() override from moon.Slider: https://github.com/enyojs/moonstone/blob/master/source/Slider.js#L632, but I don't understand why that method override is tagged as @private in the moon source. It seems to me that the getters and setters for public properties would always be public, so I feel like I'm not getting something here. Along the same lines, for someone like me slowly coming into enyo, I find it obscure that the automatically generated get, set methods are not shown in the API viewer, since as I understand it, these are the preferred methods for manipulating these basic properties unless one is binding directly.
-Thanks in advance!

bbito

Unfortunately I'm at the point where I'm getting more rather than less confused as I go through the documentation and source code...
I'm starting to think get() - e.g.:getValue() is deprecated and the new way is get("") - e.g.:get("value")
Am I getting warmer here?
I had been focused on the getValue() style from the Sampler which does not seem to use the other style at all within the sample code.
Finally, if that getValue() style is in fact on the way out, how does one make an override for get("value")?
-Thanks in advance for any help in understanding this.

aarontam

Hi @bbito, warmer indeed. The generic set('property', value) and get('property') forms are the recommended style now and the old style is deprecated (they now are essentially just aliases for the new style). Thanks for pointing out that Sampler is using the old style - we'll need to update this! What are you trying to accomplish in your override of the getter? Is this something you couldn't implement using computed properties instead?

bbito

Hi @aarontam, I am putting together a pull request for onyx.Slider that brings in some of the refinements from moon.Slider. One that seems crucial to make onyx.Slider.value bind-able is the override of getValue() so that values generated during animation are not passed to the binding: https://github.com/enyojs/moonstone/blob/master/source/Slider.js#L632

aarontam

Ah yes, this might require some more involved refactoring (to remove the old-style getters from moon.Slider - I'll try to take a crack at this later and share with you so you're not blocked on your PR. As an alternative to overriding getValue() to essentially filter these values, have you taken a look at specifying a transform function in the binding to accomplish this?

bbito

Hi @aarontam, My feeling is that if it all possible a UI widget like Slider should just plug in to the system without warnings that a developer should include a transform on the binding unless they actually want to transform the value. I would expect that if I were to init a Slider to work within my specified min, max, and increment properties, that it would just work and be cleanly bindable to a model. I would expect to perform a transform only if I actually wanted to move to a different value system, such as transforming a percentage-based slider to a time-based value.

aarontam

Yep no disagreement there - I think for now you can submit your PR with an override of getValue(), as we figure out what/how to refactor these cases. Also, I think the documentation was incorrect as getValue() should be marked as public.

Edit: @bbito, had a bit of an internal discussion. We're going to leave the get and set methods alone for now - they're essentially computed properties and still have some usefulness (as evidenced here), even though the generic get and set have their advantages. So feel free to override getValue and we'll take a look at your PR - thanks in advance for your contribution!

rubenvreeken

Does this mean the get and set methods are no longer deprecated? I kind of like the old way as overriding the generic setter in particular is unnecessarily polymorphic.

If it is un-deprecated, I think the documentation needs to be updated to reflect that.

aarontam

Edit: I may have misunderstood our stance on this. They may not necessarily be undeprecated, as they are needed in certain situations, but not necessarily recommended either. @RoySutton any thoughts?

RoySutton

Sorry I missed replying to this one. We are keeping them deprecated.