Skip to content

Use a readable landscape preset for new blank maps - #295

Merged
TheWitness merged 11 commits into
Cacti:developfrom
alcatron:feat/new-map-preset
Oct 11, 2026
Merged

TheWitness merged 11 commits into
Cacti:developfrom
alcatron:feat/new-map-preset

Conversation

@alcatron

@alcatron alcatron commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

New blank maps use a 1400x750 landscape canvas, Dynamic HTML, compact arrows, Vera Sans 9 pt traffic/comment fonts and blue comments. Apply the preset only to blank maps; copies retain their source settings.

Save portable bundled font paths and resolve them against the plugin directory during parsing, measurement and rendering, including CLI readers running from the Cacti root. Missing font files or unavailable FreeType retain built-in font defaults. The Fit background hint takes effect when the separate background-sizing renderer support is merged.

Validation: 229 PHP 8.3 tests passed in an isolated fixture layout. Real-engine tests cover preset persistence, management creation, unchanged copy settings, and font reading/rendering/rewriting from the Cacti root. Separate PHP processes exercise the actual fallback with imagettfbbox disabled and with the bundled file unavailable; no installed files are removed. Focused PHPStan level 8 passed for the preset helper. Syntax, whitespace and manifest checks passed; measured changed-line coverage is 100%, with existing entry/action exemptions. Full running-installation integration remains unverified for this revision.

Further review follow-up

Addressed in 24f3753. Require both imagettfbbox() and imagettftext() when choosing the new preset font and when loading persisted FONTDEFINE entries; updated the warning accordingly. Added real PHP subprocess tests with each function independently disabled, including rendering a saved map with TrueType references using the fallback. The subprocess execution is included in Xdebug coverage when enabled. Also corrected the two CI failures caused by working-directory assumptions: font readability uses wm_font_file(), and fallback engine fixtures enter the plugin directory before construction. The full suite passes from the Cacti root: 232 tests, 5452 assertions. Measured changed-line coverage passes at 28/28. The next GitHub run must confirm the PHP matrix.

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

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 serialized font path fails from CLI working directories, fallback coverage is missing, and new-file headers violate repository requirements.

6 open findings
What changed in this PR

Introduces readable landscape defaults for newly created blank maps while preserving copied and existing maps.

Changes:

  • Adds canvas, HTML, arrow, comment, font, and background-sizing defaults.
  • Applies the preset through management and editor creation paths.
  • Adds persistence and copy-regression tests.
File Description
CHANGELOG.md Records the feature.
lib/​editor.new-map-preset.php Defines the new-map preset.
lib/​editor.actions.php Applies it in editor creation.
weathermap-cacti-plugin-mgmt.php Applies it in management creation.
tests/​Unit/​NewMapPresetDefaultsTest.php Tests preset defaults.
tests/​Unit/​NewMapPresetEngineTest.php Runs engine regression coverage.
tests/​Support/​NewMapPresetEngineRegression.php Tests persistence and copying.

🧠 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/editor.new-map-preset.php Outdated
Comment thread lib/editor.new-map-preset.php
Comment thread lib/editor.new-map-preset.php
Comment thread tests/Support/NewMapPresetEngineRegression.php
Comment thread tests/Unit/NewMapPresetDefaultsTest.php
Comment thread tests/Unit/NewMapPresetEngineTest.php
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.

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 is scoped correctly and has comprehensive regression coverage for creation, copying, persistence, rendering, and fallback behavior.

0 open findings

2 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

Independently disabling imagettftext() can still select a TrueType font and cause rendering to fail instead of falling back.

2 open findings

🧠 Review effort: Balanced

Comment thread lib/WeatherMap.class.php
Comment thread lib/editor.new-map-preset.php Outdated
@alcatron

Copy link
Copy Markdown
Contributor Author

Merged latest develop and resolved the changelog/translation metadata conflicts in 4d3b9f7, preserving the feature changes. All 237 local PHP tests pass (5499 assertions); syntax, manifest, whitespace and translation-template checks pass. These remain isolated fixture checks, not a fresh full-installation browser audit.

@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the new changelog conflict after #292 merged. All 240 local tests pass (5524 assertions), with syntax, manifest and whitespace checks passing. The feature changes remain intact; validation uses isolated fixtures.

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 preset is correctly scoped, portable font handling is consistent, and relevant fallback paths have comprehensive regression coverage.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the CHANGELOG.md conflict after #293 merged, preserving both entries. The feature code merged cleanly; all 241 local tests pass, plus manifest and whitespace checks. Validation uses isolated fixtures.

@alcatron

Copy link
Copy Markdown
Contributor Author

Updated again after #294 merged during the previous push. Resolved the changelog/translation metadata conflicts; code merged cleanly. All 266 local tests pass on this combined revision. Manifest and whitespace checks pass; validation uses isolated fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants