Blockly: Questions RE commentIcon

[samelhusseini]

Couple of questions about commentIcon:

• Why is it private whereas other icons on the block are public, eg: warning, mutator, etc.
• Why was it changed from comment to commentIcon_?

I didn't know where to post this. @BeksOmega

[BeksOmega]

Posting here is totally cool!

Note that it was a bit ago that I made that change, so my memory may be a bit rusty hehe.

Why is it private whereas other icons on the block are public?

(Firstly, I'll say that this may have a bit to do with personal preference. I kind of don't like public properties, because I find they make it trickier to upgrade things in a backwards compatible way. So that could have definitely had an impact hehe.)

So before I made this change there was a property on the block .comment which was a string|Blockly.Comment, but in reality it was always a string. Then the blocksvg also had a property .comment which was always a Blockly.Comment.

This seemed kind of weird to me. Like, I know the compiler is cool with it. But isn't it kind of strange that the property changes types depending on the mode you're in (headless vs ui)? And then the fact that the block's .comment property had to accept a comment, even though it never did, just seemed kind of smelly. Idk do you feel the same way?

So I built a new API that (I think) made it easier to work with the comments:

// Sets the comment's text no matter what mode you are in.
setCommentText(text)
// Gets the comment's text no matter what mode you are in.
getCommentText();
// Returns the comment icon iff you are in ui mode.
getCommentIcon();

Well maybe built is a bit of an exageration, the getter and setter existed before :P But I think getting rid of the .comment property and using these guys instead makes a lot more sense. I believe I also deprecated/packaged the comment.setText function because I thought it was confusing.

So I guess I didn't /technically/ have to deprecate the .comment property, but I since I had the opportunity I wanted to try and make the API better.

Why was it changed from comment to commentIcon_?

I had to keep the comment property around for backwards compatibility. (And let me tell you, it was a bit of a pain making sure it always had the correct state.) But if I deprecated it, I couldn't reasonably keep accessing it. So I had to pick a different property, and commentIcon_ sounded ok.

---

So how do you feel about it? Did you run into a problem with the implementation? If you want to change stuff around I'm definitely cool with that!

[samelhusseini]

Thanks for the in-depth answer @BeksOmega, I know it's always tricky digging up old threads / thoughts.

I totally +1 your intuition to make things private if they don't need to be public. To back it up a little, the reason I've stumbled upon this is I was writing a custom icon in TypeScript, and it complained to me that commentIcon_ doesn't exist on a block. (When we generate our TypeScript definitions, we exclude private properties, which I think makes sense, as no one outside of Blockly needs to know about private properties)

Okay I think what threw me off is the type of comment is now {!Blockly.Comment}, whereas what you're saying is it was {!Blockly.Comment|string}.

All of the above makes sense, now to figure out what the appropriate visibility should be for commentIcon_, I think we're going to need to do some more thinking of what visibility makes sense to access when extend a block. (Taking into account https://github.com/google/blockly-samples/wiki/Using-Blockly-APIs-in-Plugins)

Here's my thinking, you can extend a block in one of these ways (Tell me if you think of more?):

Block Definition

Blockly.Block['XYZ'] = {
   ...
   methodA = function () { ... }
}

In the above, what you're really declaring is a prototype of a specific block type, any time a new block of type XYZ is created, the set of methods that you write are "mixed in" to the resulting block object. If methodA already exists on Blockly.BlockSvg (it's going to override it with your method defined here)

Block Extensions

var applyMethodA = function() {
   this.methodA = function() {
      ...
   }
}
Blockly.Extensions.register('methodAExtension', applyMethodA);

