fix: remove retired "update cursorstyle over toolbox"

[tweini]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint

The details

The Toolbox has been refactored and extends DeleteArea. As a consequence, the cursor update has been moved to the Toolbox:

blockly/core/toolbox/toolbox.js

Line 665 in 24704e0

updateCursorDeleteStyle_(addStyle) {

And there is an incorrect comment in IStyleable, as it refers to Toolbox, but it should refer more generally to "an object".

The latter change is somehow tangled to this issue, so it makes sense to bundle it here.

Resolves

Unused code causes irritation. (I was irritated, so I took action to do this PR)

Proposed Changes

There was old code which has been removed.

There are no known references

Behavior Before Change

Behavior After Change

No change. The "isDeletable()" check has been implemented in toolbox.js with "wouldDelete()", so no indication of lost functionality.

Reason for Changes

Test Coverage

Manually tested. Cursor changes when block is dragged over toolbox.

Documentation

No doc needed, change is trivial.

Additional Information

Blockly Team is awesome 🤩

[BeksOmega]

@maribethb It looks like the block dragger is included in the registry (meaning we expect people to create custom subclasses?) And this function is protected, so does that make removing it technically a breaking change?

[BeksOmega]

Just wanted to give you an update on this @tweini @maribethb is out of office this week, so it will be next week before I have info for you on this =)

[maribethb]

Sorry I missed this before I went OOO!

Yeah, I guess this would technically be a breaking change - if someone registers their own block dragger class and calls this method which will no longer exist. That seems exceedingly unlikely to me, but we're already going to be doing another major release so there is no harm in throwing this up in the breaking changes section of the release notes.

So @tweini could you change the PR title to use the prefix fix!: (with the exclamation mark) and then add in a section in the PR description that says something along the lines of "Am I affected by this breaking change? probably not unless someone registers their own block dragger class and calls this method which will no longer exist" so that when people click on this from the release notes they can understand at a glance what has changed? Thanks!

as a side note we should go through and change some of these methods from protected back to private - which would be a breaking change, but hopefully saves us in the future from having to make other trivial changes technically "breaking". we swung the pendulum too far when we made everything protected.

[tweini]

Hi,

got time for some investigations, because it seemed too hypothetical to me, that this is a breaking change:

The ability to register an own block dragger class given by this PR:

#4893

The cursor related code was added to toolbox.js with this PR one day later:

#4896

So the code which will be removed by my PR was unreachable since then. When someone would have overwritten this method, he would have noticed it doesn't work and he will have moved the changes to the new spot.

So bear with me. But there is a big difference between a hypothetical and a minimal chance.

Or another perspective is, that the code deletion was simply forgotten by #4896 and nobody will have noticed it.

Please don't get me wrong. Stability and Reliability as a goal of Blockly Team is absolutely honorable. But tagging this as a braking change is way too over engineered.

There will be harm in throwing this up in the breaking change section. Everybody who reads this wastes his time for an information which too hypothetical. It's like writing "Look up" because of falling pianos.

[maribethb]

I definitely get where you're coming from wrt not wanting this to be a breaking change. However I still think we should mark it as one -

Let's say someone registers their own block dragger and they decide that they want to trigger the toolbox delete style in certain scenarios custom to their application, so in the block dragger they call this method which conveniently already exists for them, called updateToolboxStyle_. The fact that we don't ever call that method ourselves is not relevant here. It's that we were previously making that method available for people to use and if anyone does use it we would be breaking them. I would not want to break anyone without telling them that we've done so, hence my caution here. And even though I doubt anyone has called this method, in reality I don't know how others are using the block dragger.

I do understand the cognitive load there is when reading release notes for breaking changes, so that's why I want to add a section to the PR description to make it clear who has to care about this or not. We have similar other changes where it's only under a specific circumstance that a change would break anyone, but we still want to err on the side of caution.

[BeksOmega]

I'm going to go ahead and close this one. If you're still interested in working on it, feel free to reopen!