Skip to content
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.

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

refactor!: remove constants for internal constants #5897

Merged
merged 19 commits into from
Feb 9, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
6 changes: 3 additions & 3 deletions blocks/procedures.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ const Events = goog.require('Blockly.Events');
const Procedures = goog.require('Blockly.Procedures');
const Variables = goog.require('Blockly.Variables');
const Xml = goog.require('Blockly.Xml');
const internalConstants = goog.require('Blockly.internalConstants');
const xmlUtils = goog.require('Blockly.utils.xml');
const {Align} = goog.require('Blockly.Input');
/* eslint-disable-next-line no-unused-vars */
const {Block} = goog.requireType('Blockly.Block');
/* eslint-disable-next-line no-unused-vars */
const {BlockDefinition} = goog.requireType('Blockly.blocks');
const {config} = goog.require('Blockly.config');
/* eslint-disable-next-line no-unused-vars */
const {FieldCheckbox} = goog.require('Blockly.FieldCheckbox');
const {FieldLabel} = goog.require('Blockly.FieldLabel');
Expand Down Expand Up @@ -959,8 +959,8 @@ const PROCEDURE_CALL_COMMON = {
const block = xmlUtils.createElement('block');
block.setAttribute('type', this.defType_);
const xy = this.getRelativeToSurfaceXY();
const x = xy.x + internalConstants.SNAP_RADIUS * (this.RTL ? -1 : 1);
const y = xy.y + internalConstants.SNAP_RADIUS * 2;
const x = xy.x + config.snapRadius * (this.RTL ? -1 : 1);
const y = xy.y + config.snapRadius * 2;
block.setAttribute('x', x);
block.setAttribute('y', y);
const mutation = this.mutationToDom();
Expand Down
7 changes: 4 additions & 3 deletions core/block_svg.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const {ASTNode} = goog.require('Blockly.ASTNode');
const {Block} = goog.require('Blockly.Block');
/* eslint-disable-next-line no-unused-vars */
const {Comment} = goog.requireType('Blockly.Comment');
const {config} = goog.require('Blockly.config');
const {ConnectionType} = goog.require('Blockly.ConnectionType');
/* eslint-disable-next-line no-unused-vars */
const {Connection} = goog.requireType('Blockly.Connection');
Expand Down Expand Up @@ -1552,7 +1553,7 @@ BlockSvg.prototype.bumpNeighbours = function() {
connection.targetBlock().bumpNeighbours();
}

const neighbours = connection.neighbours(internalConstants.SNAP_RADIUS);
const neighbours = connection.neighbours(config.snapRadius);
for (let j = 0, otherConnection; (otherConnection = neighbours[j]); j++) {
// If both connections are connected, that's probably fine. But if
// either one of them is unconnected, then there could be confusion.
Expand Down Expand Up @@ -1585,13 +1586,13 @@ BlockSvg.prototype.scheduleSnapAndBump = function() {
eventUtils.setGroup(group);
block.snapToGrid();
eventUtils.setGroup(false);
}, internalConstants.BUMP_DELAY / 2);
}, config.bumpDelay / 2);

setTimeout(function() {
eventUtils.setGroup(group);
block.bumpNeighbours();
eventUtils.setGroup(false);
}, internalConstants.BUMP_DELAY);
}, config.bumpDelay);
};

