From 022d43a5a542932901abded65df9bad241fbc782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Kucharczyk?= Date: Sat, 13 Jun 2026 15:12:52 +0200 Subject: [PATCH] Make component return types honest; drop str/mark_safe leftovers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup of hacky leftovers from the node-tree migration (no behaviour change): - Return annotations: the component builders return Node subtrees, not SafeText strings, but ~40 functions still declared `-> SafeText`. Correct them to `-> Node` across filters / search_select / date_range_picker / domain. The genuine string returners keep `-> SafeText`: the Alpine selectors (GameStatusSelector / SessionDeviceSelector, which build f-string markup) and the script-tag helpers (CsrfInput / ModuleScript / ExternalScript / StaticScript). - layout.render_page / layout.Page / AddForm now accept `Node` in their `content` / `scripts` / `fields` parameters (TYPE_CHECKING import in layout to avoid the components import cycle), matching what views already pass. - session._session_fields builds a `Fragment(*rows, separator="\n")` instead of `mark_safe("\n".join(str(row) ...))` — keeps the tree intact so media could bubble, per the Fragment convention. - Inline SVG icon children use `Safe(...)` nodes instead of `mark_safe(...)` strings (filters mode-toggle + collapse icons, date_range_picker calendar icon). - _filter_field reads the widget's own id from its node `.attributes` (`_widget_id`) for the label's `for`, dropping the superfluous `for_widget` argument that always rendered `for="None"`. Removes the two TODOs whose premise ("the Component function can't expose the id") the class/node refactor retired, plus RangeSlider's dead commented-out Label block. Co-Authored-By: Claude Opus 4.8 --- common/components/date_range_picker.py | 19 ++++--- common/components/domain.py | 14 ++--- common/components/filters.py | 73 ++++++++++++++------------ common/components/primitives.py | 4 +- common/components/search_select.py | 37 +++++++------ common/layout.py | 12 +++-- games/views/session.py | 7 +-- 7 files changed, 86 insertions(+), 80 deletions(-) diff --git a/common/components/date_range_picker.py b/common/components/date_range_picker.py index becc709..02b611d 100644 --- a/common/components/date_range_picker.py +++ b/common/components/date_range_picker.py @@ -17,9 +17,8 @@ widget into a ``DateCriterion`` unchanged. All behaviour is wired by ``games/static/js/date_range_picker.js``. """ -from django.utils.safestring import SafeText, mark_safe -from common.components.core import Element, HTMLAttribute, Media, Node +from common.components.core import Element, HTMLAttribute, Media, Node, Safe from common.components.primitives import Div, Input, Span from common.time import DatePartSpec, date_parts @@ -104,7 +103,7 @@ def _iso_part_values(iso_value: str, parts: list[DatePartSpec]) -> dict[str, str def _segment_input( *, part: DatePartSpec, side: str, label: str, value: str -) -> SafeText: +) -> Node: side_label = "from" if side == "min" else "to" return Input( attributes=[ @@ -125,11 +124,11 @@ def _segment_input( ) -def _segment_group(*, side: str, label: str, iso_value: str) -> SafeText: +def _segment_group(*, side: str, label: str, iso_value: str) -> Node: """One date's worth of segments (``DD - MM - YYYY``) for a range side.""" parts = date_parts() initial_values = _iso_part_values(iso_value, parts) - children: list[SafeText] = [] + children: list[Node] = [] for index, part in enumerate(parts): if index > 0: children.append( @@ -161,7 +160,7 @@ def DateRangeField( input_name_prefix: str, min_value: str = "", max_value: str = "", -) -> SafeText: +) -> Node: """The visible half of the DateRangePicker: a single-input-looking container holding two segmented dates, a calendar toggle, and the two hidden ISO inputs (``{prefix}-min`` / ``{prefix}-max``) that carry the @@ -210,13 +209,13 @@ def DateRangeField( "cursor-pointer shrink-0", ), ], - children=[mark_safe(_CALENDAR_ICON_SVG)], + children=[Safe(_CALENDAR_ICON_SVG)], ), ], ) -def _calendar_nav_button(direction: str, arrow: str, label: str) -> SafeText: +def _calendar_nav_button(direction: str, arrow: str, label: str) -> Node: return Element( "button", attributes=[ @@ -229,7 +228,7 @@ def _calendar_nav_button(direction: str, arrow: str, label: str) -> SafeText: ) -def _footer_button(action: str, label: str, button_class: str) -> SafeText: +def _footer_button(action: str, label: str, button_class: str) -> Node: return Element( "button", attributes=[ @@ -241,7 +240,7 @@ def _footer_button(action: str, label: str, button_class: str) -> SafeText: ) -def DateRangeCalendar(*, input_name_prefix: str) -> SafeText: +def DateRangeCalendar(*, input_name_prefix: str) -> Node: """The popup half of the DateRangePicker: preset column, month grid (filled client-side into ``[data-date-range-grid]``), and the Cancel / Clear / Select footer. Hidden until the calendar toggle opens it.""" diff --git a/common/components/domain.py b/common/components/domain.py index 977946c..b5b0bd3 100644 --- a/common/components/domain.py +++ b/common/components/domain.py @@ -6,7 +6,7 @@ from django.template.defaultfilters import floatformat from django.urls import reverse from django.utils.safestring import SafeText, mark_safe -from common.components.core import HTMLTag +from common.components.core import HTMLTag, Node from common.components.primitives import ( A, Div, @@ -22,7 +22,7 @@ def GameLink( game_id: int, name: str = "", children: list[HTMLTag] | HTMLTag | None = None, -) -> SafeText: +) -> Node: """Link to a game's detail page. Uses children (slot) if provided, otherwise name.""" from django.urls import reverse @@ -58,7 +58,7 @@ def GameStatus( status: str = "u", display: str = "", class_: str = "", -) -> SafeText: +) -> Node: """Colored status dot with label. Status codes: u/p/f/a/r.""" children = children or [] outer_class = ( @@ -82,7 +82,7 @@ def GameStatus( def PriceConverted( children: list[HTMLTag] | HTMLTag | None = None, -) -> SafeText: +) -> Node: """Wrap content in a span that indicates the price was converted.""" children = children or [] return Span( @@ -94,7 +94,7 @@ def PriceConverted( ) -def LinkedPurchase(purchase: Purchase) -> SafeText: +def LinkedPurchase(purchase: Purchase) -> Node: link = reverse("games:view_purchase", args=[int(purchase.id)]) link_content = "" popover_content = "" @@ -145,7 +145,7 @@ def NameWithIcon( session: Session | None = None, linkify: bool = True, emulated: bool = False, -) -> SafeText: +) -> Node: _name, platform, final_emulated, create_link, link = _resolve_name_with_icon( name, game, session, linkify ) @@ -203,7 +203,7 @@ def _resolve_name_with_icon( return _name, platform, final_emulated, create_link, link -def PurchasePrice(purchase) -> SafeText: +def PurchasePrice(purchase) -> Node: return Popover( popover_content=f"{floatformat(purchase.price)} {purchase.price_currency}", wrapped_content=f"{floatformat(purchase.converted_price)} {purchase.converted_currency}", diff --git a/common/components/filters.py b/common/components/filters.py index e38c060..1d158f1 100644 --- a/common/components/filters.py +++ b/common/components/filters.py @@ -3,9 +3,8 @@ from typing import NamedTuple from django.db import models -from django.utils.safestring import SafeText, mark_safe -from common.components.core import BaseComponent, Element, Media, Node +from common.components.core import BaseComponent, Element, Media, Node, Safe from common.components.date_range_picker import DateRangePicker from common.components.primitives import Checkbox, Div, Input, Label, Radio, Span from common.components.search_select import ( @@ -176,7 +175,7 @@ def _split_modifier(modifier: str, has_m2m: bool = False) -> str: def _enum_filter( field_name: str, options, choice: FilterChoice, *, nullable -) -> SafeText: +) -> Node: """A FilterSelect over a small, fully pre-rendered option set (enum field). Enum fields are single-valued, so no M2M modifiers (all/only are @@ -207,7 +206,7 @@ def _model_filter( search_url, nullable, m2m_modifiers: list[LabeledOption] | None = None, -) -> SafeText: +) -> Node: """A FilterSelect backed by a search endpoint. Labels are embedded in the filter JSON (Stash-style), so pills render @@ -240,34 +239,43 @@ def _filter_mins_to_hrs(val) -> str: return str(int(hrs)) if hrs == int(hrs) else f"{hrs:.1f}" -def _filter_field(label: str, widget, for_widget: str = None) -> SafeText: - """A labelled filter field:
{widget}
. - TODO: Use widget.attributes.get("id", "") to get the widget's ID - instead of the superfluous "for" argument. This requires refactoring - the Component function to be a class intead. - Also see RangeSlider's TODO +def _widget_id(widget) -> str: + """Best-effort id of a widget node, for the field label's ``for`` target. + + Widgets are nodes carrying ``.attributes``, so the id is now reachable + directly (the old free ``Component`` string couldn't expose it). """ + for name, value in getattr(widget, "attributes", []): + if name == "id": + return str(value) + return "" + + +def _filter_field(label: str, widget) -> Node: + """A labelled filter field: ``
{widget}
``. + + The label's ``for`` points at the widget's own id when it has one; + composite widgets without a single root id simply omit ``for``. + """ + label_attributes = [("class", _FILTER_LABEL_CLASS)] + widget_id = _widget_id(widget) + if widget_id: + label_attributes.append(("for", widget_id)) return Div( attributes=[("class", "flex flex-col gap-1")], children=[ - Label( - attributes=[ - ("class", _FILTER_LABEL_CLASS), - ("for", for_widget), - ], - children=[label], - ), + Label(attributes=label_attributes, children=[label]), widget, ], ) -def _filter_checkbox(name: str, label: str, checked: bool) -> SafeText: +def _filter_checkbox(name: str, label: str, checked: bool) -> Node: """Thin adapter mapping legacy checkbox filters to the generalized Checkbox primitive.""" return Checkbox(name=name, label=label, checked=checked) -def _filter_boolean_radio(name: str, label: str, value: bool | None) -> SafeText: +def _filter_boolean_radio(name: str, label: str, value: bool | None) -> Node: """Renders a filter-specific boolean radio button group with 'True' and 'False' options.""" return Div( attributes=[("class", "flex flex-col gap-1")], @@ -321,7 +329,7 @@ def RangeSlider( step: str = "1", min_placeholder: str = "", max_placeholder: str = "", -) -> SafeText: +) -> Node: """A labelled range slider with number inputs and range/point mode toggle. Renders a label row (label, two number inputs, toggle button) and a slider @@ -341,14 +349,9 @@ def RangeSlider( Div( attributes=[("class", "flex items-center gap-2 mb-1")], children=[ - # TODO: This should be done outside the RangeSlider component, but the current Component function doesn't allow getting the id - # Label( - # attributes=[ - # ("class", _FILTER_LABEL_CLASS), - # ("for", min_input_id), - # ], - # children=[label], - # ), + # The field label is rendered by the _filter_field wrapper. + # This composite widget has no single labelable root, so the + # label carries no `for` (the two inputs are named below). Input( attributes=[ ("type", "number"), @@ -410,7 +413,7 @@ def RangeSlider( + (" hidden" if point_mode else ""), ), ], - children=[mark_safe(_RANGE_ICON_SVG)], + children=[Safe(_RANGE_ICON_SVG)], ), Span( attributes=[ @@ -420,7 +423,7 @@ def RangeSlider( + ("" if point_mode else " hidden"), ), ], - children=[mark_safe(_POINT_ICON_SVG)], + children=[Safe(_POINT_ICON_SVG)], ), ], ), @@ -506,7 +509,7 @@ def DateRangeFilter( max_value: str = "", min_placeholder: str = "From", max_placeholder: str = "To", -) -> SafeText: +) -> Node: """A pair of ```` elements representing a date range. Mirrors ``RangeSlider`` in shape (two inputs named ``{prefix}-min`` and @@ -561,7 +564,7 @@ _FILTER_FORM_ID = "filter-bar-form" _FILTER_INPUT_ID = "filter-json-input" -def _filter_collapse_button() -> SafeText: +def _filter_collapse_button() -> Node: return Element( "button", attributes=[ @@ -579,7 +582,7 @@ def _filter_collapse_button() -> SafeText: ), ], children=[ - mark_safe( + Safe( '' ), "Filters", @@ -587,7 +590,7 @@ def _filter_collapse_button() -> SafeText: ) -def _filter_action_row(preset_list_url: str, preset_save_url: str) -> SafeText: +def _filter_action_row(preset_list_url: str, preset_save_url: str) -> Node: return Div( attributes=[("class", "flex gap-3 items-center")], children=[ @@ -1529,7 +1532,7 @@ def StringFilter( value: str = "", modifier: str = "EQUALS", placeholder: str = "", -) -> SafeText: +) -> Node: """Renders a string filter with 8 modifier radio options and a text input.""" from common.criteria import Modifier diff --git a/common/components/primitives.py b/common/components/primitives.py index 407dda2..90de14c 100644 --- a/common/components/primitives.py +++ b/common/components/primitives.py @@ -622,8 +622,8 @@ def AddForm( form, *, request, - fields: SafeText | str | None = None, - additional_row: SafeText | str = "", + fields: Node | SafeText | str | None = None, + additional_row: Node | SafeText | str = "", submit_class: str = "mt-3", ) -> Node: """Page body for the generic add/edit form (Python equivalent of add.html). diff --git a/common/components/search_select.py b/common/components/search_select.py index 1e2f728..054f472 100644 --- a/common/components/search_select.py +++ b/common/components/search_select.py @@ -21,7 +21,6 @@ user types. from collections.abc import Callable, Iterable from typing import TypedDict -from django.utils.safestring import SafeText from common.components.core import Element, HTMLAttribute, Media, Node from common.components.primitives import Div, Input, Pill, Span, Template @@ -144,11 +143,11 @@ def _data_attributes(data: dict[str, str]) -> list[HTMLAttribute]: return [(f"data-{key}", str(value)) for key, value in data.items()] -def _hidden_input(name: str, value) -> SafeText: +def _hidden_input(name: str, value) -> Node: return Input(type="hidden", attributes=[("name", name), ("value", str(value))]) -def _label_slot(text: str, *, extra_class: str = "") -> SafeText: +def _label_slot(text: str, *, extra_class: str = "") -> Node: """A ```` holding a row/pill's visible label. JS fills this one node when cloning the shape from a ``