8000 fix: Fix regressions in `Field`. by gonfunko · Pull Request #9011 · google/blockly · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Fix regressions in Field. #9011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 13, 2025
Merged

fix: Fix regressions in Field. #9011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion core/field.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ export abstract class Field<T = any>
*/
DEFAULT_VALUE: T | null = null;

/** Non-breaking space. */
static readonly NBSP = '\u00A0';

/**
* A value used to signal when a field's constructor should *not* set the
* field's value or run configure_, and should allow a subclass to do that
Expand All @@ -110,7 +113,28 @@ export abstract class Field<T = any>
* field is not yet initialized. Is *not* guaranteed to be accurate.
*/
private tooltip: Tooltip.TipInfo | null = null;
protected size_: Size;

/** This field's dimensions. */
private size: Size = new Size(0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style guide suggests something like sizeInternal:

Suggested change
private size: Size = new Size(0, 0);
private sizeInternal: Size = new Size(0, 0);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused about this, why? it's already private, why would we also add the word internal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 internal or wrapped.

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.


/**
* 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;
}
Comment on lines +117 to +137
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor
@cpcallen cpcallen May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the mostly-useless accessors (which most fields can just ignore) over the kludgy approaches mentioned above.


/** The rendered field's SVG group element. */
protected fieldGroup_: SVGGElement | null = null;
Expand Down Expand Up @@ -973,6 +997,8 @@ export abstract class Field<T = any>
// Truncate displayed string and add an ellipsis ('...').
text = text.substring(0, this.maxDisplayLength - 2) + '…';
}
// Replace whitespace with non-breaking spaces so the text doesn't collapse.
text = text.replace(/\s/g, Field.NBSP);
if (this.sourceBlock_ && this.sourceBlock_.RTL) {
// The SVG is LTR, force text to be RTL by adding an RLM.
text += '\u200F';
Expand Down
1 change: 0 additions & 1 deletion core/field_image.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export class FieldImage extends Field<string> {
* of the field.
*/
private static readonly Y_PADDING = 1;
protected override size_: Size;
protected readonly imageHeight: number;

/** The function to be called when this field is clicked. */
Expand Down
20 changes: 20 additions & 0 deletions core/field_input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,26 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
*/
override SERIALIZABLE = true;

/**
* Sets the size of this field. Although this appears to be a no-op, it must
* exist since the getter is overridden below.
*/
protected override set size_(newValue: Size) {
super.size_ = newValue;
}

/**
* Returns the size of this field, with a minimum width of 14.
*/
protected override get size_() {
const s = super.size_;
if (s.width < 14) {
s.width = 14;
}

return s;
}

/**
* @param value The initial value of the field. Should cast to a string.
* Defaults to an empty string if null or undefined. Also accepts
Expand Down
1 change: 0 additions & 1 deletion core/field_variable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export class FieldVariable extends FieldDropdown {
* dropdown.
*/
variableTypes: string[] | null = [];
protected override size_: Size;

/** The variable model associated with this field. */
private variable: IVariableModel<IVariableState> | null = null;
Expand Down
0