/**
Expand Down
15 changes: 3 additions & 12 deletions core/blockly.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const {Bubble} = goog.require('Blockly.Bubble');
const {CollapsibleToolboxCategory} = goog.require('Blockly.CollapsibleToolboxCategory');
const {Comment} = goog.require('Blockly.Comment');
const {ComponentManager} = goog.require('Blockly.ComponentManager');
const {config} = goog.require('Blockly.config');
const {ConnectionChecker} = goog.require('Blockly.ConnectionChecker');
const {ConnectionDB} = goog.require('Blockly.ConnectionDB');
const {ConnectionType} = goog.require('Blockly.ConnectionType');
Expand Down Expand Up @@ -655,20 +656,9 @@ const bindEventWithChecks_ = function(
exports.bindEventWithChecks_ = bindEventWithChecks_;

// Aliases to allow external code to access these values for legacy reasons.
exports.DRAG_RADIUS = internalConstants.DRAG_RADIUS;
exports.FLYOUT_DRAG_RADIUS = internalConstants.FLYOUT_DRAG_RADIUS;
exports.SNAP_RADIUS = internalConstants.SNAP_RADIUS;
exports.CONNECTING_SNAP_RADIUS = internalConstants.CONNECTING_SNAP_RADIUS;
exports.CURRENT_CONNECTION_PREFERENCE =
internalConstants.CURRENT_CONNECTION_PREFERENCE;
exports.BUMP_DELAY = internalConstants.BUMP_DELAY;
exports.COLLAPSE_CHARS = internalConstants.COLLAPSE_CHARS;
exports.DRAG_STACK = internalConstants.DRAG_STACK;
exports.SPRITE = internalConstants.SPRITE;
exports.DRAG_NONE = internalConstants.DRAG_NONE;
exports.DRAG_STICKY = internalConstants.DRAG_STICKY;
exports.DRAG_BEGIN = internalConstants.DRAG_BEGIN;
exports.DRAG_FREE = internalConstants.DRAG_FREE;
exports.SPRITE = constants.SPRITE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this export be marked as deprecated, or deleted outright?

The entry in renamings.js suggests that we expect developers to use Blockly.constants.SPRITE rather than Blockly.SPRITE. I would not suggest creating a set accessor just to print a deprecation warning, but perhaps putting a comment about it so we remember to delete it in due course, if you don't want to do so now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think it is actually fine to delete now. I looked back through GitHub and didn't see anyone using it externally.

This makes me think though- do we actually want to keep internal_constants.js around for values like this? I think the original issue says we should remove all values from internal_constants, but I think it serves a different purpose than constants which are public and we expect developers to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a good idea to me: don't make public anything which doesn't need to be. An alternative might be to introduce a core/sprite.js (Blockly.sprite perhaps?) that is imported by the two modules which need these constants. (I had a look to see if there were any other shared dependencies which might be a suitable home; the only one that looked at all promising was core/positionable_helpers.js (Blockly.uiPosition), but that doesn't seem quite right.

Maybe @rachel-fenichel has an opinion about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with deleting it from exports now and eventually moving it to its own file (core/sprite.js is reasonable). We certainly don't need to keep internal constants around long-term--everything in it should eventually end up in a more logical place.

exports.OPPOSITE_TYPE = internalConstants.OPPOSITE_TYPE;
exports.RENAME_VARIABLE_ID = internalConstants.RENAME_VARIABLE_ID;
exports.DELETE_VARIABLE_ID = internalConstants.DELETE_VARIABLE_ID;
Expand Down Expand Up @@ -828,6 +818,7 @@ exports.browserEvents = browserEvents;
exports.bumpObjects = bumpObjects;
exports.clipboard = clipboard;
exports.common = common;
exports.config = config;
/** @deprecated Use Blockly.ConnectionType instead. */
exports.connectionTypes = ConnectionType;
exports.constants = constants;
Expand Down
87 changes: 87 additions & 0 deletions core/config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* @license
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @fileoverview All the values that we expect developers to be able to change
* before injecting Blockly. Changing these values during run time is not
* generally recommended.
*/
'use strict';

/**
* All the values that we expect developers to be able to change
* before injecting Blockly. Changing these values during run time is not
* generally recommended.
* @namespace Blockly.config
*/
goog.module('Blockly.config');


/**
* All the values that we expect developers to be able to change
* before injecting Blockly.
* @typedef {{
* dragRadius: number,
* flyoutDragRadius: number,
* snapRadius: number,
* currentConnectionPreference: number,
* bumpDelay: number,
* connectingSnapRadius: number
* }}
*/
let Config; // eslint-disable-line no-unused-vars

/**
* Default snap radius.
* @type {number}
*/
const DEFAULT_SNAP_RADIUS = 28;

/**
* Object holding all the values on Blockly that we expect developers to be
* able to change.
* @type {Config}
*/
const config = {
/**
* Number of pixels the mouse must move before a drag starts.
* @alias Blockly.config.dragRadius
*/
dragRadius: 5,
/**
* Number of pixels the mouse must move before a drag/scroll starts from the
* flyout. Because the drag-intention is determined when this is reached, it
* is larger than dragRadius so that the drag-direction is clearer.
* @alias Blockly.config.flyoutDragRadius
*/
flyoutDragRadius: 10,
/**
* Maximum misalignment between connections for them to snap together.
* @alias Blockly.config.snapRadius
*/
snapRadius: DEFAULT_SNAP_RADIUS,
/**
* Maximum misalignment between connections for them to snap together.
* This should be the same as the snap radius.
* @alias Blockly.config.connectingSnapRadius
*/
connectingSnapRadius: DEFAULT_SNAP_RADIUS,
/**
* How much to prefer staying connected to the current connection over moving
* to a new connection. The current previewed connection is considered to be
* this much closer to the matching connection on the block than it actually
* is.
* @alias Blockly.config.currentConnectionPreference
*/
currentConnectionPreference: 8,
/**
* Delay in ms between trigger and bumping unconnected block out of alignment.
* @alias Blockly.config.bumpDelay
*/
bumpDelay: 250,
};

