mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-14 02:56:36 +00:00
fix: address CodeRabbit findings on the style-editor/font-selector PR
- Fix _load_font_sized double-wrapping the (font, size) tuple on the missing-font path, which handed callers a tuple instead of a font. - Fix _set_nested_value skipping an explicit None when the key already existed, which silently kept stale overrides when a user cleared a nullable per-mode field or blanked all channels of an indexed color. - Preserve BDF scalable/native_size metadata through fetchFontCatalog's catalog-format mapping so maxFixedSize filtering actually applies. - Stop caching an empty array on a failed font-catalog fetch so a later call can retry instead of being stuck with the failed result. - Keep a saved font selected in the style editor even when it no longer fits a newly declared maxFixedSize, instead of silently deselecting it. - Don't drop in-progress user edits to fallback fields when a plugin widget finishes loading asynchronously and takes over the form. - Tighten the removed font-override endpoint test to assert 405, not just != 200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -244,7 +244,7 @@ def _load_font_sized(font_name: str, size: int) -> Tuple[Any, int]:
|
|||||||
path = resolve_font_path(font_name)
|
path = resolve_font_path(font_name)
|
||||||
if path is None:
|
if path is None:
|
||||||
logger.warning("Font file not found: %s, using fallback", font_name)
|
logger.warning("Font file not found: %s, using fallback", font_name)
|
||||||
return _load_fallback_font(size), size
|
return _load_fallback_font(size)
|
||||||
|
|
||||||
cache_key = (path, size)
|
cache_key = (path, size)
|
||||||
cached = _font_cache.get(cache_key)
|
cached = _font_cache.get(cache_key)
|
||||||
|
|||||||
@@ -359,6 +359,19 @@ class TestDegradation:
|
|||||||
# The override IS honored as forced, but the face degrades safely.
|
# The override IS honored as forced, but the face degrades safely.
|
||||||
assert style.user_forced
|
assert style.user_forced
|
||||||
assert style.font is not None
|
assert style.font is not None
|
||||||
|
assert not isinstance(style.font, tuple)
|
||||||
|
|
||||||
|
def test_missing_font_load_returns_a_font_not_a_nested_tuple(self):
|
||||||
|
# Regression: the missing-font path in _load_font_sized used to wrap
|
||||||
|
# _load_fallback_font's own (font, size) tuple a second time, so
|
||||||
|
# load_font() returned a (font, size) tuple where callers (draw.text,
|
||||||
|
# layout math) expect a font object.
|
||||||
|
font, size = _load_font_sized("definitely-not-a-real-font.ttf", 8)
|
||||||
|
assert not isinstance(font, tuple)
|
||||||
|
assert size == 8
|
||||||
|
|
||||||
|
single = load_font("definitely-not-a-real-font.ttf", 8)
|
||||||
|
assert not isinstance(single, tuple)
|
||||||
|
|
||||||
def test_empty_defaults_treats_config_as_reference_to_classic(self):
|
def test_empty_defaults_treats_config_as_reference_to_classic(self):
|
||||||
# No schema defaults at all: a config value equal to the classic
|
# No schema defaults at all: a config value equal to the classic
|
||||||
|
|||||||
@@ -825,10 +825,10 @@ class TestFontsAPI:
|
|||||||
# 405, not 404: the path still matches DELETE /fonts/<font_family>,
|
# 405, not 404: the path still matches DELETE /fonts/<font_family>,
|
||||||
# which now reads "overrides" as a font name. Nothing is routed to a
|
# which now reads "overrides" as a font name. Nothing is routed to a
|
||||||
# handler for GET or POST, which is what matters here.
|
# handler for GET or POST, which is what matters here.
|
||||||
assert client.get('/api/v3/fonts/overrides').status_code != 200
|
assert client.get('/api/v3/fonts/overrides').status_code == 405
|
||||||
assert client.post('/api/v3/fonts/overrides',
|
assert client.post('/api/v3/fonts/overrides',
|
||||||
data=json.dumps({}),
|
data=json.dumps({}),
|
||||||
content_type='application/json').status_code != 200
|
content_type='application/json').status_code == 405
|
||||||
|
|
||||||
|
|
||||||
class TestAPIErrorHandling:
|
class TestAPIErrorHandling:
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ from web_interface.blueprints.api_v3 import ( # noqa: E402
|
|||||||
_parse_form_value,
|
_parse_form_value,
|
||||||
_get_schema_property,
|
_get_schema_property,
|
||||||
_set_nested_value,
|
_set_nested_value,
|
||||||
|
_SKIP_FIELD,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -221,12 +222,21 @@ class TestSetNestedValue:
|
|||||||
_set_nested_value(config, "fifa.world.enabled", True)
|
_set_nested_value(config, "fifa.world.enabled", True)
|
||||||
assert config == {"fifa.world": {"enabled": True}}
|
assert config == {"fifa.world": {"enabled": True}}
|
||||||
|
|
||||||
def test_none_does_not_overwrite_existing(self):
|
def test_none_overwrites_existing(self):
|
||||||
|
# Regression: None is a real value here (the per-mode "inherit the
|
||||||
|
# base" override for a nullable field, or a blank indexed color
|
||||||
|
# channel) and must replace whatever was already stored. Only the
|
||||||
|
# _SKIP_FIELD sentinel means "leave it alone" -- see the next test.
|
||||||
config = {"a": 1}
|
config = {"a": 1}
|
||||||
_set_nested_value(config, "a", None)
|
_set_nested_value(config, "a", None)
|
||||||
assert config == {"a": 1}
|
assert config == {"a": None}
|
||||||
|
|
||||||
def test_none_sets_missing_key(self):
|
def test_none_sets_missing_key(self):
|
||||||
config = {}
|
config = {}
|
||||||
_set_nested_value(config, "a", None)
|
_set_nested_value(config, "a", None)
|
||||||
assert config == {"a": None}
|
assert config == {"a": None}
|
||||||
|
|
||||||
|
def test_skip_field_does_not_overwrite_existing(self):
|
||||||
|
config = {"a": 1}
|
||||||
|
_set_nested_value(config, "a", _SKIP_FIELD)
|
||||||
|
assert config == {"a": 1}
|
||||||
|
|||||||
@@ -1026,8 +1026,10 @@ def _set_nested_value(config, key_path, value):
|
|||||||
current[seg] = {}
|
current[seg] = {}
|
||||||
current = current[seg]
|
current = current[seg]
|
||||||
|
|
||||||
# Set the final value (don't overwrite with empty dict if value is None and we want to preserve structure)
|
# Set the final value. _SKIP_FIELD (checked above) is the only sentinel
|
||||||
if value is not None or segments[-1] not in current:
|
# for "leave the existing value alone" -- an explicit None here is a real
|
||||||
|
# value (e.g. the per-mode "inherit the base" override) and must overwrite
|
||||||
|
# whatever was already stored.
|
||||||
current[segments[-1]] = value
|
current[segments[-1]] = value
|
||||||
def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', config_node=None):
|
def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', config_node=None):
|
||||||
"""Walk schema and set missing boolean form fields to False.
|
"""Walk schema and set missing boolean form fields to False.
|
||||||
|
|||||||
@@ -121,7 +121,9 @@
|
|||||||
family: family,
|
family: family,
|
||||||
display_name: info.display_name || generateDisplayName(info.filename || family),
|
display_name: info.display_name || generateDisplayName(info.filename || family),
|
||||||
path: info.path,
|
path: info.path,
|
||||||
type: info.type || 'unknown'
|
type: info.type || 'unknown',
|
||||||
|
scalable: info.scalable,
|
||||||
|
native_size: info.native_size
|
||||||
}));
|
}));
|
||||||
} else if (Array.isArray(data)) {
|
} else if (Array.isArray(data)) {
|
||||||
// Direct array format
|
// Direct array format
|
||||||
|
|||||||
@@ -44,7 +44,10 @@
|
|||||||
if (FONT_CACHE) { return Promise.resolve(FONT_CACHE); }
|
if (FONT_CACHE) { return Promise.resolve(FONT_CACHE); }
|
||||||
if (FONT_INFLIGHT) { return FONT_INFLIGHT; }
|
if (FONT_INFLIGHT) { return FONT_INFLIGHT; }
|
||||||
FONT_INFLIGHT = fetch('/api/v3/fonts/catalog')
|
FONT_INFLIGHT = fetch('/api/v3/fonts/catalog')
|
||||||
.then(function (r) { return r.json(); })
|
.then(function (r) {
|
||||||
|
if (!r.ok) { throw new Error('font catalog fetch failed: ' + r.status); }
|
||||||
|
return r.json();
|
||||||
|
})
|
||||||
.then(function (payload) {
|
.then(function (payload) {
|
||||||
var catalog = (payload && payload.data && payload.data.catalog) || {};
|
var catalog = (payload && payload.data && payload.data.catalog) || {};
|
||||||
FONT_CACHE = Object.keys(catalog).map(function (key) {
|
FONT_CACHE = Object.keys(catalog).map(function (key) {
|
||||||
@@ -60,8 +63,10 @@
|
|||||||
})
|
})
|
||||||
.catch(function (e) {
|
.catch(function (e) {
|
||||||
console.warn('[StyleEditor] could not load the font catalog', e);
|
console.warn('[StyleEditor] could not load the font catalog', e);
|
||||||
FONT_CACHE = [];
|
// Leave FONT_CACHE unset and clear FONT_INFLIGHT so the next
|
||||||
return FONT_CACHE;
|
// call retries instead of being stuck on an empty result.
|
||||||
|
FONT_INFLIGHT = null;
|
||||||
|
return [];
|
||||||
});
|
});
|
||||||
return FONT_INFLIGHT;
|
return FONT_INFLIGHT;
|
||||||
}
|
}
|
||||||
@@ -138,9 +143,13 @@
|
|||||||
* rather than a list curated by hand, which is what these fields used
|
* rather than a list curated by hand, which is what these fields used
|
||||||
* to carry and why an uploaded font could never appear in one.
|
* to carry and why an uploaded font could never appear in one.
|
||||||
*/
|
*/
|
||||||
function usableFonts(fonts, maxFixedSize) {
|
function usableFonts(fonts, maxFixedSize, current) {
|
||||||
if (!maxFixedSize) { return fonts; }
|
if (!maxFixedSize) { return fonts; }
|
||||||
return fonts.filter(function (f) {
|
return fonts.filter(function (f) {
|
||||||
|
// Keep the font already saved on this element even if it no
|
||||||
|
// longer fits the cap -- dropping it would leave the select
|
||||||
|
// with nothing chosen and silently misrepresent the config.
|
||||||
|
if (current && f.filename === current) { return true; }
|
||||||
if (f.scalable !== false) { return true; }
|
if (f.scalable !== false) { return true; }
|
||||||
// An unknown native size is not evidence it is too big.
|
// An unknown native size is not evidence it is too big.
|
||||||
if (!f.nativeSize) { return true; }
|
if (!f.nativeSize) { return true; }
|
||||||
@@ -358,7 +367,7 @@
|
|||||||
var xOptions = prop['x-options'] || prop['x_options'] || {};
|
var xOptions = prop['x-options'] || prop['x_options'] || {};
|
||||||
var cap = Number(xOptions.maxFixedSize) || opts.maxFixedSize || null;
|
var cap = Number(xOptions.maxFixedSize) || opts.maxFixedSize || null;
|
||||||
return fontControl(opts.name, opts.current,
|
return fontControl(opts.name, opts.current,
|
||||||
usableFonts(opts.fonts, cap),
|
usableFonts(opts.fonts, cap, opts.current),
|
||||||
opts.optional, opts.onFontChange);
|
opts.optional, opts.onFontChange);
|
||||||
}
|
}
|
||||||
if (types.indexOf('boolean') !== -1) {
|
if (types.indexOf('boolean') !== -1) {
|
||||||
|
|||||||
@@ -88,6 +88,23 @@
|
|||||||
</div>
|
</div>
|
||||||
<script>
|
<script>
|
||||||
(function() {
|
(function() {
|
||||||
|
function fallbackIsDirty(root) {
|
||||||
|
if (!root) { return false; }
|
||||||
|
var controls = root.querySelectorAll('input, select, textarea');
|
||||||
|
for (var i = 0; i < controls.length; i++) {
|
||||||
|
var c = controls[i];
|
||||||
|
if (c.type === 'checkbox' || c.type === 'radio') {
|
||||||
|
if (c.checked !== c.defaultChecked) { return true; }
|
||||||
|
} else if (c.tagName === 'SELECT') {
|
||||||
|
for (var j = 0; j < c.options.length; j++) {
|
||||||
|
if (c.options[j].selected !== c.options[j].defaultSelected) { return true; }
|
||||||
|
}
|
||||||
|
} else if (c.value !== c.defaultValue) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return false;
|
||||||
|
}
|
||||||
function initWidget() {
|
function initWidget() {
|
||||||
if (!window.LEDMatrixWidgets) { setTimeout(initWidget, 50); return; }
|
if (!window.LEDMatrixWidgets) { setTimeout(initWidget, 50); return; }
|
||||||
var widget = window.LEDMatrixWidgets.get('style-editor');
|
var widget = window.LEDMatrixWidgets.get('style-editor');
|
||||||
@@ -95,6 +112,11 @@
|
|||||||
var container = document.getElementById('{{ field_id }}_container');
|
var container = document.getElementById('{{ field_id }}_container');
|
||||||
var fallback = document.getElementById('{{ field_id }}_fallback');
|
var fallback = document.getElementById('{{ field_id }}_fallback');
|
||||||
if (!container) { return; }
|
if (!container) { return; }
|
||||||
|
// A user may already be editing the plain fallback fields
|
||||||
|
// while this widget's script is still loading; swapping
|
||||||
|
// in the widget now would replace them with the stale
|
||||||
|
// server-rendered value and silently drop that edit.
|
||||||
|
if (fallbackIsDirty(fallback)) { container.remove(); return; }
|
||||||
var value = {{ obj_value|tojson|safe }};
|
var value = {{ obj_value|tojson|safe }};
|
||||||
var config = { schema: {{ prop|tojson|safe }} };
|
var config = { schema: {{ prop|tojson|safe }} };
|
||||||
try {
|
try {
|
||||||
@@ -944,7 +966,10 @@
|
|||||||
if (!available) { container.remove(); return; }
|
if (!available) { container.remove(); return; }
|
||||||
var widget = window.LEDMatrixWidgets.get(WIDGET);
|
var widget = window.LEDMatrixWidgets.get(WIDGET);
|
||||||
if (!widget || typeof widget.render !== 'function') { container.remove(); return; }
|
if (!widget || typeof widget.render !== 'function') { container.remove(); return; }
|
||||||
var value = {{ str_value|tojson|safe }};
|
// Read the fallback's live value rather than the
|
||||||
|
// server-rendered one, so an edit typed while the
|
||||||
|
// widget script was still loading isn't lost.
|
||||||
|
var value = fallback ? fallback.value : {{ str_value|tojson|safe }};
|
||||||
var config = {
|
var config = {
|
||||||
'type': '{{ field_type }}',
|
'type': '{{ field_type }}',
|
||||||
'enum': {{ (prop.enum or [])|tojson|safe }},
|
'enum': {{ (prop.enum or [])|tojson|safe }},
|
||||||
|
|||||||
Reference in New Issue
Block a user