Skip to content

Consider introducing incompatible types for workspace coordinates and screen coordinates #9156

@cpcallen

Description

@cpcallen

The issue RaspberryPiFoundation/blockly-keyboard-experimentation#446 was caused by passing Coordinates containing a delta measured in workspace coordinates to a function expecting a delta measured in pixels. In reviewing the fix, @BenHenning made an excellent suggestion:

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).

Changing the types accepted by our API is obviously a breaking change, but might be worth considering—this feels like the sort of situation where we might actually discover existing bugs in Blockly in the process.

Technical note: due to duck-typing in TS, it's necessary that there be some structural difference between the two types in order to ensure they are incompatible. This could be achieved by adding a (possibly dummy) property to the instances, but a better approach would be to add toWorkspace and toScreen (or similar) methods to their class declarations. That would mean that the actual coordinate objects were still just {x, y}, but with different [[prototype]] slots (pointing at differently-shaped .prototype objects) would cause tsc object if we mixed them up.

One additional consideration is whether we would additionally want to distinguish between absolute positions vs. offsets—but this might be excessively picayune.

Metadata

Metadata

Assignees

No one assigned

    Labels

    breaking changeUsed to mark a PR or issue that changes our public APIs.issue: feature requestDescribes a new feature and why it should be added

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions