Skip to content

refactor(core): size and filter viewports by canvas#10475

Open
ibgreen-openai wants to merge 5 commits into
codex/multi-canvas-context-plumbingfrom
codex/multi-canvas-view-layout
Open

refactor(core): size and filter viewports by canvas#10475
ibgreen-openai wants to merge 5 commits into
codex/multi-canvas-context-plumbingfrom
codex/multi-canvas-view-layout

Conversation

@ibgreen-openai

@ibgreen-openai ibgreen-openai commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Goal

Teach the existing ViewManager to lay out and select views by presentation canvas without introducing a second canvas-size tracking system.

Why this is needed

Deck.width and Deck.height describe the default canvas, but views assigned to different presentation canvases must use each canvas's own CSS-pixel dimensions. luma's existing CanvasContext / PresentationContext already owns resize observation, CSS dimensions, device-pixel ratio, and drawing-buffer dimensions.

This PR therefore adds only an optional internal getCanvasContext(canvasId) callback. ViewManager reads context.getCSSSize() when constructing viewports and controller viewports; it does not maintain a size map, size cache, resize observer, or duplicate drawing-buffer bookkeeping. The follow-up feature PR invalidates the existing viewport layout when luma reports a presentation-canvas resize.

Changes

  • Resolve a view's canvas and read its authoritative CSS dimensions directly from the existing luma context.
  • Fall back to existing ViewManager.width / height when no context lookup is supplied.
  • Support both whole-canvas viewport queries ({canvasId}) and combined canvas/pixel queries ({x, y, canvasId}).
  • Preserve first-canvas defaults, unknown-canvas filtering, unchanged viewport identity, and explicit resize invalidation.
  • Add focused coverage for independent canvas dimensions, context-owned resize updates, whole-canvas filtering, and first-canvas fallback.

Review scope

Only modules/core/src/lib/view-manager.ts and its existing unit test file. No new public Deck props, canvas lifecycle changes, widgets, presentation code, or parallel size tracking.

Validation

  • yarn test-headless test/modules/core/lib/view-manager.spec.ts test/modules/core/lib/deck.spec.ts --retry 1 --silent — 32 passed.
  • Core TypeScript check passed.
  • Repository lint, formatting, and 21 node smoke tests passed through the pre-commit hook.
  • Downstream feature validation: 90 affected rendering, picking, widget, resize, and integration tests passed.

Stack

  1. refactor(core): honor canvas contexts throughout render passes #10474: context-aware render/picking plumbing.
  2. This PR: context-derived viewport sizing and canvas filtering.
  3. RFC/POC: Extend View system to support multi-canvas rendering #10229: multi-canvas presentation and interaction.

@coveralls

coveralls commented Jul 23, 2026

Copy link
Copy Markdown

Coverage Status

Coverage is 83.389%codex/multi-canvas-view-layout into codex/multi-canvas-context-plumbing. No base build found for codex/multi-canvas-context-plumbing.

@ibgreen
ibgreen marked this pull request as ready for review July 23, 2026 18:24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b73e172efb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modules/core/src/lib/view-manager.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants