From 5022e2819edcbef05e3d7c4ae8fdd1fdd233ff2d Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:46:01 +0200 Subject: [PATCH 1/6] Required plugin settings (M5 part 2a) Plugins can mark a setting as required with paperpi.plugin.setting(..., required=True). A switched-on plugin that is missing one is not shown and not counted as broken; the config check warns and paperpi list shows "needs ". met_no and moon_phase require lat, lon and email (met.no's terms of service ask for a real email address); the example config no longer fills in a made-up address. Part of #238 Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/decisions/plugin-interface.md | 2 + docs/writing-plugins.md | 9 ++++ paperpi.example.toml | 15 ++++--- src/paperpi/cli.py | 11 ++++- src/paperpi/config.py | 23 ++++++++++ src/paperpi/example.py | 13 +++--- src/paperpi/plugin.py | 52 +++++++++++++++++++++- src/paperpi/plugins/met_no/README.md | 6 +-- src/paperpi/plugins/met_no/__init__.py | 16 ++++--- src/paperpi/plugins/moon_phase/README.md | 6 +-- src/paperpi/plugins/moon_phase/__init__.py | 17 +++---- src/paperpi/scheduler.py | 2 +- tests/test_cli.py | 2 + tests/test_config.py | 46 +++++++++++++++++++ tests/test_example.py | 22 +++++++-- tests/test_plugin.py | 51 ++++++++++++++++++++- tests/test_scheduler.py | 14 ++++++ 18 files changed, 267 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 8a003ed..4bfbf5a 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ Run `uv run paperpi render --help` for all options. How to write a plugin: [docs ### A config file -`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. Before you use the weather blocks, put your own email address in `email` (met.no asks for it). `-o` doesn't replace a file that is already there, unless you add `--force`. +`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you put your own, real email address in `email`: met.no's terms of service ask for it, so the example leaves it empty. `paperpi list` shows them as "needs email" until then. `-o` doesn't replace a file that is already there, unless you add `--force`. ```bash uv run paperpi example-config -o paperpi.toml diff --git a/docs/decisions/plugin-interface.md b/docs/decisions/plugin-interface.md index 60f0b73..0ba9457 100644 --- a/docs/decisions/plugin-interface.md +++ b/docs/decisions/plugin-interface.md @@ -50,6 +50,8 @@ The plugin never talks to the screen. Only the scheduler does. For sample images and tests, `fetch` is skipped and the sample data goes straight to `draw`. How to write a plugin: `docs/writing-plugins.md`. +**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place where a setting asks PaperPi for help; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. + ### How a plugin is run - Every update runs in a **new, short-lived process**. When the update is done, the process exits and its memory is given back. A plugin that hangs or crashes is stopped without affecting PaperPi or the other plugins. diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 604964a..2a0ace9 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -80,6 +80,15 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Every setting needs a default. - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. +- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None` or `""` (or an empty `SecretStr`). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface say which settings are missing. The example block marks them "(required)". `setting` is the one place where a setting asks PaperPi for more help; later options, such as a helper in the web interface that looks up latitude and longitude, are added there too. + + ```python + from paperpi.plugin import PluginSettings, setting + + + class Settings(PluginSettings): + lat: float | None = setting(None, required=True, ge=-90, le=90, description="Latitude") + ``` In the config file, the plugin's settings go in its `[[plugin]]` block: diff --git a/paperpi.example.toml b/paperpi.example.toml index 769fcca..b5a0c9a 100644 --- a/paperpi.example.toml +++ b/paperpi.example.toml @@ -4,8 +4,9 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks need your own email address -# in "email": met.no asks every program for a way to contact its user. +# more than once, with a different name. The weather blocks are not shown until you fill +# in your own, real email address in "email": met.no asks every program for a way to +# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. config_version = 1 @@ -97,8 +98,9 @@ lat = 52.52 lon = 13.4 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Berlin" -# Your email address, sent only to met.no (required: met.no asks for it) -email = "you@example.com" +# Your own, real email address, sent only to met.no: their terms of service ask every program for a +# way to contact its user (required) +# email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" # Rain in millimetres or inches. One of: "mm", "inch" @@ -120,8 +122,9 @@ lat = -22.91 lon = -43.17 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Rio" -# Your email address, sent only to met.no (required: met.no asks for it) -email = "you@example.com" +# Your own, real email address, sent only to met.no: their terms of service ask every program for a +# way to contact its user (required) +# email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" # Rain in millimetres or inches. One of: "mm", "inch" diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index eea5693..b8b18ee 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -324,7 +324,7 @@ def load() -> config.Config: print(f"web interface on port {web.port}") try: with screen: - count = sum(1 for p in loaded.plugins if p.entry.enabled and p.plugin.type != "default") + count = sum(1 for p in loaded.plugins if p.shown and p.plugin.type != "default") where = f"images in {out}" if display.type == "virtual" else f"screen {display.type}" print( f"showing {count} plugin{'' if count == 1 else 's'}; {where}; " @@ -389,7 +389,7 @@ def _list(args: argparse.Namespace) -> int: ( row.name, row.type, - "yes" if row.enabled else "no", + _on(row), row.level, f"{row.display_time:g} s", f"{row.refresh:g} s", @@ -411,6 +411,13 @@ def _list(args: argparse.Namespace) -> int: return 0 +def _on(row: config.PluginRow) -> str: + """The "on" column of ``paperpi list``.""" + if not row.enabled: + return "no" + return f"needs {', '.join(row.missing)}" if row.missing else "yes" + + def _example_config(args: argparse.Namespace) -> int: text = example.example_config() if args.output is None: diff --git a/src/paperpi/config.py b/src/paperpi/config.py index d898760..35af6fb 100644 --- a/src/paperpi/config.py +++ b/src/paperpi/config.py @@ -282,6 +282,17 @@ def folder_name(self) -> str: """The name of this plugin's storage folder, made from its name.""" return folder_name(self.entry.name) + @property + def missing(self) -> tuple[str, ...]: + """Required settings that are not set yet (see :func:`paperpi.plugin.setting`). + Until they are, the plugin is not shown.""" + return self.plugin.missing(self.settings) + + @property + def shown(self) -> bool: + """Switched on, and every required setting is set.""" + return self.entry.enabled and not self.missing + @dataclass class Config: @@ -324,6 +335,8 @@ class PluginRow: """As used: the setting, or the plugin's suggestion.""" storage_days: int """As used: the setting, or the plugin's suggestion (0 = files are kept).""" + missing: tuple[str, ...] = () + """Required settings that are not set yet; until they are, the plugin is not shown.""" def plugin_rows(config: Config) -> list[PluginRow]: @@ -341,6 +354,7 @@ def plugin_rows(config: Config) -> list[PluginRow]: layout=p.layout, storage_mb=p.storage_mb, storage_days=p.storage_days, + missing=p.missing, ) for p in config.plugins ] @@ -740,6 +754,15 @@ def failed() -> bool: return None checked = PluginConfig(entry, settings, plugin, self.lines.get(section)) + if entry.enabled and checked.missing: + names = ", ".join(checked.missing) + self.add( + "warning", + f"not shown until these required settings are filled in: {names}", + section, + next((k for k in checked.missing if k in block), None), + where, + ) if entry.level == "rotation" and checked.refresh == entry.display_time: key = "refresh" if "refresh" in block else "display_time" self.add( diff --git a/src/paperpi/example.py b/src/paperpi/example.py index 07dae93..4b7ad26 100644 --- a/src/paperpi/example.py +++ b/src/paperpi/example.py @@ -32,7 +32,7 @@ DisplaySettings, WebSettings, ) -from .plugin import Plugin, PluginEntry +from .plugin import Plugin, PluginEntry, is_required #: The blocks of the example file: name, plugin type and the settings that are set. #: Weather twice, to show that one plugin type can be used more than once. @@ -41,12 +41,12 @@ ( "Weather Berlin", "met_no", - {"lat": 52.52, "lon": 13.40, "place": "Berlin", "email": "you@example.com"}, + {"lat": 52.52, "lon": 13.40, "place": "Berlin"}, ), ( "Weather Rio", "met_no", - {"lat": -22.91, "lon": -43.17, "place": "Rio", "email": "you@example.com"}, + {"lat": -22.91, "lon": -43.17, "place": "Rio"}, ), ) @@ -57,8 +57,9 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks need your own email address -# in "email": met.no asks every program for a way to contact its user. +# more than once, with a different name. The weather blocks are not shown until you fill +# in your own, real email address in "email": met.no asks every program for a way to +# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. """ #: Settings whose default is "empty", but that mean a known value; shown with that value. @@ -181,6 +182,8 @@ def _settings( def _setting(key: str, info: FieldInfo, value: Any, *, comment: bool = False) -> list[str]: """The help line and the ``key = value`` line of one setting.""" help_text = info.description or key + if is_required(info): + help_text += " (required)" choices = _choices(info.annotation) named = all(re.search(rf"\b{re.escape(str(c))}\b", help_text) for c in choices) if choices and not named: diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index 11196ea..f8e2210 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -29,6 +29,7 @@ from epdlib import Layout, ScreenMode from PIL import Image from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic.fields import FieldInfo from . import limits @@ -90,12 +91,47 @@ class Settings(PluginSettings): The same description checks the config file, builds the web interface's forms (M5) and the docs. A plugin may not use the names of the shared settings - (:data:`SHARED_SETTINGS`) for its own. + (:data:`SHARED_SETTINGS`) for its own. Use :func:`setting` instead of ``Field`` for a + setting PaperPi must know more about, such as one the user has to fill in. """ model_config = ConfigDict(extra="ignore", frozen=True) +#: The key under which :func:`setting` keeps PaperPi's own options in a pydantic field. +_OPTIONS = "paperpi" + + +def setting(default: Any = None, *, required: bool = False, **field: Any) -> Any: + """A plugin setting: pydantic's ``Field`` (with the same ``description``, ``ge``, + ``max_length``, ...) plus what PaperPi needs to know about it. This is the one place a + setting asks PaperPi for help; later options (such as a helper that looks up a place's + latitude and longitude in the web interface) are added here too:: + + lat: float | None = setting(None, required=True, description="Latitude") + + ``required``: the plugin can't work until the user fills it in (a place, an email + address, an API key). Its default must be "not set": ``None`` or ``""``. A plugin + with a required setting that is not set is not shown; the config check and the web + interface say which settings it needs. + """ + options = {"required": True} if required else {} + return Field(default, json_schema_extra={_OPTIONS: options} if options else None, **field) + + +def is_required(info: FieldInfo) -> bool: + """True for a setting made with ``setting(required=True)``.""" + extra = info.json_schema_extra + return isinstance(extra, dict) and bool(extra.get(_OPTIONS, {}).get("required")) + + +def is_set(value: Any) -> bool: + """False for "not set": ``None``, ``""`` or an empty secret.""" + if hasattr(value, "get_secret_value"): + value = value.get_secret_value() + return value is not None and value != "" + + class PluginEntry(BaseModel): """The settings every ``[[plugin]]`` block in the config file has. @@ -260,9 +296,12 @@ def __post_init__(self) -> None: if clash: problems.append(f"settings use the names of shared settings: {', '.join(clash)}") try: - self.settings() + defaults = self.settings() except ValueError: problems.append("every setting needs a default") + else: + if self.missing(defaults) != self.required: + problems.append('a required setting\'s default must be "not set" (None or "")') if not self.layouts: problems.append("needs at least one layout") if not limits.SHORTEST_REFRESH <= self.refresh <= limits.LONGEST_SETTING: @@ -281,6 +320,15 @@ def __post_init__(self) -> None: def default_layout(self) -> str: return next(iter(self.layouts)) + @property + def required(self) -> tuple[str, ...]: + """The settings the user has to fill in (see :func:`setting`).""" + return tuple(k for k, info in self.settings.model_fields.items() if is_required(info)) + + def missing(self, settings: PluginSettings) -> tuple[str, ...]: + """The required settings that are not set in ``settings``.""" + return tuple(k for k in self.required if not is_set(getattr(settings, k))) + def layout( self, name: str, settings: PluginSettings, colors: tuple[str, str] | None = None ) -> Layout: diff --git a/src/paperpi/plugins/met_no/README.md b/src/paperpi/plugins/met_no/README.md index 96dc301..1061905 100644 --- a/src/paperpi/plugins/met_no/README.md +++ b/src/paperpi/plugins/met_no/README.md @@ -50,7 +50,7 @@ Every layout shows the place: the `place` setting, or the coordinates when it is |---|---|---| | `lat` | none, required | latitude of the place, e.g. `52.52` | | `lon` | none, required | longitude of the place, e.g. `13.40` | -| `email` | none, required | your email address, sent only to met.no. met.no requires contact details from every program, so it can ask before blocking one that misbehaves | +| `email` | none, required | your own, real email address, sent only to met.no. met.no's terms of service require contact details from every program, so it can ask before blocking one that misbehaves | | `place` | `""` | name shown at the top, e.g. `"Berlin"`. Without it, the coordinates are shown ("52.52, 13.40") | | `temperature` | `"C"` | `"C"` (Celsius) or `"F"` (Fahrenheit) | | `rain` | `"mm"` | `"mm"` or `"inch"` | @@ -68,11 +68,11 @@ type = "met_no" lat = 52.52 lon = 13.40 place = "Berlin" -email = "you@example.com" +email = "you@example.com" # put your own, real address here layout = "steps_3h" # optional; without it: hours_12 ``` -Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=you@example.com`. +Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. The screen shows "Data: MET Norway" next to the "Updated" time, as met.no's data licence asks. The weather data is from [MET Norway](https://www.met.no/en) (the Norwegian Meteorological Institute), under the [Creative Commons 4.0 BY International](https://creativecommons.org/licenses/by/4.0/) licence. The weather icons are met.no's own, from [github.com/metno/weathericons](https://github.com/metno/weathericons), under the MIT licence ([`icons/LICENSE`](icons/LICENSE)). diff --git a/src/paperpi/plugins/met_no/__init__.py b/src/paperpi/plugins/met_no/__init__.py index 3b61e71..76cf1d5 100644 --- a/src/paperpi/plugins/met_no/__init__.py +++ b/src/paperpi/plugins/met_no/__init__.py @@ -16,7 +16,7 @@ from ... import webrequest from ...files import write_atomic -from ...plugin import Context, Plugin, PluginSettings, ready +from ...plugin import Context, Plugin, PluginSettings, ready, setting from . import barbs, forecast from .forecast import Forecast, Hour from .layouts import HOURS, LAYOUTS @@ -35,20 +35,22 @@ class Settings(PluginSettings): - lat: float | None = Field( - None, ge=-90, le=90, description="Latitude of the place, e.g. 52.52 (required)" + lat: float | None = setting( + None, required=True, ge=-90, le=90, description="Latitude of the place, e.g. 52.52" ) - lon: float | None = Field( - None, ge=-180, le=180, description="Longitude of the place, e.g. 13.40 (required)" + lon: float | None = setting( + None, required=True, ge=-180, le=180, description="Longitude of the place, e.g. 13.40" ) place: str = Field( "", max_length=60, description="Name shown on the screen, e.g. Berlin (else lat, lon)" ) - email: str = Field( + email: str = setting( "", + required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your email address, sent only to met.no (required: met.no asks for it)", + description="Your own, real email address, sent only to met.no: their terms of " + "service ask every program for a way to contact its user", ) temperature: Literal["C", "F"] = Field("C", description="Degrees Celsius or Fahrenheit") rain: Literal["mm", "inch"] = Field("mm", description="Rain in millimetres or inches") diff --git a/src/paperpi/plugins/moon_phase/README.md b/src/paperpi/plugins/moon_phase/README.md index c40daac..d7dad0e 100644 --- a/src/paperpi/plugins/moon_phase/README.md +++ b/src/paperpi/plugins/moon_phase/README.md @@ -29,7 +29,7 @@ met.no gives one answer per place and day. The plugin saves it in its storage fo |---|---|---| | `lat` | none, required | latitude of the place, e.g. `52.52` | | `lon` | none, required | longitude of the place, e.g. `13.40` | -| `email` | none, required | your email address, sent only to met.no. met.no requires contact details from every program, so it can ask before blocking one that misbehaves | +| `email` | none, required | your own, real email address, sent only to met.no. met.no's terms of service require contact details from every program, so it can ask before blocking one that misbehaves | It suggests a refresh every 20 minutes. @@ -41,11 +41,11 @@ name = "Moon" type = "moon_phase" lat = 52.52 lon = 13.40 -email = "you@example.com" +email = "you@example.com" # put your own, real address here layout = "moon_only" # optional; without it: moon_data ``` -Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=you@example.com`. +Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. The moon data is from [MET Norway](https://www.met.no/en), under the [Creative Commons Attribution 4.0 International](https://creativecommons.org/licenses/by/4.0/) licence (CC BY 4.0): anyone may use the data, as long as they say where it came from. The `moon_data` layout shows "Data: MET Norway", as the licence asks. The moon pictures are by [NASA's Scientific Visualization Studio](https://svs.gsfc.nasa.gov/4955) (credit: NASA's Scientific Visualization Studio). NASA's pictures are generally not protected by copyright in the United States and may be used freely, as long as NASA is credited and the use does not suggest that NASA endorses PaperPi ([NASA's media usage guidelines](https://www.nasa.gov/nasa-brand-center/images-and-media/)). They are the same files as in PaperPi v1. The font is Anton, under the SIL Open Font License ([`fonts/Anton-OFL.txt`](../../fonts/Anton-OFL.txt)), in PaperPi's shared fonts folder. diff --git a/src/paperpi/plugins/moon_phase/__init__.py b/src/paperpi/plugins/moon_phase/__init__.py index f19bc63..0542a42 100644 --- a/src/paperpi/plugins/moon_phase/__init__.py +++ b/src/paperpi/plugins/moon_phase/__init__.py @@ -10,11 +10,10 @@ from zoneinfo import ZoneInfo from PIL import Image -from pydantic import Field from ... import webrequest from ...files import write_atomic -from ...plugin import Context, Plugin, PluginSettings, ready +from ...plugin import Context, Plugin, PluginSettings, ready, setting from .layouts import LAYOUTS log = logging.getLogger(__name__) @@ -42,17 +41,19 @@ class Settings(PluginSettings): - lat: float | None = Field( - None, ge=-90, le=90, description="Latitude of the place, e.g. 52.52 (required)" + lat: float | None = setting( + None, required=True, ge=-90, le=90, description="Latitude of the place, e.g. 52.52" ) - lon: float | None = Field( - None, ge=-180, le=180, description="Longitude of the place, e.g. 13.40 (required)" + lon: float | None = setting( + None, required=True, ge=-180, le=180, description="Longitude of the place, e.g. 13.40" ) - email: str = Field( + email: str = setting( "", + required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your email address, sent only to met.no (required: met.no asks for it)", + description="Your own, real email address, sent only to met.no: their terms of " + "service ask every program for a way to contact its user", ) diff --git a/src/paperpi/scheduler.py b/src/paperpi/scheduler.py index 2c06712..02e3017 100644 --- a/src/paperpi/scheduler.py +++ b/src/paperpi/scheduler.py @@ -362,7 +362,7 @@ def _apply(self, config: Config) -> None: old = {slot.name: slot for slot in self._slots} slots = [] for found in config.plugins: - if not found.entry.enabled or found.plugin.type == "default": + if not found.shown or found.plugin.type == "default": continue slot = old.get(found.entry.name) if slot is None or redraw or not _same_plugin(slot.config, found): diff --git a/tests/test_cli.py b/tests/test_cli.py index 38419ca..be8180a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -461,6 +461,8 @@ def test_list_shows_the_plugins_as_used(capsys): assert lines[0].split() == header assert lines[1].split() == "Clock basic_clock yes rotation 120 s 60 s time 500 MB, 30 d".split() assert lines[3].startswith("Weather Rio ") + # Not shown until the user fills in their email address. + assert lines[2].split()[:5] == ["Weather", "Berlin", "met_no", "needs", "email"] def test_list_says_how_many_problems(tmp_path, capsys): diff --git a/tests/test_config.py b/tests/test_config.py index 8e71bec..c9cae70 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -679,3 +679,49 @@ def test_hint_when_the_fallback_clock_is_switched_off(): assert len(cfg.plugins) == 1 [hint] = problems(cfg, "hint") assert "fallback_clock = false is not recommended" in hint + + +WEATHER = """\ +config_version = 1 +[display] +type = "virtual" +[[plugin]] +name = "Weather" +type = "met_no" +lat = 52.52 +lon = 13.40 +""" + + +def test_plugin_without_its_required_settings_is_not_shown(): + loaded = parse(WEATHER) + weather = loaded.plugin("Weather") + assert weather.missing == ("email",) + assert weather.entry.enabled and not weather.shown + assert [(p.level, p.message, p.line) for p in loaded.problems] == [ + ("warning", "not shown until these required settings are filled in: email", 4) + ] + assert config.plugin_rows(loaded)[0].missing == ("email",) + + +def test_required_setting_warning_points_at_the_setting_when_it_is_there(): + loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lat = 52.52\nemail = ""\n')) + assert loaded.plugin("Weather").missing == ("lon", "email") + assert [(p.message, p.line) for p in loaded.problems] == [ + ("not shown until these required settings are filled in: lon, email", 8) + ] + loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lon = 13.40\nlat = ""\n')) + assert [(p.level, p.line) for p in loaded.problems] == [("error", 8)] # "" is no number + + +def test_plugin_with_its_required_settings_is_shown(): + loaded = parse(WEATHER + 'email = "me@example.com"\n') + assert loaded.problems == [] + assert loaded.plugin("Weather").shown + + +def test_switched_off_plugin_without_required_settings_gives_no_warning(): + loaded = parse(WEATHER + "enabled = false\n") + assert loaded.problems == [] + weather = loaded.plugin("Weather") + assert weather.missing == ("email",) and not weather.shown diff --git a/tests/test_example.py b/tests/test_example.py index b31014d..860a532 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -25,7 +25,12 @@ def test_example_file_is_up_to_date(): def test_example_file_works_as_it_is(): loaded = config.parse(EXAMPLE_FILE.read_text()) - assert loaded.problems == [] + # The weather blocks wait for the user's own email address (met.no's terms). + assert [(p.level, p.message, p.where) for p in loaded.problems] == [ + ("warning", "not shown until these required settings are filled in: email", where) + for where in ("[[plugin]] 'Weather Berlin'", "[[plugin]] 'Weather Rio'") + ] + assert [p.entry.name for p in loaded.plugins if p.shown] == ["Clock"] assert [(p.entry.name, p.plugin.type) for p in loaded.plugins] == [ ("Clock", "basic_clock"), ("Weather Berlin", "met_no"), @@ -46,7 +51,10 @@ def test_every_default_shown_is_a_valid_setting(plugin_type): plugin = plugins.load(plugin_type) block = plugin_block(plugin, "Test", shared=True) loaded = config.parse(HEAD + uncomment(block)) - assert [p.level for p in loaded.problems if p.level != "hint"] == [] + waits = [f"not shown until these required settings are filled in: {', '.join(plugin.required)}"] + assert [p.message for p in loaded.problems if p.level != "hint"] == ( + waits if plugin.required else [] + ) found = loaded.plugin("Test") assert found.settings == plugin.settings() assert found.refresh == plugin.refresh @@ -60,7 +68,8 @@ def test_every_default_shown_is_a_valid_setting(plugin_type): def test_every_default_in_the_example_is_valid(): # Also the [display] part. loaded = config.parse(uncomment(example_config())) - assert [p for p in loaded.problems if p.level != "hint"] == [] + waiting = "not shown until these required settings are filled in: email" + assert [p.message for p in loaded.problems if p.level != "hint"] == [waiting, waiting] assert loaded.display.size == config.DisplaySettings(type="virtual").size @@ -182,3 +191,10 @@ def test_plugin_rows(): config.PluginRow("Clock", "basic_clock", True, "rotation", 120, 60, "time", 500, 30), config.PluginRow("Big", "basic_clock", False, "interrupt", 90, 300, "time_date", 20000, 0), ] + + +def test_block_marks_the_required_settings(): + block = plugin_block(plugins.load("met_no"), "Weather", {"lat": 1, "lon": 2}) + assert "# Latitude of the place, e.g. 52.52 (required)\nlat = 1\n" in block + assert 'way to contact its user (required)\n# email = ""\n' in block + assert "Berlin (else lat, lon)\n# place" in block # not required diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 11e53cb..fca04b3 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2,7 +2,7 @@ import pytest from epdlib import ScreenMode -from pydantic import Field +from pydantic import Field, SecretStr from paperpi import plugins from paperpi.plugin import ( @@ -14,7 +14,10 @@ PluginSettings, State, draw_update, + is_required, + is_set, ready, + setting, ) @@ -59,6 +62,10 @@ class NotSettings: pass +class RequiredWithValue(PluginSettings): + email: str = setting("me@example.com", required=True) + + @pytest.mark.parametrize( ("changes", "message"), [ @@ -66,6 +73,7 @@ class NotSettings: ({"settings": NotSettings}, "subclass of PluginSettings"), ({"settings": Clash}, "names of shared settings: name, refresh"), ({"settings": NoDefault}, "every setting needs a default"), + ({"settings": RequiredWithValue}, 'a required setting\'s default must be "not set"'), ({"layouts": {}}, "at least one layout"), ({"refresh": 0}, "refresh must be between 5 and 604800 seconds"), ({"refresh": 4.9}, "refresh must be between 5 and 604800 seconds"), @@ -177,3 +185,44 @@ def test_loader_unknown_type(): def test_loader_broken_plugins(plugin_type, message): with pytest.raises(PluginDefinitionError, match=message): plugins.load(plugin_type, "tests.fake_plugins") + + +class Needs(PluginSettings): + place: str = setting("", required=True, max_length=10, description="Where") + lat: float | None = setting(None, required=True, ge=-90, le=90) + key: SecretStr = setting(SecretStr(""), required=True) + word: str = setting("hi") + + +def test_setting_is_a_field_with_paperpis_options(): + fields = Needs.model_fields + assert [k for k, info in fields.items() if is_required(info)] == ["place", "lat", "key"] + assert not is_required(Settings.model_fields["word"]) + assert fields["place"].description == "Where" + with pytest.raises(ValueError, match="at most 10 characters"): + Needs(place="far too long a place") + with pytest.raises(ValueError, match="less than or equal to 90"): + Needs(lat=91) + + +@pytest.mark.parametrize( + ("value", "expected"), + [(None, False), ("", False), (SecretStr(""), False), ("x", True), (0, True), (0.0, True), + (SecretStr("k"), True), (False, True)], +) # fmt: skip +def test_is_set(value, expected): + assert is_set(value) is expected + + +def test_missing_lists_the_required_settings_that_are_not_set(): + plugin = make(settings=Needs) + assert plugin.required == ("place", "lat", "key") + assert plugin.missing(Needs()) == ("place", "lat", "key") + assert plugin.missing(Needs(place="Rio", lat=0, key=SecretStr("k"))) == () + assert make().required == () + + +@pytest.mark.parametrize("plugin_type", ["met_no", "moon_phase"]) +def test_met_no_plugins_need_a_place_and_an_email_address(plugin_type): + # met.no's terms of service ask for a real way to contact the user. + assert plugins.load(plugin_type).required == ("lat", "lon", "email") diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index a8cf1e6..92243a4 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -540,6 +540,20 @@ def test_default_says_when_no_plugin_is_switched_on(tmp_path): assert sim.shown == ["default 0/0"] +def test_plugin_without_its_required_settings_is_not_updated(tmp_path): + weather = rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2' + sim = Sim(tmp_path, rotation("a"), weather) + sim.plan(a="A", w="W") + sim.run(until=100) + assert sim.updates.times("w") == [] + assert sim.shown == ["A"] + # Filling in the email address (a reload) starts it. + sim.next_config = make_config(rotation("a"), weather + '\nemail = "me@example.com"') + sim.at(100, sim.scheduler.reload) + sim.run(until=150) + assert sim.updates.times("w")[0] == 101 + + def clock_labels(t): """A plan for the fallback clock: a new picture every minute.""" return f"clock {int(t // 60)}" From 023705e4d5935a3c244fd954ad83c792ea36be6b Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:54:39 +0200 Subject: [PATCH 2/6] Review fixes: setting() options, screen message, docs, more tests Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +-- docs/decisions/config-format.md | 3 ++ docs/decisions/live-config-reload.md | 1 + docs/decisions/plugin-interface.md | 2 +- docs/writing-plugins.md | 4 +-- paperpi.example.toml | 13 ++++---- src/paperpi/cli.py | 12 ++++++-- src/paperpi/example.py | 7 +++-- src/paperpi/plugin.py | 36 ++++++++++++++-------- src/paperpi/plugins/default/README.md | 2 +- src/paperpi/plugins/default/__init__.py | 5 +-- src/paperpi/plugins/met_no/README.md | 2 +- src/paperpi/plugins/met_no/__init__.py | 4 +-- src/paperpi/plugins/moon_phase/README.md | 2 +- src/paperpi/plugins/moon_phase/__init__.py | 4 +-- src/paperpi/scheduler.py | 5 +-- tests/test_cli.py | 33 ++++++++++++++++++++ tests/test_config.py | 1 + tests/test_debugging.py | 2 +- tests/test_example.py | 2 +- tests/test_plugin.py | 29 +++++++++++++++-- tests/test_scheduler.py | 18 +++++++++++ 22 files changed, 144 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 4bfbf5a..3b7ef4e 100644 --- a/README.md +++ b/README.md @@ -68,14 +68,14 @@ Run `uv run paperpi render --help` for all options. How to write a plugin: [docs ### A config file -`paperpi example-config` prints an example config file that works as it is: a virtual screen, a clock, and the weather in Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you put your own, real email address in `email`: met.no's terms of service ask for it, so the example leaves it empty. `paperpi list` shows them as "needs email" until then. `-o` doesn't replace a file that is already there, unless you add `--force`. +`paperpi example-config` prints an example config file that loads without errors: a virtual screen, a clock, and weather blocks for Berlin and Rio. Every other setting is a comment with its default and a short help text. [paperpi.example.toml](paperpi.example.toml) is the same text. The weather blocks are not shown until you remove the `#` in front of `email` and fill in your own, real email address: met.no's terms of service ask for it, so the example leaves it empty. Until then `paperpi list` shows them as "needs email". `-o` doesn't replace a file that is already there, unless you add `--force`. ```bash uv run paperpi example-config -o paperpi.toml uv run paperpi list --config paperpi.toml ``` -`paperpi list` shows the plugins of a config file, one line each, in the order of the file: name, type, on or off, level, display time, refresh, layout and storage (the ones used: the setting, or else the plugin's suggestion or first layout; storage is the size limit and the age limit, for example `500 MB, 30 d`, or `500 MB, no age limit`). Anything wrong with the file is shown first; a block with an error is left out of the list. +`paperpi list` shows the plugins of a config file, one line each, in the order of the file: name, type, on (`yes`, `no`, or `needs ` when a required setting is missing), level, display time, refresh, layout and storage (the ones used: the setting, or else the plugin's suggestion or first layout; storage is the size limit and the age limit, for example `500 MB, 30 d`, or `500 MB, no age limit`). Anything wrong with the file is shown first; a block with an error is left out of the list. ``` name type on level display refresh layout diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index a097d56..684602b 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -66,6 +66,8 @@ display_time = 255 The file only holds settings that differ from the default. This keeps it short and easy to fix by hand. When a later version improves a default, you get it automatically. The web interface shows every setting with its default filled in. `paperpi.example.toml` is generated from the program (`paperpi example-config`), so it is always up to date; a test checks the copy in the repository. It is short and works as it is: `[display]` with a virtual screen, a clock, and the weather in Berlin and in Rio (the same plugin type twice). Every setting it doesn't set is a comment with its default, and its help text on the line above. The first `[[plugin]]` block lists all the settings every plugin has; the other blocks list only `refresh` and `layout`, because their defaults depend on the plugin. `storage_mb` and `storage_days` can also differ per plugin, but most plugins keep the defaults, so they are only in the first block; `paperpi list` shows the values used (M4 issue #224). The copy in the repository is not edited by hand: it is made again with `paperpi example-config -o paperpi.example.toml --force`. Agreed with txoof on 2026-10-06: the example does not list every plugin. Adding and removing plugin blocks is the job of a config manager (the web interface, M5), which builds a block the same way (`paperpi.example.plugin_block`). That function refuses unknown settings and values the plugin would not accept, and checks that PaperPi reads the block back as written. Plugin names may not hold control characters (such as tab, new line or the terminal's ESC), because they can't be written back to the file reliably. +*Update 2026-10-07 (M5 part 2a, issue #238):* the example no longer fills in a made-up email address, because met.no's terms of service ask for a real one. It loads without errors, but the weather blocks are not shown, and the check warns, until you fill in `email`. + ### Checking the file Each part of the config (display, web, each plugin type) is described in code as a list of settings, each with a type, a default and a short help text. The `pydantic` package does this. From the same description we get: @@ -82,6 +84,7 @@ What happens when something is wrong: | File can't be read at all, or the `display` / `web` part is wrong | Runs on the **last good** copy, a copy that is saved each time the config loads correctly. A warning on the screen and in the web interface names the wrong line. | | ...and there is no last good copy (first install) | Shows an error screen with a QR code (a square barcode a phone camera can scan) that opens the web interface. | | One plugin block is wrong (missing value, text where a number belongs) | Only that plugin is switched off. Everything else runs. The web interface shows which setting is wrong. | +| A switched-on plugin is missing a required setting (e.g. `email`, see `plugin-interface.md`) | The plugin is not shown and not counted as broken. A warning names the missing settings; `paperpi list` shows "needs …". | | Unknown setting name, e.g. `lattitude` | Warning only: "unknown setting, did you mean `latitude`?" | The web interface only accepts valid values, so most of these mistakes can only happen through hand edits. diff --git a/docs/decisions/live-config-reload.md b/docs/decisions/live-config-reload.md index fbd42c4..6472880 100644 --- a/docs/decisions/live-config-reload.md +++ b/docs/decisions/live-config-reload.md @@ -38,6 +38,7 @@ What is on screen only lasts until the next cycle anyway, so this is kept simple - A changed plugin that is on screen is updated and redrawn right away, so the user sees the result. - A plugin that is removed or switched off while on screen: rotation moves on to the next plugin. - A new plugin joins the end of the rotation. +- *(M5 part 2a)* A plugin that is missing a required setting is treated as switched off: filling the setting in and reloading starts it (at its place in the file), and emptying it takes the plugin out of the rotation. **Update 2026-10-05 (M4, issue #205):** plugins take turns in the order of the config file, so a new plugin takes the place where it is in the file (the end, when it is added at the end). Until reloading screen settings is built (M4 issue #222, part 5b), changed screen settings take effect at the next start, with a warning in the log. *(Built in part 5b; see the table below.)* diff --git a/docs/decisions/plugin-interface.md b/docs/decisions/plugin-interface.md index 0ba9457..fb87614 100644 --- a/docs/decisions/plugin-interface.md +++ b/docs/decisions/plugin-interface.md @@ -50,7 +50,7 @@ The plugin never talks to the screen. Only the scheduler does. For sample images and tests, `fetch` is skipped and the sample data goes straight to `draw`. How to write a plugin: `docs/writing-plugins.md`. -**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place where a setting asks PaperPi for help; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. +**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place for a setting's own options; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. ### How a plugin is run diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 2a0ace9..1a86ee3 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -80,7 +80,7 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Every setting needs a default. - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. -- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None` or `""` (or an empty `SecretStr`). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface say which settings are missing. The example block marks them "(required)". `setting` is the one place where a setting asks PaperPi for more help; later options, such as a helper in the web interface that looks up latitude and longitude, are added there too. +- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None`, empty text or an empty `SecretStr` (text of only spaces also counts as not set). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check and `paperpi list` (and from M5 part 2b the web interface) say which settings are missing. The example block marks them "(required)". `setting` is also where later options for a single setting are added, such as a web interface helper that looks up latitude and longitude (M5 part 3c). ```python from paperpi.plugin import PluginSettings, setting @@ -142,7 +142,7 @@ data = answer.json() ``` `get` takes: -- `contact`: an email or web address, added to the User-Agent header (the line in every request that names the program), so the service knows whom to ask about problems. Some services, like met.no, require it. Give your plugin a setting for it, as `met_no` does with `email`. +- `contact`: an email or web address, added to the User-Agent header (the line in every request that names the program), so the service knows whom to ask about problems. Some services, like met.no, require it. Give your plugin a setting for it, as `met_no` does with `email`, and make it required with `setting(..., required=True)`. - `headers=`: more headers to send, for example an API key. They are sent only to the server of `url`: if that server redirects to another one, they are left out. - `if_modified_since=`: the `last_modified` of an earlier answer. If nothing changed since, `answer.not_modified` is `True` and `answer.body` is empty. Save `last_modified` (it can be `None` when the server didn't send one) and the data in `context.storage`, so the next update can ask. - `max_bytes=`: a higher size limit, for example for large images. diff --git a/paperpi.example.toml b/paperpi.example.toml index b5a0c9a..62c7ebf 100644 --- a/paperpi.example.toml +++ b/paperpi.example.toml @@ -4,9 +4,10 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks are not shown until you fill -# in your own, real email address in "email": met.no asks every program for a way to -# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. +# more than once, with a different name. The weather blocks are not shown until you remove +# the # in front of "email" and fill in your own, real email address: met.no asks every +# program for a way to contact its user. Settings marked "(required)" must be filled in +# before a plugin is shown. config_version = 1 @@ -98,8 +99,7 @@ lat = 52.52 lon = 13.4 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Berlin" -# Your own, real email address, sent only to met.no: their terms of service ask every program for a -# way to contact its user (required) +# Your own, real email address, sent only to met.no (their terms of service ask for one) (required) # email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" @@ -122,8 +122,7 @@ lat = -22.91 lon = -43.17 # Name shown on the screen, e.g. Berlin (else lat, lon) place = "Rio" -# Your own, real email address, sent only to met.no: their terms of service ask every program for a -# way to contact its user (required) +# Your own, real email address, sent only to met.no (their terms of service ask for one) (required) # email = "" # Degrees Celsius or Fahrenheit. One of: "C", "F" # temperature = "C" diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index b8b18ee..678b272 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -146,7 +146,9 @@ def _parser() -> argparse.ArgumentParser: "Show the plugins of a config file, one line each, in the order of the file. " "Anything wrong with the file is shown first; a block with an error is not in the " "list. Refresh and layout are the ones used: the setting, or else the plugin's " - "suggested refresh and its first layout." + 'suggested refresh and its first layout. The "on" column says "needs ' + '" for a plugin that is switched on but not shown, because a required ' + "setting is missing." ), ) listing.add_argument( @@ -237,6 +239,10 @@ def _render(args: argparse.Namespace) -> int: if layout not in job.plugin.layouts: known = ", ".join(job.plugin.layouts) raise UsageError(f"unknown layout {layout!r}; choose from: {known}") + missing = job.plugin.missing(job.settings) + if args.live and missing: + sets = " ".join(f"--set {k}=..." for k in missing) + raise UsageError(f"{job.plugin.type} needs these required settings: {sets}") time_limit = job.time_limit if args.time_limit is None else args.time_limit if not 0 < time_limit <= limits.PLUGIN_UPDATE_MAX: raise UsageError(f"--time-limit must be above 0 and at most {limits.PLUGIN_UPDATE_MAX:g}") @@ -389,7 +395,7 @@ def _list(args: argparse.Namespace) -> int: ( row.name, row.type, - _on(row), + _on_column(row), row.level, f"{row.display_time:g} s", f"{row.refresh:g} s", @@ -411,7 +417,7 @@ def _list(args: argparse.Namespace) -> int: return 0 -def _on(row: config.PluginRow) -> str: +def _on_column(row: config.PluginRow) -> str: """The "on" column of ``paperpi list``.""" if not row.enabled: return "no" diff --git a/src/paperpi/example.py b/src/paperpi/example.py index 4b7ad26..ffc7b7f 100644 --- a/src/paperpi/example.py +++ b/src/paperpi/example.py @@ -57,9 +57,10 @@ # The file only needs the settings that differ from the default. Every other setting is # shown as a comment with its default: remove the # in front of it to change it. # Each [[plugin]] block is one plugin on the screen; the same plugin type can be used -# more than once, with a different name. The weather blocks are not shown until you fill -# in your own, real email address in "email": met.no asks every program for a way to -# contact its user. Settings marked "(required)" must be filled in before a plugin is shown. +# more than once, with a different name. The weather blocks are not shown until you remove +# the # in front of "email" and fill in your own, real email address: met.no asks every +# program for a way to contact its user. Settings marked "(required)" must be filled in +# before a plugin is shown. """ #: Settings whose default is "empty", but that mean a known value; shown with that value. diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index f8e2210..d17589f 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -102,34 +102,41 @@ class Settings(PluginSettings): _OPTIONS = "paperpi" -def setting(default: Any = None, *, required: bool = False, **field: Any) -> Any: +def setting(default: Any, *, required: bool = False, **field: Any) -> Any: """A plugin setting: pydantic's ``Field`` (with the same ``description``, ``ge``, ``max_length``, ...) plus what PaperPi needs to know about it. This is the one place a - setting asks PaperPi for help; later options (such as a helper that looks up a place's - latitude and longitude in the web interface) are added here too:: + setting gets PaperPi's own options; later ones (such as a web interface helper that + looks up a place's latitude and longitude, M5 part 3c) are added here too:: lat: float | None = setting(None, required=True, description="Latitude") ``required``: the plugin can't work until the user fills it in (a place, an email - address, an API key). Its default must be "not set": ``None`` or ``""``. A plugin - with a required setting that is not set is not shown; the config check and the web - interface say which settings it needs. + address, an API key). Its default must be "not set" (see :func:`is_set`). A plugin + with a required setting that is not set is not shown; the config check and + ``paperpi list`` (and from M5 part 2b the web interface) say which settings it needs. """ - options = {"required": True} if required else {} - return Field(default, json_schema_extra={_OPTIONS: options} if options else None, **field) + extra = field.pop("json_schema_extra", None) or {} + if not isinstance(extra, dict): + raise TypeError("setting() takes json_schema_extra only as a dictionary") + if required: + extra = extra | {_OPTIONS: {"required": True}} + return Field(default, json_schema_extra=extra or None, **field) def is_required(info: FieldInfo) -> bool: """True for a setting made with ``setting(required=True)``.""" extra = info.json_schema_extra - return isinstance(extra, dict) and bool(extra.get(_OPTIONS, {}).get("required")) + options = extra.get(_OPTIONS) if isinstance(extra, dict) else None + return isinstance(options, dict) and bool(options.get("required")) def is_set(value: Any) -> bool: - """False for "not set": ``None``, ``""`` or an empty secret.""" + """False for "not set": ``None``, empty text (also only spaces) or an empty secret.""" if hasattr(value, "get_secret_value"): value = value.get_secret_value() - return value is not None and value != "" + if isinstance(value, str | bytes): + return bool(value.strip()) + return value is not None class PluginEntry(BaseModel): @@ -227,7 +234,7 @@ class PluginsStatus: failing: int """Plugins that failed their last update, or are left out after failing again and again.""" total: int - """Plugins that are switched on.""" + """Plugins that are ready to show: switched on, with all their required settings.""" @dataclass(frozen=True) @@ -301,7 +308,10 @@ def __post_init__(self) -> None: problems.append("every setting needs a default") else: if self.missing(defaults) != self.required: - problems.append('a required setting\'s default must be "not set" (None or "")') + problems.append( + 'a required setting\'s default must be "not set" (None, "" or an empty ' + "secret)" + ) if not self.layouts: problems.append("needs at least one layout") if not limits.SHORTEST_REFRESH <= self.refresh <= limits.LONGEST_SETTING: diff --git a/src/paperpi/plugins/default/README.md b/src/paperpi/plugins/default/README.md index 0784601..29618b6 100644 --- a/src/paperpi/plugins/default/README.md +++ b/src/paperpi/plugins/default/README.md @@ -1,6 +1,6 @@ # default -Shown when nothing else can be shown because plugins are failing, or because no plugin is switched on. The scheduler tells it how many plugins are not working, and it shows e.g. "3 of 4 plugins are not working. See the web interface for more information." It needs no network. +Shown when nothing else can be shown because plugins are failing, or because no plugin is ready to show (switched on, with all its required settings filled in). The scheduler tells it how many plugins are not working, and it shows e.g. "3 of 4 plugins are not working. See the web interface for more information." It needs no network. PaperPi always has this plugin, also when the config file has no block for it. A `[[plugin]]` block with `type = "default"` changes its settings; it never takes part in the rotation. The QR code that opens the web interface comes with the web interface (M5). diff --git a/src/paperpi/plugins/default/__init__.py b/src/paperpi/plugins/default/__init__.py index e43ecbf..9ba3fee 100644 --- a/src/paperpi/plugins/default/__init__.py +++ b/src/paperpi/plugins/default/__init__.py @@ -1,4 +1,5 @@ -"""default: shown when nothing else can be: plugins are failing, or none are switched on. +"""default: shown when nothing else can be: plugins are failing, or none is ready to show +(none is switched on with all its required settings filled in). The scheduler tells it how many plugins are failing (``context.status``). The QR code that opens the web interface is added with the web interface (M5). @@ -19,7 +20,7 @@ def fetch(context: Context): def message(status: PluginsStatus) -> str: if status.total == 0: - return "No plugins are switched on." + return "No plugins are ready to show." noun = "plugin is" if status.total == 1 else "plugins are" return f"{status.failing} of {status.total} {noun} not working." diff --git a/src/paperpi/plugins/met_no/README.md b/src/paperpi/plugins/met_no/README.md index 1061905..0216931 100644 --- a/src/paperpi/plugins/met_no/README.md +++ b/src/paperpi/plugins/met_no/README.md @@ -72,7 +72,7 @@ email = "you@example.com" # put your own, real address here layout = "steps_3h" # optional; without it: hours_12 ``` -Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. +Try it without a screen: `uv run paperpi render met_no --set place=Berlin` (sample data), or with real data: `uv run paperpi render met_no --live --set lat=52.52 --set lon=13.40 --set email=you@example.com` (put your own address in place of `you@example.com`). If lat, lon or email is missing, the plugin is not shown; the config check and `paperpi list` say which one. The screen shows "Data: MET Norway" next to the "Updated" time, as met.no's data licence asks. The weather data is from [MET Norway](https://www.met.no/en) (the Norwegian Meteorological Institute), under the [Creative Commons 4.0 BY International](https://creativecommons.org/licenses/by/4.0/) licence. The weather icons are met.no's own, from [github.com/metno/weathericons](https://github.com/metno/weathericons), under the MIT licence ([`icons/LICENSE`](icons/LICENSE)). diff --git a/src/paperpi/plugins/met_no/__init__.py b/src/paperpi/plugins/met_no/__init__.py index 76cf1d5..6069cf3 100644 --- a/src/paperpi/plugins/met_no/__init__.py +++ b/src/paperpi/plugins/met_no/__init__.py @@ -49,8 +49,8 @@ class Settings(PluginSettings): required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your own, real email address, sent only to met.no: their terms of " - "service ask every program for a way to contact its user", + description="Your own, real email address, sent only to met.no (their terms of " + "service ask for one)", ) temperature: Literal["C", "F"] = Field("C", description="Degrees Celsius or Fahrenheit") rain: Literal["mm", "inch"] = Field("mm", description="Rain in millimetres or inches") diff --git a/src/paperpi/plugins/moon_phase/README.md b/src/paperpi/plugins/moon_phase/README.md index d7dad0e..a7b4386 100644 --- a/src/paperpi/plugins/moon_phase/README.md +++ b/src/paperpi/plugins/moon_phase/README.md @@ -45,7 +45,7 @@ email = "you@example.com" # put your own, real address here layout = "moon_only" # optional; without it: moon_data ``` -Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=`. Without lat, lon and email the plugin is not shown, and the config check says which of them is missing. +Try it without a screen: `uv run paperpi render moon_phase` (sample data), or with real data: `uv run paperpi render moon_phase --live --set lat=52.52 --set lon=13.40 --set email=you@example.com` (put your own address in place of `you@example.com`). If lat, lon or email is missing, the plugin is not shown; the config check and `paperpi list` say which one. The moon data is from [MET Norway](https://www.met.no/en), under the [Creative Commons Attribution 4.0 International](https://creativecommons.org/licenses/by/4.0/) licence (CC BY 4.0): anyone may use the data, as long as they say where it came from. The `moon_data` layout shows "Data: MET Norway", as the licence asks. The moon pictures are by [NASA's Scientific Visualization Studio](https://svs.gsfc.nasa.gov/4955) (credit: NASA's Scientific Visualization Studio). NASA's pictures are generally not protected by copyright in the United States and may be used freely, as long as NASA is credited and the use does not suggest that NASA endorses PaperPi ([NASA's media usage guidelines](https://www.nasa.gov/nasa-brand-center/images-and-media/)). They are the same files as in PaperPi v1. The font is Anton, under the SIL Open Font License ([`fonts/Anton-OFL.txt`](../../fonts/Anton-OFL.txt)), in PaperPi's shared fonts folder. diff --git a/src/paperpi/plugins/moon_phase/__init__.py b/src/paperpi/plugins/moon_phase/__init__.py index 0542a42..065f7b9 100644 --- a/src/paperpi/plugins/moon_phase/__init__.py +++ b/src/paperpi/plugins/moon_phase/__init__.py @@ -52,8 +52,8 @@ class Settings(PluginSettings): required=True, max_length=200, pattern=r"^$|^[^@\s]+@[^@\s]+$", - description="Your own, real email address, sent only to met.no: their terms of " - "service ask every program for a way to contact its user", + description="Your own, real email address, sent only to met.no (their terms of " + "service ask for one)", ) diff --git a/src/paperpi/scheduler.py b/src/paperpi/scheduler.py index 02e3017..6f3e95f 100644 --- a/src/paperpi/scheduler.py +++ b/src/paperpi/scheduler.py @@ -40,7 +40,8 @@ - A failed update is skipped and tried again at the next refresh. After :data:`~paperpi.limits.FAILURES_BEFORE_LEFT_OUT` failures in a row the plugin is left out for :data:`~paperpi.limits.LEFT_OUT` seconds. When nothing can be shown because plugins - fail, or no plugin is switched on, the ``default`` plugin says so. When no plugin has + fail, or no plugin is ready to show (switched on, with all its required settings filled + in), the ``default`` plugin says so. When no plugin has anything to show and none fail, a small fallback clock is shown (``[display] fallback_clock``), so an empty screen is never mistaken for a broken one. @@ -639,7 +640,7 @@ def _next(self, group: list[_Slot], after: str | None) -> _Slot: def _choose_idle(self, now: float) -> _Slot | None: """No plugin has anything to show. - When plugins fail, or none are switched on: the ``default`` plugin, which says so. + When plugins fail, or none is ready to show: the ``default`` plugin, which says so. Otherwise (e.g. only a music plugin, and no music) the fallback clock, so the screen still changes every minute and can be told apart from a broken one. """ diff --git a/tests/test_cli.py b/tests/test_cli.py index be8180a..74ae37b 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -443,6 +443,39 @@ def test_run_cleans_every_plugin_folder_at_start(tmp_path, monkeypatch): assert not old.exists() +def test_run_counts_only_the_plugins_that_are_shown(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(cli.Scheduler, "run", lambda self: None) + cfg = tmp_path / "paperpi.toml" + cfg.write_text( + 'config_version = 1\n[display]\ntype = "virtual"\n' + '[[plugin]]\nname = "Clock"\ntype = "basic_clock"\n' + '[[plugin]]\nname = "Weather"\ntype = "met_no"\nlat = 1\nlon = 2\n' + ) + args = ["run", "--no-web", "--config", str(cfg), "--state-dir", str(tmp_path)] + assert main([*args, "--health-file", str(tmp_path / "health")]) == 0 + assert "showing 1 plugin;" in capsys.readouterr().out + + +def test_list_says_which_required_settings_are_missing(tmp_path, capsys): + cfg = tmp_path / "paperpi.toml" + cfg.write_text( + 'config_version = 1\n[display]\ntype = "virtual"\n' + '[[plugin]]\nname = "Weather"\ntype = "met_no"\n' + '[[plugin]]\nname = "Off"\ntype = "met_no"\nenabled = false\n' + ) + assert main(["list", "--config", str(cfg)]) == 0 + lines = capsys.readouterr().out.splitlines() + assert lines[1].split()[:6] == ["Weather", "met_no", "needs", "lat,", "lon,", "email"] + assert lines[2].split()[:3] == ["Off", "met_no", "no"] # switched off: no "needs" + + +def test_render_live_says_which_required_settings_are_missing(capsys): + assert main(["render", "met_no", "--live", "--set", "lat=1"]) == 2 + assert "met_no needs these required settings: --set lon=... --set email=..." in ( + capsys.readouterr().err + ) + + def test_list_shows_no_age_limit(tmp_path, capsys): cfg = tmp_path / "paperpi.toml" cfg.write_text( diff --git a/tests/test_config.py b/tests/test_config.py index c9cae70..3d0a56d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -710,6 +710,7 @@ def test_required_setting_warning_points_at_the_setting_when_it_is_there(): assert [(p.message, p.line) for p in loaded.problems] == [ ("not shown until these required settings are filled in: lon, email", 8) ] + # An empty value for a number is no "not set": it is an error, as before. loaded = parse(WEATHER.replace("lat = 52.52\nlon = 13.40\n", 'lon = 13.40\nlat = ""\n')) assert [(p.level, p.line) for p in loaded.problems] == [("error", 8)] # "" is no number diff --git a/tests/test_debugging.py b/tests/test_debugging.py index 6f8b32a..26f2e2e 100644 --- a/tests/test_debugging.py +++ b/tests/test_debugging.py @@ -59,7 +59,7 @@ def test_delay(tmp_path): [ (3, 4, "3 of 4 plugins are not working."), (1, 1, "1 of 1 plugin is not working."), - (0, 0, "No plugins are switched on."), + (0, 0, "No plugins are ready to show."), ], ) def test_default_message(failing, total, text): diff --git a/tests/test_example.py b/tests/test_example.py index 860a532..b2c2284 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -196,5 +196,5 @@ def test_plugin_rows(): def test_block_marks_the_required_settings(): block = plugin_block(plugins.load("met_no"), "Weather", {"lat": 1, "lon": 2}) assert "# Latitude of the place, e.g. 52.52 (required)\nlat = 1\n" in block - assert 'way to contact its user (required)\n# email = ""\n' in block + assert 'terms of service ask for one) (required)\n# email = ""\n' in block assert "Berlin (else lat, lon)\n# place" in block # not required diff --git a/tests/test_plugin.py b/tests/test_plugin.py index fca04b3..924fa03 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2,7 +2,7 @@ import pytest from epdlib import ScreenMode -from pydantic import Field, SecretStr +from pydantic import Field, SecretBytes, SecretStr from paperpi import plugins from paperpi.plugin import ( @@ -197,7 +197,7 @@ class Needs(PluginSettings): def test_setting_is_a_field_with_paperpis_options(): fields = Needs.model_fields assert [k for k, info in fields.items() if is_required(info)] == ["place", "lat", "key"] - assert not is_required(Settings.model_fields["word"]) + assert not is_required(fields["word"]) assert fields["place"].description == "Where" with pytest.raises(ValueError, match="at most 10 characters"): Needs(place="far too long a place") @@ -207,13 +207,36 @@ def test_setting_is_a_field_with_paperpis_options(): @pytest.mark.parametrize( ("value", "expected"), - [(None, False), ("", False), (SecretStr(""), False), ("x", True), (0, True), (0.0, True), + [(None, False), ("", False), (" ", False), (SecretStr(""), False), (SecretStr(" "), False), + (SecretBytes(b""), False), (b"", False), ("x", True), (0, True), (0.0, True), (SecretStr("k"), True), (False, True)], ) # fmt: skip def test_is_set(value, expected): assert is_set(value) is expected +def test_setting_keeps_other_schema_extras(): + class Extra(PluginSettings): + word: str = setting("", required=True, json_schema_extra={"examples": ["hi"]}) + other: str = setting("", json_schema_extra={"paperpi": "wrong shape"}) + + info = Extra.model_fields["word"] + assert info.json_schema_extra == {"examples": ["hi"], "paperpi": {"required": True}} + assert is_required(info) + assert not is_required(Extra.model_fields["other"]) # no crash on a wrong shape + + +def test_setting_needs_a_default(): + with pytest.raises(TypeError): + setting(description="no default") + + +def test_built_in_plugins_need_no_settings(): + # PaperPi runs these with their defaults (the screen shown when nothing else can be). + for plugin_type in ("basic_clock", "default"): + assert plugins.load(plugin_type).required == () + + def test_missing_lists_the_required_settings_that_are_not_set(): plugin = make(settings=Needs) assert plugin.required == ("place", "lat", "key") diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 92243a4..bb2cd9e 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -554,6 +554,24 @@ def test_plugin_without_its_required_settings_is_not_updated(tmp_path): assert sim.updates.times("w")[0] == 101 +def test_reload_without_a_required_setting_takes_the_plugin_off_screen(tmp_path): + weather = rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2' + sim = Sim(tmp_path, rotation("a"), weather + '\nemail = "me@example.com"') + sim.plan(a="A", w="W") + sim.run(until=150) # w is on screen since 101 + sim.next_config = make_config(rotation("a"), weather) + sim.scheduler.reload() + sim.run(until=300) + assert sim.writes[:3] == [(1, "A"), (101, "W"), (150, "A")] + assert [t for t in sim.updates.times("w") if t >= 150] == [] + + +def test_default_says_so_when_every_plugin_waits_for_its_settings(tmp_path): + sim = Sim(tmp_path, rotation("w") + '\ntype = "met_no"\nlat = 1\nlon = 2') + sim.run(until=100) + assert sim.shown == ["default 0/0"] # "No plugins are ready to show." + + def clock_labels(t): """A plan for the fallback clock: a new picture every minute.""" return f"clock {int(t // 60)}" From d639c7ddbabd33bd8f663caac8dfccc79e505251 Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:14:32 +0200 Subject: [PATCH 3/6] Editing plugin blocks in the config file (M5 part 2b-1) Part of #238. Co-Authored-By: Claude Opus 5.5 --- docs/decisions/config-format.md | 1 + src/paperpi/web/config_file.py | 271 ++++++++++++++++++++++++++++++++ src/paperpi/web/password.py | 21 +-- tests/test_web_config_file.py | 129 +++++++++++++++ 4 files changed, 407 insertions(+), 15 deletions(-) create mode 100644 src/paperpi/web/config_file.py create mode 100644 tests/test_web_config_file.py diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index 684602b..f6bcc6a 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -92,6 +92,7 @@ The web interface only accepts valid values, so most of these mistakes can only ### Writing the file - The web interface keeps comments that were written by hand. + *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. Single settings (such as the password) are still written with tomlkit. - The file is written safely: first to a temporary file, then that file replaces the old one in one step. A power cut during a save can't leave a half-written config. ### Secrets diff --git a/src/paperpi/web/config_file.py b/src/paperpi/web/config_file.py new file mode 100644 index 0000000..7f6c287 --- /dev/null +++ b/src/paperpi/web/config_file.py @@ -0,0 +1,271 @@ +"""Changing the config file from the web interface, keeping everything else in it as it is. + +- :func:`read` and :func:`write` read the file and save a changed copy that keeps the old + file's permissions (and owner, when run as root). +- The plugin changes (:func:`move`, :func:`remove`, :func:`set_enabled`, :func:`add`) work on + the file's text, one ``[[plugin]]`` block at a time. The comment lines just above a + ``[[plugin]]`` line belong to that block and move with it. +- Every change is checked before it is saved: PaperPi must read the new text back as exactly + the old settings plus the change. A file written in a way these functions don't + understand is never saved wrongly; :class:`EditError` says so instead. +- Each change names its block by place and name, so a block that was moved or removed by + hand a moment ago is not changed by mistake (:class:`ChangedMeanwhile`). +""" + +from __future__ import annotations + +import os +import re +import stat +import tomllib +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from .. import config +from ..files import write_atomic + + +class EditError(ValueError): + """The config file can't be read, changed or saved; the message says why.""" + + +class ChangedMeanwhile(EditError): + """The block to change is not where the page showed it: the file changed meanwhile.""" + + +def read(path: Path) -> tuple[Path, str]: + """The real path of the config file (links followed) and its text.""" + # A config file that is a link to another file: change the file it points to. + path = Path(os.path.realpath(path)) + try: + return path, config.read_text(path) + except config.ConfigError as error: + raise EditError(str(error)) from None + + +def write(path: Path, text: str) -> None: + """Save ``text`` as the config file at ``path`` (a real path, from :func:`read`).""" + try: + # The new file keeps the old one's permissions and, when run with sudo, its owner, + # so the PaperPi service can still read it. + info = os.stat(path) + owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None + write_atomic(path, text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) + except OSError as error: + raise EditError(f"can't save {path}: {error}") from None + + +def load(text: str) -> dict[str, Any]: + """The settings in ``text``, as PaperPi reads them.""" + try: + return tomllib.loads(text) + except (tomllib.TOMLDecodeError, RecursionError, ValueError) as error: + raise EditError(f"the config file is not valid TOML: {error}") from None + + +@dataclass(frozen=True) +class Block: + """Where one ``[[plugin]]`` block is in the file (line numbers count from 0).""" + + start: int + """The first line: the comments just above the ``[[plugin]]`` line, or that line.""" + header: int + """The ``[[plugin]]`` line.""" + own_end: int + """The line after its own settings (where a ``[plugin.x]`` part or the next part starts).""" + end: int + """The line after the block, including blank lines and ``[plugin.x]`` parts.""" + + +_HEADER = re.compile(r"\s*(\[\[?)\s*([A-Za-z0-9_.-]+)\s*\]\]?\s*(#.*)?") +_ENABLED = re.compile(r"\s*enabled\s*=") + + +def blocks(text: str) -> list[Block]: + """The ``[[plugin]]`` blocks in ``text``, in file order.""" + lines = text.splitlines() + starts: list[tuple[int, str]] = [] # (line, "plugin" / "part of plugin" / "other") + quote = None # inside a multi-line string: the quotes it started with + for number, line in enumerate(lines): + inside = quote is not None + for found in re.finditer(r'"""|\'\'\'', line): + if quote is None: + quote = found.group() + elif found.group() == quote: + quote = None + header = None if inside else _HEADER.fullmatch(line) + if header: + brackets, name = header.group(1), header.group(2) + if brackets == "[[" and name == "plugin": + starts.append((number, "plugin")) + elif name.startswith("plugin."): + starts.append((number, "part of plugin")) + else: + starts.append((number, "other")) + found: list[Block] = [] + for index, (header, kind) in enumerate(starts): + if kind != "plugin": + continue + later = starts[index + 1 :] + own_end = later[0][0] if later else len(lines) + after = next((n for n, k in later if k != "part of plugin"), len(lines)) + end = _comments_above(lines, after, header + 1) if after < len(lines) else after + lowest = max((n + 1 for n, _ in starts[:index]), default=0) + found.append(Block(_comments_above(lines, header, lowest), header, own_end, end)) + return found + + +def _comments_above(lines: list[str], line: int, lowest: int) -> int: + """The first of the comment lines just above ``line`` (or ``line`` itself).""" + while line > lowest and lines[line - 1].lstrip().startswith("#"): + line -= 1 + return line + + +def plugin_blocks(data: dict[str, Any]) -> list[Any]: + """The ``[[plugin]]`` blocks of the loaded settings (as written, also broken ones).""" + found = data.get("plugin", []) + return found if isinstance(found, list) else [] + + +def name_of(block: Any) -> str: + """The name of a loaded plugin block, or ``""`` when it has none (a broken block).""" + name = block.get("name") if isinstance(block, dict) else None + return name if isinstance(name, str) else "" + + +def move(text: str, index: int, name: str, step: int) -> str: + """``text`` with plugin block ``index`` moved one place up (``step=-1``) or down (``1``).""" + data, found = _check(text, index, name) + other = index + step + if step not in (-1, 1) or not 0 <= other < len(found): + raise EditError("this plugin can't move further") + first, second = sorted((index, other)) + a, b = found[first], found[second] + lines = _lines(text) + new = [ + *lines[: a.start], + *_spaced(lines[b.start : b.end]), + *lines[a.end : b.start], + *lines[a.start : a.end], + *lines[b.end :], + ] + expected = plugin_blocks(data) + expected[first], expected[second] = expected[second], expected[first] + return _checked("".join(new), data | {"plugin": expected}) + + +def remove(text: str, index: int, name: str) -> str: + """``text`` without plugin block ``index``.""" + data, found = _check(text, index, name) + block = found[index] + lines = _lines(text) + new = "".join([*lines[: block.start], *lines[block.end :]]) + expected = plugin_blocks(data) + del expected[index] + data = {k: v for k, v in data.items() if k != "plugin"} + return _checked(new, data | {"plugin": expected} if expected else data) + + +def set_enabled(text: str, index: int, name: str, enabled: bool) -> str: + """``text`` with plugin block ``index`` switched on or off. + + Off writes ``enabled = false``; on removes that line, because the file only holds + settings that differ from the default. + """ + data, found = _check(text, index, name) + block = found[index] + lines = _lines(text) + own = range(block.header + 1, block.own_end) + at = [n for n in own if _ENABLED.match(lines[n]) and not _in_string(text, n)] + if enabled: + new = [line for n, line in enumerate(lines) if n not in at] + else: + new = list(lines) + if at: + new[at[0]] = "enabled = false\n" + else: + after = _setting_line(lines, own, "type") or _setting_line(lines, own, "name") + new.insert((after or block.header) + 1, "enabled = false\n") + expected = plugin_blocks(data) + changed = {k: v for k, v in expected[index].items() if k != "enabled"} + expected[index] = changed if enabled else changed | {"enabled": False} + return _checked("".join(new), data | {"plugin": expected}) + + +def add(text: str, block_text: str) -> str: + """``text`` with ``block_text`` (one ``[[plugin]]`` block, as written by + :func:`paperpi.example.plugin_block`) added after the last plugin block.""" + data = load(text) + found = blocks(text) + if len(found) != len(plugin_blocks(data)): + raise EditError(_UNKNOWN_FORM) + new_block = plugin_blocks(load(block_text)) + lines = _lines(text) + at = found[-1].end if found else len(lines) + before = "".join(lines[:at]) + after = "".join(lines[at:]) + gap = "" if not before or before.endswith("\n\n") else "\n" + new = before + gap + block_text + ("\n" + after if after else "") + return _checked(new, data | {"plugin": [*plugin_blocks(data), *new_block]}) + + +_UNKNOWN_FORM = ( + "the plugin blocks in the config file are written in a way the web interface can't " + "change safely; change it by hand" +) + + +def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], list[Block]]: + data = load(text) + found = blocks(text) + raw = plugin_blocks(data) + if len(found) != len(raw): + raise EditError(_UNKNOWN_FORM) + if not 0 <= index < len(raw): + raise ChangedMeanwhile(_MEANWHILE) + if name_of(raw[index]) != name: + raise ChangedMeanwhile(_MEANWHILE) + return data, found + + +_MEANWHILE = "the config file was changed meanwhile; look at the list again and retry" + + +def _checked(new: str, expected: dict[str, Any]) -> str: + """``new``, after checking that PaperPi reads it back as ``expected``.""" + try: + read_back = tomllib.loads(new) + except tomllib.TOMLDecodeError: + read_back = None + if read_back != expected: + raise EditError(_UNKNOWN_FORM) + return new + + +def _lines(text: str) -> list[str]: + """The lines of ``text``, each ending with a line break.""" + return [line if line.endswith("\n") else line + "\n" for line in text.splitlines(True)] + + +def _spaced(chunk: list[str]) -> list[str]: + """``chunk`` ending with a blank line, so it stays apart from the block after it.""" + return chunk if chunk and not chunk[-1].strip() else [*chunk, "\n"] + + +def _setting_line(lines: list[str], within: range, key: str) -> int | None: + pattern = re.compile(rf"\s*{key}\s*=") + return next((n for n in within if pattern.match(lines[n])), None) + + +def _in_string(text: str, line: int) -> bool: + """True when line ``line`` starts inside a multi-line string.""" + before = "\n".join(text.splitlines()[:line]) + quote = None + for found in re.finditer(r'"""|\'\'\'', before): + if quote is None: + quote = found.group() + elif found.group() == quote: + quote = None + return quote is not None diff --git a/src/paperpi/web/password.py b/src/paperpi/web/password.py index 972f0fa..5f26208 100644 --- a/src/paperpi/web/password.py +++ b/src/paperpi/web/password.py @@ -13,17 +13,14 @@ import base64 import hashlib import hmac -import os import secrets -import stat import tomllib from pathlib import Path import tomlkit from tomlkit.exceptions import TOMLKitError -from .. import config -from ..files import write_atomic +from . import config_file #: Shortest and longest password the web interface accepts. PASSWORD_LENGTH = (8, 1000) @@ -99,11 +96,9 @@ def save_password_hash(path: Path, stored: str | None) -> bool: permissions. Returns False when there was nothing to change. Raises :class:`PasswordError` when the file can't be read, changed or written. """ - # A config file that is a link to another file: change the file it points to. - path = Path(os.path.realpath(path)) try: - text = config.read_text(path) - except config.ConfigError as error: + path, text = config_file.read(path) + except config_file.EditError as error: raise PasswordError(str(error)) from None try: document = tomlkit.parse(text) @@ -131,13 +126,9 @@ def save_password_hash(path: Path, stored: str | None) -> bool: ): raise PasswordError(f"{path} can't be written back with the new password") try: - # The new file keeps the old one's permissions and, when run with sudo, its owner, - # so the PaperPi service can still read it. - info = os.stat(path) - owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None - write_atomic(path, new_text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) - except OSError as error: - raise PasswordError(f"can't save {path}: {error}") from None + config_file.write(path, new_text) + except config_file.EditError as error: + raise PasswordError(str(error)) from None return True diff --git a/tests/test_web_config_file.py b/tests/test_web_config_file.py new file mode 100644 index 0000000..e8eb2d5 --- /dev/null +++ b/tests/test_web_config_file.py @@ -0,0 +1,129 @@ +import tomllib + +import pytest + +from paperpi import example, plugins +from paperpi.web import config_file +from paperpi.web.config_file import ChangedMeanwhile, EditError + +CONFIG = """\ +config_version = 1 +[display] +type = "virtual" + +# the clock in the kitchen +[[plugin]] +name = "Clock" +type = "basic_clock" +# refresh = 60 + +# words +[[plugin]] +name = "Words" +type = "word_clock" +enabled = true # for now +[plugin.extra] +a = 1 + +[web] +port = 8081 +""" + + +def names(text): + return [b["name"] for b in tomllib.loads(text).get("plugin", [])] + + +def test_blocks_start_at_their_comments(): + found = config_file.blocks(CONFIG) + lines = CONFIG.splitlines() + assert [lines[b.start] for b in found] == ["# the clock in the kitchen", "# words"] + assert [lines[b.header] for b in found] == ["[[plugin]]", "[[plugin]]"] + # The second block holds its [plugin.extra] part, not [web]. + assert lines[found[1].own_end] == "[plugin.extra]" + assert lines[found[1].end] == "[web]" + + +def test_moving_a_block_takes_its_comments_and_parts_along(): + moved = config_file.move(CONFIG, 1, "Words", -1) + assert names(moved) == ["Words", "Clock"] + assert moved.index("# words") < moved.index('name = "Words"') < moved.index("[plugin.extra]") + assert moved.index("[plugin.extra]") < moved.index("# the clock in the kitchen") + assert tomllib.loads(moved)["plugin"][0]["extra"] == {"a": 1} + assert tomllib.loads(moved)["web"] == {"port": 8081} + # And back again: the same settings, nothing lost. + back = config_file.move(moved, 0, "Words", 1) + assert tomllib.loads(back) == tomllib.loads(CONFIG) + for comment in ["# the clock in the kitchen", "# refresh = 60", "# words", "# for now"]: + assert back.count(comment) == 1 + + +def test_a_block_cant_move_past_the_ends(): + with pytest.raises(EditError, match="can't move further"): + config_file.move(CONFIG, 0, "Clock", -1) + with pytest.raises(EditError, match="can't move further"): + config_file.move(CONFIG, 1, "Words", 1) + + +def test_removing_a_block_removes_its_comments(): + removed = config_file.remove(CONFIG, 0, "Clock") + assert names(removed) == ["Words"] + assert "kitchen" not in removed and "# refresh" not in removed + gone = config_file.remove(removed, 0, "Words") + assert "plugin" not in tomllib.loads(gone) + assert tomllib.loads(gone)["web"] == {"port": 8081} + + +def test_switching_off_and_on(): + off = config_file.set_enabled(CONFIG, 0, "Clock", False) + assert 'type = "basic_clock"\nenabled = false\n' in off + assert config_file.set_enabled(off, 0, "Clock", True) == CONFIG + # An "enabled" line written by hand is changed; switching on removes it (the default). + words_off = config_file.set_enabled(CONFIG, 1, "Words", False) + assert tomllib.loads(words_off)["plugin"][1]["enabled"] is False + assert ( + "enabled" + not in tomllib.loads(config_file.set_enabled(words_off, 1, "Words", True))["plugin"][1] + ) + + +def test_adding_a_block_after_the_last_plugin(): + block = example.plugin_block(plugins.load("dec_binary_clock"), "Dots", {}) + added = config_file.add(CONFIG, block) + assert names(added) == ["Clock", "Words", "Dots"] + assert added.index('name = "Dots"') < added.index("[web]") + assert tomllib.loads(added)["web"] == {"port": 8081} + # A file without plugins: at the end. + empty = 'config_version = 1\n[display]\ntype = "virtual"' + assert names(config_file.add(empty, block)) == ["Dots"] + + +def test_a_block_changed_meanwhile_is_not_touched(): + with pytest.raises(ChangedMeanwhile): + config_file.remove(CONFIG, 0, "Words") + with pytest.raises(ChangedMeanwhile): + config_file.move(CONFIG, 5, "Clock", 1) + + +def test_files_it_doesnt_understand_are_not_changed(): + # A [[plugin]] line inside a multi-line string is not a block... + text = CONFIG.replace('type = "virtual"', 'type = "virtual"\nnote = """\n[[plugin]]\n"""') + assert names(config_file.remove(text, 0, "Clock")) == ["Words"] + # ... and a header written in another way is not found, so nothing is changed. + quoted = CONFIG.replace('[[plugin]]\nname = "Words"', '[["plugin"]]\nname = "Words"') + with pytest.raises(EditError, match="change it by hand"): + config_file.move(quoted, 0, "Clock", 1) + with pytest.raises(EditError, match="not valid TOML"): + config_file.move("[[plugin]\n", 0, "Clock", 1) + + +def test_saving_keeps_the_permissions(tmp_path): + path = tmp_path / "paperpi.toml" + path.write_text(CONFIG) + path.chmod(0o640) + real, text = config_file.read(path) + config_file.write(real, config_file.remove(text, 0, "Clock")) + assert names(path.read_text()) == ["Words"] + assert path.stat().st_mode & 0o777 == 0o640 + with pytest.raises(EditError, match="file not found"): + config_file.read(tmp_path / "nothing.toml") From de1a04769d1445a902090944a607f2942db82f4b Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:14:37 +0200 Subject: [PATCH 4/6] Active Plugins and Plugin Library pages (M5 part 2b-2) Part of #238. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/decisions/web-interface.md | 10 + docs/writing-plugins.md | 2 +- src/paperpi/cli.py | 4 +- src/paperpi/config.py | 6 +- src/paperpi/plugin.py | 2 +- src/paperpi/web/app.py | 107 +++++++++- src/paperpi/web/plugins.py | 258 +++++++++++++++++++++++++ src/paperpi/web/server.py | 40 +++- src/paperpi/web/static/style.css | 7 + src/paperpi/web/templates/add.html | 24 +++ src/paperpi/web/templates/home.html | 6 +- src/paperpi/web/templates/library.html | 18 ++ src/paperpi/web/templates/plugins.html | 47 +++++ src/paperpi/web/templates/remove.html | 13 ++ tests/test_cli.py | 2 +- tests/test_web.py | 4 +- tests/test_web_plugins.py | 226 ++++++++++++++++++++++ 18 files changed, 755 insertions(+), 23 deletions(-) create mode 100644 src/paperpi/web/plugins.py create mode 100644 src/paperpi/web/templates/add.html create mode 100644 src/paperpi/web/templates/library.html create mode 100644 src/paperpi/web/templates/plugins.html create mode 100644 src/paperpi/web/templates/remove.html create mode 100644 tests/test_web_plugins.py diff --git a/README.md b/README.md index 3b7ef4e..968899a 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ uv run paperpi run --config paperpi.toml --out screen/ --state-dir state/ --heal ### The web interface -`paperpi run` also starts the web interface, on port 8080. Open it by the Pi's IP address on a phone or computer on the same home network, for example `http://192.168.1.20:8080` (your router's list of devices shows the Pi's address; on the Pi, `hostname -I` prints it). Names such as `paperpi.local` are refused for now, for safety (see [docs/decisions/web-interface.md](docs/decisions/web-interface.md)). For now it has the log-in and an empty home page; the pages for plugins and settings follow (issue #238). +`paperpi run` also starts the web interface, on port 8080. Open it by the Pi's IP address on a phone or computer on the same home network, for example `http://192.168.1.20:8080` (your router's list of devices shows the Pi's address; on the Pi, `hostname -I` prints it). Names such as `paperpi.local` are refused for now, for safety (see [docs/decisions/web-interface.md](docs/decisions/web-interface.md)). It has two pages for plugins: **Active Plugins** (switch plugins on or off, move them up or down, remove one) and **Plugin Library** (every plugin that comes with PaperPi; add one at the end of the list). A change there is saved in the config file and applies at once; comments in the file stay. A plugin that needs settings (such as `met_no`, which needs a place and your email address) is added switched off; until the settings page follows (issue #238), fill those in in the config file. - **First visit:** the first person to open it sets the web password (at least 8 characters). The browser then stays logged in for a year, until **Log out**. Log out only logs out that browser; a new password logs out every browser. - **Forgotten password:** you need access to the Pi itself (a keyboard and screen, or SSH). diff --git a/docs/decisions/web-interface.md b/docs/decisions/web-interface.md index 6c9ea22..b19edb6 100644 --- a/docs/decisions/web-interface.md +++ b/docs/decisions/web-interface.md @@ -19,6 +19,16 @@ In v2 the web interface is the main way to set up and change PaperPi: plugins, s - Pages are built on the Pi. htmx updates parts of a page, such as the screen preview and the list of warnings. - A page that needs dragging or resizing gets a small JavaScript library made for that, loaded as a plain file with no build step. The first case will be the dashboard editor (see "Later"). +### Plugin list pages + +*Added in M5 part 2b (issue #238), agreed with txoof on 2026-10-07.* +- **Active Plugins** lists every `[[plugin]]` block in the config file, in file order (the order the plugins take turns), also blocks with errors. Each has: Switch on / Switch off, **Up** and **Down** buttons (no dragging, so no extra JavaScript library; works the same on a phone), and Remove, which asks first. +- **Plugin Library** lists every plugin type that comes with PaperPi, with its one-line description, except `default` (PaperPi's own "nothing to show" message) and `debugging` (for testing PaperPi). Choosing one asks for a name (a free one is suggested, e.g. "Basic clock 2") and adds the block at the end of Active Plugins. Sample pictures follow in part 3b. +- A plugin with required settings (see `plugin-interface.md`) is added **switched off**, and the add page lists those settings with their help text (for `met_no`: a real email address, as met.no's terms ask). Active Plugins shows "Needs settings: ..." and can't switch it on until they are filled in. +- Each change is saved at once (the config file then holds the change and nothing else changed: comments stay, and the comments just above a `[[plugin]]` line move with its block) and PaperPi reloads the file, so it applies at once. +- **Hand edits not applied yet:** a change from the web interface is made to the file as it is on disk, so hand edits stay and apply together with it. The page then says "Your hand edits to the config file were applied too." (The other choice, refusing to save until the hand edits are applied, needs an extra step every time.) +- A change names its block by place and name. If the block was moved or renamed by hand since the page was shown, nothing is changed and the page says the file changed meanwhile. A file the web interface can't change safely (for example a `[[plugin]]` line written in an unusual way) is never saved wrongly: every change is read back and compared before saving. + ### Plugin settings forms - One page handles every plugin. It reads the plugin's settings description (see `plugin-interface.md`) and builds the form from it: number field, dropdown, password field for API keys, and so on. diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 1a86ee3..9e856a6 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -80,7 +80,7 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Every setting needs a default. - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. -- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None`, empty text or an empty `SecretStr` (text of only spaces also counts as not set). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check and `paperpi list` (and from M5 part 2b the web interface) say which settings are missing. The example block marks them "(required)". `setting` is also where later options for a single setting are added, such as a web interface helper that looks up latitude and longitude (M5 part 3c). +- A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None`, empty text or an empty `SecretStr` (text of only spaces also counts as not set). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface's Active Plugins page say which settings are missing. The example block marks them "(required)". `setting` is also where later options for a single setting are added, such as a web interface helper that looks up latitude and longitude (M5 part 3c). ```python from paperpi.plugin import PluginSettings, setting diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index 678b272..c4efb5e 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -283,7 +283,7 @@ def _run(args: argparse.Namespace) -> int: def load() -> config.Config: loaded = config.load(args.config, state_dir=args.state_dir) if web is not None and not loaded.from_last_good: - web.use(loaded.web) + web.use(loaded) return loaded try: @@ -325,7 +325,7 @@ def load() -> config.Config: # Imported here: the web packages take a moment to load, and no other command needs them. from .web import server - web = server.start(args.config, loaded.web) + web = server.start(args.config, loaded.web, reload=scheduler.reload, text=loaded.text) if web is not None: print(f"web interface on port {web.port}") try: diff --git a/src/paperpi/config.py b/src/paperpi/config.py index 9ee7674..d13702c 100644 --- a/src/paperpi/config.py +++ b/src/paperpi/config.py @@ -305,6 +305,8 @@ class Config: from_last_good: bool = False """True when the file was wrong and the last good copy is used instead.""" web: WebSettings = field(default_factory=WebSettings) + text: str = "" + """The file text it was read from (with ``from_last_good``: the last good copy's).""" @property def errors(self) -> list[Problem]: @@ -431,7 +433,9 @@ def parse(text: str, source: str = "config") -> Config: # E.g. lists nested thousands deep, or a number with thousands of digits. problem = f"not valid TOML: {type(error).__name__}: {str(error)[:200]}" raise ConfigError([Problem("error", problem, source)]) from None - return _Checker(text, source).check(data) + config = _Checker(text, source).check(data) + config.text = text + return config def _log_problems(problems: list[Problem]) -> None: diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index 8d68b9c..ca9f2bf 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -113,7 +113,7 @@ def setting(default: Any, *, required: bool = False, **field: Any) -> Any: ``required``: the plugin can't work until the user fills it in (a place, an email address, an API key). Its default must be "not set" (see :func:`is_set`). A plugin with a required setting that is not set is not shown; the config check and - ``paperpi list`` (and from M5 part 2b the web interface) say which settings it needs. + ``paperpi list`` and the web interface's Active Plugins page say which settings it needs. """ extra = field.pop("json_schema_extra", None) or {} if not isinstance(extra, dict): diff --git a/src/paperpi/web/app.py b/src/paperpi/web/app.py index f8e41d0..1f83774 100644 --- a/src/paperpi/web/app.py +++ b/src/paperpi/web/app.py @@ -7,6 +7,9 @@ ``/setup`` the first visitor sets the password (only while none is set) ``/login`` log in; also says how to reset a forgotten password ``/logout`` log out (a button on every page) +``/plugins`` Active Plugins: the plugins in the config file; switch on or off, move + up or down, remove (``/plugins//...``, n = place in the file) +``/library`` Plugin Library: every plugin type; ``/library/`` adds one ``/static/...`` the style sheet and htmx (a small JavaScript file that updates one part of a page without loading the whole page again) ================= ============================================================== @@ -23,9 +26,9 @@ import ipaddress from collections.abc import Awaitable, Callable from pathlib import Path -from urllib.parse import urlsplit +from urllib.parse import urlencode, urlsplit -from fastapi import FastAPI, Request +from fastapi import FastAPI, Form, Request from fastapi.responses import HTMLResponse, RedirectResponse, Response from fastapi.staticfiles import StaticFiles from fastapi.templating import Jinja2Templates @@ -33,7 +36,9 @@ from .. import __version__ from .auth import COOKIE_SECONDS, Auth, new_cookie +from .config_file import EditError from .password import PasswordError, password_problem +from .plugins import PluginEditor, library, library_item COOKIE = "paperpi_login" _HERE = Path(__file__).parent @@ -41,15 +46,18 @@ _OPEN = ("/setup", "/login") -def create_app(auth: Auth) -> FastAPI: - """The web interface, using ``auth`` for the password and log-in.""" +def create_app(auth: Auth, editor: PluginEditor | None = None) -> FastAPI: + """The web interface, using ``auth`` for the password and log-in and ``editor`` to + change the plugins in the config file.""" + editor = editor or PluginEditor(auth.config_file) app = FastAPI(title="PaperPi", docs_url=None, redoc_url=None, openapi_url=None) app.mount("/static", StaticFiles(directory=_HERE / "static"), name="static") templates = Jinja2Templates(directory=_HERE / "templates") templates.env.globals.update(version=__version__, auth=auth) + templates.env.filters["sentence"] = lambda text: text[:1].upper() + text[1:] - def page(request: Request, name: str, status: int = 200, **values) -> HTMLResponse: - return templates.TemplateResponse(request, name, values, status_code=status) + def page(request: Request, template: str, status: int = 200, **values) -> HTMLResponse: + return templates.TemplateResponse(request, template, values, status_code=status) def logged_in(request: Request, stored: str) -> RedirectResponse: response = RedirectResponse("/", status_code=303) @@ -137,6 +145,93 @@ async def login(request: Request): return logged_in(request, stored) return page(request, "login.html", 401, problem="Wrong password.") + def plugin_list(request: Request, status: int = 200, problem: str | None = None): + try: + found = editor.plugin_list() + except EditError as error: + problems = [str(error)] if str(error) != problem else [] + status = 500 if status == 200 else status + return page( + request, "plugins.html", status, rows=[], problems=problems, problem=problem + ) + return page( + request, + "plugins.html", + status, + rows=found.rows, + problems=found.problems, + problem=problem, + done=request.query_params.get("done"), + name=request.query_params.get("name"), + hand_edits=request.query_params.get("hand") == "1", + ) + + def saved(done: str, hand_edits: bool, name: str = "") -> RedirectResponse: + query = urlencode({"done": done, "name": name} | ({"hand": "1"} if hand_edits else {})) + return RedirectResponse(f"/plugins?{query}", status_code=303) + + @app.get("/plugins", response_class=HTMLResponse) + def plugins_page(request: Request): + return plugin_list(request) + + @app.post("/plugins/{index}/move", response_class=HTMLResponse) + def move(request: Request, index: int, name: str = Form(""), step: str = Form("")): + try: + hand_edits = editor.move(index, name, -1 if step == "up" else 1) + except EditError as error: + return plugin_list(request, 409, str(error)) + return saved("saved", hand_edits) + + @app.post("/plugins/{index}/enabled", response_class=HTMLResponse) + def switch(request: Request, index: int, name: str = Form(""), on: str = Form("")): + try: + hand_edits = editor.set_enabled(index, name, on == "1") + except EditError as error: + return plugin_list(request, 409, str(error)) + return saved("saved", hand_edits) + + @app.get("/plugins/{index}/remove", response_class=HTMLResponse) + def remove_page(request: Request, index: int, name: str = ""): + try: + row = editor.row(index, name) + except EditError as error: + return plugin_list(request, 409, str(error)) + return page(request, "remove.html", row=row) + + @app.post("/plugins/{index}/remove", response_class=HTMLResponse) + def remove(request: Request, index: int, name: str = Form("")): + try: + hand_edits = editor.remove(index, name) + except EditError as error: + return plugin_list(request, 409, str(error)) + return saved("removed", hand_edits, name) + + @app.get("/library", response_class=HTMLResponse) + def library_page(request: Request): + return page(request, "library.html", items=library()) + + def add_page(request: Request, plugin_type: str, status: int = 200, **values): + item = library_item(plugin_type) + if item is None: + return page(request, "library.html", 404, items=library(), problem="No such plugin.") + values.setdefault("name", editor.suggested_name(item)) + return page(request, "add.html", status, item=item, **values) + + @app.get("/library/{plugin_type}", response_class=HTMLResponse) + def add_form(request: Request, plugin_type: str): + return add_page(request, plugin_type) + + @app.post("/library/{plugin_type}", response_class=HTMLResponse) + def add(request: Request, plugin_type: str, name: str = Form("")): + item = library_item(plugin_type) + if item is None: + return add_page(request, plugin_type) + try: + hand_edits = editor.add(item, name) + except EditError as error: + return add_page(request, plugin_type, 400, name=name, problem=str(error)) + return saved("added", hand_edits, name.strip()) + @app.post("/logout") def logout(): response = RedirectResponse("/login", status_code=303) diff --git a/src/paperpi/web/plugins.py b/src/paperpi/web/plugins.py new file mode 100644 index 0000000..5855bb5 --- /dev/null +++ b/src/paperpi/web/plugins.py @@ -0,0 +1,258 @@ +"""What the Active Plugins and Plugin Library pages show, and the changes they make. + +- :class:`PluginEditor` reads the plugin list from the config file and makes one change at a + time (:mod:`paperpi.web.config_file`). After each change it asks PaperPi to load the file + again, so the change applies at once. +- A change is made to the file as it is on disk now, so hand edits that were not applied + yet stay in it and apply together with the change (agreed with txoof, M5 part 2b). The + page then says so. +""" + +from __future__ import annotations + +import threading +from collections.abc import Callable +from dataclasses import dataclass, field +from pathlib import Path + +from pydantic import ValidationError + +from .. import config, example, plugins +from ..plugin import Plugin, PluginEntry +from . import config_file + +#: Plugin types the Plugin Library doesn't offer: ``default`` is PaperPi's own message when +#: nothing else can be shown, and ``debugging`` is for testing PaperPi. +HIDDEN = ("default", "debugging") + + +@dataclass(frozen=True) +class Row: + """One ``[[plugin]]`` block, as the Active Plugins page shows it.""" + + index: int + """Its place in the file, from 0.""" + name: str + """``""`` when the block has no proper name.""" + type: str + enabled: bool + missing: tuple[str, ...] = () + """Required settings that are not set yet; the plugin can't be switched on until they are.""" + errors: tuple[str, ...] = () + """What is wrong with the block; PaperPi leaves it out until it is fixed.""" + + @property + def status(self) -> str: + if self.errors: + return "Not used: has errors" + if self.missing: + return "Needs settings: " + ", ".join(self.missing) + return "On" if self.enabled else "Off" + + +@dataclass(frozen=True) +class PluginList: + rows: list[Row] + problems: list[str] = field(default_factory=list) + """Problems with the whole file (no plugin can be used until they are fixed).""" + + +@dataclass(frozen=True) +class LibraryItem: + """One plugin type in the Plugin Library.""" + + type: str + description: str + plugin: Plugin | None = None + """``None`` when the plugin itself is broken (``description`` says why).""" + + @property + def required(self) -> list[tuple[str, str]]: + """The settings the user has to fill in, with their help text.""" + if self.plugin is None: + return [] + fields = self.plugin.settings.model_fields + return [(key, fields[key].description or "") for key in self.plugin.required] + + +class PluginEditor: + """Reads and changes the plugin blocks of one config file.""" + + def __init__(self, path: Path, reload: Callable[[], None] | None = None): + self.path = Path(path) + self._reload = reload + self._lock = threading.Lock() + self._applied: str | None = None + """The file text PaperPi uses now (``None``: not known).""" + self._written: str | None = None + """The text the web interface saved last (it may not be applied yet).""" + + def loaded(self, text: str) -> None: + """PaperPi loaded the config file ``text`` (at the start or a reload).""" + self._applied = text + + def plugin_list(self) -> PluginList: + """The plugin blocks in the file now. Raises :class:`config_file.EditError` when the + file can't be read at all.""" + path, text = config_file.read(self.path) + data = config_file.load(text) + found = config_file.blocks(text) + problems: list[config.Problem] = [] + checked: dict[str, config.PluginConfig] = {} + file_problems = [] + try: + loaded = config.parse(text, path.name) + except config.ConfigError as error: + problems = error.problems + file_problems = [str(p) for p in error.problems if p.level == "error"] + else: + problems = loaded.problems + checked = {p.entry.name: p for p in loaded.plugins} + rows = [] + for index, block in enumerate(config_file.plugin_blocks(data)): + block = block if isinstance(block, dict) else {} + name = config_file.name_of(block) + plugin_type = block.get("type") + enabled = block.get("enabled", True) + errors = () + if index < len(found) and not file_problems: + place = found[index] + errors = tuple( + _without_place(p) + for p in problems + if p.level == "error" and p.line and place.start < p.line <= place.end + ) + good = checked.get(name) + rows.append( + Row( + index=index, + name=name, + type=plugin_type if isinstance(plugin_type, str) else "", + enabled=enabled if isinstance(enabled, bool) else True, + missing=good.missing if good is not None else (), + errors=errors if good is None else (), + ) + ) + if len(found) != len(rows): + file_problems.append(config_file._UNKNOWN_FORM) + return PluginList(rows, file_problems) + + def change(self, edit: Callable[[str], str]) -> bool: + """Change the file with ``edit`` (old text -> new text), save it and apply it. + + Returns True when the file had hand edits that were not applied yet; they apply + too. Raises :class:`config_file.EditError` when the change can't be made. + """ + with self._lock: + path, text = config_file.read(self.path) + hand_edits = self._applied is not None and text not in (self._applied, self._written) + new = edit(text) + config_file.write(path, new) + self._written = new + if self._reload is not None: + self._reload() + return hand_edits + + def move(self, index: int, name: str, step: int) -> bool: + return self.change(lambda text: config_file.move(text, index, name, step)) + + def set_enabled(self, index: int, name: str, enabled: bool) -> bool: + if enabled: + row = self.row(index, name) + if row.missing: + missing = ", ".join(row.missing) + raise config_file.EditError( + f"{name} can't be switched on yet: fill in these settings first: {missing}" + ) + return self.change(lambda text: config_file.set_enabled(text, index, name, enabled)) + + def remove(self, index: int, name: str) -> bool: + return self.change(lambda text: config_file.remove(text, index, name)) + + def row(self, index: int, name: str) -> Row: + """The block at ``index``, if it is still called ``name``.""" + rows = self.plugin_list().rows + if not 0 <= index < len(rows) or rows[index].name != name: + raise config_file.ChangedMeanwhile(config_file._MEANWHILE) + return rows[index] + + def add(self, item: LibraryItem, name: str) -> bool: + """Add a block for ``item`` called ``name`` at the end of the plugin list. A plugin + with required settings is added switched off. Raises + :class:`config_file.EditError` for a name that can't be used.""" + if item.plugin is None: + raise config_file.EditError(f"{item.type} can't be added: {item.description}") + name = name.strip() + problem = name_problem(name) + if problem: + raise config_file.EditError(problem) + values = {"enabled": False} if item.plugin.required else {} + block = example.plugin_block(item.plugin, name, values) + + def add(text: str) -> str: + used = _used_names(text) + if config.folder_name(name) in used: + raise config_file.EditError( + f"the name {name!r} is already used by another plugin (also ignoring " + "capitals and punctuation); choose another one" + ) + return config_file.add(text, block) + + return self.change(add) + + def suggested_name(self, item: LibraryItem) -> str: + """A name for a new block of ``item`` that is not used yet: "Basic clock", + "Basic clock 2", ...""" + try: + used = _used_names(config_file.read(self.path)[1]) + except config_file.EditError: + used = set() + base = item.type.replace("_", " ").capitalize() + name, number = base, 1 + while config.folder_name(name) in used: + number += 1 + name = f"{base} {number}" + return name + + +def library() -> list[LibraryItem]: + """The plugin types the Plugin Library offers, sorted by type.""" + items = [] + for plugin_type in plugins.available(): + if plugin_type in HIDDEN: + continue + try: + plugin = plugins.load(plugin_type) + except Exception as error: # noqa: BLE001 - a broken plugin is shown as broken + items.append(LibraryItem(plugin_type, f"This plugin is broken: {error}")) + else: + items.append(LibraryItem(plugin_type, plugin.description, plugin)) + return items + + +def library_item(plugin_type: str) -> LibraryItem | None: + return next((item for item in library() if item.type == plugin_type), None) + + +def name_problem(name: str) -> str | None: + """Why ``name`` can't be a plugin's name, or ``None`` when it can.""" + try: + PluginEntry.model_validate({"name": name, "type": "x"}) + except ValidationError as error: + found = error.errors()[0] + if found["type"] == "string_too_short": + return "Give the plugin a name." + if found["type"] == "string_too_long": + return f"The name can have at most {found['ctx']['max_length']} characters." + return f"The name {found['msg'].removeprefix('Value error, ')}." + return None + + +def _used_names(text: str) -> set[str]: + blocks = config_file.plugin_blocks(config_file.load(text)) + return {config.folder_name(n) for n in map(config_file.name_of, blocks) if n} + + +def _without_place(problem: config.Problem) -> str: + """The problem's message with its line number, without the file name.""" + return f"line {problem.line}: {problem.message}" if problem.line else problem.message diff --git a/src/paperpi/web/server.py b/src/paperpi/web/server.py index 20d0371..07f9490 100644 --- a/src/paperpi/web/server.py +++ b/src/paperpi/web/server.py @@ -10,6 +10,7 @@ import logging import socket import threading +from collections.abc import Callable from pathlib import Path import uvicorn @@ -17,6 +18,7 @@ from .. import config from .app import create_app from .auth import Auth +from .plugins import PluginEditor log = logging.getLogger(__name__) @@ -27,8 +29,15 @@ class WebServer: """The web interface of one ``paperpi run``. Start it with :func:`start`.""" - def __init__(self, auth: Auth, settings: config.WebSettings, sock: socket.socket): + def __init__( + self, + auth: Auth, + settings: config.WebSettings, + sock: socket.socket, + editor: PluginEditor | None = None, + ): self.auth = auth + self.editor = editor or PluginEditor(auth.config_file) self.settings = settings """The settings it was started with; a change of address or port needs a restart.""" self._socket = sock @@ -36,7 +45,7 @@ def __init__(self, auth: Auth, settings: config.WebSettings, sock: socket.socket """The port it listens on (useful with ``port = 0`` in tests).""" self._server = uvicorn.Server( uvicorn.Config( - create_app(auth), + create_app(auth, self.editor), log_config=None, # PaperPi's own logging stays as it is log_level="warning", access_log=False, @@ -53,10 +62,12 @@ def _serve(self) -> None: except BaseException: # noqa: BLE001 - also SystemExit, which uvicorn uses for errors log.exception("the web interface stopped because of an error") - def use(self, settings: config.WebSettings) -> None: - """Apply reloaded ``[web]`` settings: log-in changes apply at once, the rest at the - next start.""" + def use(self, loaded: config.Config) -> None: + """Apply a reloaded config: ``[web]`` log-in changes apply at once, the rest of + ``[web]`` at the next start.""" + settings = loaded.web self.auth.use(settings) + self.editor.loaded(loaded.text) changed = [ k for k in config.WEB_NEXT_START if getattr(settings, k) != getattr(self.settings, k) ] @@ -75,8 +86,18 @@ def stop(self) -> None: self._socket.close() -def start(config_file: Path, settings: config.WebSettings) -> WebServer | None: - """Start the web interface, or log why it can't start and return ``None``.""" +def start( + config_file: Path, + settings: config.WebSettings, + *, + reload: Callable[[], None] | None = None, + text: str | None = None, +) -> WebServer | None: + """Start the web interface, or log why it can't start and return ``None``. + + ``reload`` makes PaperPi load the config file again (after a change in the web + interface); ``text`` is the config file text PaperPi uses now. + """ try: sock = _listen(settings.address, settings.port) except OSError as error: @@ -87,7 +108,10 @@ def start(config_file: Path, settings: config.WebSettings) -> WebServer | None: error, ) return None - server = WebServer(Auth(config_file, settings), settings, sock) + editor = PluginEditor(config_file, reload) + if text is not None: + editor.loaded(text) + server = WebServer(Auth(config_file, settings), settings, sock, editor) server._thread.start() return server diff --git a/src/paperpi/web/static/style.css b/src/paperpi/web/static/style.css index d4d2d42..56d7036 100644 --- a/src/paperpi/web/static/style.css +++ b/src/paperpi/web/static/style.css @@ -15,3 +15,10 @@ button { padding: 0.4rem 0.9rem; font-size: 1rem; } .note { padding: 0.5rem; border-left: 4px solid #a60; background: #fff6e5; } details { margin-top: 2rem; } code { background: #eee; padding: 0 0.2rem; } +.saved { padding: 0.5rem; border-left: 4px solid #060; background: #eef8ee; } +.plugins { padding-left: 1.5rem; } +.plugins li { margin: 0.75rem 0; padding-bottom: 0.75rem; border-bottom: 1px solid #ddd; } +.plugins .type, .plugins .hint { color: #555; font-size: 0.9rem; } +.buttons { display: flex; flex-wrap: wrap; gap: 0.5rem; align-items: center; margin-top: 0.4rem; } +.buttons form { margin: 0; display: flex; gap: 0.5rem; } +.library li { margin: 0.5rem 0; } diff --git a/src/paperpi/web/templates/add.html b/src/paperpi/web/templates/add.html new file mode 100644 index 0000000..bc6a5e4 --- /dev/null +++ b/src/paperpi/web/templates/add.html @@ -0,0 +1,24 @@ +{% extends "base.html" %} +{% set logged_in = true %} +{% block title %}Add {{ item.type }} · PaperPi{% endblock %} +{% block main %} +

Add {{ item.type }}

+

{{ item.description }}

+{% if problem %}

{{ problem|sentence }}

{% endif %} +{% if item.required %} +
+

This plugin is not shown until these settings are filled in, so it is added switched off:

+
    + {% for key, help in item.required %}
  • {{ key }}: {{ help }}
    • {% endfor %} +
+

Until the settings page is ready (a later version), fill them in in the config file, + then switch the plugin on in Active Plugins.

+
+{% endif %} +
+ + + Cancel +
+{% endblock %} diff --git a/src/paperpi/web/templates/home.html b/src/paperpi/web/templates/home.html index e811239..dcd1e62 100644 --- a/src/paperpi/web/templates/home.html +++ b/src/paperpi/web/templates/home.html @@ -2,7 +2,11 @@ {% set logged_in = true %} {% block main %}

PaperPi

-

PaperPi is running. The pages for plugins and settings are coming in the next versions.

+

PaperPi is running.

+ {% if not auth.login %}

Log-in is switched off (login = false in [web] of the config file): anyone on your home network can change PaperPi's settings.

diff --git a/src/paperpi/web/templates/library.html b/src/paperpi/web/templates/library.html new file mode 100644 index 0000000..b8492dc --- /dev/null +++ b/src/paperpi/web/templates/library.html @@ -0,0 +1,18 @@ +{% extends "base.html" %} +{% set logged_in = true %} +{% block title %}Plugin Library · PaperPi{% endblock %} +{% block main %} +

Plugin Library

+{% if problem %}

{{ problem }}

{% endif %} +

Every plugin that comes with PaperPi. Choose one to add it at the end of +Active Plugins. The same plugin can be added more than once, with +different settings (e.g. the weather for two places).

+
    +{% for item in items %} +
  • + {{ item.type }}: {{ item.description }} + {% if item.plugin %}Add{% endif %} +
    • +{% endfor %} +
+{% endblock %} diff --git a/src/paperpi/web/templates/plugins.html b/src/paperpi/web/templates/plugins.html new file mode 100644 index 0000000..4f2a613 --- /dev/null +++ b/src/paperpi/web/templates/plugins.html @@ -0,0 +1,47 @@ +{% extends "base.html" %} +{% set logged_in = true %} +{% block title %}Active Plugins · PaperPi{% endblock %} +{% block main %} +

Active Plugins

+{% if done %} +

+ {%- if done == "added" %}Added {{ name }}.{% elif done == "removed" %}Removed {{ name }}. + {%- else %}Saved.{% endif %} PaperPi applies the change now. + {%- if hand_edits %} Your hand edits to the config file were applied too.{% endif %}

+{% endif %} +{% if problem %}

{{ problem|sentence }}

{% endif %} +{% for found in problems %}

{{ found|sentence }}

{% endfor %} +

The plugins that are switched on take turns on the screen, in this order. +Add a plugin from the Plugin Library.

+{% if not rows %}

No plugins yet.

{% endif %} +
    +{% for row in rows %} +
  1. +
    {{ row.name or "(no name)" }} {{ row.type }}
    +
    {{ row.status }}
    + {% for error in row.errors %}
    {{ error }}
    {% endfor %} + {% if row.missing %} +
    Until the settings page is ready (a later version), fill these in in the + config file.
    + {% endif %} +
    +
    + + {% if row.enabled %} + + {% else %} + + + {% endif %} +
    +
    + + + +
    + Remove +
    +
    2. +{% endfor %} +
+{% endblock %} diff --git a/src/paperpi/web/templates/remove.html b/src/paperpi/web/templates/remove.html new file mode 100644 index 0000000..31d0e04 --- /dev/null +++ b/src/paperpi/web/templates/remove.html @@ -0,0 +1,13 @@ +{% extends "base.html" %} +{% set logged_in = true %} +{% block title %}Remove {{ row.name }} · PaperPi{% endblock %} +{% block main %} +

Remove {{ row.name or "(no name)" }}?

+

This removes the {{ row.type }} plugin {{ row.name or "(no name)" }} +and all its settings from the config file. To only stop showing it, switch it off instead.

+
+ + + Cancel +
+{% endblock %} diff --git a/tests/test_cli.py b/tests/test_cli.py index 74ae37b..c2bf434 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -537,7 +537,7 @@ def test_run_starts_the_web_interface_unless_asked_not_to( from paperpi.web import server calls = [] - monkeypatch.setattr(server, "start", lambda *a: calls.append(a)) + monkeypatch.setattr(server, "start", lambda *a, **kw: calls.append(a)) monkeypatch.setattr(cli.Scheduler, "run", lambda self: None) cfg = tmp_path / "paperpi.toml" cfg.write_text(f'config_version = 1\n[display]\ntype = "virtual"\n{web}') diff --git a/tests/test_web.py b/tests/test_web.py index dd8566a..9efd2be 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1,6 +1,7 @@ import logging import socket import urllib.request +from dataclasses import replace import pytest from fastapi.testclient import TestClient @@ -202,7 +203,8 @@ def test_server_runs_in_a_thread(cfg, caplog): assert page.url.endswith("/setup") # A reload with a new port: it applies at the next start. with caplog.at_level(logging.WARNING): - web.use(config.WebSettings(port=9000, login=False)) + loaded = config.parse(cfg.read_text()) + web.use(replace(loaded, web=config.WebSettings(port=9000, login=False))) assert "[web] address, port: changes apply at the next start" in caplog.text assert not web.auth.login # log-in changes apply at once finally: diff --git a/tests/test_web_plugins.py b/tests/test_web_plugins.py new file mode 100644 index 0000000..ffb8593 --- /dev/null +++ b/tests/test_web_plugins.py @@ -0,0 +1,226 @@ +import tomllib +import urllib.request + +import pytest +from fastapi.testclient import TestClient + +from paperpi import config +from paperpi.web import auth +from paperpi.web.app import create_app +from paperpi.web.plugins import PluginEditor + +CONFIG = """\ +config_version = 1 +[display] +type = "virtual" + +# the clock in the kitchen +[[plugin]] +name = "Clock" +type = "basic_clock" + +[[plugin]] +name = "Weather" +type = "met_no" +lat = 52.52 +lon = 13.40 +enabled = false + +[[plugin]] +name = "Broken" +type = "basic_clock" +refresh = 1 +""" + +# The test client opens the pages as a phone on the home network would: by IP address. +PI = "http://192.168.1.20:8080" + + +@pytest.fixture +def cfg(tmp_path): + path = tmp_path / "paperpi.toml" + path.write_text(CONFIG) + return path + + +@pytest.fixture +def reloads(): + return [] + + +@pytest.fixture +def editor(cfg, reloads): + found = PluginEditor(cfg, lambda: reloads.append(1)) + found.loaded(CONFIG) + return found + + +@pytest.fixture +def client(cfg, editor): + paperpi_auth = auth.Auth(cfg, config.WebSettings(login=False)) + return TestClient(create_app(paperpi_auth, editor), follow_redirects=False, base_url=PI) + + +def blocks(cfg): + return tomllib.loads(cfg.read_text()).get("plugin", []) + + +def names(cfg): + return [b["name"] for b in blocks(cfg)] + + +def test_active_plugins_shows_every_block_with_its_state(client): + page = client.get("/plugins") + assert page.status_code == 200 + text = page.text + assert text.index("Clock") < text.index("Weather") < text.index("Broken") + assert "Needs settings: email" in text + assert "Not used: has errors" in text and "refresh" in text + assert text.count("Switch off") == 2 # Clock and Broken; Weather is off + # Weather can't be switched on until its email is filled in. + assert '' in text + assert 'value="up" disabled' in text and 'value="down" disabled' in text + + +def test_moving_a_plugin_saves_and_applies_at_once(client, cfg, reloads): + response = client.post("/plugins/0/move", data={"name": "Clock", "step": "down"}) + assert response.status_code == 303 + assert response.headers["location"].startswith("/plugins?done=saved") + assert names(cfg) == ["Weather", "Clock", "Broken"] + assert '# the clock in the kitchen\n[[plugin]]\nname = "Clock"' in cfg.read_text() + assert reloads == [1] + page = client.get(response.headers["location"]) + assert "Saved. PaperPi applies the change now." in page.text + assert "hand edits" not in page.text + client.post("/plugins/1/move", data={"name": "Clock", "step": "up"}) + assert names(cfg) == ["Clock", "Weather", "Broken"] + + +def test_switching_off_and_on(client, cfg): + client.post("/plugins/0/enabled", data={"name": "Clock", "on": "0"}) + assert blocks(cfg)[0]["enabled"] is False + client.post("/plugins/0/enabled", data={"name": "Clock", "on": "1"}) + assert "enabled" not in blocks(cfg)[0] + + +def test_a_plugin_with_missing_settings_cant_be_switched_on(client, cfg, reloads): + response = client.post("/plugins/1/enabled", data={"name": "Weather", "on": "1"}) + assert response.status_code == 409 + assert "fill in these settings first: email" in response.text + assert cfg.read_text() == CONFIG and not reloads + + +def test_removing_asks_first(client, cfg): + page = client.get("/plugins/0/remove", params={"name": "Clock"}) + assert "Remove Clock?" in page.text and cfg.read_text() == CONFIG + response = client.post("/plugins/0/remove", data={"name": "Clock"}) + assert response.headers["location"] == "/plugins?done=removed&name=Clock" + assert names(cfg) == ["Weather", "Broken"] + assert "kitchen" not in cfg.read_text() + assert "Removed Clock." in client.get(response.headers["location"]).text + + +def test_a_list_changed_by_hand_meanwhile_is_not_changed(client, cfg, reloads): + # The page showed Clock first, but it was removed by hand since then. + cfg.write_text(CONFIG.replace('name = "Clock"', 'name = "Kitchen"')) + response = client.post("/plugins/0/remove", data={"name": "Clock"}) + assert response.status_code == 409 and "changed meanwhile" in response.text + assert names(cfg) == ["Kitchen", "Weather", "Broken"] and not reloads + + +def test_hand_edits_are_kept_and_the_page_says_so(client, cfg, editor): + cfg.write_text(CONFIG + '\n[[plugin]]\nname = "Words"\ntype = "word_clock"\n') + response = client.post("/plugins/0/move", data={"name": "Clock", "step": "down"}) + assert "hand=1" in response.headers["location"] + assert names(cfg) == ["Weather", "Clock", "Broken", "Words"] + page = client.get(response.headers["location"]) + assert "Your hand edits to the config file were applied too." in page.text + # The next change, before PaperPi reloaded: the file holds only the web's own change. + response = client.post("/plugins/1/move", data={"name": "Clock", "step": "up"}) + assert "hand=1" not in response.headers["location"] + + +def test_the_library_lists_plugins_to_add(client): + page = client.get("/library").text + assert "basic_clock" in page and "met_no" in page + assert "debugging" not in page and "default" not in page + assert "/library/basic_clock" in page + + +def test_adding_a_plugin(client, cfg, reloads): + form = client.get("/library/word_clock").text + assert 'value="Word clock"' in form # a name that is not used yet + response = client.post("/library/word_clock", data={"name": " Words "}) + assert response.headers["location"] == "/plugins?done=added&name=Words" + assert blocks(cfg)[-1] == {"name": "Words", "type": "word_clock"} + assert reloads == [1] + # The next one gets another name. + client.post("/library/word_clock", data={"name": "Word clock"}) + assert 'value="Word clock 2"' in client.get("/library/word_clock").text + + +def test_a_plugin_with_required_settings_is_added_switched_off(client, cfg): + form = client.get("/library/met_no").text + assert "real email address" in form and "added switched off" in form + client.post("/library/met_no", data={"name": "Weather Rio"}) + assert blocks(cfg)[-1] == {"name": "Weather Rio", "type": "met_no", "enabled": False} + assert config.parse(cfg.read_text()).plugin("Weather Rio").missing == ("lat", "lon", "email") + + +@pytest.mark.parametrize( + ("name", "problem"), + [ + ("", "Give the plugin a name."), + ("x" * 101, "at most 100 characters"), + ("a\tb", "must not hold control characters"), + ("clock!", "already used by another plugin"), + ], +) +def test_names_that_cant_be_used(client, cfg, name, problem): + response = client.post("/library/word_clock", data={"name": name}) + assert response.status_code == 400 and problem in response.text + assert cfg.read_text() == CONFIG + + +def test_unknown_plugin_types(client): + assert client.get("/library/nothing").status_code == 404 + assert client.post("/library/debugging", data={"name": "x"}).status_code == 404 + + +def test_a_broken_file_is_shown_not_changed(client, cfg): + cfg.write_text("[[plugin]\n") + page = client.get("/plugins") + assert page.status_code == 500 and "The config file is not valid TOML" in page.text + assert client.post("/plugins/0/move", data={"name": "", "step": "up"}).status_code == 409 + assert cfg.read_text() == "[[plugin]\n" + + +def test_the_plugin_pages_need_a_log_in(cfg): + paperpi_auth = auth.Auth(cfg, config.WebSettings()) + client = TestClient(create_app(paperpi_auth), follow_redirects=False, base_url=PI) + for path in ["/plugins", "/library", "/library/basic_clock", "/plugins/0/remove"]: + assert client.get(path).headers["location"] == "/setup" + assert client.post("/plugins/0/remove", data={"name": "Clock"}).status_code == 303 + assert cfg.read_text() == CONFIG + + +def test_a_change_in_the_running_web_interface_reloads_paperpi(cfg, reloads): + from paperpi.web import server + + # Port 0: any free port (not allowed in the config file, so made without its checks). + settings = config.WebSettings.model_construct( + **(config.WebSettings().model_dump() | {"address": "127.0.0.1", "port": 0, "login": False}) + ) + web = server.start(cfg, settings, reload=lambda: reloads.append(1), text=CONFIG) + assert web is not None + try: + request = urllib.request.Request( + f"http://127.0.0.1:{web.port}/plugins/0/enabled", + data=b"name=Clock&on=0", + headers={"Origin": f"http://127.0.0.1:{web.port}"}, + ) + with urllib.request.urlopen(request, timeout=30) as page: + assert "/plugins?done=saved" in page.url + finally: + web.stop() + assert reloads == [1] and blocks(cfg)[0]["enabled"] is False From 0f5eae66c198ad9b7a154659363dca5b6c9ec8d2 Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:22:14 +0200 Subject: [PATCH 5/6] Review fixes: line breaks, Windows line endings, size limit, shared lock Co-Authored-By: Claude Opus 5.5 --- docs/decisions/config-format.md | 2 +- src/paperpi/config.py | 4 +- src/paperpi/plugin.py | 3 +- src/paperpi/web/config_file.py | 207 +++++++++++++++++++------------- src/paperpi/web/password.py | 5 + tests/test_example.py | 4 + tests/test_web_config_file.py | 83 ++++++++++++- 7 files changed, 223 insertions(+), 85 deletions(-) diff --git a/docs/decisions/config-format.md b/docs/decisions/config-format.md index f6bcc6a..535b500 100644 --- a/docs/decisions/config-format.md +++ b/docs/decisions/config-format.md @@ -92,7 +92,7 @@ The web interface only accepts valid values, so most of these mistakes can only ### Writing the file - The web interface keeps comments that were written by hand. - *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. Single settings (such as the password) are still written with tomlkit. + *Update (M5 part 2b, issue #238):* `[[plugin]]` blocks are moved, removed, switched on or off and added by changing the file's text one block at a time (`paperpi.web.config_file`), not with tomlkit: tomlkit counts the comment lines just above a `[[plugin]]` line as part of the block before it, so they would move with the wrong plugin. Here they belong to the block below them (when a blank line is above them), and they are removed with it; a comment on the same line as `enabled =` is lost when the plugin is switched on or off. Every change is read back and compared before it is saved; a file written in a way this code doesn't understand is not changed, and the web interface says to change it by hand. A change that would make the file larger than PaperPi reads is refused. Single settings (such as the password) are still written with tomlkit. - The file is written safely: first to a temporary file, then that file replaces the old one in one step. A power cut during a save can't leave a half-written config. ### Secrets diff --git a/src/paperpi/config.py b/src/paperpi/config.py index 35af6fb..9ee7674 100644 --- a/src/paperpi/config.py +++ b/src/paperpi/config.py @@ -800,7 +800,9 @@ def _key_lines(text: str) -> dict[tuple, int]: counts: dict[str, int] = {} section: tuple = () quote = None # inside a multi-line string: the quotes it started with (""" or ''') - for number, line in enumerate(text.splitlines(), start=1): + # Split at \n only, like TOML: splitlines() also splits at U+2028 and others, which TOML + # allows inside strings and comments. + for number, line in enumerate(text.split("\n"), start=1): starts_inside = quote is not None for found in re.finditer(r'"""|\'\'\'', line): if quote is None: diff --git a/src/paperpi/plugin.py b/src/paperpi/plugin.py index d17589f..8d68b9c 100644 --- a/src/paperpi/plugin.py +++ b/src/paperpi/plugin.py @@ -212,7 +212,8 @@ class PluginEntry(BaseModel): def _no_control_characters(cls, name: str) -> str: # They can't be written back to the file reliably, and would change the terminal's # output in ``paperpi list``. - if re.search(r"[\x00-\x1f\x7f]", name): + # Also the characters Python counts as line breaks (U+0085, U+2028, U+2029). + if re.search(r"[\x00-\x1f\x7f-\x9f\u2028\u2029]", name): raise ValueError("must not hold control characters (such as tab or new line)") return name diff --git a/src/paperpi/web/config_file.py b/src/paperpi/web/config_file.py index 7f6c287..0223cec 100644 --- a/src/paperpi/web/config_file.py +++ b/src/paperpi/web/config_file.py @@ -1,13 +1,19 @@ """Changing the config file from the web interface, keeping everything else in it as it is. - :func:`read` and :func:`write` read the file and save a changed copy that keeps the old - file's permissions (and owner, when run as root). + file's permissions (and owner, when run as root). Hold :data:`LOCK` from reading to + saving, so two changes at the same moment (a plugin and the password) can't undo each + other. :func:`saved_here` tells whether a text is the one the web interface saved last. - The plugin changes (:func:`move`, :func:`remove`, :func:`set_enabled`, :func:`add`) work on the file's text, one ``[[plugin]]`` block at a time. The comment lines just above a - ``[[plugin]]`` line belong to that block and move with it. + ``[[plugin]]`` line (with a blank line above them) belong to that block: they move with it + and are removed with it. A comment on the same line as ``enabled =`` is lost when the + plugin is switched on or off. - Every change is checked before it is saved: PaperPi must read the new text back as exactly the old settings plus the change. A file written in a way these functions don't - understand is never saved wrongly; :class:`EditError` says so instead. + understand (for example a ``[[plugin]]`` line written differently, or a value line + that looks like a ``[part]`` line) is never saved wrongly: :class:`UnknownForm` says to + change it by hand instead. - Each change names its block by place and name, so a block that was moved or removed by hand a moment ago is not changed by mistake (:class:`ChangedMeanwhile`). """ @@ -17,14 +23,19 @@ import os import re import stat +import threading import tomllib from dataclasses import dataclass from pathlib import Path from typing import Any -from .. import config +from .. import config, limits from ..files import write_atomic +#: Held from reading the config file to saving the changed copy. +LOCK = threading.Lock() +_saved: dict[Path, str] = {} + class EditError(ValueError): """The config file can't be read, changed or saved; the message says why.""" @@ -33,6 +44,21 @@ class EditError(ValueError): class ChangedMeanwhile(EditError): """The block to change is not where the page showed it: the file changed meanwhile.""" + def __init__(self) -> None: + super().__init__( + "The config file changed after this page was opened. Check the list and try again." + ) + + +class UnknownForm(EditError): + """The file is written in a way these functions can't change safely.""" + + def __init__(self) -> None: + super().__init__( + "The plugin list in the config file is written in a way the web interface can't " + "change safely. Make this change in the config file itself." + ) + def read(path: Path) -> tuple[Path, str]: """The real path of the config file (links followed) and its text.""" @@ -46,14 +72,26 @@ def read(path: Path) -> tuple[Path, str]: def write(path: Path, text: str) -> None: """Save ``text`` as the config file at ``path`` (a real path, from :func:`read`).""" + data = text.encode("utf-8") + if len(data) > limits.CONFIG_FILE_BYTES: + raise EditError( + f"The config file would be larger than {limits.CONFIG_FILE_BYTES} bytes, which " + "PaperPi can't read. Remove a plugin first." + ) try: # The new file keeps the old one's permissions and, when run with sudo, its owner, # so the PaperPi service can still read it. info = os.stat(path) owner = (info.st_uid, info.st_gid) if os.geteuid() == 0 else None - write_atomic(path, text.encode("utf-8"), mode=stat.S_IMODE(info.st_mode), owner=owner) + write_atomic(path, data, mode=stat.S_IMODE(info.st_mode), owner=owner) except OSError as error: raise EditError(f"can't save {path}: {error}") from None + _saved[path] = text + + +def saved_here(path: Path, text: str) -> bool: + """True when ``text`` is what the web interface saved last as the config file ``path``.""" + return _saved.get(Path(os.path.realpath(path))) == text def load(text: str) -> dict[str, Any]: @@ -79,15 +117,28 @@ class Block: _HEADER = re.compile(r"\s*(\[\[?)\s*([A-Za-z0-9_.-]+)\s*\]\]?\s*(#.*)?") -_ENABLED = re.compile(r"\s*enabled\s*=") -def blocks(text: str) -> list[Block]: - """The ``[[plugin]]`` blocks in ``text``, in file order.""" - lines = text.splitlines() +@dataclass(frozen=True) +class _Scan: + lines: list[str] + """The lines, each with its line break (TOML only breaks lines at ``\\n``).""" + in_string: frozenset[int] + """Lines that start inside a multi-line string.""" + blocks: list[Block] + + +def _scan(text: str) -> _Scan: + lines = re.split(r"(?<=\n)", text) + if lines and lines[-1] == "": + lines.pop() + plain = [line.rstrip("\r\n") for line in lines] starts: list[tuple[int, str]] = [] # (line, "plugin" / "part of plugin" / "other") + in_string = set() quote = None # inside a multi-line string: the quotes it started with - for number, line in enumerate(lines): + for number, line in enumerate(plain): + if quote is not None: + in_string.add(number) inside = quote is not None for found in re.finditer(r'"""|\'\'\'', line): if quote is None: @@ -110,16 +161,25 @@ def blocks(text: str) -> list[Block]: later = starts[index + 1 :] own_end = later[0][0] if later else len(lines) after = next((n for n, k in later if k != "part of plugin"), len(lines)) - end = _comments_above(lines, after, header + 1) if after < len(lines) else after + end = _comments_above(plain, after, header + 1) if after < len(lines) else after lowest = max((n + 1 for n, _ in starts[:index]), default=0) - found.append(Block(_comments_above(lines, header, lowest), header, own_end, end)) - return found + found.append(Block(_comments_above(plain, header, lowest), header, own_end, end)) + return _Scan(lines, frozenset(in_string), found) + + +def blocks(text: str) -> list[Block]: + """The ``[[plugin]]`` blocks in ``text``, in file order.""" + return _scan(text).blocks def _comments_above(lines: list[str], line: int, lowest: int) -> int: - """The first of the comment lines just above ``line`` (or ``line`` itself).""" - while line > lowest and lines[line - 1].lstrip().startswith("#"): - line -= 1 + """The first of the comment lines just above ``line``, when a blank line (or the start + of the file) is above them; else ``line`` itself: then they belong to what is above.""" + start = line + while start > lowest and lines[start - 1].lstrip().startswith("#"): + start -= 1 + if start == line or start == 0 or not lines[start - 1].strip(): + return start return line @@ -137,16 +197,16 @@ def name_of(block: Any) -> str: def move(text: str, index: int, name: str, step: int) -> str: """``text`` with plugin block ``index`` moved one place up (``step=-1``) or down (``1``).""" - data, found = _check(text, index, name) + data, scan = _check(text, index, name) other = index + step - if step not in (-1, 1) or not 0 <= other < len(found): - raise EditError("this plugin can't move further") + if step not in (-1, 1) or not 0 <= other < len(scan.blocks): + raise EditError("This plugin can't move further.") first, second = sorted((index, other)) - a, b = found[first], found[second] - lines = _lines(text) + a, b = scan.blocks[first], scan.blocks[second] + lines = _ended(scan.lines) new = [ *lines[: a.start], - *_spaced(lines[b.start : b.end]), + *_spaced(lines[b.start : b.end], _newline(lines)), *lines[a.end : b.start], *lines[a.start : a.end], *lines[b.end :], @@ -157,11 +217,10 @@ def move(text: str, index: int, name: str, step: int) -> str: def remove(text: str, index: int, name: str) -> str: - """``text`` without plugin block ``index``.""" - data, found = _check(text, index, name) - block = found[index] - lines = _lines(text) - new = "".join([*lines[: block.start], *lines[block.end :]]) + """``text`` without plugin block ``index`` (and the comments just above it).""" + data, scan = _check(text, index, name) + block = scan.blocks[index] + new = "".join([*scan.lines[: block.start], *scan.lines[block.end :]]) expected = plugin_blocks(data) del expected[index] data = {k: v for k, v in data.items() if k != "plugin"} @@ -174,20 +233,21 @@ def set_enabled(text: str, index: int, name: str, enabled: bool) -> str: Off writes ``enabled = false``; on removes that line, because the file only holds settings that differ from the default. """ - data, found = _check(text, index, name) - block = found[index] - lines = _lines(text) - own = range(block.header + 1, block.own_end) - at = [n for n in own if _ENABLED.match(lines[n]) and not _in_string(text, n)] + data, scan = _check(text, index, name) + block = scan.blocks[index] + lines = _ended(scan.lines) + own = [n for n in range(block.header + 1, block.own_end) if n not in scan.in_string] + at = [n for n in own if re.match(r"\s*enabled\s*=", lines[n])] + line = "enabled = false" + _newline(lines) if enabled: - new = [line for n, line in enumerate(lines) if n not in at] + new = [kept for n, kept in enumerate(lines) if n not in at] else: new = list(lines) if at: - new[at[0]] = "enabled = false\n" + new[at[0]] = line else: after = _setting_line(lines, own, "type") or _setting_line(lines, own, "name") - new.insert((after or block.header) + 1, "enabled = false\n") + new.insert((after or block.header) + 1, line) expected = plugin_blocks(data) changed = {k: v for k, v in expected[index].items() if k != "enabled"} expected[index] = changed if enabled else changed | {"enabled": False} @@ -198,39 +258,29 @@ def add(text: str, block_text: str) -> str: """``text`` with ``block_text`` (one ``[[plugin]]`` block, as written by :func:`paperpi.example.plugin_block`) added after the last plugin block.""" data = load(text) - found = blocks(text) - if len(found) != len(plugin_blocks(data)): - raise EditError(_UNKNOWN_FORM) + scan = _scan(text) + if len(scan.blocks) != len(plugin_blocks(data)): + raise UnknownForm() new_block = plugin_blocks(load(block_text)) - lines = _lines(text) - at = found[-1].end if found else len(lines) - before = "".join(lines[:at]) - after = "".join(lines[at:]) - gap = "" if not before or before.endswith("\n\n") else "\n" - new = before + gap + block_text + ("\n" + after if after else "") + lines = _ended(scan.lines) + newline = _newline(lines) + at = scan.blocks[-1].end if scan.blocks else len(lines) + before, after = "".join(lines[:at]), "".join(lines[at:]) + gap = "" if not before or before.endswith(newline * 2) else newline + block_text = block_text.replace("\n", newline) + new = before + gap + block_text + (newline + after if after else "") return _checked(new, data | {"plugin": [*plugin_blocks(data), *new_block]}) -_UNKNOWN_FORM = ( - "the plugin blocks in the config file are written in a way the web interface can't " - "change safely; change it by hand" -) - - -def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], list[Block]]: +def _check(text: str, index: int, name: str) -> tuple[dict[str, Any], _Scan]: data = load(text) - found = blocks(text) + scan = _scan(text) raw = plugin_blocks(data) - if len(found) != len(raw): - raise EditError(_UNKNOWN_FORM) - if not 0 <= index < len(raw): - raise ChangedMeanwhile(_MEANWHILE) - if name_of(raw[index]) != name: - raise ChangedMeanwhile(_MEANWHILE) - return data, found - - -_MEANWHILE = "the config file was changed meanwhile; look at the list again and retry" + if len(scan.blocks) != len(raw): + raise UnknownForm() + if not 0 <= index < len(raw) or name_of(raw[index]) != name: + raise ChangedMeanwhile() + return data, scan def _checked(new: str, expected: dict[str, Any]) -> str: @@ -240,32 +290,27 @@ def _checked(new: str, expected: dict[str, Any]) -> str: except tomllib.TOMLDecodeError: read_back = None if read_back != expected: - raise EditError(_UNKNOWN_FORM) + raise UnknownForm() return new -def _lines(text: str) -> list[str]: - """The lines of ``text``, each ending with a line break.""" - return [line if line.endswith("\n") else line + "\n" for line in text.splitlines(True)] +def _ended(lines: list[str]) -> list[str]: + """``lines``, the last one with a line break too.""" + if lines and not lines[-1].endswith("\n"): + return [*lines[:-1], lines[-1] + _newline(lines)] + return lines + + +def _newline(lines: list[str]) -> str: + """The file's line break: ``\\r\\n`` (Windows) or ``\\n``.""" + return "\r\n" if lines and lines[0].endswith("\r\n") else "\n" -def _spaced(chunk: list[str]) -> list[str]: +def _spaced(chunk: list[str], newline: str) -> list[str]: """``chunk`` ending with a blank line, so it stays apart from the block after it.""" - return chunk if chunk and not chunk[-1].strip() else [*chunk, "\n"] + return chunk if chunk and not chunk[-1].strip() else [*chunk, newline] -def _setting_line(lines: list[str], within: range, key: str) -> int | None: +def _setting_line(lines: list[str], within: list[int], key: str) -> int | None: pattern = re.compile(rf"\s*{key}\s*=") return next((n for n in within if pattern.match(lines[n])), None) - - -def _in_string(text: str, line: int) -> bool: - """True when line ``line`` starts inside a multi-line string.""" - before = "\n".join(text.splitlines()[:line]) - quote = None - for found in re.finditer(r'"""|\'\'\'', before): - if quote is None: - quote = found.group() - elif found.group() == quote: - quote = None - return quote is not None diff --git a/src/paperpi/web/password.py b/src/paperpi/web/password.py index 5f26208..f50fa6d 100644 --- a/src/paperpi/web/password.py +++ b/src/paperpi/web/password.py @@ -96,6 +96,11 @@ def save_password_hash(path: Path, stored: str | None) -> bool: permissions. Returns False when there was nothing to change. Raises :class:`PasswordError` when the file can't be read, changed or written. """ + with config_file.LOCK: + return _save_password_hash(path, stored) + + +def _save_password_hash(path: Path, stored: str | None) -> bool: try: path, text = config_file.read(path) except config_file.EditError as error: diff --git a/tests/test_example.py b/tests/test_example.py index b2c2284..916d13b 100644 --- a/tests/test_example.py +++ b/tests/test_example.py @@ -117,6 +117,10 @@ def test_block_refuses_what_paperpi_could_not_read_back(): # tomlkit writes the control character ESC in a form Python's TOML reader doesn't know. with pytest.raises(ValueError, match="control characters"): plugin_block(plugins.load("basic_clock"), "B\x1bad") + # Characters Python counts as line breaks, which the web interface's line counting skips. + for name in ["a\u2028b", "a\u2029b", "a\x85b"]: + with pytest.raises(ValueError, match="control characters"): + plugin_block(plugins.load("basic_clock"), name) with pytest.raises(ValueError, match="reads it back"): plugin_block(plugins.load("met_no"), "Weather", {"place": "B\x1bad"}) diff --git a/tests/test_web_config_file.py b/tests/test_web_config_file.py index e8eb2d5..d5f6baa 100644 --- a/tests/test_web_config_file.py +++ b/tests/test_web_config_file.py @@ -111,7 +111,7 @@ def test_files_it_doesnt_understand_are_not_changed(): assert names(config_file.remove(text, 0, "Clock")) == ["Words"] # ... and a header written in another way is not found, so nothing is changed. quoted = CONFIG.replace('[[plugin]]\nname = "Words"', '[["plugin"]]\nname = "Words"') - with pytest.raises(EditError, match="change it by hand"): + with pytest.raises(EditError, match="config file itself"): config_file.move(quoted, 0, "Clock", 1) with pytest.raises(EditError, match="not valid TOML"): config_file.move("[[plugin]\n", 0, "Clock", 1) @@ -127,3 +127,84 @@ def test_saving_keeps_the_permissions(tmp_path): assert path.stat().st_mode & 0o777 == 0o640 with pytest.raises(EditError, match="file not found"): config_file.read(tmp_path / "nothing.toml") + + +def test_comments_right_after_a_setting_stay_with_the_block_above(): + text = ( + '[[plugin]]\nname = "A"\ntype = "x"\n# refresh = 60\n[[plugin]]\nname = "B"\ntype = "y"\n' + ) + assert ( + config_file.remove(text, 1, "B") == '[[plugin]]\nname = "A"\ntype = "x"\n# refresh = 60\n' + ) + + +def test_windows_line_endings_are_kept(): + text = CONFIG.replace("\n", "\r\n") + block = example.plugin_block(plugins.load("dec_binary_clock"), "Dots", {}) + for changed in [ + config_file.move(text, 1, "Words", -1), + config_file.set_enabled(text, 0, "Clock", False), + config_file.set_enabled(text, 1, "Words", True), + config_file.remove(text, 0, "Clock"), + config_file.add(text, block), + ]: + assert "\n" not in changed.replace("\r\n", "") + + +def test_a_file_without_a_last_line_break(): + text = CONFIG.removesuffix("\n") + assert tomllib.loads(config_file.move(text, 1, "Words", -1))["web"] == {"port": 8081} + last = '[[plugin]]\nname = "A"\ntype = "x"' + off = config_file.set_enabled(last, 0, "A", False) + assert tomllib.loads(off)["plugin"][0]["enabled"] is False + + +def test_other_parts_between_blocks_stay_where_they_are(): + text = CONFIG.replace("[web]\nport = 8081\n", "").replace( + "# words\n", "[web]\nport = 8081\n\n# words\n" + ) + moved = config_file.move(text, 1, "Words", -1) + assert names(moved) == ["Words", "Clock"] + # The blocks swap places; [web] stays between them. + assert moved.index('name = "Words"') < moved.index("[web]") < moved.index('name = "Clock"') + assert tomllib.loads(moved)["web"] == {"port": 8081} + + +def test_enabled_in_a_part_or_a_string_is_not_the_switch(): + text = ( + '[[plugin]]\nname = "A"\nnote = """\nenabled = true\n"""\ntype = "x"\n' + "[plugin.extra]\nenabled = true\n" + ) + off = config_file.set_enabled(text, 0, "A", False) + data = tomllib.loads(off)["plugin"][0] + assert data["enabled"] is False and data["extra"] == {"enabled": True} + assert data["note"] == "enabled = true\n" + assert config_file.set_enabled(off, 0, "A", True) == text + # Only a "type" inside a string: the new line goes right below [[plugin]]. + hidden = '[[plugin]]\nnote = """\ntype = "y"\n"""\nname = "B"\n' + assert tomllib.loads(config_file.set_enabled(hidden, 0, "B", False))["plugin"][0] == { + "note": 'type = "y"\n', + "name": "B", + "enabled": False, + } + + +def test_unicode_line_breaks_inside_a_value_are_not_line_breaks(): + text = CONFIG.replace('name = "Words"', 'name = "Words"\nplace = "a
b\x85c"') + assert names(config_file.remove(text, 0, "Clock")) == ["Words"] + off = config_file.set_enabled(text, 1, "Words", False) + assert tomllib.loads(off)["plugin"][1]["place"] == "a
b\x85c" + + +def test_a_file_too_large_for_paperpi_is_not_saved(tmp_path, monkeypatch): + from paperpi import limits + + path = tmp_path / "paperpi.toml" + path.write_text(CONFIG) + monkeypatch.setattr(limits, "CONFIG_FILE_BYTES", len(CONFIG)) + with pytest.raises(EditError, match="larger than"): + config_file.write(path, CONFIG + "#\n") + assert path.read_text() == CONFIG + config_file.write(path, CONFIG) + assert config_file.saved_here(path, CONFIG) + assert not config_file.saved_here(path, CONFIG + "#\n") From f19015b2fd60684e4e3af7f6a1aef24da0d4b537 Mon Sep 17 00:00:00 2001 From: txoof-bot <337660340+txoof-bot@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:30:20 +0200 Subject: [PATCH 6/6] Review fixes: rows by place, refuse changes PaperPi can't apply, clearer texts, more tests Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/decisions/plugin-interface.md | 2 +- docs/decisions/web-interface.md | 6 +- docs/writing-plugins.md | 1 + src/paperpi/cli.py | 7 +- src/paperpi/web/app.py | 30 +++-- src/paperpi/web/plugins.py | 154 ++++++++++++++----------- src/paperpi/web/server.py | 4 +- src/paperpi/web/templates/add.html | 4 +- src/paperpi/web/templates/library.html | 2 +- src/paperpi/web/templates/plugins.html | 14 ++- tests/test_cli.py | 13 ++- tests/test_config.py | 3 +- tests/test_web_plugins.py | 122 +++++++++++++++++++- 14 files changed, 262 insertions(+), 102 deletions(-) diff --git a/README.md b/README.md index 968899a..cb7e7c5 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ uv run paperpi run --config paperpi.toml --out screen/ --state-dir state/ --heal ### The web interface -`paperpi run` also starts the web interface, on port 8080. Open it by the Pi's IP address on a phone or computer on the same home network, for example `http://192.168.1.20:8080` (your router's list of devices shows the Pi's address; on the Pi, `hostname -I` prints it). Names such as `paperpi.local` are refused for now, for safety (see [docs/decisions/web-interface.md](docs/decisions/web-interface.md)). It has two pages for plugins: **Active Plugins** (switch plugins on or off, move them up or down, remove one) and **Plugin Library** (every plugin that comes with PaperPi; add one at the end of the list). A change there is saved in the config file and applies at once; comments in the file stay. A plugin that needs settings (such as `met_no`, which needs a place and your email address) is added switched off; until the settings page follows (issue #238), fill those in in the config file. +`paperpi run` also starts the web interface, on port 8080. Open it by the Pi's IP address on a phone or computer on the same home network, for example `http://192.168.1.20:8080` (your router's list of devices shows the Pi's address; on the Pi, `hostname -I` prints it). Names such as `paperpi.local` are refused for now, for safety (see [docs/decisions/web-interface.md](docs/decisions/web-interface.md)). It has two pages for plugins: **Active Plugins** (switch plugins on or off, move them up or down, remove one) and **Plugin Library** (every plugin that comes with PaperPi; add one at the end of the list). A change there is saved in the config file and applies at once. Comments in the file stay, except the comments of a plugin you remove and a comment on the same line as `enabled =`. If the config file changed since the page was opened, is written in a way the web interface can't change safely, or has a problem that stops PaperPi from using it, nothing is saved and the page says so. A plugin that needs settings (such as `met_no`, which needs a place and your email address) is added switched off; until the settings page follows (issue #238), fill those in in the config file. - **First visit:** the first person to open it sets the web password (at least 8 characters). The browser then stays logged in for a year, until **Log out**. Log out only logs out that browser; a new password logs out every browser. - **Forgotten password:** you need access to the Pi itself (a keyboard and screen, or SSH). diff --git a/docs/decisions/plugin-interface.md b/docs/decisions/plugin-interface.md index fb87614..63b2f91 100644 --- a/docs/decisions/plugin-interface.md +++ b/docs/decisions/plugin-interface.md @@ -50,7 +50,7 @@ The plugin never talks to the screen. Only the scheduler does. For sample images and tests, `fetch` is skipped and the sample data goes straight to `draw`. How to write a plugin: `docs/writing-plugins.md`. -**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface (part 2b) shows "Needs settings" in place of the on switch. The example config no longer fills in a made-up email address. `setting` is the one standard place for a setting's own options; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. +**Update 2026-10-07 (M5 part 2a, issue #238, agreed with txoof):** a plugin can mark a setting as **required** with `setting(..., required=True)`, for settings it can't work without, such as an API key. The first ones are `lat`, `lon` and `email` of `met_no` and `moon_phase`: met.no's terms of service ask for a real email address, and a user should not start these plugins without giving one. A plugin that is missing a required setting can be in the config file and switched on, but it is **not shown and not counted as broken**: the config check gives a warning naming the missing settings, `paperpi list` shows "needs email" in its "on" column, and the web interface's Active Plugins page shows "Needs settings: ..." and its Switch on button can't be used until they are filled in. The example config no longer fills in a made-up email address. `setting` is the one standard place for a setting's own options; the input helpers of `web-interface.md` (such as `location`, which looks up latitude and longitude; part 3c) will be options of `setting` too. ### How a plugin is run diff --git a/docs/decisions/web-interface.md b/docs/decisions/web-interface.md index b19edb6..5d9e54a 100644 --- a/docs/decisions/web-interface.md +++ b/docs/decisions/web-interface.md @@ -23,11 +23,11 @@ In v2 the web interface is the main way to set up and change PaperPi: plugins, s *Added in M5 part 2b (issue #238), agreed with txoof on 2026-10-07.* - **Active Plugins** lists every `[[plugin]]` block in the config file, in file order (the order the plugins take turns), also blocks with errors. Each has: Switch on / Switch off, **Up** and **Down** buttons (no dragging, so no extra JavaScript library; works the same on a phone), and Remove, which asks first. -- **Plugin Library** lists every plugin type that comes with PaperPi, with its one-line description, except `default` (PaperPi's own "nothing to show" message) and `debugging` (for testing PaperPi). Choosing one asks for a name (a free one is suggested, e.g. "Basic clock 2") and adds the block at the end of Active Plugins. Sample pictures follow in part 3b. +- **Plugin Library** lists every plugin type that comes with PaperPi, with its one-line description, except `default` (PaperPi's own "nothing to show" message) and `debugging` (for testing PaperPi). Choosing one asks for a name (a free one is suggested, for example "Basic clock 2") and adds the block at the end of Active Plugins. Sample pictures follow in part 3b. - A plugin with required settings (see `plugin-interface.md`) is added **switched off**, and the add page lists those settings with their help text (for `met_no`: a real email address, as met.no's terms ask). Active Plugins shows "Needs settings: ..." and can't switch it on until they are filled in. -- Each change is saved at once (the config file then holds the change and nothing else changed: comments stay, and the comments just above a `[[plugin]]` line move with its block) and PaperPi reloads the file, so it applies at once. +- Each change is saved at once (nothing else in the config file changes: comments stay, and the comments just above a `[[plugin]]` line move and are removed with its block; a comment on the same line as `enabled =` is lost) and PaperPi reloads the file, so it applies at once. - **Hand edits not applied yet:** a change from the web interface is made to the file as it is on disk, so hand edits stay and apply together with it. The page then says "Your hand edits to the config file were applied too." (The other choice, refusing to save until the hand edits are applied, needs an extra step every time.) -- A change names its block by place and name. If the block was moved or renamed by hand since the page was shown, nothing is changed and the page says the file changed meanwhile. A file the web interface can't change safely (for example a `[[plugin]]` line written in an unusual way) is never saved wrongly: every change is read back and compared before saving. +- A change names its block by place and name. If the block was moved or renamed by hand since the page was shown, nothing is changed and the page says the file changed meanwhile. A file the web interface can't change safely (for example a `[[plugin]]` line written in an unusual way) is never saved wrongly: every change is read back and compared before saving. A change is also refused while the file has a problem that stops PaperPi from using it (PaperPi then runs on the last good copy, so the change would not apply), and when the file would become larger than PaperPi reads. ### Plugin settings forms diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index 9e856a6..4247a1d 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -81,6 +81,7 @@ Each setting is a field of the plugin's `Settings` class, with a type, a default - Don't use the names of the shared settings, which every `[[plugin]]` block already has: `name`, `type`, `enabled`, `level`, `display_time`, `refresh`, `time_limit`, `layout`, `alert_reminder`, `alert_max_time`. - Use `pydantic.SecretStr` as the type for API keys and passwords. PaperPi then never shows their values in error messages or logs. - A setting the user must fill in before the plugin can work (a place, an email address, an API key) is made with `paperpi.plugin.setting(..., required=True)` instead of `Field`. It takes the same arguments as `Field`. Its default must be "not set": `None`, empty text or an empty `SecretStr` (text of only spaces also counts as not set). Until every required setting is filled in, the plugin is not shown and not counted as broken; the config check, `paperpi list` and the web interface's Active Plugins page say which settings are missing. The example block marks them "(required)". `setting` is also where later options for a single setting are added, such as a web interface helper that looks up latitude and longitude (M5 part 3c). +- The web interface shows the plugin's `description` in its Plugin Library, and the help text of each required setting on the page that adds the plugin. Write both for someone at home who is not a programmer. ```python from paperpi.plugin import PluginSettings, setting diff --git a/src/paperpi/cli.py b/src/paperpi/cli.py index c4efb5e..1a75764 100644 --- a/src/paperpi/cli.py +++ b/src/paperpi/cli.py @@ -325,7 +325,12 @@ def load() -> config.Config: # Imported here: the web packages take a moment to load, and no other command needs them. from .web import server - web = server.start(args.config, loaded.web, reload=scheduler.reload, text=loaded.text) + web = server.start( + args.config, + loaded.web, + reload=scheduler.reload, + text=None if loaded.from_last_good else loaded.text, + ) if web is not None: print(f"web interface on port {web.port}") try: diff --git a/src/paperpi/web/app.py b/src/paperpi/web/app.py index 1f83774..2505f23 100644 --- a/src/paperpi/web/app.py +++ b/src/paperpi/web/app.py @@ -8,7 +8,8 @@ ``/login`` log in; also says how to reset a forgotten password ``/logout`` log out (a button on every page) ``/plugins`` Active Plugins: the plugins in the config file; switch on or off, move - up or down, remove (``/plugins//...``, n = place in the file) + up or down, remove (``/plugins//...``, n = place in the file, + counted from 0) ``/library`` Plugin Library: every plugin type; ``/library/`` adds one ``/static/...`` the style sheet and htmx (a small JavaScript file that updates one part of a page without loading the whole page again) @@ -149,25 +150,30 @@ def plugin_list(request: Request, status: int = 200, problem: str | None = None) try: found = editor.plugin_list() except EditError as error: - problems = [str(error)] if str(error) != problem else [] + rows, problems = [], [str(error)] status = 500 if status == 200 else status - return page( - request, "plugins.html", status, rows=[], problems=problems, problem=problem - ) + else: + rows, problems = found.rows, found.problems + done = request.query_params.get("done") + # The name in the address is only shown when it is in the list: a link can't put + # other text on the page. + name = request.query_params.get("name") return page( request, "plugins.html", status, - rows=found.rows, - problems=found.problems, + rows=rows, + problems=[p for p in dict.fromkeys(problems) if p != problem], problem=problem, - done=request.query_params.get("done"), - name=request.query_params.get("name"), + done=done if done in ("saved", "added", "removed") else None, + name=name if any(row.name == name for row in rows) else None, hand_edits=request.query_params.get("hand") == "1", + config_file=auth.config_file, ) def saved(done: str, hand_edits: bool, name: str = "") -> RedirectResponse: - query = urlencode({"done": done, "name": name} | ({"hand": "1"} if hand_edits else {})) + values = {"done": done} | ({"name": name} if name else {}) + query = urlencode(values | ({"hand": "1"} if hand_edits else {})) return RedirectResponse(f"/plugins?{query}", status_code=303) @app.get("/plugins", response_class=HTMLResponse) @@ -176,6 +182,8 @@ def plugins_page(request: Request): @app.post("/plugins/{index}/move", response_class=HTMLResponse) def move(request: Request, index: int, name: str = Form(""), step: str = Form("")): + if step not in ("up", "down"): + return plugin_list(request, 400, "Choose up or down.") try: hand_edits = editor.move(index, name, -1 if step == "up" else 1) except EditError as error: @@ -204,7 +212,7 @@ def remove(request: Request, index: int, name: str = Form("")): hand_edits = editor.remove(index, name) except EditError as error: return plugin_list(request, 409, str(error)) - return saved("removed", hand_edits, name) + return saved("removed", hand_edits) @app.get("/library", response_class=HTMLResponse) def library_page(request: Request): diff --git a/src/paperpi/web/plugins.py b/src/paperpi/web/plugins.py index 5855bb5..2eab5f7 100644 --- a/src/paperpi/web/plugins.py +++ b/src/paperpi/web/plugins.py @@ -6,11 +6,13 @@ - A change is made to the file as it is on disk now, so hand edits that were not applied yet stay in it and apply together with the change (agreed with txoof, M5 part 2b). The page then says so. +- A change is refused while the file has a problem that stops PaperPi from using it (such + as a wrong ``[display]``): PaperPi would keep running on the last good copy, so the change + would not apply. """ from __future__ import annotations -import threading from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path @@ -31,7 +33,7 @@ class Row: """One ``[[plugin]]`` block, as the Active Plugins page shows it.""" index: int - """Its place in the file, from 0.""" + """Its place in the file, counted from 0.""" name: str """``""`` when the block has no proper name.""" type: str @@ -40,9 +42,13 @@ class Row: """Required settings that are not set yet; the plugin can't be switched on until they are.""" errors: tuple[str, ...] = () """What is wrong with the block; PaperPi leaves it out until it is fixed.""" + known: bool = True + """False when the file can't be checked, so whether PaperPi uses the block is not known.""" @property def status(self) -> str: + if not self.known: + return "Unknown" if self.errors: return "Not used: has errors" if self.missing: @@ -54,7 +60,7 @@ def status(self) -> str: class PluginList: rows: list[Row] problems: list[str] = field(default_factory=list) - """Problems with the whole file (no plugin can be used until they are fixed).""" + """Problems with the whole file (no change can be made until they are fixed).""" @dataclass(frozen=True) @@ -81,11 +87,8 @@ class PluginEditor: def __init__(self, path: Path, reload: Callable[[], None] | None = None): self.path = Path(path) self._reload = reload - self._lock = threading.Lock() self._applied: str | None = None """The file text PaperPi uses now (``None``: not known).""" - self._written: str | None = None - """The text the web interface saved last (it may not be applied yet).""" def loaded(self, text: str) -> None: """PaperPi loaded the config file ``text`` (at the start or a reload).""" @@ -95,47 +98,7 @@ def plugin_list(self) -> PluginList: """The plugin blocks in the file now. Raises :class:`config_file.EditError` when the file can't be read at all.""" path, text = config_file.read(self.path) - data = config_file.load(text) - found = config_file.blocks(text) - problems: list[config.Problem] = [] - checked: dict[str, config.PluginConfig] = {} - file_problems = [] - try: - loaded = config.parse(text, path.name) - except config.ConfigError as error: - problems = error.problems - file_problems = [str(p) for p in error.problems if p.level == "error"] - else: - problems = loaded.problems - checked = {p.entry.name: p for p in loaded.plugins} - rows = [] - for index, block in enumerate(config_file.plugin_blocks(data)): - block = block if isinstance(block, dict) else {} - name = config_file.name_of(block) - plugin_type = block.get("type") - enabled = block.get("enabled", True) - errors = () - if index < len(found) and not file_problems: - place = found[index] - errors = tuple( - _without_place(p) - for p in problems - if p.level == "error" and p.line and place.start < p.line <= place.end - ) - good = checked.get(name) - rows.append( - Row( - index=index, - name=name, - type=plugin_type if isinstance(plugin_type, str) else "", - enabled=enabled if isinstance(enabled, bool) else True, - missing=good.missing if good is not None else (), - errors=errors if good is None else (), - ) - ) - if len(found) != len(rows): - file_problems.append(config_file._UNKNOWN_FORM) - return PluginList(rows, file_problems) + return _plugin_list(text, path.name) def change(self, edit: Callable[[str], str]) -> bool: """Change the file with ``edit`` (old text -> new text), save it and apply it. @@ -143,12 +106,23 @@ def change(self, edit: Callable[[str], str]) -> bool: Returns True when the file had hand edits that were not applied yet; they apply too. Raises :class:`config_file.EditError` when the change can't be made. """ - with self._lock: + with config_file.LOCK: path, text = config_file.read(self.path) - hand_edits = self._applied is not None and text not in (self._applied, self._written) + hand_edits = ( + self._applied is not None + and text != self._applied + and not config_file.saved_here(path, text) + ) new = edit(text) + try: + config.parse(new, path.name) + except config.ConfigError as error: + found = "; ".join(str(p) for p in error.problems if p.level == "error") + raise config_file.EditError( + "The config file has a problem that must be fixed first, in the file " + f"itself: {found}" + ) from None config_file.write(path, new) - self._written = new if self._reload is not None: self._reload() return hand_edits @@ -157,31 +131,30 @@ def move(self, index: int, name: str, step: int) -> bool: return self.change(lambda text: config_file.move(text, index, name, step)) def set_enabled(self, index: int, name: str, enabled: bool) -> bool: - if enabled: - row = self.row(index, name) - if row.missing: - missing = ", ".join(row.missing) + def switch(text: str) -> str: + row = _row(_plugin_list(text), index, name) + if enabled and row.missing: raise config_file.EditError( - f"{name} can't be switched on yet: fill in these settings first: {missing}" + f"{name} can't be switched on yet. Fill in these settings first: " + f"{', '.join(row.missing)}." ) - return self.change(lambda text: config_file.set_enabled(text, index, name, enabled)) + return config_file.set_enabled(text, index, name, enabled) + + return self.change(switch) def remove(self, index: int, name: str) -> bool: return self.change(lambda text: config_file.remove(text, index, name)) def row(self, index: int, name: str) -> Row: """The block at ``index``, if it is still called ``name``.""" - rows = self.plugin_list().rows - if not 0 <= index < len(rows) or rows[index].name != name: - raise config_file.ChangedMeanwhile(config_file._MEANWHILE) - return rows[index] + return _row(self.plugin_list(), index, name) def add(self, item: LibraryItem, name: str) -> bool: """Add a block for ``item`` called ``name`` at the end of the plugin list. A plugin with required settings is added switched off. Raises :class:`config_file.EditError` for a name that can't be used.""" if item.plugin is None: - raise config_file.EditError(f"{item.type} can't be added: {item.description}") + raise config_file.EditError(f"{item.type} can't be added. {item.description}") name = name.strip() problem = name_problem(name) if problem: @@ -190,11 +163,10 @@ def add(self, item: LibraryItem, name: str) -> bool: block = example.plugin_block(item.plugin, name, values) def add(text: str) -> str: - used = _used_names(text) - if config.folder_name(name) in used: + if config.folder_name(name) in _used_names(text): raise config_file.EditError( - f"the name {name!r} is already used by another plugin (also ignoring " - "capitals and punctuation); choose another one" + "Another plugin already has this name (capitals and punctuation don't " + "count). Choose another name." ) return config_file.add(text, block) @@ -215,6 +187,58 @@ def suggested_name(self, item: LibraryItem) -> str: return name +def _plugin_list(text: str, source: str = "config") -> PluginList: + data = config_file.load(text) + found = config_file.blocks(text) + raw = config_file.plugin_blocks(data) + file_problems: list[str] = [] + problems: list[config.Problem] = [] + checked: dict[int, config.PluginConfig] = {} + try: + loaded = config.parse(text, source) + except config.ConfigError as error: + file_problems = [str(p) for p in error.problems if p.level == "error"] + else: + problems = loaded.problems + # By the line of its [[plugin]] line: two blocks may have the same name (then + # PaperPi uses only the first). + checked = {p.line: p for p in loaded.plugins if p.line is not None} + known = len(found) == len(raw) and not file_problems + if len(found) != len(raw): + file_problems.append(str(config_file.UnknownForm())) + rows = [] + for index, block in enumerate(raw): + block = block if isinstance(block, dict) else {} + plugin_type, enabled = block.get("type"), block.get("enabled", True) + good = errors = None + if known: + place = found[index] + good = checked.get(place.header + 1) + errors = tuple( + _without_place(p) + for p in problems + if p.level == "error" and p.line and place.start < p.line <= place.end + ) + rows.append( + Row( + index=index, + name=config_file.name_of(block), + type=plugin_type if isinstance(plugin_type, str) else "", + enabled=enabled if isinstance(enabled, bool) else True, + missing=good.missing if good is not None else (), + errors=() if good is not None or not known else (errors or ("can't be used",)), + known=known, + ) + ) + return PluginList(rows, file_problems) + + +def _row(found: PluginList, index: int, name: str) -> Row: + if not 0 <= index < len(found.rows) or found.rows[index].name != name: + raise config_file.ChangedMeanwhile() + return found.rows[index] + + def library() -> list[LibraryItem]: """The plugin types the Plugin Library offers, sorted by type.""" items = [] diff --git a/src/paperpi/web/server.py b/src/paperpi/web/server.py index 07f9490..0bbd127 100644 --- a/src/paperpi/web/server.py +++ b/src/paperpi/web/server.py @@ -34,10 +34,10 @@ def __init__( auth: Auth, settings: config.WebSettings, sock: socket.socket, - editor: PluginEditor | None = None, + editor: PluginEditor, ): self.auth = auth - self.editor = editor or PluginEditor(auth.config_file) + self.editor = editor self.settings = settings """The settings it was started with; a change of address or port needs a restart.""" self._socket = sock diff --git a/src/paperpi/web/templates/add.html b/src/paperpi/web/templates/add.html index bc6a5e4..e5dbfbc 100644 --- a/src/paperpi/web/templates/add.html +++ b/src/paperpi/web/templates/add.html @@ -11,8 +11,8 @@

Add {{ item.type }}

    {% for key, help in item.required %}
  • {{ key }}: {{ help }}
    • {% endfor %}
-

Until the settings page is ready (a later version), fill them in in the config file, - then switch the plugin on in Active Plugins.

+

Until the settings page is ready (a later version), type them into the config file on + the Pi ({{ auth.config_file }}), then switch the plugin on in Active Plugins.

{% endif %}
diff --git a/src/paperpi/web/templates/library.html b/src/paperpi/web/templates/library.html index b8492dc..2a2a007 100644 --- a/src/paperpi/web/templates/library.html +++ b/src/paperpi/web/templates/library.html @@ -6,7 +6,7 @@

Plugin Library

{% if problem %}

{{ problem }}

{% endif %}

Every plugin that comes with PaperPi. Choose one to add it at the end of Active Plugins. The same plugin can be added more than once, with -different settings (e.g. the weather for two places).

+different settings (for example the weather for two places).

    {% for item in items %}
  • diff --git a/src/paperpi/web/templates/plugins.html b/src/paperpi/web/templates/plugins.html index 4f2a613..8557afd 100644 --- a/src/paperpi/web/templates/plugins.html +++ b/src/paperpi/web/templates/plugins.html @@ -5,7 +5,8 @@

    Active Plugins

    {% if done %}

    - {%- if done == "added" %}Added {{ name }}.{% elif done == "removed" %}Removed {{ name }}. + {%- if done == "added" %}{% if name %}Added {{ name }}.{% else %}Plugin added.{% endif %} + {%- elif done == "removed" %}Plugin removed. {%- else %}Saved.{% endif %} PaperPi applies the change now. {%- if hand_edits %} Your hand edits to the config file were applied too.{% endif %}

    {% endif %} @@ -13,17 +14,18 @@

    Active Plugins

    {% for found in problems %}

    {{ found|sentence }}

    {% endfor %}

    The plugins that are switched on take turns on the screen, in this order. Add a plugin from the Plugin Library.

    -{% if not rows %}

    No plugins yet.

    {% endif %} +{% if rows | selectattr("missing") | list %} +

    Some plugins need settings before they are shown. Until the settings page is +ready (a later version), type them into the config file on the Pi: +{{ config_file }}.

    +{% endif %} +{% if not rows and not problems %}

    No plugins yet.

    {% endif %}
      {% for row in rows %}
    1. {{ row.name or "(no name)" }} {{ row.type }}
      {{ row.status }}
      {% for error in row.errors %}
      {{ error }}
      {% endfor %} - {% if row.missing %} -
      Until the settings page is ready (a later version), fill these in in the - config file.
      - {% endif %}
      diff --git a/tests/test_cli.py b/tests/test_cli.py index c2bf434..4ef5fa6 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -537,10 +537,19 @@ def test_run_starts_the_web_interface_unless_asked_not_to( from paperpi.web import server calls = [] - monkeypatch.setattr(server, "start", lambda *a, **kw: calls.append(a)) + monkeypatch.setattr(server, "start", lambda *a, **kw: calls.append((a, kw))) + reloads = [] + monkeypatch.setattr(cli.Scheduler, "reload", lambda self: reloads.append(1)) monkeypatch.setattr(cli.Scheduler, "run", lambda self: None) cfg = tmp_path / "paperpi.toml" - cfg.write_text(f'config_version = 1\n[display]\ntype = "virtual"\n{web}') + text = f'config_version = 1\n[display]\ntype = "virtual"\n{web}' + cfg.write_text(text) run = ["run", *args, "--config", str(cfg), "--state-dir", str(tmp_path)] assert main([*run, "--health-file", str(tmp_path / "health")]) == 0 assert bool(calls) == started + if started: + # A change in the web interface reloads PaperPi; the web knows the text in use. + ((_, kw),) = calls + assert kw["text"] == text + kw["reload"]() + assert reloads == [1] diff --git a/tests/test_config.py b/tests/test_config.py index 3d0a56d..c1b72b1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -414,7 +414,7 @@ def files(tmp_path): def test_good_load_saves_last_good_copy(files): path, state, last_good = files - load(path, state_dir=state) + assert load(path, state_dir=state).text == GOOD assert last_good.read_text() == GOOD assert last_good.stat().st_mode & 0o777 == 0o600 # it may hold passwords assert list(state.iterdir()) == [last_good] # no temporary files left @@ -444,6 +444,7 @@ def test_broken_file_uses_last_good_copy(files): path.write_text("config_version = 1\n[display\n") cfg = load(path, state_dir=state) assert cfg.from_last_good + assert cfg.text == GOOD # the text in use: the last good copy's assert [p.entry.name for p in cfg.plugins] == ["Clock"] assert problems(cfg) == [ "paperpi.toml line 2: not valid TOML: Expected ']' at the end of a table declaration " diff --git a/tests/test_web_plugins.py b/tests/test_web_plugins.py index ffb8593..6c3fee9 100644 --- a/tests/test_web_plugins.py +++ b/tests/test_web_plugins.py @@ -4,8 +4,10 @@ import pytest from fastapi.testclient import TestClient -from paperpi import config +from paperpi import config, plugins +from paperpi.plugin import PluginDefinitionError from paperpi.web import auth +from paperpi.web import password as web_password from paperpi.web.app import create_app from paperpi.web.plugins import PluginEditor @@ -106,7 +108,7 @@ def test_switching_off_and_on(client, cfg): def test_a_plugin_with_missing_settings_cant_be_switched_on(client, cfg, reloads): response = client.post("/plugins/1/enabled", data={"name": "Weather", "on": "1"}) assert response.status_code == 409 - assert "fill in these settings first: email" in response.text + assert "Fill in these settings first: email." in response.text assert cfg.read_text() == CONFIG and not reloads @@ -114,17 +116,17 @@ def test_removing_asks_first(client, cfg): page = client.get("/plugins/0/remove", params={"name": "Clock"}) assert "Remove Clock?" in page.text and cfg.read_text() == CONFIG response = client.post("/plugins/0/remove", data={"name": "Clock"}) - assert response.headers["location"] == "/plugins?done=removed&name=Clock" + assert response.headers["location"] == "/plugins?done=removed" assert names(cfg) == ["Weather", "Broken"] assert "kitchen" not in cfg.read_text() - assert "Removed Clock." in client.get(response.headers["location"]).text + assert "Plugin removed." in client.get(response.headers["location"]).text def test_a_list_changed_by_hand_meanwhile_is_not_changed(client, cfg, reloads): # The page showed Clock first, but it was removed by hand since then. cfg.write_text(CONFIG.replace('name = "Clock"', 'name = "Kitchen"')) response = client.post("/plugins/0/remove", data={"name": "Clock"}) - assert response.status_code == 409 and "changed meanwhile" in response.text + assert response.status_code == 409 and "changed after this page was opened" in response.text assert names(cfg) == ["Kitchen", "Weather", "Broken"] and not reloads @@ -173,7 +175,7 @@ def test_a_plugin_with_required_settings_is_added_switched_off(client, cfg): ("", "Give the plugin a name."), ("x" * 101, "at most 100 characters"), ("a\tb", "must not hold control characters"), - ("clock!", "already used by another plugin"), + ("clock!", "Another plugin already has this name"), ], ) def test_names_that_cant_be_used(client, cfg, name, problem): @@ -224,3 +226,111 @@ def test_a_change_in_the_running_web_interface_reloads_paperpi(cfg, reloads): finally: web.stop() assert reloads == [1] and blocks(cfg)[0]["enabled"] is False + + +def test_a_reload_tells_the_web_interface_which_text_is_in_use(cfg, reloads): + from paperpi.web import server + + settings = config.WebSettings.model_construct( + **(config.WebSettings().model_dump() | {"address": "127.0.0.1", "port": 0}) + ) + web = server.start(cfg, settings) + try: + edited = CONFIG + "# a hand edit\n" + cfg.write_text(edited) + web.use(config.parse(edited)) # PaperPi reloaded the hand edit + assert not web.editor.move(0, "Clock", 1) + finally: + web.stop() + + +def test_which_changes_count_as_hand_edits(cfg, editor): + # Applied by a reload: not a hand edit any more. + edited = CONFIG + "# a hand edit\n" + cfg.write_text(edited) + editor.loaded(edited) + assert not editor.move(0, "Clock", 1) + # After the web's own change was applied, a new hand edit is one. + editor.loaded(cfg.read_text()) + cfg.write_text(cfg.read_text() + "# another one\n") + assert editor.move(1, "Clock", -1) + # A password saved by the web interface is not a hand edit. + editor.loaded(cfg.read_text()) + web_password.save_password_hash(cfg, "scrypt:16:1:1:c2FsdA:" + "A" * 43) + assert not editor.move(0, "Clock", 1) + # An editor that doesn't know what PaperPi uses never says so. + cfg.write_text(cfg.read_text() + "# and one more\n") + assert not PluginEditor(cfg).move(1, "Clock", -1) + + +def test_a_broken_plugin_is_shown_but_cant_be_added(client, cfg, monkeypatch): + real = plugins.load + + def load(plugin_type, *args): + if plugin_type == "word_clock": + raise PluginDefinitionError("plugin 'word_clock': needs at least one layout") + return real(plugin_type, *args) + + monkeypatch.setattr(plugins, "load", load) + assert "This plugin is broken: plugin 'word_clock'" in client.get("/library").text + assert client.get("/library/word_clock").status_code == 200 + response = client.post("/library/word_clock", data={"name": "Words"}) + assert response.status_code == 400 and "can't be added" in response.text + assert cfg.read_text() == CONFIG + + +def test_a_repeated_name_shows_the_second_block_as_not_used(client, cfg): + cfg.write_text(CONFIG + '\n[[plugin]]\nname = "Clock"\ntype = "word_clock"\n') + page = client.get("/plugins").text + assert page.count("Not used: has errors") == 2 # Broken and the second Clock + assert "is already used by the plugin block" in page + + +def test_a_change_is_refused_while_paperpi_cant_use_the_file(client, cfg, reloads): + broken = CONFIG.replace('type = "virtual"', 'type = "nothing"') + cfg.write_text(broken) + page = client.get("/plugins") + assert "unknown screen type" in page.text and "Unknown" in page.text + response = client.post("/plugins/0/move", data={"name": "Clock", "step": "down"}) + assert response.status_code == 409 and "must be fixed first" in response.text + assert cfg.read_text() == broken and not reloads + + +def test_every_change_needs_a_log_in_and_a_form_from_paperpi(cfg): + forms = { + "/plugins/0/move": {"name": "Clock", "step": "down"}, + "/plugins/0/enabled": {"name": "Clock", "on": "0"}, + "/plugins/0/remove": {"name": "Clock"}, + "/library/word_clock": {"name": "Words"}, + } + stored = web_password.hash_password("correct horse") + paperpi_auth = auth.Auth(cfg, config.WebSettings(password_hash=stored)) + client = TestClient(create_app(paperpi_auth), follow_redirects=False, base_url=PI) + for path, form in forms.items(): + assert client.post(path, data=form).headers["location"] == "/login" + open_client = TestClient( + create_app(auth.Auth(cfg, config.WebSettings(login=False))), + follow_redirects=False, + base_url=PI, + ) + for path, form in forms.items(): + other_site = {"origin": "http://evil.example", "sec-fetch-site": "cross-site"} + assert open_client.post(path, data=form, headers=other_site).status_code == 403 + assert cfg.read_text() == CONFIG + + +def test_odd_requests(client, cfg): + response = client.post("/plugins/0/move", data={"name": "Clock", "step": "sideways"}) + assert response.status_code == 400 and cfg.read_text() == CONFIG + assert client.get("/plugins/0/remove", params={"name": "Kitchen"}).status_code == 409 + # Text in the address is only shown when it names a plugin in the list. + page = client.get("/plugins", params={"done": "added", "name": "Your password was reset"}) + assert "Your password was reset" not in page.text and "Plugin added." in page.text + page = client.get("/plugins", params={"done": "added", "name": "Clock"}) + assert "Added Clock." in page.text + + +def test_the_file_keeps_its_permissions(client, cfg): + cfg.chmod(0o640) + client.post("/plugins/0/move", data={"name": "Clock", "step": "down"}) + assert cfg.stat().st_mode & 0o777 == 0o640