fix: Fix regressions in Field.
#9011
Conversation
| /** This field's dimensions. */ | ||
| private size: Size = new Size(0, 0); | ||
|
|
||
| /** | ||
| * Gets the size of this field. Because getSize() and updateSize() have side | ||
| * effects, this acts as a shim for subclasses which wish to adjust field | ||
| * bounds when setting/getting the size without triggering unwanted rendering | ||
| * or other side effects. Note that subclasses must override *both* get and | ||
| * set if either is overridden; the implementation may just call directly | ||
| * through to super, but it must exist per the JS spec. | ||
| */ | ||
| protected get size_(): Size { | ||
| return this.size; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the size of this field. | ||
| */ | ||
| protected set size_(newValue: Size) { | ||
| this.size = newValue; | ||
| } |
There was a problem hiding this comment.
Styleguide says "At least one accessor for a property must be non-trivial: do not define "pass-through" accessors only for the purpose of hiding a property."
I gather you are doing this in order to be able to write super.size_ = newValue in FieldInput's get size_ accessor, but I think a better approach might be to have updateSize_ enforce a minimum width (possibly in Field but preferably in FieldInput). (Alas, updateSize_ isn't written in a way that makes it easy to override for this particular purpose.)
There was a problem hiding this comment.
As you noted, updateSize_() is not well-suited to being overridden, and short of introducing a setSizeWithoutSideEffects() method I couldn't see a way to refactor it to be more amendable. Likewise, overriding getSize() (or calling it from updateSize_() instead of accessing the size_ property directly) is problematic due to its side effects of rerendering the field. Hence this seemed like the least-bad option, as it preserves backwards compatibility and allows for subclasses to adjust the size as they need. If you have an idea for how updateSize_() might be refactored I'd certainly be open to that though!
There was a problem hiding this comment.
I had to think about this for a bit, but I think I see a way to do this without needing to have accessors on Field.
The obvious approach is to have the get and set accessors on the subclasses that want to enforce a minimum width only, and leave Field alone. As you doubtless noticed, the reason this doesn't work is that the Field constructor sets .size_, and that results in (e.g.) FieldInput instances having a .size_ property that is found before the get size_/set size_ accessors on FieldInput.prototype.
But if the Field constructor either didn't set that property if there were accessors, like so:
class FieldInput {
constructor() {
this.size_ ??= new Size(0, 0);
}
}or if the FieldInput constructor removed it, like so:
class FieldInput {
constructor() {
super();
delete this.size_;
}
}then I think it's possible to have the behaviour you have here without the mostly-useless accessors on Field.prototype. Both approaches are a bit kludgy, and the delete approach might have some performance consequences (it's generally preferable not to change the shape of an object, though here at least it's being done at construction time). I'm genuinely not sure either is clearly preferable to your proposal, but I offer them for your consideration.
There was a problem hiding this comment.
Additionally, I think that even if you keep the get/set accessors on Field it might be preferable to not call them from the corresponding accessors on FieldInput, either by making size protected (so that that FieldInput's accessors can use it directly) or by having FieldInput keep it's own separate Size property. A pair of accessors ought to be interchangeable with a data property, in my view, but you have revealed that that isn't always the case. I'd like to at least not tie ourselves too tightly to using accessors everywhere.
There was a problem hiding this comment.
I prefer the mostly-useless accessors (which most fields can just ignore) over the kludgy approaches mentioned above.
|
(Restoration of |
| protected size_: Size; | ||
|
|
||
| /** This field's dimensions. */ | ||
| private size: Size = new Size(0, 0); |
There was a problem hiding this comment.
Style guide suggests something like sizeInternal:
| private size: Size = new Size(0, 0); | |
| private sizeInternal: Size = new Size(0, 0); |
There was a problem hiding this comment.
I am confused about this, why? it's already private, why would we also add the word internal
There was a problem hiding this comment.
Quoting the Styleguide section on get and set accesors says:
If an accessor is used to hide a class property, the hidden property may be prefixed or suffixed with any whole word, like
internalorwrapped.
They do not provide a rational, but I suspect the reason is to avoid clashing with the name of the accessors. In this case the accessors are named size_ (which, with the trailing underscore, is contrary to current naming policy) but if it were size then the internal property would need to be named something else.
| /** This fi 8000 eld's dimensions. */ | ||
| private size: Size = new Size(0, 0); | ||
|
|
||
| /** | ||
| * Gets the size of this field. Because getSize() and updateSize() have side | ||
| * effects, this acts as a shim for subclasses which wish to adjust field | ||
| * bounds when setting/getting the size without triggering unwanted rendering | ||
| * or other side effects. Note that subclasses must override *both* get and | ||
| * set if either is overridden; the implementation may just call directly | ||
| * through to super, but it must exist per the JS spec. | ||
| */ | ||
| protected get size_(): Size { | ||
| return this.size; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the size of this field. | ||
| */ | ||
| protected set size_(newValue: Size) { | ||
| this.size = newValue; | ||
| } |
There was a problem hiding this comment.
I prefer the mostly-useless accessors (which most fields can just ignore) over the kludgy approaches mentioned above.
| protected size_: Size; | ||
|
|
||
| /** This field's dimensions. */ | ||
| private size: Size = new Size(0, 0); |
There was a problem hiding this comment.
I am confused about this, why? it's already private, why would we also add the word internal
The basics
The details
Resolves
Fixes #9005
Proposed Changes
This PR fixes several regressions in
Fieldintroduced by an earlier change:Field.NBSPconstant, which was in fact in use