Skip to content

Generate sharper gallery thumbnails by default - #292

Merged
TheWitness merged 7 commits into
Cacti:developfrom
alcatron:feat/sharp-thumbnail-default
Oct 10, 2026
Merged

TheWitness merged 7 commits into
Cacti:developfrom
alcatron:feat/sharp-thumbnail-default

Conversation

@alcatron

@alcatron alcatron commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Large gallery cards can upscale the old 250-pixel thumbnails. Increase the default to 1000 pixels and initialize absent, zero or negative settings to that value. Existing positive preferences remain unchanged; the existing poller regenerates previews.

Validation: 227 PHP 8.3 tests passed in an isolated fixture layout. Tests execute weathermap_setup_table() for absent, zero, negative and positive values, alongside the settings-default check. Syntax, whitespace and manifest checks passed. Changed-line coverage passed at 100% of measured lines; schema initialization retains its existing coverage exemption. Higher resolution increases image size and rendering cost. Full running-installation integration remains unverified for this revision.

Latest review follow-up

Moved invalid thumbnail-size normalization outside the schema/version guard. Tests now cover fresh and current-version installations with existing tables, including unchanged positive values and no schema queries on the current-version path. Latest local validation: 227 PHP tests (5429 assertions); measured changed-line coverage 1/1. includes/database.php is outside the configured coverage source set, so its regression is verified through direct execution rather than that gate.

These follow-up checks use isolated local fixtures, not a running Cacti installation. PHP 8.3 syntax, whitespace, manifest and translation-template checks passed. GitHub CI must still confirm the revised commits.

@TheWitness
TheWitness requested review from TheWitness and a balanced review from Copilot and removed request for Copilot October 9, 2026 15:11

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 test does not exercise the changed absent and non-positive initialization behavior.

2 open findings
What changed in this PR

Raises the default gallery thumbnail size from 250 to 1000 pixels for sharper previews while preserving positive configured values.

Changes:

  • Updates the registered and initialization defaults to 1000 pixels.
  • Adds a regression test and changelog entry.
File Description
setup.php Registers the new default.
includes/​database.php Initializes absent or non-positive values to 1000.
tests/​Unit/​ThumbnailDefaultTest.php Tests metadata and preservation of 250.
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 tests/Unit/ThumbnailDefaultTest.php
Comment thread tests/Unit/ThumbnailDefaultTest.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:08

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

Existing 1.7 installations bypass the guarded initialization, leaving non-positive settings unchanged.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread includes/database.php Outdated
@alcatron
alcatron force-pushed the feat/sharp-thumbnail-default branch from aab9264 to e4b9b9a Compare October 9, 2026 23:41

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 implementation appears sound, but no GitHub checks are present and running-installation integration remains unverified.

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.

🟢 Approval recommended

The implementation matches the stated behavior and covers fresh, current-version, invalid, and preserved-value scenarios.

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.

🟢 Approval recommended

The implementation matches the stated behavior and covers fresh, current-version, invalid, and preserved-value paths.

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

The new default unnecessarily upscales source maps smaller than 1000 pixels, increasing rendering and storage costs without improving detail.

1 open finding

🧠 Review effort: Balanced

Comment thread setup.php
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