Skip to content

Fit or stretch background images within the map canvas - #298

Merged
TheWitness merged 7 commits into
Cacti:developfrom
alcatron:feat/background-sizing
Oct 10, 2026
Merged

TheWitness merged 7 commits into
Cacti:developfrom
alcatron:feat/background-sizing

Conversation

@alcatron

@alcatron alcatron commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Selecting a background previously changed the canvas to the image dimensions. Add Fit, Stretch and original Image size choices to Map Properties, persisted as a background_sizing hint.

Fit centers the image within the current canvas while preserving proportions; Stretch fills the canvas. Image size preserves existing behavior. Missing or invalid hints use Image size for compatibility. PHP locals follow snake_case conventions.

Validation: 226 PHP 8.3 tests passed in an isolated fixture layout. A real-engine regression saves each mode through setMapProperties(), reloads it, renders the image and checks canvas dimensions and pixels. It also checks invalid editor input preserves the existing hint and invalid stored hints use the renderer fallback. Syntax, whitespace and manifest checks passed; measured changed-line coverage is 100%, with existing editor entry/action exemptions. Full running-installation integration remains unverified for this revision.

Further review follow-up

Addressed in 2824a1e. Moved background_sizing lookup and validation after the post-processing loop. Added actual engine rendering checks for post-processor overrides to fit, stretch, image and an invalid value, verifying canvas dimensions and background pixels. All 226 plugin tests pass (5437 assertions); measured changed-line coverage passes at 19/19.

@TheWitness
TheWitness requested review from TheWitness and a balanced review from Copilot October 9, 2026 15:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new files and variables violate repository standards, and the editor persistence path lacks direct coverage.

5 open findings
What changed in this PR

Adds configurable background-image sizing while preserving backward-compatible behavior.

Changes:

  • Adds Fit, Stretch, and Image size editor options.
  • Persists sizing as a map hint and applies it during rendering.
  • Adds rendering regression coverage and translations.
File Description
weathermap-cacti-plugin-editor.php Adds the sizing selector.
lib/​editor.actions.php Saves the selected mode.
lib/​WeatherMap.class.php Implements background scaling and centering.
tests/​Unit/​BackgroundSizingEngineTest.php Runs the isolated regression test.
tests/​Support/​BackgroundSizingEngineRegression.php Tests persistence and rendered dimensions.
locales/​po/​cacti.pot Adds translated UI strings.
CHANGELOG.md Documents the feature.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/WeatherMap.class.php Outdated
Comment thread lib/editor.actions.php Outdated
Comment thread tests/Support/BackgroundSizingEngineRegression.php Outdated
Comment thread tests/Unit/BackgroundSizingEngineTest.php Outdated
Comment thread weathermap-cacti-plugin-editor.php Outdated
alcatron pushed a commit to alcatron/plugin_weathermap that referenced this pull request Oct 9, 2026
@TheWitness
TheWitness requested a balanced review from Copilot October 9, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The fit-mode regression test can pass even when the fitted image is never rendered.

0 open findings

5 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Assert fitted image pixels are rendered, not just padding

tests/​Support/​BackgroundSizingEngineRegression.php:85

The fit-mode assertion only proves that the letterbox pixel is not red. A regression that never copies the fitted image would still pass, because the canvas background also satisfies this condition. Please also assert that a pixel inside the expected fitted area is red so the test verifies the image was rendered, not merely that padding exists.

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation preserves compatibility, validates inputs, and includes focused end-to-end regression coverage.

0 open findings

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Background sizing is read before post-processors, so their hint updates cannot affect rendering.

1 open finding

🧠 Review effort: Balanced

Comment thread lib/WeatherMap.class.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation matches the stated behavior and includes focused regression coverage for all modes and fallbacks.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The regression fixture defines a Cacti core helper locally instead of using the shared configurable test bootstrap.

1 open finding

🧠 Review effort: Balanced

Comment thread tests/Support/BackgroundSizingEngineRegression.php Outdated
@TheWitness
TheWitness merged commit 4f5fcbd into Cacti:develop Oct 10, 2026
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