Permit single field disabling (#4932) #4941

jschanker · 2021-06-20T05:39:43Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

#2763, #4932

Proposed Changes

Allows the developer to specify which fields on an editable block should be disabled.

Behavior Before Change

Editors for fields would appear when clicked exactly when their source blocks were editable. This meant that the developer could not call a core Blockly method to disable a block's field without disabling all other fields on that block. To enable/disable all fields on a block, he/she could call the Block's setEditable method.

Behavior After Change

The developer can now choose which fields to disable via the newly added Field method setEnabled. To be clickable, the field must be enabled and its source block must be editable. A field can be enabled even if its source block is non-editable, it just won't be clickable.

Reason for Changes

Developers may want to disable some fields without disabling all of them.

Test Coverage

Tested locally on block with multiple fields using the different combinations (editable block/enabled field, editable block/disabled field, non-editable block/enabled field, non-editable block/disabled field).

Documentation

Probably not beyond the property/function annotations that were added/updated, which I'm assuming are autogenerated.

* Fixed lint error (moved && to end of line and adjusted line breaks accordingly)

jschanker · 2021-06-20T07:50:21Z

Thinking about this more, I'm assuming we'd want to include if a given field was disabled in the block serialization. I ~~can make~~ made additional commits ~~later~~, accordingly. ~~However, I'll wait for comments on whether the approach I've taken is how we'd want to handle disabling individual fields.~~ Edit: I decided to make the extra commits so everything could be reviewed together.

* Call parent method in FieldDropdown's fromXml * Added protected helper methods to handle serialization/deserialization of enabled property/attribute of fields * Minor changes to annotations to account for field disabling and 4 spaces per line break per style guide

alschmiedt · 2021-06-22T14:59:36Z

Thanks for your PR!

I have assigned someone to look at it but just as a heads up we have an internal event this week so we might be a little bit slower than normal reviewing it. Thanks for your patience! Also, since we are getting close to releasing this will most likely go into the next release. Thanks again!

rachel-fenichel · 2021-07-09T02:14:55Z

Bump @maribethb now that we're done with releasing.

maribethb

This looks good to me (with some minor comments). Sorry for the long delay in reviewing. Thanks for your work on this!

maribethb · 2021-07-13T18:07:10Z

core/field.js

@@ -202,6 +202,13 @@ Blockly.Field.prototype.isDirty_ = true;
 */
 Blockly.Field.prototype.visible_ = true;

+/**
+ * Can the field value be changed using the editor on an editable block?


Nit: For consistency, "Whether the field value can be changed using the editor on an editable block."

Sounds good, I made the change. I'm holding off on making another commit before the serialization questions are resolved.

Actually one follow-up: Should other comments be changed as well? I used a question here because some other properties were commented that way.

You're right, this is consistent with how this file does it. I wasn't familiar with this style inside of Blockly before. You can just leave it like you had it. Sorry for the churn.

core/field.js

BeksOmega · 2021-07-13T19:09:18Z

Ope sorry! Somehow I missed the XML stuff before. This doesn't actually need to be serialized because it's not user-facing. Since it's only triggered by devs programmatically, we should just expect devs to re-trigger it if necessary (like how warnings work).

jschanker · 2021-07-14T02:22:05Z

Ope sorry! Somehow I missed the XML stuff before. This doesn't actually need to be serialized because it's not user-facing. Since it's only triggered by devs programmatically, we should just expect devs to re-trigger it if necessary (like how warnings work).

I added code to serialize because I noticed that if a developer sets a block's editable_ property to false via setEditable, then this would be reflected in the XML. I thought this would be analogous?

BeksOmega · 2021-07-14T13:58:40Z

Ope sorry! Somehow I missed the XML stuff before. This doesn't actually need to be serialized because it's not user-facing. Since it's only triggered by devs programmatically, we should just expect devs to re-trigger it if necessary (like how warnings work).

I added code to serialize because I noticed that if a developer sets a block's editable_ property to false via setEditable, then this would be reflected in the XML. I thought this would be analogous?

Hmm I thought editablility was surfaced through the context menu, but you're totally right it's only available to the developer. In that case I'm of the opinion that neither of those things should be serialized. Either (a) the developer is triggering this setting based on other state, in which case all of the relevant information is already encoded in XML, or (b) the developer is surfacing this toggle to the user themselves, in which case it is the developer's responsibility to serialize it, not Blockly's.

Does that feel logical? or does it feel like I'm off the mark?

Sadly we can't get rid of the existing editable save information because devs could be relying on it. But I think that we don't want to add to the XML unless we have to.

jschanker · 2021-07-15T04:21:28Z

it's only available to the developer. In that case I'm of the opinion that neither of those things should be serialized. Either (a) the developer is triggering this setting based on other state, in which case all of the relevant information is already encoded in XML, or (b) the developer is surfacing this toggle to the user themselves, in which case it is the developer's responsibility to serialize it, not Blockly's.

Does that feel logical? or does it feel like I'm off the mark?

That seems reasonable to me and is consistent with how other developer-only facing attributes such as a block's color are handled. Serializing these attributes may be unnecessary or even undesirable in certain applications. It can also make maintenance of the core a little more difficult.

That being said, as you noted in (b), it's possible some developers may decide to make one or more of these attributes user-facing. So if there's sufficient demand, perhaps a plugin could be created to detect and serialize attributes that can be changed by a user (or allows the developer to choose some subset of them for serialization).

Sadly we can't get rid of the existing editable save information because devs could be relying on it.

For consistency, we could always add a context menu item allowing users to toggle the editability of a block. But this is probably not a good idea since it might be confused with disabling a block and adds a likely seldomly used option.

This reverts commit 1964e86.

jschanker · 2021-07-15T04:47:25Z

I reverted the (de)serialization code additions.

BeksOmega · 2021-07-15T13:48:20Z

That seems reasonable to me and is consistent with how other developer-only facing attributes such as a block's color are handled. Serializing these attributes may be unnecessary or even undesirable in certain applications. It can also make maintenance of the core a little more difficult.

Fwew I'm glad it sounds ok to you. Honestly I was going back and forth on it after posting my previous message. I do like consistency, and there's no really consistent solution here :/

That being said, as you noted in (b), it's possible some developers may decide to make one or more of these attributes user-facing. So if there's sufficient demand, perhaps a plugin could be created to detect and serialize attributes that can be changed by a user (or allows the developer to choose some subset of them for serialization).

Yeah definitely! And if enough people ask for it we can always add it directly into core too.

I'm going to leave the final review of this to @maribethb =) Thanks for hashing this out with me hehe.

maribethb

Thank you and Beka for working through the xml details :)

maribethb · 2021-07-16T23:14:26Z

core/field.js

@@ -202,6 +202,13 @@ Blockly.Field.prototype.isDirty_ = true;
 */
 Blockly.Field.prototype.visible_ = true;

+/**
+ * Can the field value be changed using the editor on an editable block?


You're right, this is consistent with how this file does it. I wasn't familiar with this style inside of Blockly before. You can just leave it like you had it. Sorry for the churn.

maribethb · 2021-07-16T23:21:34Z

core/field_dropdown.js

- * called by Blockly.Xml.
- * @param {!Element} fieldElement The element containing info about the
- *    field's state.
+ * Sets the field's value based on the given XML element. Should only be called


I think you could probably remove this file from the PR now right?

I removed the file. It's not related to the pull request anymore, but I thought I would adjust the styling of the comment by (a) moving up "field's" to the previous line since the line would have 80 characters and (b) indenting the wrapped line with 4 spaces.

Regarding (b), I noticed that there is some inconsistency with comments' styling in general as some are indented by 4 spaces when they wrap to the next line and others are not.

They should ideally be indented, but there are many places they aren't currently. However, we are Soon (TM) going to be using clang-format for indentation so we will never have to think about it (or line wrapping) ever again :D So for now it's fine as long as eslint doesn't complain.

jschanker · 2021-07-17T05:24:30Z

You're right, this is consistent with how this file does it. I wasn't familiar with this style inside of Blockly before. You can just leave it like you had it. Sorry for the churn.

I reverted to the question form, but as was the case with the indentation of comments, I'm not sure what the expected style is. I see that Boolean properties seem to have question annotations while Boolean parameters and return values use statements?

maribethb · 2021-07-20T01:02:08Z

You're right, this is consistent with how this file does it. I wasn't familiar with this style inside of Blockly before. You can just leave it like you had it. Sorry for the churn.

I reverted to the question form, but as was the case with the indentation of comments, I'm not sure what the expected style is. I see that Boolean properties seem to have question annotations while Boolean parameters and return values use statements?

It does look like we have a mix of both, and the style guide seems to have no opinion, so we should just be consistent with what's nearby. menuitem.js splits the difference with Is this menu item currently highlighted. which is probably the worst of all options... so we're not consistent and I probably should have checked this file before asking you to change it, but I hadn't realized how prevalent it was.

* Permit single field disabling (#4932) * Permit single field disabling (#4932) * Fixed lint error (moved && to end of line and adjusted line breaks accordingly) * Added XML Field (De)Serialization * Call parent method in FieldDropdown's fromXml * Added protected helper methods to handle serialization/deserialization of enabled property/attribute of fields * Minor changes to annotations to account for field disabling and 4 spaces per line break per style guide * Revert "Added XML Field (De)Serialization" This reverts commit 1964e86. * Comment style changes * Comment reversions * Indentation fix * Indentation reversion

jschanker added 2 commits June 20, 2021 00:41

Permit single field disabling (google#4932)

1aa1186

Permit single field disabling (google#4932)

d630d76

* Fixed lint error (moved && to end of line and adjusted line breaks accordingly)

alschmiedt requested a review from maribethb June 22, 2021 14:55

alschmiedt assigned maribethb Jun 22, 2021

maribethb approved these changes Jul 13, 2021

View reviewed changes

Revert "Added XML Field (De)Serialization"

a1ab67d

This reverts commit 1964e86.

jschanker requested a review from a team as a code owner July 15, 2021 04:34

Comment style changes

473aa68

maribethb reviewed Jul 16, 2021

View reviewed changes

Comment reversions

186adac

jschanker added 2 commits July 17, 2021 01:33

Indentation fix

e4014ef

Indentation reversion

07f7de7

maribethb merged commit e8c4af3 into google:develop Jul 20, 2021

jschanker mentioned this pull request Jul 20, 2021

Disabling only one field on a block #4932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit single field disabling (#4932) #4941

Permit single field disabling (#4932) #4941

jschanker commented Jun 20, 2021 •

edited

Loading

jschanker commented Jun 20, 2021 •

edited

Loading

alschmiedt commented Jun 22, 2021

rachel-fenichel commented Jul 9, 2021

maribethb left a comment

maribethb Jul 13, 2021

jschanker Jul 14, 2021

jschanker Jul 14, 2021

maribethb Jul 16, 2021

BeksOmega commented Jul 13, 2021

jschanker commented Jul 14, 2021 •

edited

Loading

BeksOmega commented Jul 14, 2021

jschanker commented Jul 15, 2021 •

edited

Loading

jschanker commented Jul 15, 2021

BeksOmega commented Jul 15, 2021

maribethb left a comment

maribethb Jul 16, 2021

maribethb Jul 16, 2021

jschanker Jul 17, 2021

maribethb Jul 20, 2021

jschanker commented Jul 17, 2021

maribethb commented Jul 20, 2021

Permit single field disabling (#4932) #4941

Permit single field disabling (#4932) #4941

Conversation

jschanker commented Jun 20, 2021 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

jschanker commented Jun 20, 2021 • edited Loading

alschmiedt commented Jun 22, 2021

rachel-fenichel commented Jul 9, 2021

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega commented Jul 13, 2021

jschanker commented Jul 14, 2021 • edited Loading

BeksOmega commented Jul 14, 2021

jschanker commented Jul 15, 2021 • edited Loading

jschanker commented Jul 15, 2021

BeksOmega commented Jul 15, 2021

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschanker commented Jul 17, 2021

maribethb commented Jul 20, 2021

jschanker commented Jun 20, 2021 •

edited

Loading

jschanker commented Jun 20, 2021 •

edited

Loading

jschanker commented Jul 14, 2021 •

edited

Loading

jschanker commented Jul 15, 2021 •

edited

Loading