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 1 commit
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
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
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
2 changes: 1 addition & 1 deletion core/workspace_comment_svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ export class WorkspaceCommentSvg extends WorkspaceComment implements
* @returns Object with .x and .y properties in workspace coordinates.
* @internal
*/
getRelativeToSurfaceXY(): Coordinate {
override getRelativeToSurfaceXY(): Coordinate {
let x = 0;
let y = 0;

Expand Down
4 changes: 2 additions & 2 deletions tests/mocha/workspace_comment_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,14 @@ suite('Workspace comment', function() {
});

test('Initial position', function() {
const xy = this.comment.getXY();
const xy = this.comment.getRelativeToSurfaceXY();
chai.assert.equal(xy.x, 0, 'Initial X position');
chai.assert.equal(xy.y, 0, 'Initial Y position');
});

test('moveBy', function() {
this.comment.moveBy(10, 100);
const xy = this.comment.getXY();
const xy = this.comment.getRelativeToSurfaceXY();
chai.assert.equal(xy.x, 10, 'New X position');
chai.assert.equal(xy.y, 100, 'New Y position');
});
Expand Down