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.

Sign up for GitHub

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

Jump to bottom

chore: Remove some more uses of AnyDuringMigration #6785

Merged
merged 10 commits into from
Feb 3, 2023
Merged

chore: Remove some more uses of AnyDuringMigration #6785

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
129d4c9
chore: Remove uses of AnyDuringMigration in workspace_comment_svg.ts.
gonfunko Jan 20, 2023
dcfcfe1
chore: Remove uses of AnyDuringMigration in toolbox.ts.
gonfunko Jan 20, 2023
c6800e5
chore: Remove uses of AnyDuringMigration in field_colour.ts.
gonfunko Jan 20, 2023
5084ac3
chore: Remove uses of AnyDuringMigration from field_image.ts.
gonfunko Jan 20, 2023
d0e14cd
chore: Remove uses of AnyDuringMigration from workspace.ts.
gonfunko Jan 20, 2023
0260f1b
Update core/workspace_comment_svg.ts
gonfunko Jan 31, 2023
5b6d5f0
chore: Remove uses of AnyDuringMigration in collapsible_category.ts.
gonfunko Jan 23, 2023
fdc1340
chore: Revert unary - change to make clang-format happy.
gonfunko Jan 31, 2023
67322aa
chore: Remove unnecessary quotes in toolbox.ts.
gonfunko Jan 31, 2023
685a2b9
Update core/workspace_comment_svg.ts
gonfunko Jan 31, 2023
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 core/events/events_comment_move.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class CommentMove extends CommentBase {
}

this.comment_ = opt_comment;
this.oldCoordinate_ = opt_comment.getXY();
this.oldCoordinate_ = opt_comment.getRelativeToSurfaceXY();
}

/**
Expand All @@ -70,7 +70,7 @@ export class CommentMove extends CommentBase {
'The comment is undefined. Pass a comment to ' +
'the constructor if you want to use the record functionality');
}
this.newCoordinate_ = this.comment_.getXY();
this.newCoordinate_ = this.comment_.getRelativeToSurfaceXY();
}

/**
Expand Down Expand Up @@ -180,7 +180,7 @@ export class CommentMove extends CommentBase {
'or call fromJson');
}
// TODO: Check if the comment is being dragged, and give up if so.
const current = comment.getXY();
const current = comment.getRelativeToSurfaceXY();
comment.moveBy(target.x - current.x, target.y - current.y);
}
}
Expand Down
28 changes: 9 additions & 19 deletions core/field_colour.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,13 @@ export class FieldColour extends Field<string> {
protected override isDirty_ = false;

/** Array of colours used by this field. If null, use the global list. */
// AnyDuringMigration because: Type 'null' is not assignable to type
// 'string[]'.
private colours_: string[] = null as AnyDuringMigration;
private colours_: string[]|null = null;

/**
* Array of colour tooltips used by this field. If null, use the global
* list.
*/
// AnyDuringMigration because: Type 'null' is not assignable to type
// 'string[]'.
private titles_: string[] = null as AnyDuringMigration;
private titles_: string[]|null = null;

