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: convert some classes to ES6 classes #5928

Merged
4 changes: 2 additions & 2 deletions core/events/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ goog.module('Blockly.Events');

const deprecation = goog.require('Blockly.utils.deprecation');
const eventUtils = goog.require('Blockly.Events.utils');
const {Abstract} = goog.require('Blockly.Events.Abstract');
const {Abstract: AbstractEvent} = goog.require('Blockly.Events.Abstract');
const {BlockBase} = goog.require('Blockly.Events.BlockBase');
const {BlockChange} = goog.require('Blockly.Events.BlockChange');
const {BlockCreate} = goog.require('Blockly.Events.BlockCreate');
Expand Down Expand Up @@ -47,7 +47,7 @@ const {ViewportChange} = goog.require('Blockly.Events.ViewportChange');


// Events.
exports.Abstract = Abstract;
exports.Abstract = AbstractEvent;
exports.BubbleOpen = BubbleOpen;
exports.BlockBase = BlockBase;
exports.BlockChange = BlockChange;
Expand Down
161 changes: 86 additions & 75 deletions core/events/events_abstract.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,98 +24,109 @@ const {Workspace} = goog.requireType('Blockly.Workspace');

/**
* Abstract class for an event.
* @constructor
* @alias Blockly.Events.Abstract
* @abstract
*/
const Abstract = function() {
class Abstract {
Comment on lines 28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be declared @abstract, either in this PR or a subsequent one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {?boolean}
* @alias Blockly.Events.Abstract
*/
this.isBlank = null;
constructor() {
/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {?boolean}
*/
this.isBlank = null;

/**
* The workspace identifier for this event.
* @type {string|undefined}
*/
this.workspaceId = undefined;

/**
* The event group id for the group this event belongs to. Groups define
* events that should be treated as an single action from the user's
* perspective, and should be undone together.
* @type {string}
*/
this.group = eventUtils.getGroup();

/**
* Sets whether the event should be added to the undo stack.
* @type {boolean}
*/
this.recordUndo = eventUtils.getRecordUndo();

/**
* Whether or not the event is a UI event.
* @type {boolean}
*/
this.isUiEvent = false;

/**
* Type of this event.
* @type {string|undefined}
*/
this.type = undefined;
Comment on lines +66 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be preferable if .type had @type {string}, rather than allowing undefined. What about adding a type parameter to the constructor and thereby force subclasses to provide a type tag when they call super?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to note, I'm pretty sure we let external peeps define events (I think AppInventor does this?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could reasonably go to a string and have the empty string if not set--but when I tried that assorted tests broke, mostly because of assertions about what the type property was, so I left it alone in the interest of making a minimal change. I expect to return to this to address typescript compiler warnings about this value possibly being undefined.

}

/**
* The workspace identifier for this event.
* @type {string|undefined}
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
this.workspaceId = undefined;
toJson() {
const json = {'type': this.type};
if (this.group) {
json['group'] = this.group;
}
return json;
}

/**
* The event group id for the group this event belongs to. Groups define
* events that should be treated as an single action from the user's
* perspective, and should be undone together.
* @type {string}
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
this.group = eventUtils.getGroup();
fromJson(json) {
this.isBlank = false;
this.group = json['group'];
}

/**
* Sets whether the event should be added to the undo stack.
* @type {boolean}
* Does this event record any change of state?
* @return {boolean} True if null, false if something changed.
*/
this.recordUndo = eventUtils.getRecordUndo();
isNull() {
return false;
}

/**
* Whether or not the event is a UI event.
* @type {boolean}
* Run an event.
* @param {boolean} _forward True if run forward, false if run backward
* (undo).
*/
this.isUiEvent = false;
};

/**
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
Abstract.prototype.toJson = function() {
const json = {'type': this.type};
if (this.group) {
json['group'] = this.group;
run(_forward) {
// Defined by subclasses.
}
return json;
};

/**
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
Abstract.prototype.fromJson = function(json) {
this.isBlank = false;
this.group = json['group'];
};

/**
* Does this event record any change of state?
* @return {boolean} True if null, false if something changed.
*/
Abstract.prototype.isNull = function() {
return false;
};

/**
* Run an event.
* @param {boolean} _forward True if run forward, false if run backward (undo).
*/
Abstract.prototype.run = function(_forward) {
// Defined by subclasses.
};

/**
* Get workspace the event belongs to.
* @return {!Workspace} The workspace the event belongs to.
* @throws {Error} if workspace is null.
* @protected
*/
Abstract.prototype.getEventWorkspace_ = function() {
let workspace;
if (this.workspaceId) {
const {Workspace} = goog.module.get('Blockly.Workspace');
workspace = Workspace.getById(this.workspaceId);
}
if (!workspace) {
throw Error(
'Workspace is null. Event must have been generated from real' +
' Blockly events.');
/**
* Get workspace the event belongs to.
* @return {!Workspace} The workspace the event belongs to.
* @throws {Error} if workspace is null.
* @protected
*/
getEventWorkspace_() {
let workspace;
if (this.workspaceId) {
const {Workspace} = goog.module.get('Blockly.Workspace');
workspace = Workspace.getById(this.workspaceId);
}
if (!workspace) {
throw Error(
'Workspace is null. Event must have been generated from real' +
' Blockly events.');
}
return workspace;
}
return workspace;
};
}

exports.Abstract = Abstract;
6 changes: 3 additions & 3 deletions core/events/events_block_base.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@
*/
goog.module('Blockly.Events.BlockBase');

const {Abstract} = goog.require('Blockly.Events.Abstract');
const {Abstract: AbstractEvent} = goog.require('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
const {Block} = goog.requireType('Blockly.Block');


/**
* Abstract class for a block event.
* @extends {Abstract}
* @extends {AbstractEvent}
*/
class BlockBase extends Abstract {
class BlockBase extends AbstractEvent {
/**
* @param {!Block=} opt_block The block this event corresponds to.
* Undefined for a blank event.
Expand Down
6 changes: 3 additions & 3 deletions core/events/events_comment_base.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ goog.module('Blockly.Events.CommentBase');
const Xml = goog.require('Blockly.Xml');
const eventUtils = goog.require('Blockly.Events.utils');
const utilsXml = goog.require('Blockly.utils.xml');
const {Abstract} = goog.require('Blockly.Events.Abstract');
const {Abstract: AbstractEvent} = goog.require('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
const {CommentCreate} = goog.requireType('Blockly.Events.CommentCreate');
/* eslint-disable-next-line no-unused-vars */
Expand All @@ -29,9 +29,9 @@ const {WorkspaceComment} = goog.requireType('Blockly.WorkspaceComment');

/**
* Abstract class for a comment event.
* @extends {Abstract}
* @extends {AbstractEvent}
*/
class CommentBase extends Abstract {
class CommentBase extends AbstractEvent {
/**
* @param {!WorkspaceComment=} opt_comment The comment this event
* corresponds to. Undefined for a blank event.
Expand Down
6 changes: 3 additions & 3 deletions core/events/events_ui_base.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*/
goog.module('Blockly.Events.UiBase');

const {Abstract} = goog.require('Blockly.Events.Abstract');
const {Abstract: AbstractEvent} = goog.require('Blockly.Events.Abstract');


/**
Expand All @@ -26,9 +26,9 @@ const {Abstract} = goog.require('Blockly.Events.Abstract');
* editing to work (e.g. scrolling the workspace, zooming, opening toolbox
* categories).
* UI events do not undo or redo.
* @extends {Abstract}
* @extends {AbstractEvent}
*/
class UiBase extends Abstract {
class UiBase extends AbstractEvent {
/**
* @param {string=} opt_workspaceId The workspace identifier for this event.
* Undefined for a blank event.
Expand Down
6 changes: 3 additions & 3 deletions core/events/events_var_base.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@
*/
goog.module('Blockly.Events.VarBase');

const {Abstract} = goog.require('Blockly.Events.Abstract');
const {Abstract: AbstractEvent} = goog.require('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
const {VariableModel} = goog.requireType('Blockly.VariableModel');


/**
* Abstract class for a variable event.
* @extends {Abstract}
* @extends {AbstractEvent}
*/
class VarBase extends Abstract {
class VarBase extends AbstractEvent {
/**
* @param {!VariableModel=} opt_variable The variable this event
* corresponds to. Undefined for a blank event.
Expand Down
35 changes: 23 additions & 12 deletions core/events/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const registry = goog.require('Blockly.registry');
/* eslint-disable-next-line no-unused-vars */
const {Abstract} = goog.requireType('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
const {BlockChange} = goog.requireType('Blockly.Events.BlockChange');
/* eslint-disable-next-line no-unused-vars */
const {BlockCreate} = goog.requireType('Blockly.Events.BlockCreate');
/* eslint-disable-next-line no-unused-vars */
const {BlockMove} = goog.requireType('Blockly.Events.BlockMove');
Expand All @@ -32,6 +34,8 @@ const {CommentCreate} = goog.requireType('Blockly.Events.CommentCreate');
/* eslint-disable-next-line no-unused-vars */
const {CommentMove} = goog.requireType('Blockly.Events.CommentMove');
/* eslint-disable-next-line no-unused-vars */
const {ViewportChange} = goog.requireType('Blockly.Events.ViewportChange');
/* eslint-disable-next-line no-unused-vars */
const {Workspace} = goog.requireType('Blockly.Workspace');


Expand Down Expand Up @@ -307,6 +311,7 @@ exports.BUMP_EVENTS = BUMP_EVENTS;

/**
* List of events queued for firing.
* @type {!Array<!Abstract>}
*/
const FIRE_QUEUE = [];

Expand Down Expand Up @@ -365,7 +370,9 @@ const filter = function(queueIn, forward) {
if (!event.isNull()) {
// Treat all UI events as the same type in hash table.
const eventType = event.isUiEvent ? UI : event.type;
const key = [eventType, event.blockId, event.workspaceId].join(' ');
// TODO(#5927): Ceck whether `blockId` exists before accessing it.
const blockId = /** @type {*} */ (event).blockId;
const key = [eventType, blockId, event.workspaceId].join(' ');

const lastEntry = hash[key];
const lastEvent = lastEntry ? lastEntry.event : null;
Expand All @@ -376,22 +383,25 @@ const filter = function(queueIn, forward) {
hash[key] = {event: event, index: i};
mergedQueue.push(event);
} else if (event.type === MOVE && lastEntry.index === i - 1) {
const moveEvent = /** @type {!BlockMove} */ (event);
Comment on lines 385 to +386
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (event.type === MOVE && lastEntry.index === i - 1) {
const moveEvent = /** @type {!BlockMove} */ (event);
} else if (event instanceof BlockMove) && lastEntry.index === i - 1) {

and then you don't need the cast or moveEvent.

// Merge move events.
lastEvent.newParentId = event.newParentId;
lastEvent.newInputName = event.newInputName;
lastEvent.newCoordinate = event.newCoordinate;
lastEvent.newParentId = moveEvent.newParentId;
lastEvent.newInputName = moveEvent.newInputName;
lastEvent.newCoordinate = moveEvent.newCoordinate;
lastEntry.index = i;
} else if (
event.type === CHANGE && event.element === lastEvent.element &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
event.type === CHANGE && event.element === lastEvent.element &&
(event.type instanceof BlockChange) && event.element === lastEvent.element &&

and dispense with the cast and changeEvent.

event.name === lastEvent.name) {
const changeEvent = /** @type {!BlockChange} */ (event);
// Merge change events.
lastEvent.newValue = event.newValue;
lastEvent.newValue = changeEvent.newValue;
} else if (event.type === VIEWPORT_CHANGE) {
const viewportEvent = /** @type {!ViewportChange} */ (event);
// Merge viewport change events.
lastEvent.viewTop = event.viewTop;
lastEvent.viewLeft = event.viewLeft;
lastEvent.scale = event.scale;
lastEvent.oldScale = event.oldScale;
lastEvent.viewTop = viewportEvent.viewTop;
lastEvent.viewLeft = viewportEvent.viewLeft;
lastEvent.scale = viewportEvent.scale;
lastEvent.oldScale = viewportEvent.oldScale;
} else if (event.type === CLICK && lastEvent.type === BUBBLE_OPEN) {
// Drop click events caused by opening/closing bubbles.
} else {
Expand Down Expand Up @@ -546,12 +556,13 @@ exports.get = get;
*/
const disableOrphans = function(event) {
if (event.type === MOVE || event.type === CREATE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (event.type === MOVE || event.type === CREATE) {
if ((event instanceof BlockMove) || (event instanceof BlockCreate)) {

and dispense with the cast and blockEvent.

if (!event.workspaceId) {
const blockEvent = /** @type {!BlockMove|!BlockCreate} */ (event);
if (!blockEvent.workspaceId) {
return;
}
const {Workspace} = goog.module.get('Blockly.Workspace');
const eventWorkspace = Workspace.getById(event.workspaceId);
let block = eventWorkspace.getBlockById(event.blockId);
const eventWorkspace = Workspace.getById(blockEvent.workspaceId);
let block = eventWorkspace.getBlockById(blockEvent.blockId);
if (block) {
// Changing blocks as part of this event shouldn't be undoable.
const initialUndoFlag = recordUndo;
Expand Down
Loading