// Block definition
Blockly.defineBlocksWithJsonArray([
  {
    "type": "xyz",
      ...
     "extensions": ["methodAExtension"]
  }];

The above is similar to a block definition (in scope), the this in applyMethodA refers to the block (before it is mixed in).

Monkey patching

Blockly.BlockSvg.prototype.methodA = function () {
 ... 
}

This is frowned upon, so I'll move right past it.

Subclassing

class MyBlocklySvg extends Blockly.BlockSvg {
 ...
}

This is only really valuable if you only use JS to create your blocks and I think in order to make everything work properly you'd also need to override the workspace's newBlock to use your custom blockSvg class instead.

I think in all of the above scenarios you should only really be able to access @protected, @package and @public properties and methods and your scope is most similar to that of subclassing a block.

Excuse me for thinking out loud, but I think this leads me to say that since we don't really know all the ways someone might extend things in Blockly, and since we only want people using supported methods. In places where your scope is "inside" of Blockly, eg: extending a Field, extending a Renderer, or extending / defining a Block, you should be able to access protected methods.
Specifically for commentIcon_, I think it should be @protected

What are your thoughts on this? @rachel-fenichel FYI

[BeksOmega]

I was writing a custom icon in TypeScript... now to figure out what the appropriate visibility should be for commentIcon_

Could you talk a little bit more about what you were trying to do with your custom icon? I'm just thinking that depending on what you want to do, there could be different solutions.

Like if you want to allow people to create custom icons in the same way that people can create custom fields, it might be better to create some sort of registry, and move away from the static .commentIcon_, .warning, .mutator properties.

(Another option would be to get rid of icons, since they're less flexible, and add bubble support to fields instead.)

If you just want people to be able to extend the existing icons, then making .commentIcon_ protected makes sense.

---

All of your information about ways of extending blocks makes sense! One thing I'll add is that extensions currently don't allow you to override pre-defined block functions. They throw an error here. If you want to override a pre-defined function with an extension you need to use the mixin function (passing true to disable checks). I ran into this issue with procedures and the +/- mutator extension.

I'm not sure if that's the behavior that we want going forward or not, I just wanted to mention it's the current behavior.

In places where your scope is "inside" of Blockly, eg: extending a Field, extending a Renderer, or extending / defining a Block, you should be able to access protected methods.

Sounds legit to me.

[samelhusseini]

Could you talk a little bit more about what you were trying to do with your custom icon? I'm just thinking that depending on what you want to do, there could be different solutions.

Agreed that icons are less flexible, however the fact they show up at the front of the block and that they apply to all blocks is a nice feature. Yes it's doable with fields, but its more painful on the developers end to manage it.

My exact scenario is I was trying to add as a "plugin", a new icon. I was able to do most of what I wanted with block extensions: in order to add the new functionality and icon to the block. However, I had to override getIcons (which really is because there is no registry currently, so that's a solvable problem in of itself), but needless to say getIcons calls commentIcon_ which brings us here!

All of your information about ways of extending blocks makes sense! One thing I'll add is that extensions currently don't allow you to override pre-defined block functions. They throw an error here.

Hmm, that's not the behaviour I was seeing, I was able to override a `Blockly.BlockSvg`` defined method, so will look into that..

[BeksOmega]

My exact scenario is I was trying to add as a "plugin", a new icon. I was able to do most of what I wanted with block extensions: in order to add the new functionality and icon to the block. However, I had to override getIcons (which really is because there is no registry currently, so that's a solvable problem in of itself), but needless to say getIcons calls commentIcon_ which brings us here!

Ah ok that makes sense! I would agree that an icon registry is probably the best option. If you have to override getIcons to add your icon, and then another plugin has to override getIcons to add /its/ icon. Well you could only have one plugin-icon at a time, and that would suck.

[rachel-fenichel]

Can you do this with a block extension that adds an image field at the beginning of the block, regardless of the rest of the contents?

[samelhusseini]

Ah ok that makes sense! I would agree that an icon registry is probably the best option. If you have to override getIcons to add your icon, and then another plugin has to override getIcons to add /its/ icon. Well you could only have one plugin-icon at a time, and that would suck.

Yeah exactly, not ideal for sure.

Can you do this with a block extension that adds an image field at the beginning of the block, regardless of the rest of the contents?

In that case you still wouldn't be able to have your field render before other icons like the comment icon, etc. (Course without some significant muddling with blockly internals)

[samelhusseini]

Thanks again @BeksOmega