/**
* Number of colour columns used by this field. If 0, use the global
Expand Down Expand Up @@ -211,8 +207,7 @@ export class FieldColour extends Field<string> {
* @param opt_newValue The input value.
* @returns A valid colour, or null if invalid.
*/
protected override doClassValidation_(opt_newValue?: AnyDuringMigration):
string|null {
protected override doClassValidation_(opt_newValue?: any): string|null {
if (typeof opt_newValue !== 'string') {
return null;
}
Expand Down Expand Up @@ -415,19 +410,15 @@ export class FieldColour extends Field<string> {

/** Handle a mouse enter event. Focus the picker. */
private onMouseEnter_() {
// AnyDuringMigration because: Property 'focus' does not exist on type
// 'Element'.
(this.picker_ as AnyDuringMigration)!.focus({preventScroll: true});
this.picker_?.focus({preventScroll: true});
}

/**
* Handle a mouse leave event. Blur the picker and unhighlight
* the currently highlighted colour.
*/
private onMouseLeave_() {
// AnyDuringMigration because: Property 'blur' does not exist on type
// 'Element'.
(this.picker_ as AnyDuringMigration)!.blur();
this.picker_?.blur();
const highlighted = this.getHighlighted_();
if (highlighted) {
dom.removeClass(highlighted, 'blocklyColourHighlighted');
Expand Down Expand Up @@ -473,11 +464,10 @@ export class FieldColour extends Field<string> {
this.highlightedIndex_ = index;

// Update accessibility roles.
// AnyDuringMigration because: Argument of type 'string | null' is not
// assignable to parameter of type 'string | number | boolean | string[]'.
aria.setState(
this.picker_ as Element, aria.State.ACTIVEDESCENDANT,
cell.getAttribute('id') as AnyDuringMigration);
const cellId = cell.getAttribute('id');
if (cellId && this.picker_) {
aria.setState(this.picker_, aria.State.ACTIVEDESCENDANT, cellId);
}
}

/** Create a colour picker dropdown editor. */
Expand Down
13 changes: 5 additions & 8 deletions core/field_image.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ export class FieldImage extends Field<string> {
private readonly imageHeight_: number;

/** The function to be called when this field is clicked. */
private clickHandler_: ((p1: FieldImage) => AnyDuringMigration)|null = null;
private clickHandler_: ((p1: FieldImage) => void)|null = null;

/** The rendered field's image element. */
// AnyDuringMigration because: Type 'null' is not assignable to type
// 'SVGImageElement'.
private imageElement_: SVGImageElement = null as AnyDuringMigration;
private imageElement_: SVGImageElement|null = null;

/**
* Editable fields usually show some sort of UI indicating they are
Expand Down Expand Up @@ -79,7 +77,7 @@ export class FieldImage extends Field<string> {
*/
constructor(
src: string|Sentinel, width: string|number, height: string|number,
opt_alt?: string, opt_onClick?: (p1: FieldImage) => AnyDuringMigration,
opt_alt?: string, opt_onClick?: (p1: FieldImage) => void,
opt_flipRtl?: boolean, opt_config?: FieldImageConfig) {
super(Field.SKIP_SETUP);

Expand Down Expand Up @@ -164,8 +162,7 @@ export class FieldImage extends Field<string> {
* @param opt_newValue The input value.
* @returns A string, or null if invalid.
*/
protected override doClassValidation_(opt_newValue?: AnyDuringMigration):
string|null {
protected override doClassValidation_(opt_newValue?: any): string|null {
if (typeof opt_newValue !== 'string') {
return null;
}
Expand Down Expand Up @@ -226,7 +223,7 @@ export class FieldImage extends Field<string> {
* @param func The function that is called when the image is clicked, or null
* to remove.
*/
setOnClickHandler(func: ((p1: FieldImage) => AnyDuringMigration)|null) {
setOnClickHandler(func: ((p1: FieldImage) => void)|null) {
this.clickHandler_ = func;
}

Expand Down
2 changes: 1 addition & 1 deletion core/interfaces/i_copyable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export namespace ICopyable {
export interface CopyData {
saveInfo: Object|Element;
source: WorkspaceSvg;
typeCounts: Object|null;
typeCounts: {[key: string]: number}|null;
}
}

Expand Down
8 changes: 8 additions & 0 deletions core/interfaces/i_selectable_toolbox_item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,11 @@ export interface ISelectableToolboxItem extends IToolboxItem {
*/
onClick(_e: Event): void;
}

/**
* Type guard that checks whether an IToolboxItem is an ISelectableToolboxItem.
*/
export function isSelectableToolboxItem(toolboxItem: IToolboxItem):
toolboxItem is ISelectableToolboxItem {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i-dont-understand-is-it-modern

(I'll go look at the typescript docs--I know I've skimmed pass this section before.)

return toolboxItem.isSelectable();
}
48 changes: 16 additions & 32 deletions core/toolbox/toolbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type {IDraggable} from '../interfaces/i_draggable.js';
import type {IFlyout} from '../interfaces/i_flyout.js';
import type {IKeyboardAccessible} from '../interfaces/i_keyboard_accessible.js';
import type {ISelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js';
import {isSelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js';
import type {IStyleable} from '../interfaces/i_styleable.js';
import type {IToolbox} from '../interfaces/i_toolbox.js';
import type {IToolboxItem} from '../interfaces/i_toolbox_item.js';
Expand Down Expand Up @@ -100,7 +101,6 @@ export class Toolbox extends DeleteArea implements IAutoHideable,
* Ex: [[node, name, func], [node, name, func]].
*/
protected boundEvents_: browserEvents.Data[] = [];
override wouldDelete_: AnyDuringMigration;

/** The workspace this toolbox is on. */
protected readonly workspace_: WorkspaceSvg;
Expand All @@ -112,10 +112,9 @@ export class Toolbox extends DeleteArea implements IAutoHideable,
this.workspace_ = workspace;

/** The JSON describing the contents of this toolbox. */
// AnyDuringMigration because: Type 'ToolboxInfo | { contents: never[]; }'
// is not assignable to type 'ToolboxInfo'.
this.toolboxDef_ = (workspace.options.languageTree || {'contents': []}) as
AnyDuringMigration;
this.toolboxDef_ =
(workspace.options.languageTree ||
{'contents': new Array<toolbox.ToolboxItemInfo>()});
cpcallen marked this conversation as resolved.
Show resolved Hide resolved

/** Whether the toolbox should be laid out horizontally. */
this.horizontalLayout_ = workspace.options.horizontalLayout;
Expand Down Expand Up @@ -299,9 +298,8 @@ export class Toolbox extends DeleteArea implements IAutoHideable,
if (!handled && this.selectedItem_) {
// TODO(#6097): Figure out who implements onKeyDown and which interface it
// should be part of.
const untypedItem = this.selectedItem_ as AnyDuringMigration;
if (untypedItem.onKeyDown) {
handled = untypedItem.onKeyDown(e);
if ((this.selectedItem_ as any).onKeyDown) {
handled = (this.selectedItem_ as any).onKeyDown(e);
cpcallen marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -796,33 +794,20 @@ export class Toolbox extends DeleteArea implements IAutoHideable,
setSelectedItem(newItem: IToolboxItem|null) {
const oldItem = this.selectedItem_;

if (!newItem && !oldItem || newItem && !newItem.isSelectable()) {
if (!newItem && !oldItem || newItem && !isSelectableToolboxItem(newItem)) {
return;
}
newItem = newItem as ISelectableToolboxItem;

// AnyDuringMigration because: Argument of type 'IToolboxItem' is not
// assignable to parameter of type 'ISelectableToolboxItem'.
if (this.shouldDeselectItem_(oldItem, newItem as AnyDuringMigration) &&
oldItem !== null) {
if (this.shouldDeselectItem_(oldItem, newItem) && oldItem !== null) {
this.deselectItem_(oldItem);
}

// AnyDuringMigration because: Argument of type 'IToolboxItem' is not
// assignable to parameter of type 'ISelectableToolboxItem'.
if (this.shouldSelectItem_(oldItem, newItem as AnyDuringMigration) &&
newItem !== null) {
// AnyDuringMigration because: Argument of type 'IToolboxItem' is not
// assignable to parameter of type 'ISelectableToolboxItem'.
this.selectItem_(oldItem, newItem as AnyDuringMigration);
if (this.shouldSelectItem_(oldItem, newItem) && newItem !== null) {
this.selectItem_(oldItem, newItem);
}

// AnyDuringMigration because: Argument of type 'IToolboxItem' is not
// assignable to parameter of type 'ISelectableToolboxItem'.
this.updateFlyout_(oldItem, newItem as AnyDuringMigration);
// AnyDuringMigration because: Argument of type 'IToolboxItem' is not
// assignable to parameter of type 'ISelectableToolboxItem'.
this.fireSelectEvent_(oldItem, newItem as AnyDuringMigration);
this.updateFlyout_(oldItem, newItem);
this.fireSelectEvent_(oldItem, newItem);
}

/**
Expand Down Expand Up @@ -1044,11 +1029,10 @@ export class Toolbox extends DeleteArea implements IAutoHideable,
this.boundEvents_ = [];
this.contents_ = [];

// AnyDuringMigration because: Argument of type 'HTMLDivElement | null' is
// not assignable to parameter of type 'Element'.
this.workspace_.getThemeManager().unsubscribe(
this.HtmlDiv as AnyDuringMigration);
dom.removeNode(this.HtmlDiv);
if (this.HtmlDiv) {
this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv);
dom.removeNode(this.HtmlDiv);
}
}
}

Expand Down
69 changes: 14 additions & 55 deletions core/workspace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,22 +172,11 @@ export class Workspace implements IASTNodeLocation {
*/
private sortObjects_(a: Block|WorkspaceComment, b: Block|WorkspaceComment):
number {
// AnyDuringMigration because: Property 'getRelativeToSurfaceXY' does not
// exist on type 'Block | WorkspaceComment'.
const aXY = (a as AnyDuringMigration).getRelativeToSurfaceXY();
// AnyDuringMigration because: Property 'getRelativeToSurfaceXY' does not
// exist on type 'Block | WorkspaceComment'.
const bXY = (b as AnyDuringMigration).getRelativeToSurfaceXY();
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) => number'.
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) => number'.
return aXY.y +
(Workspace.prototype.sortObjects_ as AnyDuringMigration).offset *
aXY.x -
(bXY.y +
(Workspace.prototype.sortObjects_ as AnyDuringMigration).offset *
bXY.x);
const offset =
Math.sin(math.toRadians(Workspace.SCAN_ANGLE)) * (this.RTL ? -1 : 1);
gonfunko marked this conversation as resolved.
Show resolved Hide resolved
const aXY = a.getRelativeToSurfaceXY();
const bXY = b.getRelativeToSurfaceXY();
return aXY.y + offset * aXY.x - (bXY.y + offset * bXY.x);
}

/**
Expand Down Expand Up @@ -221,17 +210,7 @@ export class Workspace implements IASTNodeLocation {
// Copy the topBlocks list.
const blocks = (new Array<Block>()).concat(this.topBlocks);
if (ordered && blocks.length > 1) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) => number'.
(this.sortObjects_ as AnyDuringMigration).offset =
Math.sin(math.toRadians(Workspace.SCAN_ANGLE));
if (this.RTL) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) =>
// number'.
(this.sortObjects_ as AnyDuringMigration).offset *= -1;
}
blocks.sort(this.sortObjects_);
blocks.sort(this.sortObjects_.bind(this));
}
return blocks;
}
Expand Down Expand Up @@ -274,20 +253,10 @@ export class Workspace implements IASTNodeLocation {
}
const blocks = this.typedBlocksDB.get(type)!.slice(0);
if (ordered && blocks && blocks.length > 1) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) => number'.
(this.sortObjects_ as AnyDuringMigration).offset =
Math.sin(math.toRadians(Workspace.SCAN_ANGLE));
if (this.RTL) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) =>
// number'.
(this.sortObjects_ as AnyDuringMigration).offset *= -1;
}
blocks.sort(this.sortObjects_);
blocks.sort(this.sortObjects_.bind(this));
}

return blocks.filter(function(block: AnyDuringMigration) {
return blocks.filter(function(block: Block) {
return !block.isInsertionMarker();
});
}
Expand Down Expand Up @@ -340,17 +309,7 @@ export class Workspace implements IASTNodeLocation {
// Copy the topComments list.
const comments = (new Array<WorkspaceComment>()).concat(this.topComments);
if (ordered && comments.length > 1) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) => number'.
(this.sortObjects_ as AnyDuringMigration).offset =
Math.sin(math.toRadians(Workspace.SCAN_ANGLE));
if (this.RTL) {
// AnyDuringMigration because: Property 'offset' does not exist on type
// '(a: Block | WorkspaceComment, b: Block | WorkspaceComment) =>
// number'.
(this.sortObjects_ as AnyDuringMigration).offset *= -1;
}
comments.sort(this.sortObjects_);
comments.sort(this.sortObjects_.bind(this));
}
return comments;
}
Expand All @@ -363,7 +322,7 @@ export class Workspace implements IASTNodeLocation {
* @returns Array of blocks.
*/
getAllBlocks(ordered: boolean): Block[] {
let blocks: AnyDuringMigration[];
let blocks: Block[];
if (ordered) {
// Slow, but ordered.
const topBlocks = this.getTopBlocks(true);
Expand Down Expand Up @@ -598,7 +557,7 @@ export class Workspace implements IASTNodeLocation {
* to be created).
* @returns True if there is capacity for the given map, false otherwise.
*/
isCapacityAvailable(typeCountsMap: AnyDuringMigration): boolean {
isCapacityAvailable(typeCountsMap: {[key: string]: number}): boolean {
if (!this.hasBlockLimits()) {
return true;
}
Expand Down Expand Up @@ -661,9 +620,9 @@ export class Workspace implements IASTNodeLocation {
// Do another undo/redo if the next one is of the same group.
while (inputStack.length && inputEvent.group &&
inputEvent.group === inputStack[inputStack.length - 1].group) {
// AnyDuringMigration because: Argument of type 'Abstract | undefined' is
// not assignable to parameter of type 'Abstract'.
events.push(inputStack.pop() as AnyDuringMigration);
const event = inputStack.pop();
if (!event) continue;
events.push(event);
}
// Push these popped events on the opposite stack.
for (let i = 0; i < events.length; i++) {
Expand Down
2 changes: 1 addition & 1 deletion core/workspace_comment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class WorkspaceComment {
* This is not valid if the comment is currently being dragged.
* @internal
*/
getXY(): Coordinate {
getRelativeToSurfaceXY(): Coordinate {
Copy link
Contributor

Choose a reason for hiding this comment

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

This rename would appear to be a breaking change to the API. Is that intended? If so, please update commit message and PR title accordingly. If not, please (at most) deprecate getXY rather than removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is @internal, so I was under the impression we can do whatever we want with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to get @rachel-fenichel and/or @maribethb's take on that. My understanding is that there are some cases where we have previously suggested using (previously) @private methods and now treat them as if they had been @public for the purpose of deciding what's a breaking change.

If this method is not one we've ever suggested people use then SGTM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is safe--we rarely recommend devs do anything with workspace comments, and I don't remember us ever suggesting people use this. And we use the same name (getRelativeToSurfaceXY) for the same functionality in block/block_svg.

return new Coordinate(this.xy_.x, this.xy_.y);
}

Expand Down
Loading