exports.config = config;
13 changes: 13 additions & 0 deletions core/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,16 @@ exports.COLLAPSED_INPUT_NAME = COLLAPSED_INPUT_NAME;
*/
const COLLAPSED_FIELD_NAME = '_TEMP_COLLAPSED_FIELD';
exports.COLLAPSED_FIELD_NAME = COLLAPSED_FIELD_NAME;

/**
* Contains the path to a single png tat holds the images for the trashcan
* as well as the zoom controls.
* @const {!Object}
* @alias Blockly.constants.SPRITE
*/
const SPRITE = {
width: 96,
height: 124,
url: 'sprites.png',
};
exports.SPRITE = SPRITE;
8 changes: 4 additions & 4 deletions core/contextmenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ const clipboard = goog.require('Blockly.clipboard');
const deprecation = goog.require('Blockly.utils.deprecation');
const dom = goog.require('Blockly.utils.dom');
const eventUtils = goog.require('Blockly.Events.utils');
const internalConstants = goog.require('Blockly.internalConstants');
const userAgent = goog.require('Blockly.utils.userAgent');
const svgMath = goog.require('Blockly.utils.svgMath');
/* eslint-disable-next-line no-unused-vars */
const {Block} = goog.requireType('Blockly.Block');
const {config} = goog.require('Blockly.config');
const {Coordinate} = goog.require('Blockly.utils.Coordinate');
const {MenuItem} = goog.require('Blockly.MenuItem');
const {Menu} = goog.require('Blockly.Menu');
Expand Down Expand Up @@ -266,11 +266,11 @@ const callbackFactory = function(block, xml) {
// Move the new block next to the old block.
const xy = block.getRelativeToSurfaceXY();
if (block.RTL) {
xy.x -= internalConstants.SNAP_RADIUS;
xy.x -= config.snapRadius;
} else {
xy.x += internalConstants.SNAP_RADIUS;
xy.x += config.snapRadius;
}
xy.y += internalConstants.SNAP_RADIUS * 2;
xy.y += config.snapRadius * 2;
newBlock.moveBy(xy.x, xy.y);
} finally {
eventUtils.enable();
Expand Down
5 changes: 3 additions & 2 deletions core/gesture.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const registry = goog.require('Blockly.registry');
/* eslint-disable-next-line no-unused-vars */
const {BlockSvg} = goog.requireType('Blockly.BlockSvg');
const {BubbleDragger} = goog.require('Blockly.BubbleDragger');
const {config} = goog.require('Blockly.config');
const {Coordinate} = goog.require('Blockly.utils.Coordinate');
/* eslint-disable-next-line no-unused-vars */
const {Field} = goog.requireType('Blockly.Field');
Expand Down Expand Up @@ -307,8 +308,8 @@ class Gesture {
const currentDragDelta = Coordinate.magnitude(this.currentDragDeltaXY_);

// The flyout has a different drag radius from the rest of Blockly.
const limitRadius = this.flyout_ ? internalConstants.FLYOUT_DRAG_RADIUS :
internalConstants.DRAG_RADIUS;
const limitRadius =
this.flyout_ ? config.flyoutDragRadius : config.dragRadius;

this.hasExceededDragRadius_ = currentDragDelta > limitRadius;
return this.hasExceededDragRadius_;
Expand Down
9 changes: 4 additions & 5 deletions core/insertion_marker_manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ const blockAnimations = goog.require('Blockly.blockAnimations');
const common = goog.require('Blockly.common');
const constants = goog.require('Blockly.constants');
const eventUtils = goog.require('Blockly.Events.utils');
const internalConstants = goog.require('Blockly.internalConstants');
/* eslint-disable-next-line no-unused-vars */
const {BlockSvg} = goog.requireType('Blockly.BlockSvg');
const {ComponentManager} = goog.require('Blockly.ComponentManager');
const {config} = goog.require('Blockly.config');
const {ConnectionType} = goog.require('Blockly.ConnectionType');
/* eslint-disable-next-line no-unused-vars */
const {Coordinate} = goog.requireType('Blockly.utils.Coordinate');
Expand Down Expand Up @@ -389,8 +389,7 @@ class InsertionMarkerManager {
// Slightly prefer the existing preview over a new preview.
return !(
candidateClosest &&
radius >
curDistance - internalConstants.CURRENT_CONNECTION_PREFERENCE);
radius > curDistance - config.currentConnectionPreference);
} else if (!this.localConnection_ && !this.closestConnection_) {
// We weren't showing a preview before, but we should now.
return true;
Expand Down Expand Up @@ -459,9 +458,9 @@ class InsertionMarkerManager {
// of range. By increasing radiusConnection when a connection already
// exists, we never "lose" the connection from the offset.
if (this.closestConnection_ && this.localConnection_) {
return internalConstants.CONNECTING_SNAP_RADIUS;
return config.connectingSnapRadius;
}
return internalConstants.SNAP_RADIUS;
return config.snapRadius;
}

/**
Expand Down
91 changes: 0 additions & 91 deletions core/internal_constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,53 +21,6 @@ goog.module('Blockly.internalConstants');
const {ConnectionType} = goog.require('Blockly.ConnectionType');


/**
* Number of pixels the mouse must move before a drag starts.
* @alias Blockly.internalConstants.DRAG_RADIUS
*/
const DRAG_RADIUS = 5;
exports.DRAG_RADIUS = DRAG_RADIUS;

/**
* Number of pixels the mouse must move before a drag/scroll starts from the
* flyout. Because the drag-intention is determined when this is reached, it is
* larger than DRAG_RADIUS so that the drag-direction is clearer.
* @alias Blockly.internalConstants.FLYOUT_DRAG_RADIUS
*/
const FLYOUT_DRAG_RADIUS = 10;
exports.FLYOUT_DRAG_RADIUS = FLYOUT_DRAG_RADIUS;

/**
* Maximum misalignment between connections for them to snap together.
* @alias Blockly.internalConstants.SNAP_RADIUS
*/
const SNAP_RADIUS = 28;
exports.SNAP_RADIUS = SNAP_RADIUS;

/**
* Maximum misalignment between connections for them to snap together,
* when a connection is already highlighted.
* @alias Blockly.internalConstants.CONNECTING_SNAP_RADIUS
*/
const CONNECTING_SNAP_RADIUS = SNAP_RADIUS;
exports.CONNECTING_SNAP_RADIUS = CONNECTING_SNAP_RADIUS;

/**
* How much to prefer staying connected to the current connection over moving to
* a new connection. The current previewed connection is considered to be this
* much closer to the matching connection on the block than it actually is.
* @alias Blockly.internalConstants.CURRENT_CONNECTION_PREFERENCE
*/
const CURRENT_CONNECTION_PREFERENCE = 8;
exports.CURRENT_CONNECTION_PREFERENCE = CURRENT_CONNECTION_PREFERENCE;

/**
* Delay in ms between trigger and bumping unconnected block out of alignment.
* @alias Blockly.internalConstants.BUMP_DELAY
*/
const BUMP_DELAY = 250;
exports.BUMP_DELAY = BUMP_DELAY;

/**
* Number of characters to truncate a collapsed block to.
* @alias Blockly.internalConstants.COLLAPSE_CHARS
Expand All @@ -83,50 +36,6 @@ exports.COLLAPSE_CHARS = COLLAPSE_CHARS;
const DRAG_STACK = true;
exports.DRAG_STACK = DRAG_STACK;

/**
* Sprited icons and images.
* @alias Blockly.internalConstants.SPRITE
*/
const SPRITE = {
width: 96,
height: 124,
url: 'sprites.png',
};
exports.SPRITE = SPRITE;

/**
* ENUM for no drag operation.
* @const
* @alias Blockly.internalConstants.DRAG_NONE
*/
const DRAG_NONE = 0;
exports.DRAG_NONE = DRAG_NONE;

/**
* ENUM for inside the sticky DRAG_RADIUS.
* @const
* @alias Blockly.internalConstants.DRAG_STICKY
*/
const DRAG_STICKY = 1;
exports.DRAG_STICKY = DRAG_STICKY;

/**
* ENUM for inside the non-sticky DRAG_RADIUS, for differentiating between
* clicks and drags.
* @const
* @alias Blockly.internalConstants.DRAG_BEGIN
*/
const DRAG_BEGIN = 1;
exports.DRAG_BEGIN = DRAG_BEGIN;

/**
* ENUM for freely draggable (outside the DRAG_RADIUS, if one applies).
* @const
* @alias Blockly.internalConstants.DRAG_FREE
*/
const DRAG_FREE = 2;
exports.DRAG_FREE = DRAG_FREE;

/**
* Lookup table for determining the opposite type of a connection.
* @const
Expand Down
Loading