Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 5 additions & 2 deletions src/actions/mover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ export class Mover {

info.dragger.onDrag(
info.fakePointerEvent('pointermove', direction),
info.totalDelta,
info.totalDelta.clone().scale(workspace.scale),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes me wonder if we should have distinct types for different coordinate spaces, e.g. ScreenCoordinate and WorkspaceCoordinate. It's not obvious from code that scaling performs the space conversion, or what coordinate system we're currently in at this point (since if totalDelta is already in screen space then scaling again would be wrong).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would be a great idea; I've filed RaspberryPiFoundation/blockly#9156 for future consideration.

Relatedly, one of the things I somewhat dislike about TypeScript (and also Go) is duck typing: it should be possible to give different names to the same type specification and have the compiler flag any mixed use (absent explicit casts). For object types it's pretty easy to make things different shapes (adding a dummy method to the class definition changes the type without changing the actuals shape of the objects in memory), but a classic use of nominal typing is to distinguish between strings containing HTML and ones containing plain text, and enforce calling a function to do escaping when converting the latter to the former. TypeScript doesn't let you do that—you'd have to use subclasses of the String object type rather than plain strings.

);

info.updateTotalDelta();
Expand All @@ -327,7 +327,10 @@ export class Mover {
info.totalDelta.x += x * UNCONSTRAINED_MOVE_DISTANCE * workspace.scale;
info.totalDelta.y += y * UNCONSTRAINED_MOVE_DISTANCE * workspace.scale;

info.dragger.onDrag(info.fakePointerEvent('pointermove'), info.totalDelta);
info.dragger.onDrag(
info.fakePointerEvent('pointermove'),
info.totalDelta.clone().scale(workspace.scale),
);
this.scrollCurrentBlockIntoView(workspace);
return true;
}
Expand Down
77 changes: 77 additions & 0 deletions test/webdriverio/test/move_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
tabNavigateToWorkspace,
testFileLocations,
testSetup,
sendKeyAndWait,
keyDown,
} from './test_setup.js';

suite('Move tests', function () {
Expand Down Expand Up @@ -145,6 +147,62 @@ suite('Move tests', function () {
await this.browser.keys(Key.Escape);
}
});

// When a top-level block with no previous, next or output
// connections is subject to a constrained move, it should not move.
//
// This includes a regression test for issue #446 (fixed in PR #599)
// where, due to an implementation error in Mover, constrained
// movement following unconstrained movement would result in the
// block unexpectedly moving (unless workspace scale was === 1).
test('Constrained move of unattachable top-level block', async function () {
// Block ID of an unconnectable block.
const BLOCK = 'p5_setup_1';

// Scale workspace.
await this.browser.execute(() => {
(Blockly.getMainWorkspace() as Blockly.WorkspaceSvg).setScale(0.9);
});

// Navigate to unconnectable block, get initial coords and start move.
await tabNavigateToWorkspace(this.browser);
await focusOnBlock(this.browser, BLOCK);
const startCoordinate = await getCoordinate(this.browser, BLOCK);
await this.browser.keys('m');

// Check constrained moves have no effect.
await keyDown(this.browser, 5);
const coordinate = await getCoordinate(this.browser, BLOCK);
chai.assert.deepEqual(
coordinate,
startCoordinate,
'constrained move should have no effect',
);

// Unconstrained moves.
await sendKeyAndWait(this.browser, [Key.Alt, Key.ArrowDown]);
await sendKeyAndWait(this.browser, [Key.Alt, Key.ArrowRight]);
const newCoordinate = await getCoordinate(this.browser, BLOCK);
chai.assert.notDeepEqual(
newCoordinate,
startCoordinate,
'unconstrained move should have effect',
);

// Try multiple constrained moves, as first might (correctly) do nothing.
for (let i = 0; i < 5; i++) {
await keyDown(this.browser);
const coordinate = await getCoordinate(this.browser, BLOCK);
chai.assert.deepEqual(
coordinate,
newCoordinate,
'constrained move after unconstrained move should have no effect',
);
}

// Abort move.
await this.browser.keys(Key.Escape);
});
});

/**
Expand Down Expand Up @@ -218,3 +276,22 @@ function getConnectedBlockInfo(browser: Browser, id: string, index: number) {
index,
);
}

/**
* Given a block ID, get the coordinates of that block, as returned by
* getRelativeTosSurfaceXY().
*
* @param browser The webdriverio browser session.
* @param id The ID of the block having the connection we wish to examine.
* @returns The coordinates of the block.
*/
function getCoordinate(
browser: Browser,
id: string,
): Promise<Blockly.utils.Coordinate> {
return browser.execute((id: string) => {
const block = Blockly.getMainWorkspace().getBlockById(id);
if (!block) throw new Error('block not found');
return block.getRelativeToSurfaceXY();
}, id);
}
22 changes: 10 additions & 12 deletions test/webdriverio/test/test_setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,7 @@ export async function tabNavigateToWorkspace(
* @param browser The active WebdriverIO Browser object.
*/
export async function tabNavigateForward(browser: WebdriverIO.Browser) {
await browser.keys(webdriverio.Key.Tab);
await browser.pause(PAUSE_TIME);
await sendKeyAndWait(browser, webdriverio.Key.Tab);
}

/**
Expand All @@ -426,8 +425,7 @@ export async function tabNavigateForward(browser: WebdriverIO.Browser) {
* @param browser The active WebdriverIO Browser object.
*/
export async function tabNavigateBackward(browser: WebdriverIO.Browser) {
await browser.keys([webdriverio.Key.Shift, webdriverio.Key.Tab]);
await browser.pause(PAUSE_TIME);
await sendKeyAndWait(browser, [webdriverio.Key.Shift, webdriverio.Key.Tab]);
}

/**
Expand Down Expand Up @@ -471,20 +469,20 @@ export async function keyDown(browser: WebdriverIO.Browser, times = 1) {
}

/**
* Sends the specified key for the specified number of times, waiting between
* each key press to allow changes to keep up.
* Sends the specified key(s) for the specified number of times,
* waiting between each key press to allow changes to keep up.
*
* @param browser The active WebdriverIO Browser object.
* @param key The WebdriverIO representative key value to press.
* @param times The number of times to repeat the key press.
* @param keys The WebdriverIO representative key value(s) to press.
* @param times The number of times to repeat the key press (default 1).
*/
async function sendKeyAndWait(
export async function sendKeyAndWait(
browser: WebdriverIO.Browser,
key: string,
times: number,
keys: string | string[],
times = 1,
) {
for (let i = 0; i < times; i++) {
await browser.keys(key);
await browser.keys(keys);
await browser.pause(PAUSE_TIME);
}
}
Expand Down