From 26d2cd01300464b91f435437cedc5f29cbb16b3a Mon Sep 17 00:00:00 2001 From: Michael Fisher Date: Tue, 22 Sep 2026 15:39:42 -0400 Subject: [PATCH 1/5] docs: plans: update app level scripting plans --- docs/plans/extensions.md | 74 +++--- docs/plans/scripting-audit.md | 195 ++++++++++++++ docs/plans/session-proxy.md | 282 ++++++++++++++++++++ docs/plans/session-scripts.md | 470 ++++++++++++++++++++++++++-------- 4 files changed, 884 insertions(+), 137 deletions(-) create mode 100644 docs/plans/scripting-audit.md create mode 100644 docs/plans/session-proxy.md diff --git a/docs/plans/extensions.md b/docs/plans/extensions.md index c7d0b7476..5d20b1cf1 100644 --- a/docs/plans/extensions.md +++ b/docs/plans/extensions.md @@ -1,14 +1,17 @@ # `.element` Extension Format + App-Side Lua Runtime -> **Phase 0 prerequisite:** [session-scripts.md](session-scripts.md) — session-embedded -> hook scripts. `HookBus`, `el.hooks`, the restricted-environment/capability model, and -> `ScriptingService` are built there first; Phase 3 below then reduces to wiring -> `app.*` events and extension-owned registration. See also [luajit.md](luajit.md) for -> the LuaJIT assessment. +> **Phase 0 prerequisites:** [session-proxy.md](session-proxy.md) — the `SessionProxy` +> through which every topology mutation (UI, undo, Lua, session load) flows and from which +> hook events are dispatched — and [session-scripts.md](session-scripts.md) — console, +> `HookBus`, `el.hooks`, mutation verbs on `el.Session`/`el.Graph`, the +> restricted-environment/capability model, and `ScriptingService`. Those are built first; +> Phase 2 below reduces to graph providers over the model verbs and Phase 3 to +> extension-owned registration. [scripting-audit.md](scripting-audit.md) records the +> starting state. See also [luajit.md](luajit.md) for the LuaJIT assessment. ## Context -Element has a mature Lua substrate (embedded Lua 5.4, sol2 bindings under `src/el/`, a shared `ScriptingEngine` on `Context`) but the app-side surface is thin: no startup script execution (`ScriptingEngine::execute` is declared but never defined), no Lua graph-mutation API, no lifecycle hooks, and GUI scripting is limited to embedded View scripts. The goal is a new **`*.element` extension format** — a directory (like `.lv2`/`.vst3`) acting as a package/extension that can carry Lua modules, hook scripts, views/panels, DSP scripts, graphs (serialized data **or** Lua builder scripts — manifest decides), presets, resources, and bundled plugins (CLAP first) — plus the app-side Lua runtime it requires: graph building, lifecycle hooks, and GUI extensibility. +Element has a mature Lua substrate (embedded Lua 5.4, sol2 bindings under `src/el/`, a shared `ScriptingEngine` on `Context`) but the app-side surface is thin (see [scripting-audit.md](scripting-audit.md)). Phase 0 supplies startup script execution (`ScriptingEngine::execute`, user `init.lua`), the Lua graph-mutation API (model verbs) and lifecycle hooks; GUI scripting remains limited to embedded View scripts until Phase 4. The goal is a new **`*.element` extension format** — a directory (like `.lv2`/`.vst3`) acting as a package/extension that can carry Lua modules, hook scripts, views/panels, DSP scripts, graphs (serialized data **or** Lua builder scripts — manifest decides), presets, resources, and bundled plugins (CLAP first) — plus the app-side Lua runtime it requires: graph building, lifecycle hooks, and GUI extensibility. **Decisions made with user:** - Terminology: **Extension** everywhere (classes, service, Lua modules). Directory suffix stays `.element`. @@ -63,53 +66,52 @@ C++ side: `struct ExtensionManifest` — plain struct parsed once from the sol t |---|---| | `ExtensionManifest`, `Extension` | `src/scripting/extension.hpp/.cpp` | | `ExtensionManager` (scan/load/unload registry) | `src/scripting/extensionmanager.hpp/.cpp` | -| `ScriptingService` (new `Service`) | `src/services/scriptingservice.hpp/.cpp` | -| `HookBus` (C++ event dispatcher) | `src/scripting/hookbus.hpp/.cpp` | -| `el.engine`, `el.hooks`, `el.ui` Lua modules | `src/el/Engine.cpp`, `Hooks.cpp`, `UI.cpp` | +| `ScriptingService` (`Service`, **Phase 0**) | `src/services/scriptingservice.hpp/.cpp` | +| `HookBus` (C++ event dispatcher, **Phase 0**, owned by `Context`) | `include/element/hooks.hpp`, `src/hooks.cpp` | +| `SessionProxy` (**Phase 0**, see session-proxy.md) | `src/engine/sessionproxy.hpp/.cpp` | +| `el.hooks` (**Phase 0**), `el.ui` Lua modules | `src/el/Hooks.cpp` + `hooks.lua`, `src/el/UI.cpp` | | `ViewFactory` (slug → ContentView registry) | `src/ui/viewfactory.hpp/.cpp`, owned by `GuiService` | | `ScriptContentView` + `MissingExtensionView` | `src/ui/scriptcontentview.hpp/.cpp` | Key existing seams to reuse (verified): -- `ScriptingEngine::addPackage(name, loader)` runtime package registry ([scripting.cpp:118](src/scripting.cpp#L118)) — extension Lua modules register here (namespaced; no global `package.path` pollution). -- `EngineService` mutation vocabulary ([engine.hpp](include/element/engine.hpp)): `addGraph/addNode/addPlugin/addConnection/connectChannels/connect(PortType,...)/removeNode/disconnectNode` — Lua rides the existing message/undo/engine-sync path. -- `StandardContent::createContentView(const String&)` virtual, consulted first by `setMainView`/`setSecondaryView` ([standard.hpp:88](include/element/ui/standard.hpp#L88), [standard.cpp:541,633](src/ui/standard.cpp#L541)). -- `NavigationConcertinaPanel::addPanel(desc, factory, header)` public ([navigation.hpp:73](include/element/ui/navigation.hpp#L73)); panel state keys by name — extension slugs must be stable. -- `ScriptView` per-view `sol::environment` + descriptor pattern ([scriptview.cpp](src/ui/scriptview.cpp)) — the sandbox precedent for all extension script execution. -- `SessionService::sigSessionLoaded/sigWillSave` ([sessionservice.hpp:37-38](src/services/sessionservice.hpp#L37-L38)), `EngineService::sigNodeRemoved`. -- `Node::parse` tolerant multi-format graph reader ([node.cpp](src/node.cpp)) for `type="data"` providers. +- `ScriptingEngine::addPackage(name, loader)` runtime package registry ([scripting.cpp:118](../../src/scripting.cpp#L118)) — extension Lua modules register here (namespaced; no global `package.path` pollution). +- Mutation verbs on the models (`Session::addGraph/removeGraph/moveGraph/setActiveGraph`, `Graph::addNode/addPlugin/removeNode/connect/disconnect/connectChannels`), backed by `SessionProxy` ([session-proxy.md](session-proxy.md)) and already bound as `el.Session`/`el.Graph` in Phase 0. `EngineService` keeps its public signatures as thin forwards for UI/undo callers. +- `StandardContent::createContentView(const String&)` virtual, consulted first by `setMainView`/`setSecondaryView` ([standard.hpp:88](../../include/element/ui/standard.hpp#L88), [standard.cpp:541,633](../../src/ui/standard.cpp#L541)). +- `NavigationConcertinaPanel::addPanel(desc, factory, header)` public ([navigation.hpp:73](../../include/element/ui/navigation.hpp#L73)); panel state keys by name — extension slugs must be stable. +- `ScriptView` per-view `sol::environment` + descriptor pattern ([scriptview.cpp](../../src/ui/scriptview.cpp)) — the sandbox precedent for all extension script execution. +- `HookBus` events (`session.loaded`, `session.saving`, `session.closed`, `graph.*`, `node.*`, `connection.*`, `app.started`, `app.shutdown`) — all dispatched in Phase 0; `EngineService::sigNodeRemoved` is gone. +- `Node::parse` tolerant multi-format graph reader ([node.cpp](../../src/node.cpp)) for `type="data"` providers. -Ownership/lifetime rule: `ExtensionManager` is owned by `ScriptingEngine::Impl` (inside the Lua state's lifetime — sol::environment destruction order). `ScriptingService::deactivate()` unloads extensions **before** state teardown. `ScriptingService` registers in `Services` ctor ([services.cpp](src/services.cpp)) after `EngineService`, before `SessionService`, so extension views/graphs exist before the startup session restores. +Ownership/lifetime rule: `ExtensionManager` is owned by `ScriptingEngine::Impl` (inside the Lua state's lifetime — sol::environment destruction order). `HookBus` is owned by `Context` and freed after `services` but before the Lua state ([session-scripts.md](session-scripts.md) § HookBus). `ScriptingService::deactivate()` unloads extensions **before** state teardown. `ScriptingService` already exists from Phase 0 and registers in `Services` ctor ([services.cpp](../../src/services.cpp)) after `EngineService`, before `SessionService`, so extension views/graphs exist before the startup session restores. ## Phases ### Phase 1 — Extension core: format, discovery, module/script loading - New: `extension.hpp/.cpp`, `extensionmanager.hpp/.cpp`, `scriptingservice.hpp/.cpp`, `test/scripting/extensiontests.cpp`, fixture `test/scripting/fixtures/TestPack.element/`. -- Modified: `src/scripting.hpp/.cpp` (implement dead `ScriptingEngine::execute` as protected `lua.script()` in fresh env returning `Result`; expose `extensions()`), `src/datapath.cpp` + `include/element/datapath.hpp` (`defaultExtensionsDir()` = `~/Music/Element/Extensions`, create in `initializeUserLibrary`; also scan `applicationDataDir()/Extensions`; dev env var `ELEMENT_EXTENSIONS_PATH` mirroring `ELEMENT_SCRIPTS_PATH` handling in [bindings.cpp](src/scripting/bindings.cpp)), `src/services.cpp`, `src/scripting/scriptmanager.cpp/.hpp` (**additive** scan — current `scanDirectory` replaces the registry), `include/element/tags.hpp` (`EL_TAG(Extension)`, `tags::extensionId`, `tags::extensionVersion`, `tags::requires`), `src/CMakeLists.txt`, `test/CMakeLists.txt` (+ `add_test`). +- Modified: `src/scripting.hpp/.cpp` (implement dead `ScriptingEngine::execute` as protected `lua.script()` in fresh env returning `Result`; expose `extensions()`), `src/datapath.cpp` + `include/element/datapath.hpp` (`defaultExtensionsDir()` = `~/Music/Element/Extensions`, create in `initializeUserLibrary`; also scan `applicationDataDir()/Extensions`; dev env var `ELEMENT_EXTENSIONS_PATH` mirroring `ELEMENT_SCRIPTS_PATH` handling in [bindings.cpp](../../src/scripting/bindings.cpp)), `src/services.cpp`, `src/scripting/scriptmanager.cpp/.hpp` (**additive** scan — current `scanDirectory` replaces the registry), `include/element/tags.hpp` (`EL_TAG(Extension)`, `tags::extensionId`, `tags::extensionVersion`, `tags::requires`), `src/CMakeLists.txt`, `test/CMakeLists.txt` (+ `add_test`). - Load sequence: scan (parse manifests only, no code) → for enabled extensions: register modules via `addPackage`, register DSP/View scripts with `ScriptManager`, run entry script in `sol::environment(lua, sol::create, lua.globals())`; every failure → `logError`, status `error`, never throws out. Enable/disable persisted in `Settings` (`extensionsDisabled` list). `Commands::reloadExtensions` for dev iteration. - Unload = disconnect hooks, drop env, clear `package.loaded` + registered packages, remove ScriptManager entries and view/panel registrations. Full hot-unload of usertypes is explicitly out of scope (documented). -### Phase 2 — Graph-building Lua API + graph providers -- New: `src/el/Engine.cpp` (`luaopen_el_Engine`), `test/scripting/enginescripttests.cpp`. -- Modified: `src/scripting/bindings.cpp` (register module), `src/el/CMakeLists.txt`, `src/services/scriptingservice.cpp` (provider instantiation), `src/ui/mainmenu.cpp` + commands (File → New Graph From Extension ▸ submenu). -- Facade binds a thin wrapper resolving `EngineService` per call (never bind the service class raw); assert message thread; Lua two-value `nil, "message"` error convention: +### Phase 2 — Graph providers (the Lua graph API comes from Phase 0) +- New: `test/scripting/graphprovidertests.cpp`. +- Modified: `src/services/scriptingservice.cpp` (provider instantiation), `src/ui/mainmenu.cpp` + commands (File → New Graph From Extension ▸ submenu). +- Builder scripts use the Phase 0 model verbs; no `el.engine` module exists. Lua two-value `nil, "message"` error convention: ```lua - local engine = require ("el.engine") - local g = engine.addGraph ("My Rig") - local n = engine.addNode (g, "element.volume") - local p = engine.addPlugin (g, { format = "CLAP", id = "org.surge..." }) - engine.connect (g, p, 0, n, 0) -- optional 5th arg "midi" for PortType - engine.remove (g, n); engine.saveGraph (g, path) + local s = require ('el.Context').instance():session() + local g = s:addGraph ("My Rig") + local n = g:addNode ("element.volume") + local p = g:addPlugin { format = "CLAP", id = "org.surge..." } + g:connectChannels (p, 0, n, 0) -- optional 5th arg "midi" for PortType + g:removeNode (n); g:writeFile (path) ``` - `addPlugin` looks up `context().plugins().getKnownPlugins()` by format + identifier/uid/name. + `addPlugin` looks up `context().plugins().getKnownPlugins()` by format + identifier/uid/name (bound in Phase 0). - Providers: `type="data"` → `Node::parse` → re-UUID (same as `.elg` import) → `EngineService::addGraph(node, true)`. `type="script"` → protected run in fresh env, wrapped in a **single UndoManager transaction** (fallback: per-op undo, documented, if bracketing proves infeasible). - Tests headless with existing `Context` + engine fixtures: builder script topology assertions, data-provider instantiation, plugin-miss returns nil+msg. -### Phase 3 — Hook system -- New: `src/scripting/hookbus.hpp/.cpp`, `src/el/Hooks.cpp`, `test/scripting/hooktests.cpp`. -- Modified: `include/element/engine.hpp` + `src/services/engineservice.cpp` — add `sigNodeAdded`, `sigGraphAdded`, `sigGraphRemoved` at the same sites that fire `sigNodeRemoved`; `scriptingservice.cpp` wires all signals → `HookBus`. -- Events: `app.started`, `app.shutdown`, `session.loaded`, `session.saving`, `graph.added`, `graph.removed`, `graph.changed`, `node.added`, `node.removed`; extensions can `hooks.emit` custom events. -- `HookBus`: message-thread only; handlers are `sol::protected_function` tagged with owner extension id (ExtensionManager sets a "current extension" scope during entry scripts; console registrations = `"user"`); dispatch iterates a copy; reentrancy guard (`dispatching` flag + pending queue); auto-disable a handler after 3 consecutive errors; `removeOwner(id)` on unload. -- Lua: `hooks.on(event, fn) → handle`, `hooks.off(handle)`, `hooks.emit(event, ...)`. +### Phase 3 — Extension-owned hooks (the bus and events come from Phase 0) +- Modified: `src/scripting/extensionmanager.cpp` — set the `HookBus` "current owner" to the extension id while running its entry script so `hooks.action` registrations are tagged automatically; `removeOwner (id)` on unload. `test/scripting/extensiontests.cpp` gains load→unload→load without duplicate handlers. +- Extension default grant is friendlier than session scripts (trusted-but-isolated): `session`, `ui` granted without a prompt; `io` still declared. +- Events and the `el.hooks` API (`hooks.action/filter/off/emit/owner`) are as defined in [session-scripts.md](session-scripts.md); extensions can `hooks.emit` custom events. ### Phase 4 — GUI extensibility - New: `viewfactory.hpp/.cpp`, `scriptcontentview.hpp/.cpp`, `src/el/UI.cpp`. @@ -131,7 +133,7 @@ Ownership/lifetime rule: `ExtensionManager` is owned by `ScriptingEngine::Impl` ## Verification -- Per-phase Boost.Test suites in `test/scripting/` (each registered in `test/CMakeLists.txt` with `add_test`): manifest parse/reject, discovery, `require` resolution, entry-script error isolation (Phase 1); builder-script graph topology + undo-transaction rollback (Phase 2); hook dispatch/auto-disable/removeOwner/reentrancy (Phase 3); ViewFactory register/lookup/missing-placeholder + headless descriptor instantiation (Phase 4); search-path merge + preset discovery from fixture (Phase 5); requires-tree round-trip + missing-extension session load degradation + load→unload→load cycle without duplicate handlers (Phase 6). +- Per-phase Boost.Test suites in `test/scripting/` (each registered in `test/CMakeLists.txt` with `add_test`): manifest parse/reject, discovery, `require` resolution, entry-script error isolation (Phase 1); builder-script graph topology + undo-transaction rollback (Phase 2); extension owner tagging + unload/reload without duplicate handlers (Phase 3, the bus itself is tested in Phase 0); ViewFactory register/lookup/missing-placeholder + headless descriptor instantiation (Phase 4); search-path merge + preset discovery from fixture (Phase 5); requires-tree round-trip + missing-extension session load degradation + load→unload→load cycle without duplicate handlers (Phase 6). - `cmake --build build && ctest --test-dir build --output-on-failure -R Extension...` per suite. - Manual end-to-end after Phase 4: drop `TestPack.element` into `~/Music/Element/Extensions`, launch app, confirm entry script runs, `New Graph From Extension` builds a graph, hooks fire in the Lua console, and the registered view opens via `presentView`. diff --git a/docs/plans/scripting-audit.md b/docs/plans/scripting-audit.md new file mode 100644 index 000000000..b447d3de0 --- /dev/null +++ b/docs/plans/scripting-audit.md @@ -0,0 +1,195 @@ +# App-Side Scripting — Audit (September 2026) + +State of Element's Lua scripting *outside* the ScriptNode, taken before building the +console REPL and the hook system. Companion docs: [session-proxy.md](session-proxy.md) +(the mutation route), [session-scripts.md](session-scripts.md) (Phase 0: hooks, console, +session scripts), [extensions.md](extensions.md), [luajit.md](luajit.md). + +ScriptNode (`src/nodes/scriptnode*`) has its own `sol::state` and its own lifecycle and is +out of scope here. + +## Verdict + +| Layer | State | Evidence | +|---|---|---| +| Lua core: vendored 5.4 + sol2, `el.*` module searcher, `ScriptLoader`, `DSPScript` | **Solid** | Tested in `test/scripting/*`; `ScriptLoader` and `DSPScript` are exercised by ScriptNode in production. | +| Model bindings `el.Session` / `el.Node` / `el.Graph` | **Solid, read-mostly** | Userdata wraps the *live* tree (`std::make_shared (tree, false)`, `SessionPtr`), not copies. Can set name/tempo and save/restore state, nothing else. | +| App-side scripting: `ScriptingEngine`, `ScriptManager`, console, `el.command`, `el.Content` | **Flaky — abandoned mid-build** | Console bootstrap fails silently in dev builds, `el.command` calls a method that does not exist, several declared-but-undefined or permanently dead members (below). | +| Sandboxing | **None** | `Lua::initializeState` opens every stdlib. `ScriptView` creates a `sol::environment` and never applies it, so View scripts embedded in a session run in raw globals with `io`/`os`/`debug`. | +| Event / hook layer | **Absent** | No registry, no emit, no priorities. The signals that exist are asymmetric and incomplete. | + +## 1. The Lua state + +- One root `sol::state` per `Context`, owned by the pimpl `ScriptingEngine::State` + ([scripting.cpp:17-49](../../src/scripting.cpp#L17)). `Context::Impl::init()` creates + the engine and calls `initialize (owner)` ([context.cpp:80-81](../../src/context.cpp#L80)). + Reached via `context().scripting().getLuaState()`. +- Init is two-phase and partly duplicated: the `State` ctor opens `base`+`string` and + installs a searcher `resolve_internal_package` whose `builtins`/`packages` maps are + **never filled** (`fill_builtins` is commented out, `addPackage` has no callers). Then + `Lua::initializeState` ([bindings.cpp:413-435](../../src/scripting/bindings.cpp#L413)) + opens *all* libraries, installs the real searcher `searchInternalModules` (a 160-line + if/else chain kept in sync by hand with ~30 `extern "C"` declarations), and sets + `package.path`, `package.cpath` (always empty) and the non-standard `package.spath`. +- `_G["el.context"]` is a raw `std::ref` ([bindings.cpp:402-405](../../src/scripting/bindings.cpp#L402)), + nilled in `~ScriptingEngine`. Nothing guards Lua values that captured it earlier. +- No thread assertions anywhere in `src/scripting/`, `src/el/`, `src/scripting.cpp`. + Everything app-side runs on the message thread by convention only. + +## 2. The console today + +Classes: `Console` ([console.hpp](../../src/ui/console.hpp)) → `LuaConsole` +([luaconsole.hpp](../../src/ui/luaconsole.hpp)); `LuaConsoleView` +([luaconsoleview.hpp](../../src/ui/luaconsoleview.hpp)) is the `ContentView` that owns a +`LuaConsole` and listens to `Log`. Shown only as the secondary (bottom) pane via +`Commands::showConsole` ([standard.cpp:1154-1163](../../src/ui/standard.cpp#L1154)), +instantiated at [standard.cpp:644-647](../../src/ui/standard.cpp#L644). No feature flag. + +Eval chain: `ConsolePrompt::onReturnKey` → `Console::handleTextEntry` → +`LuaConsole::textEntered` ([luaconsole.cpp:25-73](../../src/ui/luaconsole.cpp#L25)): +compile-probe `"return ;"`, else run the raw text, through +`sol::state_view::script (code, env, "console=")`. The env is created once per view: +`sol::environment (view, sol::create, view.globals())` +([luaconsoleview.cpp:21-26](../../src/ui/luaconsoleview.cpp#L21)) — reads fall through +to `_G`, writes stay local. `print` is an env-local lambda pushing to a `StringArray` +drained by a `Timer`. Up/Down history (100 entries, not persisted). + +Reaching the session works right now: + +```lua +local ctx = require ('el.Context').instance() -- reads _G["el.context"] +local s = ctx:session(); print (#s, s[1].name) +``` + +Defects, in priority order: + +1. **Prelude fails silently.** `setEnvironment` runs + `require('el.script').exec('console', _ENV)`. `el.script.exec` *returns* the error + string instead of raising ([script.lua:30-34](../../src/el/script.lua#L30)), so the + `valid()` check never fires. `scripts/console.lua` is looked up on `package.spath` + (`~/Music/Element/Scripts`, app-data `Scripts`, + `Element.app/Contents/Resources/Scripts`); none exist in a dev build and + `scripts/CMakeLists.txt` only installs to `share/element/scripts`. The scripts *are* + already compiled into binary data (`scripts::console_lua` in `luascripts.hpp`) but + only ScriptNode uses that. +2. **`el.command` is dead.** [command.lua:10-13](../../src/el/command.lua#L10) calls + `Context.instance():commands()`; `el.Context` binds no such method + ([Context.cpp:39-58](../../src/el/Context.cpp#L39)) and `Context` has no accessor — + `Commands` lives on `GuiService::Impl`. Every `command.invoke` asserts. + `scripts/commands.lua` requires a non-existent `el.CommandManager`. +3. **No topology mutation API.** Nothing in `src/el/` calls + `addNode/addPlugin/addConnection/removeNode/addGraph`. +4. **State dies on toggle.** `showConsole` deletes the view; env, history and buffer are + rebuilt each time. +5. **Threading.** `LuaConsoleView::messageLogged` is called from `Log::logMessage` on the + *caller's* thread inside `Log`'s lock and writes a `TextEditor`. `printMessages` is a + plain `StringArray` shared between the Lua `print` lambda and the timer. +6. `_G.print` is swapped for the duration of every eval + ([luaconsole.cpp:33,71](../../src/ui/luaconsole.cpp#L33)); re-entrancy (modal, + `os.exit`) leaves it swapped. `lastError` is never assigned and `errorHandler` is + declared but never defined. The `else` error branch is unreachable (sol throws by + default). `buffer.length()` (characters) is passed as the byte length to + `load_buffer`. No `debug.traceback`. `EL_VIEW_CONSOLE` is `"LuaConsoleViw"` and is + persisted to settings. No default keypress. `console.log` in the prelude writes to + stdout, not the console. + +## 3. `ScriptingEngine` / `ScriptManager` dead weight + +| Item | Where | Fact | +|---|---|---| +| `ScriptingEngine::execute (const String&)` | [scripting.hpp:28](../../src/scripting.hpp#L28) | Declared, never defined. | +| `lua_State* L` | [scripting.hpp:40](../../src/scripting.hpp#L40) | Never assigned. | +| `State::resolve_internal_package`, `builtins`, `packages`, `addPackage` | [scripting.cpp:84-126](../../src/scripting.cpp#L84) | Searcher can never resolve; `addPackage` has no callers. | +| `EL_LUA_SPATH` | [scripting.cpp:10](../../src/scripting.cpp#L10) | Defined, never referenced. | +| `ScriptManager` | [scriptmanager.cpp](../../src/scripting/scriptmanager.cpp) | Never scans in the app: `Application::setupScripting` is `ignoreUnused (scripts)`; `Impl::scanDefaultLoctaion` (sic) has no callers. Test-only. | +| `ScriptInstance::object` | [scriptinstance.hpp](../../src/scripting/scriptinstance.hpp) | Private, no setter, so `cleanup()` is unreachable. | +| `DSPUIScript`, `ScriptSource` | `src/scripting/` | Empty class / no users. | +| `DSPScript::validate` | [dspscript.cpp:434-438](../../src/scripting/dspscript.cpp#L434) | Returns `ok()` for any non-empty string; real body is `#if 0`. | +| `el.vector` | `src/el/vector.c` | Compiled, not registered in `searchInternalModules`. | +| `widget.hpp` `__props` | `src/el/widget.hpp` | Missing comma fuses `"visible" "opaque"` into `"visibleopaque"`. | +| `el/session.lua` | [session.lua:12](../../src/el/session.lua#L12) | Caches the `Session` userdata at require time; stale after reload. | +| `ScriptView::Impl::env` | [scriptview.cpp:24,71-78](../../src/ui/scriptview.cpp#L24) | Created, never passed to `loader.call()`; View scripts run in raw globals. | +| `getLuaCPath`, `getLocalScriptsDir`, `getLocalLuaDirs` | [bindings.cpp](../../src/scripting/bindings.cpp#L79) | Stubs returning empty. `EL_LUADIR`/`EL_SCRIPTSDIR`/`LUA_PATH_DEFAULT` are branched on but defined by no CMake file. | + +## 4. `el.*` bindings inventory + +Lua-source modules (compiled via `src/el/CMakeLists.txt` into `luamods.hpp`): +`el.AudioBuffer`, `el.object` (the proxy/OO system used by widgets), `el.script` +(loader + type constants), `el.session` (two helpers), `el.command` (broken), +`el.strings`, `el.color` (empty). + +C/C++ modules: `el.Context`, `el.Session`, `el.Node`, `el.Graph`, `el.Commands`, +`el.Content`, `el.View`, `el.GraphEditor`, `el.MidiPipe` (defined in +`src/engine/midipipe.cpp`), `el.Widget`, `el.TextButton`, `el.Slider`, `el.Desktop`, +`el.Graphics`, `el.MouseEvent`, `el.Bounds`/`Rectangle`/`Point`/`Range`, +`el.AudioBuffer32/64`, `el.MidiBuffer`, `el.MidiMessage`, `el.audio`, `el.midi`, +`el.bytes`, `el.round`; experimental `el.DocumentWindow`, `el.File`. + +What the model bindings can do today: + +- `el.Session` ([Session.cpp](../../src/el/Session.cpp)): `#s`, `s[i]` (live child + wrapped as `Node`), `name` and `tempo` get/set, `toXmlString`, `saveState`, + `restoreState`. +- `el.Node` / `el.Graph` ([nodetype.hpp](../../src/el/nodetype.hpp)): `name` get/set, + `valid`, `displayName`, `pluginName`, `uuidString`, `nodeId`, `nodeType`, `isGraph`, + `isRoot`, `toXmlString`, `saveState`/`restoreState`, `missing`, `enabled`, + `bypassed`, `muted` (read-only), `writeFile`, `resetPorts`, `hasEditor`, + `hasViewScript`, `viewScript`. The index metamethod returns every child as a plain + `Node`, even graphs. +- `el.Commands`: `invokeDirectly`, static `standard()`, `toString()`. No way to obtain + an instance. +- `el.Content`: `showToolbar`, `presentView (name)`; `presentViewObject` is an empty stub. + +## 5. Notification plumbing + +`include/element/signals.hpp` is three `boost::signals2` aliases. Signals that describe +a model or engine change: + +| Signal | Emitted from | Gap | +|---|---|---| +| `SessionService::sigSessionLoaded` | `refreshOtherControllers()` ([sessionservice.cpp:321-328](../../src/services/sessionservice.cpp#L321)) **and** directly from [pluginprocessor.cpp:800](../../src/pluginprocessor.cpp#L800) | The plugin path skips `EngineService::sessionReloaded()` and `MappingService::refresh()`. | +| `SessionService::sigWillSave` | `saveSession` | — | +| `EngineService::sigNodeRemoved` | `removeNode (const Node&)`, `removeGraph` | Not fired by `removeNode (uint32)` or by `GraphManager::removeNode`. | +| `GuiService::nodeSelected`, `sigRefreshed` | UI | `sigRefreshed` has no subscribers. | +| `DeviceService::sigMidiDevicesChanged`, `sigAudioDeviceStatus` | devices | — | + +Missing entirely: node added, graph added/removed/moved/activated, connection +added/removed, session closed/new. + +- `Session` is a `ChangeBroadcaster`; its four `ValueTree::Listener` overrides all + collapse into one undifferentiated `notifyChanged()` + ([session.cpp:255-294](../../src/session.cpp#L255)). Its only listener is + `SessionDocument` (dirty flag). +- `GraphManager::changed()` is a payload-less `sendChangeMessage()`. +- `AppMessage` types ([messages.hpp](../../src/messages.hpp)) are dispatched by a + `dynamic_cast` chain in `Services::handleMessage`; only `GuiService` overrides + `handleMessage`. +- Commands are integer ids and a `switch` in `GuiService::perform`; no runtime + registration. `Commands::toString` is incomplete, which silently truncates the + constants `command.lua` generates. +- Threading: all `Service` methods and all signals above run on the message thread. + Engine-side state changes reach the message thread through `AsyncUpdater` + (`RootGraphRender`, `GraphNode`, `Processor::EnablementUpdater`, + `AudioProcessorParameterCapture`, `MappingEngine`). + +## 6. Tests + +`test/scripting/`: `ScriptLoaderTest`, `DSPScriptTest`, `PresetScriptsTest`, +`MidiScriptTests`, `BytesTest`, `ScriptInfoTest`, `ScriptManagerTest`, `ScriptPlayground`. + +- `LuaFixture` ([luatest.hpp](../../test/scripting/luatest.hpp)) stands up a bare + `sol::state` with the *no-Context* `initializeState`, so `el.Context`, `el.Session`, + `el.command` and anything needing the app are untestable through it. `resetPaths()` + is CWD-relative while `getSnippetFile()` uses `EL_TEST_SOURCE_ROOT`. +- `test/TestMain.cpp` provides `element::test::context()`, which constructs a `Context` + and activates services — the right fixture for proxy/hook tests. +- `ScriptManagerTest` asserts a hard-coded `getNumScripts() == 13`. +- `test/snippets/sol3_parent.lua` and `stream_from_c.lua` are orphaned. + +## Not verified in this audit + +- Whether any shipped installer places `scripts/*.lua` in the macOS bundle + (`Contents/Resources/Scripts`); only the CMake install rule and the dev bundle were + checked. +- Behaviour of `DSPScript::process`'s unprotected `lua_call` on error with Lua built as + C (`longjmp` vs C++ exceptions) — noted, not exercised. diff --git a/docs/plans/session-proxy.md b/docs/plans/session-proxy.md new file mode 100644 index 000000000..31096b140 --- /dev/null +++ b/docs/plans/session-proxy.md @@ -0,0 +1,282 @@ +# Session Proxy — model-initiated engine changes + +Architectural decision behind [session-scripts.md](session-scripts.md): how Lua (and +anything else) mutates the live session, and where hook events are dispatched from. +See [scripting-audit.md](scripting-audit.md) for the state of the code this builds on. + +## Motivation + +Direction: any modification to engine-layer state is **initiated in the model** +(`Session`, `Graph`, `Node`). Model objects hold a shared internal object — the +*proxy* — that performs the engine-side work and syncs state back. This is explicitly +not "the model emits a signal and hopes a service reacts". + +Today `EngineService` is the only coordination point and it keeps model and engine +aligned by ordering and by index: + +- Root graphs: `EngineService::addGraph (const Node&, bool)` + ([engineservice.cpp:347-386](../../src/services/engineservice.cpp#L347)) creates a + `RootGraphHolder`, `attach (engine)`, *then* `session->addGraph (node, makeActive)`; + `Session::addGraph` ([session.cpp:109](../../src/session.cpp#L109)) is a pure tree op. +- Nodes: `GraphManager::addNode` ([graphmanager.cpp:376-413](../../src/engine/graphmanager.cpp#L376)) + is engine-first: create the `Processor`, copy the tree, set `id`/`object`/`type`, + `nodes.addChild`. +- `EngineService::moveGraph` calls `AudioEngine::moveGraph` then `Session::moveGraph`. + That "pure model primitive + pure engine primitive, coordinated by the service" split + (#1184) was done deliberately so a proxy could call both. + +The proxy is also the natural **single dispatch point for hooks**: if every topology +change passes through it, `graph.added` / `node.removed` / `connection.added` fire once +regardless of source (menu, undoable action, Lua, session load). + +## Options considered + +**A — proxy object stored in the tree** (a ref-counted object under a `tags::proxy` +property, the `tags::updater` idiom). Rejected on lifetime grounds: + +- `Context::Impl::freeAll()` frees `services` first and `session` later + ([context.cpp:94-106](../../src/context.cpp#L94)); `PluginProcessor::~PluginProcessor` + deactivates services and *then* calls `session->clear()` + ([pluginprocessor.cpp:373-390](../../src/pluginprocessor.cpp#L373)). A proxy living in + the tree would hold pointers into a dead `EngineService`/`RootGraphs` unless it is + disarmed on every deactivate path. +- `Session::clear()` (`setMissingProperties (true)`) and `Session::loadData()` (whole-tree + swap, [session.cpp:150-160](../../src/session.cpp#L150)) strip or replace the tree. +- A `var`-held object serializes as `"Object 0x…"` unless added to + `Node::sanitizeProperties`. +- It moves all of `RootGraphs` into an object whose lifetime is governed by `ValueTree` + refcounts. + +**B — service facade** (a `GraphService`/`EngineService` vocabulary plus an `el.engine` +Lua module resolving the service per call). Least change, but the models stay inert, +`EngineService` stays the god object, and model-initiated mutation is not achieved. + +**C — service-owned proxy, registered on the Session, discoverable from the tree.** +Chosen. Lifetime follows the service; the model holds only a `weak_ptr`; a tiny handle +in the tree lets any `Node` copy find its `Session`. + +## Design + +### Ownership and lifetime + +- `SessionProxy` (`src/engine/sessionproxy.hpp/.cpp`) becomes the home of + `RootGraphHolder` and `RootGraphs`, moved verbatim from + [engineservice.cpp:78-305](../../src/services/engineservice.cpp#L78). The constructor + captures `Context&`, `AudioEnginePtr`, `SessionPtr` and `HookBus&`. The destructor + never calls `context()` (services may already be gone). +- `EngineService` owns `std::shared_ptr proxy`: created in `activate()` + after `engine->activate()` and before `sessionReloaded()`, followed by + `session->setProxy (proxy)`; in `deactivate()` after `session->saveGraphState()`: + `session->setProxy (nullptr)` then `proxy.reset()` (replaces `graphs->clear()`). +- `Session::Impl` ([session.cpp:78-90](../../src/session.cpp#L78), currently empty) holds + `std::weak_ptr proxy`. Mis-ordered teardown degrades to "no proxy → + pure tree op", never a dangling call. +- The proxy survives session reload for free: `loadData()`/`clear()` touch + `objectData`, not `Impl`. Holders are rebuilt by `sessionReloaded()` → `proxy->reload()`. + +### How a `Node` copy finds the proxy + +`Node` is a value type; copies share only the tree. So the tree must lead back to the +`Session`: + +- `Session::Handle : juce::ReferenceCountedObject { Session* session; }` is planted on + the session root tree under a new `tags::sessionObject` — the same idiom as + `GraphManager` planting `NodeModelUpdater` under `tags::updater` + ([graphmanager.cpp:651-668](../../src/engine/graphmanager.cpp#L651)). +- `static SessionPtr Session::findFor (const juce::ValueTree& any)` reads + `any.getRoot()[tags::sessionObject]` → handle → `Session*` → `SessionPtr`. +- Plant: `Session` ctor, `loadData`, `setMissingProperties (reset = true)`. + Null: `~Session()` before `clear()`. + Ignore: `Session::valueTreePropertyChanged` + ([session.cpp:262](../../src/session.cpp#L262), alongside `object`/`updater`). + Strip: `Node::sanitizeProperties` ([node.cpp:349](../../src/node.cpp#L349)), which + `createXml`/`writeToFile` already call on the session copy. +- Detached copies (`.elg` export, `RemoveNodeAction::nodeData`, `createCopy()` of a + graph subtree) carry no root handle → no proxy → pure tree ops. That is exactly the + headless/undo-friendly behaviour wanted. + +### `SessionProxy` API + +All methods return `bool`/`Node`, never show an `AlertWindow`, and assert the message +thread. + +```cpp +// root graphs +bool addGraph (const Node& graph, bool makeActive); +Node addGraph (const juce::String& name); // default ports from engine->getNumChannels +bool removeGraph (int index); +bool moveGraph (const Node& graph, int newIndex); +bool setActiveGraph (int index); // today's setRootNode + Session::setActiveGraphData +void reload(); // today's sessionReloaded, under HookBus::ScopedSuspend +void clear(); void syncModels(); +// nodes +Node addNode (const Node& graph, const Node& nodeTemplate, const ConnectionBuilder& = {}); +Node addPlugin (const Node& graph, const juce::PluginDescription&, double rx = .5, double ry = .5); +bool removeNode (const Node&); // incl. the IO-port workaround at engineservice.cpp:717-738 +Node replace (const Node&, const juce::PluginDescription&); +bool changeBusesLayout (const Node&, const juce::AudioProcessor::BusesLayout&); +// connections +bool addConnection (const Node& graph, uint32 s, uint32 sp, uint32 d, uint32 dp); +bool removeConnection (const Node& graph, uint32 s, uint32 sp, uint32 d, uint32 dp); +bool connectChannels (const Node& graph, const Node& src, int sc, const Node& dst, int dc, + PortType = PortType::Audio, int count = 1); +void disconnectNode (const Node&, bool inputs, bool outputs, bool audio, bool midi); // per-arc removeConnection +// lookup +GraphManager* findGraphManagerFor (const Node&) const; +RootGraphManager* findActiveRootGraphManager() const; +``` + +`addNode`/`addPlugin` pre-validate through `PluginManager::findDescriptionFor` / +`getKnownPlugins()` so a bad identifier from Lua returns an invalid `Node` instead of +reaching the modal `AlertWindow` inside `GraphManager::addNode` +([graphmanager.cpp:380,408](../../src/engine/graphmanager.cpp#L380)). + +### Model verbs + +`Session` (`include/element/session.hpp`, `src/session.cpp`): + +- `setProxy (std::shared_ptr)`, `proxy()`, `static findFor (ValueTree)`. +- Proxy-aware, existing names keep working: `addGraph (const Node&, bool)`, + `moveGraph (int, int)`, `setActiveGraph (int)`; new `removeGraph (int)`, + `Node addGraph (const juce::String& name)`. Each is + `if (auto p = proxy()) return p->X (...); return XData (...);`. +- Pure primitives stay public, named like `loadData`: `addGraphData`, `removeGraphData`, + `moveGraphData`, `setActiveGraphData`. +- `SessionService::loadNewSessionData` + ([sessionservice.cpp:316](../../src/services/sessionservice.cpp#L316)) switches to + `addGraphData`; with a proxy installed at that point the default graph would attach + immediately and then be detached/re-attached by `refreshOtherControllers()`. + +`Graph` (`include/element/graph.hpp`, `src/graph.cpp`): + +- `Node addNode (const Node& nodeTemplate)`, `Node addNode (const juce::String& id, const juce::String& format = EL_NODE_FORMAT_NAME)`, + `Node addPlugin (const juce::PluginDescription&)`, `bool removeNode (const Node&)`, + `bool connect (uint32, uint32, uint32, uint32)`, `bool disconnect (…)`, + `bool connectChannels (const Node& src, int sc, const Node& dst, int dc, PortType = Audio)`. +- Fallback without a proxy: copy the template into `nodes` with ids reset (what + `SessionLoadBenchTests::makeGraphModel` does by hand), append/remove an `Arc` in + `arcs`. +- `graph.cpp` includes `` and `"engine/sessionproxy.hpp"`; + `graph.hpp` forward-declares only. + +### `EngineService` afterwards + +Every public signature in [engine.hpp](../../include/element/engine.hpp) stays, as a +thin forward to the proxy plus the UI/plugin-list policy that does not belong in the +proxy: `AlertWindow` messages, `presentPluginWindow` (only on the entry points that show +it today, so undo/redo never pops windows), `detail::verifyPlugin` / `saveUserPlugins` / +`addToKnownPlugins`, `addMidiDeviceNode` (rewritten over `proxy->addPlugin` + +`Node::getObject()`), `stabilizeViews` after `replace`/`changeBusesLayout`. + +Callers that keep compiling unchanged: `src/messages.cpp` undo actions, +`src/services.cpp:224-320`, `guiservice.cpp:1070-1073`, +`sessiontreepanel.cpp:592/693/1079`, `sessionservice.cpp:104`. + +The active-graph-only overloads (`addConnection (s, sp, d, dp)`, `removeNode (uint32)`) +resolve `session->getActiveGraph()` and forward — this also fixes `removeNode (uint32)` +([engineservice.cpp:755-763](../../src/services/engineservice.cpp#L755)) bypassing +notification today. + +Moves out of `EngineService` entirely: `RootGraphHolder`, `RootGraphs`, the bodies of +`addGraph (Node, bool)`, `removeGraph`, `moveGraph`, `setRootNode`, `sessionReloaded`, +`syncModels`, `addNode (Node, Node, Builder)`, private `addPlugin (GraphManager&, …)`, +`removeNode (Node)`, the graph overloads of `addConnection`/`removeConnection`, +`connectChannels`/`connect`, `disconnectNode`, `replace`, `changeBusesLayout`, `clear`. + +`sigNodeRemoved` is deleted. Its consumer +([grapheditorview.cpp:80,158](../../src/ui/grapheditorview.cpp#L80)) subscribes to +`graph.removed` on `context().hooks()` instead. + +### Undo + +No structural change. `AddPluginAction`, `RemoveNodeAction`, `AddConnectionAction`, +`RemoveConnectionAction` ([messages.cpp](../../src/messages.cpp)) keep calling +`EngineService`; each `perform()`/`undo()` is exactly one proxy mutation and therefore +exactly one hook. Lua-initiated mutations are **not undoable** in this phase: they bypass +`GuiService::handleMessage` +([guiservice.cpp:1235-1245](../../src/services/guiservice.cpp#L1235)). Making them +undoable later means posting `AppMessage`s, which is asynchronous and cannot return the +created node — a deliberate non-goal for now. + +### GUI reacts through hooks + +- `GuiService::activate()` registers handlers (ids stored, removed in `deactivate()`): + `node.removing` → `closePluginWindowsFor (node, true)` + deselect (moved from + [engineservice.cpp:706-712](../../src/services/engineservice.cpp#L706)); + `graph.removed` → `stabilizeContent()` (replaces the `// FIXME: dont notify the UI + top-down` at `:489`); `graph.activated` → close/show plugin windows (moved from + [sessiontreepanel.cpp:618-627](../../src/ui/sessiontreepanel.cpp#L618) and + [guiservice.cpp:171-175](../../src/services/guiservice.cpp#L171)). +- Direct writes of `tags::active` in `sessiontreepanel.cpp:624-626`, `:758` and + `guiservice.cpp:173-174` become `session->setActiveGraph (index)`; otherwise + Lua-initiated activation and hooks diverge from the UI path. + +### Hook dispatch points + +Only `SessionProxy` fires topology actions: `graph.added`, `graph.removing`, +`graph.removed`, `graph.moved`, `graph.activated`, `node.added`, `node.removing`, +`node.removed`, `connection.added`, `connection.removed`. Pre-hooks (`*.removing`) exist +so the GUI can close plugin windows *before* the processor dies regardless of who +initiated the removal. + +- `reload()` runs under `HookBus::ScopedSuspend`, so session load fires nothing per node + (`GraphManager::setNodeModel` and `IONodeEnforcer` operate below the proxy anyway) and + the internal `setActiveGraph` during load does not leak `graph.activated`. +- `session.loaded` is fired once from a new `SessionService::notifySessionLoaded()`, + which also emits `sigSessionLoaded()`; called from `refreshOtherControllers()` and + from [pluginprocessor.cpp:800](../../src/pluginprocessor.cpp#L800) in place of the + direct emit. Not fired from `reload()`: `EngineService::sessionReloaded()` is also + called by `PluginProcessor::reloadEngine()` on every host re-prepare. +- `session.saving` next to `sigWillSave`; `session.closed` in `closeSession()`. +- `Session::ScopedFrozenLock` is left alone: it guards the document-dirty + `ChangeBroadcaster`, a different concern, and `Services::run()` keeps the whole + default-session open frozen, which would wrongly swallow a legitimate `graph.added` + from an `.elg` import. + +## Tests + +`test/SessionProxyTests.cpp`, suite `SessionProxyTests`, using `element::test::context()` +([TestMain.cpp:15-23](../../test/TestMain.cpp#L15), which activates services so the proxy +is installed) and a test node provider extracted from `CountingNodeProvider` +([SessionLoadBenchTests.cpp:58-82](../../test/SessionLoadBenchTests.cpp#L58)) into +`test/fixture/TestNodeProvider.h`: + +- `ProxyInstalledAndCleared` — own `Context`, `services().activate()/deactivate()`. +- `AddGraphAttachesEngine` — `session->addGraph` → `getObject()` non-null, `graph.added` once. +- `AddNodeCreatesProcessor`, `ConnectDisconnect` (arc present in `arcs`, hooks fired). +- `RemoveNodeOrdering` — `node.removing` before `node.removed`. +- `RemoveGraphFixesActive`, `SetActiveGraphSyncsEngine` (`engine->getActiveGraphIndex()`). +- `ReloadFiresNoPerNodeHooks` — `loadData` + `sessionReloaded()` → zero `node.added`. +- `FallbackWithoutProxy` — bare `Context`, `Graph::addNode` adds a tree child, `getObject()` null. + +Register with `add_test (NAME "SessionProxyTests" COMMAND test_element --run_test=SessionProxyTests)`. +Existing `SessionTests` keep covering the pure-tree path unchanged. Remember: a +`GraphNode` driven by `GraphManager` must be heap-allocated (`ProcessorPtr keep (new GraphNode (ctx))`). + +## Risks / not verified + +- Headless attach with real graphs: `SessionTests::MoveEngineGraph` proves + `AudioEngine::addGraph` works without a device and `test::context()` runs + `sessionReloaded()` with zero graphs, but the full `attach` + `setRootNode` + (`setPlayConfigFor (devices)`) path with real graphs has no current test; sample + rate/block size may be 0 headless. +- `GraphManager::addNode` shows *modal* alerts on instantiation failure; the proxy's + pre-validation must cover every path or a headless test hangs. What `createGraphNode` + does with unknown identifiers was not traced. +- Engine-initiated active-graph changes (MIDI program change, + [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657), writes `tags::active` + from an async update) bypass the proxy. If `graph.activated` must cover them, the + proxy needs a `ValueTree::Listener` on the `graphs` child with a self-change flag. + Follow-up. +- `PluginProcessor::reloadEngine` → `reload()` must keep the full detach/re-attach + behaviour; `prepareExternalPlayback` re-preparation was not traced. +- Re-entrant hooks (a handler adding a node from `node.added`) work through the depth + guard but interleave `GraphManager::changed()` broadcasts. Untested territory. +- `RootGraphHolder` touches `Processor` internals as `friend class EngineService` + ([processor.hpp:539](../../include/element/processor.hpp#L539)); `SessionProxy` may + need adding to the friend list. +- `new_nodetype`'s index metamethod already returns non-graph children as + `Graph` today; the `Graph (const Node&)` constructor asserts type == Graph in debug + builds. The `sol::make_object` split in session-scripts.md is required, not optional. +- The #1184 issue text was not read; the split-primitive intent is inferred from the + `Session::moveGraph` doc comment and `EngineService::moveGraph`. diff --git a/docs/plans/session-scripts.md b/docs/plans/session-scripts.md index 3328f6e70..c9892ea00 100644 --- a/docs/plans/session-scripts.md +++ b/docs/plans/session-scripts.md @@ -1,9 +1,18 @@ -# Phase 0 — Session Scripts & the Hook System +# Phase 0 — Console, Hooks & Session Scripts -Predecessor to [extensions.md](extensions.md). Front-loads the pieces the extension format -needs anyway — the hook bus, the script descriptor conventions, and the restricted -environment / capability model — but delivers them first for scripts **embedded in the -session file**, where DSP/DSPUI scripts already live today. +Predecessor to [extensions.md](extensions.md). Front-loads what the extension format +needs anyway — the hook bus, the script descriptor conventions, the restricted +environment / capability model — and delivers them first for the Lua **console** and for +scripts **embedded in the session file**, where DSP/DSPUI scripts already live today. + +Prerequisites and companions: + +- [scripting-audit.md](scripting-audit.md) — what exists today and what is broken. +- [session-proxy.md](session-proxy.md) — the `SessionProxy`: model-initiated engine + mutation and the single hook dispatch point. **Read it first**; this document assumes + it. Consequences here: hooks fire from the proxy, and Lua mutates the session through + model verbs on `el.Session`/`el.Graph`. The `el.engine` facade module previously + planned is dropped. ## Context @@ -12,58 +21,244 @@ gzip'd code), with `Graph::findViewScript()` resolving the `View` script and `Sc embedding DSP source in its state blob. The Session root does **not** have a scripts tree (`src/session.cpp` — children are `graphs`, `controllers`, `maps`, `midiMappings`, `ui`). -Phase 0 extends the same pattern to the session: sessions can carry scripts that *run* at -defined lifecycle points. Scripts follow the **same descriptor format planned for -extensions**, so a script is portable between "written into the session" and "shipped in -a `.element` extension" without edits. +The console exists (`src/ui/luaconsole.cpp`) and evaluates in the shared app state, but +its prelude never loads in dev builds, `el.command` is broken, and there is no way to +change graph topology from Lua. There is no hook/event registry of any kind. ## Design principles 1. **WordPress-inspired hooks, not a clone.** Two dispatch kinds: - **actions** — fire-and-forget notifications (`node.added`, `session.loaded`, ...) - **filters** — each handler receives a value and returns a (possibly modified) value; - the host uses the final result (e.g. filter a display name, a save payload, a menu). - Handlers have an optional integer priority (default 10, lower runs first) and an owner - tag for bulk teardown. No WordPress-style global mutable everything — events are - explicit, dispatch is message-thread only, and the C++ side owns the registry. + the host uses the final result. + Handlers have an integer priority (default 10, lower runs first) and an owner tag for + bulk teardown. Events are explicit, dispatch is message-thread only, and the C++ side + owns the registry. 2. **Untrusted by default — capabilities are earned.** A session file is a *document*; - documents that carry executable code must not silently get the keys to the app. Session - scripts run in a restricted `sol::environment`: - - Base env: safe stdlib subset (no `io`, no `os` beyond `time`/`clock`, no `require` - of arbitrary modules), plus pure-data `el.*` modules (bytes, midi, colors, strings). - - Anything powerful — `el.engine`, `el.Context`, `el.ui`, file access — must be - declared in the script descriptor (`requires = { "engine", "ui" }`) and granted by - the host. Grant policy v1: per-session trust prompt on first run ("This session - contains scripts that want: engine access. Run / Run always for this session / - Don't run"), persisted in settings keyed by a session content hash or path. - - Extensions (user-installed) get a more permissive default later; the *mechanism* - (declared requires → injected capabilities) is identical. - -3. **Bind less C++, write more Lua.** The rule going forward: C++ binds a minimal opaque - userdata handle ("impl"), and the ergonomic API is a native Lua module holding that - handle as a private member. Precedent already in-tree: `src/el/object.lua`, - `session.lua`, `command.lua`, `script.lua` wrap C bindings in Lua tables. New surface - (`el.hooks`, later `el.engine` sugar) should be Lua-first with a thin C core — this - avoids sol2 usertype boilerplate for every class and keeps the public script API - decoupled from C++ headers. + documents that carry executable code must not silently get the keys to the app. + Session scripts run in a restricted `sol::environment`; anything powerful must be + declared in the descriptor (`requires = { 'session' }`) and granted by the host. + +3. **Bind less C++, write more Lua.** C++ binds a minimal opaque handle; the ergonomic + API is a native Lua module holding it. Precedent: `src/el/object.lua`, `command.lua`, + `script.lua`. New surface (`el.hooks`) is Lua-first with a thin C core. + +4. **One mutation path.** Every topology change — UI, undo, Lua, session load — goes + through `SessionProxy`. Hooks are dispatched there and nowhere else for those events. ## What gets built -### 1. Model: session-level scripts +### 1. `HookBus` (`include/element/hooks.hpp`, `src/hooks.cpp`) + +A plain registry; no `boost::signals2` groups needed. + +```cpp +struct HookEvent { + juce::String action; + Node node; // the node/graph the event is about (invalid if n/a) + Node graph; // its parent graph, or the root graph for graph.* events + Arc arc; // connection.* events + int index = -1; // graph.moved / graph.activated +}; + +class HookBus { +public: + using Action = std::function; + using Filter = std::function; + + int addAction (const juce::String& name, Action, int priority = 10, const juce::String& owner = {}); + int addFilter (const juce::String& name, Filter, int priority = 10, const juce::String& owner = {}); + void remove (int id); + void removeOwner (const juce::String& owner); + + void doAction (const juce::String& name, const HookEvent& = {}); + juce::var applyFilters (const juce::String& name, juce::var value, const HookEvent& = {}); + bool hasActions (const juce::String& name) const; + + struct ScopedSuspend { explicit ScopedSuspend (HookBus&); ~ScopedSuspend(); }; + +private: + struct Handler { int id; int priority; juce::String owner; Action action; Filter filter; int errors = 0; bool enabled = true; }; + std::map> actions, filters; + int suspendCount = 0, depth = 0; + std::deque> pending; +}; + +namespace hooks { +constexpr const char* appStarted = "app.started"; +constexpr const char* appShutdown = "app.shutdown"; +constexpr const char* sessionLoaded = "session.loaded"; +constexpr const char* sessionSaving = "session.saving"; +constexpr const char* sessionClosed = "session.closed"; +constexpr const char* graphAdded = "graph.added"; +constexpr const char* graphRemoving = "graph.removing"; +constexpr const char* graphRemoved = "graph.removed"; +constexpr const char* graphMoved = "graph.moved"; +constexpr const char* graphActivated = "graph.activated"; +constexpr const char* nodeAdded = "node.added"; +constexpr const char* nodeRemoving = "node.removing"; +constexpr const char* nodeRemoved = "node.removed"; +constexpr const char* connectionAdded = "connection.added"; +constexpr const char* connectionRemoved = "connection.removed"; +constexpr const char* graphDefaultName = "graph.defaultName"; // filter +} +``` + +Rules: + +- `doAction`/`applyFilters` assert the message thread. While `suspendCount > 0` they are + no-ops. +- Dispatch iterates a *copy* of the handler list sorted by priority (handlers may add or + remove during dispatch). +- Nested `doAction` (a handler mutating topology) is queued in `pending` and drained after + the outer dispatch returns; `depth` is asserted `< 8`. +- Every handler call is wrapped; an exception increments `errors`, three consecutive + errors disable the handler; a success resets the count. Errors go to + `Context::logger()` (and, for Lua handlers, `ScriptingEngine::logError`). Nothing + propagates. +- Filters thread the `juce::var`; a handler returning `void`/`nil` leaves the value + unchanged. + +Ownership: `Context::Impl` owns `std::unique_ptr hooks` with accessor +`HookBus& Context::hooks()`. Created in `init()` **before** `services` (so services +subscribe in `activate()`). In `freeAll()` +([context.cpp:94-106](../../src/context.cpp#L94)) reset **right after `services`** and +before `lua`: Lua-backed handlers hold `sol::protected_function`s and must die while the +state is alive. + +Proof-of-shape filter: `graph.defaultName`, applied in `SessionProxy::addGraph (name)` +to the generated `"Graph N"` string. Single call site, no UI paint-path cost. Filters +like `node.displayName` are deferred until a call site that is not in a paint loop is +chosen. + +### 2. Dispatch sites + +See [session-proxy.md](session-proxy.md) § *Hook dispatch points*. Summary: every +`SessionProxy` mutation fires its action; `reload()` runs under `ScopedSuspend`; +`SessionService::notifySessionLoaded()` fires `session.loaded` (replacing the direct +`sigSessionLoaded()` at `pluginprocessor.cpp:800` too); `session.saving` beside +`sigWillSave`; `session.closed` in `closeSession()`; `app.started` at the end of +`Services::launch`; `app.shutdown` in `ScriptingService::deactivate()`. + +`EngineService::sigNodeRemoved` is deleted; `GuiService` and `GraphEditorView` subscribe +to hooks instead (details in the proxy doc). + +### 3. Console foundation (`src/ui/luaconsole.*`, `luaconsoleview.*`, `src/scripting.*`) + +- **Evaluator in the engine.** Implement the declared-but-missing + `ScriptingEngine::execute` as + `juce::Result execute (const juce::String& code, sol::environment env)`: the + `"return ;"` compile probe, `sol::protected_function` with a `debug.traceback` + handler, byte length from `toRawUTF8()`. Result message carries the traceback. + `LuaConsole::textEntered` becomes a thin caller; drop the `_G.print` swap, the dead + `lastError`/`errorHandler`, and the unreachable error branch. +- **Persistent environment.** `sol::environment& ScriptingEngine::consoleEnvironment()`, + lazily created with globals fallback (`sol::environment (state, sol::create, + state.globals())`). `LuaConsoleView::initializeView` uses it, so variables, `print` + override and registered hooks survive view toggles. History moves alongside it + (`ScriptingEngine::consoleHistory()` or keep it in the env as a Lua table). +- **Prelude from binary data.** `LuaConsole::setEnvironment` runs `scripts::console_lua` + (`luascripts.hpp`, already compiled by `scripts/CMakeLists.txt`) in the env with + `sol::script_pass_on_error` and reports failures to the console. `el.script.exec` + is changed to `error()` when `load` fails instead of returning the message. + `scripts/console.lua` gains: + + ```lua + hooks = require ('el.hooks') + Context = require ('el.Context') + session = function() return Context.instance():session() end -- a function: never cache the session + console.log = function (...) print (...) end -- to the console, not stdout + ``` + +- **Threading.** `LuaConsoleView::messageLogged` posts to the message thread with + `juce::MessageManager::callAsync` and a `Component::SafePointer`; `printMessages` is + guarded by a `juce::CriticalSection` (or replaced with an `AsyncUpdater`). +- **`el.command`.** `GuiService::activate()` sets `_G["el.commands"]` to a + `std::ref` (mirroring `el.context`, cleared in `deactivate()`); + `el.Commands.instance()` reads it; `command.lua` uses that instead of + `Context:commands()`. `scripts/commands.lua` is deleted. +- Cosmetic: `EL_VIEW_CONSOLE` becomes `"LuaConsoleView"` (accept the old + `"LuaConsoleViw"` when restoring `ContentContainer_lastSecondaryView`); `showConsole` + gets a default keypress alongside `showPatchBay`/`showGraphEditor`. + +### 4. Lua surface for mutation (`src/el/Session.cpp`, `Graph.cpp`, `nodetype.hpp`) + +All of these forward to the model verbs defined in the proxy doc and therefore fire +hooks and work headless (pure tree) when no proxy is installed. + +`el.Session`: + +```lua +local s = session() +g = s:addGraph ("Rig" [, active]) -- string or el.Graph template; returns el.Graph +s:removeGraph (index) s:moveGraph (from, to) s:setActiveGraph (index) +s:activeGraph() s.activeGraphIndex s:findNodeById (uuid) +s[i] -- now returns el.Graph, not el.Node +``` + +`el.Graph`: + +```lua +n = g:addNode ("element.volume" [, format]) -- returns el.Node or nil, "message" +p = g:addPlugin { format = "VST3", name = "..." | id = "..." } +g:removeNode (n) +g:connect (srcId, srcPort, dstId, dstPort) g:disconnect (...) +g:connectChannels (src, sc, dst, dc [, "midi"]) +for node in g:nodes() do ... end for arc in g:connections() do ... end +``` + +- `luaopen_el_Session` requires `el.Graph` (as `Context.cpp:60-63` requires `Node`/`Session`). +- In `nodetype.hpp` the index metamethod returns a `Graph` userdata when + `child.isGraph()` via `sol::make_object`, else a `Node`, so the `Graph (const Node&)` + type assert never fires for plain nodes. +- Lua two-value error convention: `nil, "message"`; never throws into the console. + +### 5. `el.hooks` — Lua-first + +Thin C core `src/el/Hooks.cpp` (`luaopen_el_Hooks`), registered in +`searchInternalModules` and `src/el/CMakeLists.txt`: + +``` +register (kind, name, fn, priority, owner) -> id -- kind = "action" | "filter" +unregister (id) +unregister_owner (owner) +emit (name, event_table) +``` + +Wrapped by `src/el/hooks.lua`: + +```lua +local hooks = require ('el.hooks') +local id = hooks.action ('node.added', function (e) print (e.node.name, e.graph.name) end [, priority]) +hooks.filter ('graph.defaultName', function (name, e) return name .. ' *' end) +hooks.off (id) +hooks.emit ('my.event', { ... }) -- custom events +local mine = hooks.owner ('my-script') -- scoped table: same API, owner pre-filled +mine.action (...); mine.clear() +``` + +Handlers receive `{ action=, node=, graph=, arc=, index= }`. +Filters receive `(value, event)` and return the value. The C side keeps a per-state map +`id → sol::protected_function` so `unregister` and state teardown can drop references. +Console registrations default to owner `"console"`; `ScriptingService` sets a "current +owner" while running a script so `hooks.action` picks it up without the caller passing +it. + +### 6. Model: session-level scripts - `Session` gains a `scripts` child tree, identical shape to `Node::getScriptsValueTree()` - (`include/element/node.hpp:434`). Add `Session::scripts()` / `addScript()` / - `removeScript()` accessors mirroring `Node::addScript` (`src/node.cpp:476`). -- New script type tag: `EL_TAG(Hook)` in `include/element/tags.hpp` (joins `DSP`, `View`, - `GraphView`, `Anonymous`). `Script::make` (`src/script.cpp:183`) accepts `types::Hook` - and seeds a template. + ([node.hpp:442](../../include/element/node.hpp#L442)). Add `Session::scripts()`, + `addScript()`, `removeScript()` mirroring `Node::addScript` + ([node.cpp:476](../../src/node.cpp#L476)). +- New script type tag `EL_TAG (Hook)` in `include/element/tags.hpp` (joins `DSP`, + `View`, `GraphView`, `Anonymous`). `Script::make` ([script.cpp:183](../../src/script.cpp#L183)) + accepts `types::Hook` and seeds the template below. - Additive child tree — no `EL_SESSION_VERSION` bump; add a no-op guard in `Session::migrate`. -- Persistence is free: `Script` code is already gzip'd into the tree, so `.els` stays a - single file. +- Persistence is free: `Script` code is already gzip'd into the tree. -### 2. Hook script descriptor (portable session ↔ extension) +### 7. Hook script descriptor (portable session ↔ extension) ```lua --- Session hooks example. @@ -71,90 +266,163 @@ a `.element` extension" without edits. -- @type Hook return { type = 'Hook', - requires = { 'engine' }, -- capabilities this script needs - attach = function (hooks, ctx) -- called once when the script is activated + requires = { 'session' }, -- capabilities this script needs + attach = function (hooks, ctx) -- called once when the script is activated hooks.action ('session.loaded', function() ... end) - hooks.action ('node.added', function (node) ... end, 20) -- priority - hooks.filter ('node.displayName', function (name, node) + hooks.action ('node.added', function (e) ... end, 20) -- priority + hooks.filter ('graph.defaultName', function (name, e) return name .. ' *' end) + if ctx.session then ctx.session():addGraph ('Auto') end end, - detach = function() ... end, -- optional cleanup + detach = function() ... end, -- optional cleanup } ``` -`ctx` is the capability table: only granted entries are present (`ctx.engine`, -`ctx.context`, ...). Scripts that got nothing still get `hooks` — pure observers that, -e.g., filter cosmetic values, need no grants at all. +`ctx` is the capability table: only granted entries are present. Scripts that got nothing +still get `hooks` — pure observers and cosmetic filters need no grants. + +### 8. Restricted environment and capabilities + +`ScriptingEngine::createRestrictedEnvironment()` returns a `sol::environment` with **no** +globals fallback, populated with: + +- Base: `assert error ipairs next pairs pcall select tonumber tostring type unpack + xpcall rawequal rawget rawset setmetatable getmetatable` and the tables `string`, + `table`, `math`, `utf8`; `os` limited to `time clock date difftime`. No `io`, no + `debug`, no `load`/`loadstring`/`dofile`/`loadfile`. +- `require` replaced by a function that only resolves an allowlist: + `el.bytes`, `el.midi`, `el.strings`, `el.color`, `el.object`, `el.hooks`, + `el.MidiMessage`, `el.MidiBuffer`, `el.AudioBuffer`. + +Capabilities → `ctx` keys, granted per script from its `requires`: + +| `requires` entry | `ctx` key | What it injects | +|---|---|---| +| `session` | `ctx.session` | function returning the live `el.Session` (carries mutation) | +| `engine` | `ctx.engine` | alias of `session` for readability; reserved for future engine-only calls | +| `ui` | `ctx.ui` | `el.Content` + `el.Commands.instance()` | +| `io` | `ctx.io` | the real `io` table | + +The same restricted env (with the widget modules `el.View`, `el.Widget`, `el.Slider`, +`el.TextButton`, `el.Graphics`, `el.Bounds`… added to the allowlist) is applied in +`ScriptView::setScript` via `loader.call (env)`, closing the "View scripts run in raw +globals" hole. + +### 9. Trust + +- `Settings` gains `sessionScriptTrust`: map of *scripts-subtree SHA1* → `allow | deny`. + Hashing only the `scripts` subtree invalidates trust on any edit and ignores unrelated + session changes. +- On `session.loaded`, if any `Hook` script has a non-empty `requires` and the hash is + unknown: asynchronous `AlertWindow` — "This session contains scripts that want: session + access. **Run** / **Always for this session** / **Don't run**". *Run* grants for this + process only; *Always* persists `allow`; *Don't run* persists `deny`. Scripts with + empty `requires` attach immediately with no prompt. +- A "Re-ask trust" action clears the entry for the current hash. + +### 10. Runner: `ScriptingService` (`src/services/scriptingservice.hpp/.cpp`) + +New `Service`, registered in the `Services` ctor after `EngineService` and before +`SessionService`. Per the teardown rule, `activate()` captures `HookBus*`, +`ScriptingEngine*`, `SessionService*`, `Settings*` and never calls `context()` in the +destructor. -### 3. `HookBus` (C++, `src/scripting/hookbus.hpp/.cpp`) +- `activate()`: subscribe `session.loaded` → `attachAll()`; `session.closed` → `detachAll()`; + run `~/Music/Element/Scripts/init.lua` if present (owner `"user"`, globals-fallback env, + no trust prompt — user-installed); fire `app.started` when `Services::launch` completes. +- `attachAll()`: for each `Hook` script in `session->scripts()`: restricted env → + protected run → descriptor table → check `requires` against grants (prompt if needed) + → `attach (hooks_for_owner, ctx)`. Owner id = the script's tree UUID. +- `detachAll()` / reload: call `detach` (protected), `HookBus::removeOwner (uuid)`, drop + the env. +- `reattach (Script)`: called by `ScriptEditorView` after saving a hook script (detach → + re-run), so edits take effect without reloading the session. +- `deactivate()`: `detachAll()`, fire `app.shutdown`, unsubscribe. -Same class the extensions plan specifies (its Phase 3 shrinks to "wire more signals"): +### 11. UI (minimal) -- `addAction (event, sol::protected_function, priority, owner)` / `addFilter (...)` → - `Handle`; `remove (Handle)`; `removeOwner (ownerId)`. -- `dispatchAction (event, pusher)` and `applyFilters (event, initialValue, pusher)`. -- Message-thread only (assert); dispatch iterates a copy (handlers may add/remove); - reentrancy guard — nested dispatch queues and drains after; a handler is auto-disabled - after 3 consecutive errors; every call is protected, errors go to - `ScriptingEngine::logError`, never propagate. +- Session panel (`src/ui/sessiontreepanel.cpp`, where node scripts are listed around + `:507` and the `TODO enable script types at the graph level` at `:822`): list session + scripts, add (`Script::make (types::Hook)`) / remove, open in the existing + `ScriptEditorView`. +- Status line "scripts: N attached, M blocked" with the "Re-ask trust" action. -Initial events (actions): `session.loaded`, `session.saving`, `graph.added`, -`graph.removed`, `graph.changed`, `node.added`, `node.removed`. Initial filters: start -with one or two proving the shape (e.g. `node.displayName`) — filters are the part to -grow cautiously. Signal sources: `SessionService::sigSessionLoaded` / `sigWillSave` -(`src/services/sessionservice.hpp:37-38`), `EngineService::sigNodeRemoved`, plus new -`sigNodeAdded` / `sigGraphAdded` / `sigGraphRemoved` emitted at the same sites in -`src/services/engineservice.cpp`. +### 12. Dead-code cleanup (same PR series) -### 4. `el.hooks` Lua module — Lua-first +Delete: `ScriptingEngine::L`; `State::resolve_internal_package`, `builtins`, +`packages` and `EL_LUA_SPATH`; `Impl::scanDefaultLoctaion`; `ScriptInstance::object` and +`cleanup()`; `DSPUIScript`; `ScriptSource`/`ValueTreeScriptSource`; `scripts/commands.lua`; +the orphaned `test/snippets/sol3_parent.lua` and `stream_from_c.lua`. Keep `addPackage` +only if it is wired into `searchInternalModules` (extensions will want it); otherwise +delete. Register `el.vector` or delete `vector.c`. Fix the missing comma in +`widget.hpp` `__props`. Make `el/session.lua` resolve the session per call. Leave +`ScriptManager` with a comment that it is test-only until extensions land. Replace the +hard-coded `== 13` in `ScriptManagerTest` with a lower bound. -Per principle 3: a small C binding exposing an opaque bus handle + `register/unregister/ -emit` primitives (`src/el/Hooks.cpp`), wrapped by `src/el/hooks.lua` providing the -friendly `action`/`filter`/`off` API, priority defaults, and owner scoping. The `hooks` -object passed to `attach` is a Lua table closing over the owner id — no usertype needed. +## Order of work -### 5. Runner & lifecycle (`ScriptingService`, new — shared with extensions plan) +Each step builds and passes `ctest` before the next. -- On `sigSessionLoaded`: read the session `scripts` tree, for each `Hook` script: - restricted env → run → descriptor table → check `requires` vs grants → prompt if - needed → call `attach (hooks, ctx)`. Owner id = script's tree UUID. -- On session unload/reload/close: call `detach` (protected), `HookBus::removeOwner`, - drop envs. Also handles in-session edits: saving a hook script in the editor - re-attaches it (detach → re-run). -- `app.*` events and extension ownership come later (extensions plan); the bus API is - already owner-based so nothing changes shape. +1. `HookBus` + `HookBusTests` (no callers yet). +2. `Session::Handle`, `findFor`, `*Data` primitives (no behaviour change). +3. `SessionProxy` + `EngineService` forwards in one PR so all callers keep compiling; + `GuiService`/`GraphEditorView` hook handlers; `session.*` events; `SessionProxyTests`. +4. Console foundation + `el.command` fix; `ConsoleTests`. +5. Lua surface: `el.Session`/`el.Graph` verbs, `el.hooks`; `SessionLuaTests`. +6. Session `scripts` tree, `types::Hook`, restricted env, trust, `ScriptingService`, + panel UI; `HookScriptTests`. +7. Dead-code cleanup. -### 6. UI (minimal) +Format with `util/format.py`. Headers under `src/` first; `include/element/` only for +the public model API (`hooks.hpp`, `session.hpp`, `graph.hpp`, `tags.hpp`). -- Session properties / session panel: list session scripts, add/remove, open in the - existing `ScriptEditorView` (`src/ui/scripteditorview.cpp`). -- A "scripts blocked / granted" indicator with a way to re-open the trust prompt. +## Testing (Boost.Test) -## Testing (Boost.Test, `test/scripting/`) +Register every suite in `test/CMakeLists.txt` with +`add_test (NAME "" COMMAND test_element --run_test=)`. -- Session scripts tree round-trip (add → save XML → load → scripts intact). -- HookBus: priority order, filter value threading, auto-disable after errors, - removeOwner, reentrancy (action handler emitting another action queues, no recursion). -- Runner: descriptor with `requires` not granted → `attach` never called, no error spam; - granted → ctx contains exactly the granted capabilities. -- Restricted env: `io`/`os.execute`/raw `require` unavailable inside a hook script. -- Register suites in `test/CMakeLists.txt` with `add_test`. +- `HookBusTests` (`test/HookBusTests.cpp`): priority order; filter value threading; + `remove`/`removeOwner`; auto-disable after 3 errors and re-enable after success; + nested `doAction` queued not recursed; `ScopedSuspend`; off-thread `doAction` asserts. +- `SessionProxyTests` — see [session-proxy.md](session-proxy.md). +- `ConsoleTests` (`test/scripting/consoletests.cpp`): `execute ("1+1")` prints `2`; + `execute ("x = 5")` then `execute ("x")` prints `5` in the persistent env; a syntax + error yields a failed `Result` whose message contains a traceback line; prelude loads + from binary data and defines `hooks`, `session`, `console`. +- `SessionLuaTests` (`test/scripting/sessionluatest.cpp`, snippet + `test/snippets/session_mutate.lua`): state initialised with `test::context()`; + `session():addGraph ('lua')`, `g:addNode ('element.volume')`, `hooks.action + ('node.removed', …)`, `g:removeNode (n)` → handler ran once; `g:addNode ('bogus')` + returns `nil, msg`. +- `HookScriptTests` (`test/scripting/hookscripttests.cpp`): session `scripts` tree + round-trips through XML; descriptor with `requires = {'session'}` not granted → + `attach` never called and no error spam; granted → `ctx.session` present and nothing + else; restricted env has no `io`, `os.execute`, `load`, or `require ('el.Context')`; + `detach` + `removeOwner` leave no handlers; `reattach` after an edit replaces them. +- Manual: View → Console; `session():addGraph ("Test")`; + `hooks.action ('node.added', function (e) print (e.node.name) end)` then add a plugin + from the UI → name printed; undo/redo the add → exactly one `node.removed` then one + `node.added`; add a `Hook` script to the session, save, reopen → trust prompt, then + `attach` runs. ## Relationship to the extensions plan -- HookBus, `el.hooks`, restricted-env + capability machinery, and `ScriptingService` - move **here** (Phase 0). [extensions.md](extensions.md) Phase 3 reduces to wiring - `app.started`/`app.shutdown` and extension-owned registration; extension entry scripts - reuse the same descriptor/capability conventions with a friendlier default grant. +- `HookBus`, `el.hooks`, the restricted-env + capability machinery, `ScriptingService` + and the mutation verbs all land here. [extensions.md](extensions.md) Phase 2 becomes + "reuse the model verbs"; Phase 3 reduces to wiring extension ownership and a + friendlier default grant; extension entry scripts reuse the same descriptor and + capability conventions. - The capability model is also the answer to "extensions are trusted-but-isolated" — one mechanism, two default policies. ## Open questions -1. Trust persistence key: content hash (safer, invalidates on edit) vs file path - (friendlier). Suggest hash of the scripts subtree only. -2. Do hook scripts on *nodes/graphs* (not just the session root) run too? Suggest: yes - eventually (graph-scoped hooks fire only while that graph is active), but session-root - only in Phase 0. -3. Filter catalog — which host values are worth filtering first. +1. Trust key: content hash of the `scripts` subtree (chosen) vs file path. Revisit if the + prompt is too frequent in practice. +2. Hook scripts on *graphs* (not just the session root): yes eventually, graph-scoped + hooks firing only while that graph is active; session-root only in Phase 0. +3. Filter catalogue beyond `graph.defaultName`: candidates are `session.saving` payload + and `node.displayName`, the latter only once a non-paint call site is identified. +4. Undoable Lua mutations: needs `AppMessage`-based routing or an explicit + `UndoManager` transaction API on the proxy. Out of scope for Phase 0. From c74f3feeee98fb6a457290cc2f2d76295f949467 Mon Sep 17 00:00:00 2001 From: Michael Fisher Date: Tue, 22 Sep 2026 22:16:09 -0400 Subject: [PATCH 2/5] docs: refactor scripting plans --- docs/plans/extensions.md | 2 +- .../plans/kushview-licensing-authorization.md | 311 ------------------ docs/plans/scripting-audit.md | 12 +- docs/plans/session-proxy.md | 3 +- docs/plans/session-scripts.md | 60 +++- 5 files changed, 56 insertions(+), 332 deletions(-) delete mode 100644 docs/plans/kushview-licensing-authorization.md diff --git a/docs/plans/extensions.md b/docs/plans/extensions.md index 5d20b1cf1..b59c5e29e 100644 --- a/docs/plans/extensions.md +++ b/docs/plans/extensions.md @@ -88,7 +88,7 @@ Ownership/lifetime rule: `ExtensionManager` is owned by `ScriptingEngine::Impl` ### Phase 1 — Extension core: format, discovery, module/script loading - New: `extension.hpp/.cpp`, `extensionmanager.hpp/.cpp`, `scriptingservice.hpp/.cpp`, `test/scripting/extensiontests.cpp`, fixture `test/scripting/fixtures/TestPack.element/`. -- Modified: `src/scripting.hpp/.cpp` (implement dead `ScriptingEngine::execute` as protected `lua.script()` in fresh env returning `Result`; expose `extensions()`), `src/datapath.cpp` + `include/element/datapath.hpp` (`defaultExtensionsDir()` = `~/Music/Element/Extensions`, create in `initializeUserLibrary`; also scan `applicationDataDir()/Extensions`; dev env var `ELEMENT_EXTENSIONS_PATH` mirroring `ELEMENT_SCRIPTS_PATH` handling in [bindings.cpp](../../src/scripting/bindings.cpp)), `src/services.cpp`, `src/scripting/scriptmanager.cpp/.hpp` (**additive** scan — current `scanDirectory` replaces the registry), `include/element/tags.hpp` (`EL_TAG(Extension)`, `tags::extensionId`, `tags::extensionVersion`, `tags::requires`), `src/CMakeLists.txt`, `test/CMakeLists.txt` (+ `add_test`). +- Modified: `src/scripting.hpp/.cpp` (`ScriptingEngine::execute (code, env)` is provided by Phase 0 — see [session-scripts.md](session-scripts.md) § Console foundation — and is reused here with a fresh env per entry script; expose `extensions()`), `src/datapath.cpp` + `include/element/datapath.hpp` (`defaultExtensionsDir()` = `~/Music/Element/Extensions`, create in `initializeUserLibrary`; also scan `applicationDataDir()/Extensions`; dev env var `ELEMENT_EXTENSIONS_PATH` mirroring `ELEMENT_SCRIPTS_PATH` handling in [bindings.cpp](../../src/scripting/bindings.cpp)), `src/services.cpp`, `src/scripting/scriptmanager.cpp/.hpp` (**additive** scan — current `scanDirectory` replaces the registry), `include/element/tags.hpp` (`EL_TAG(Extension)`, `tags::extensionId`, `tags::extensionVersion`, `tags::requires`), `src/CMakeLists.txt`, `test/CMakeLists.txt` (+ `add_test`). - Load sequence: scan (parse manifests only, no code) → for enabled extensions: register modules via `addPackage`, register DSP/View scripts with `ScriptManager`, run entry script in `sol::environment(lua, sol::create, lua.globals())`; every failure → `logError`, status `error`, never throws out. Enable/disable persisted in `Settings` (`extensionsDisabled` list). `Commands::reloadExtensions` for dev iteration. - Unload = disconnect hooks, drop env, clear `package.loaded` + registered packages, remove ScriptManager entries and view/panel registrations. Full hot-unload of usertypes is explicitly out of scope (documented). diff --git a/docs/plans/kushview-licensing-authorization.md b/docs/plans/kushview-licensing-authorization.md deleted file mode 100644 index ca145f0ee..000000000 --- a/docs/plans/kushview-licensing-authorization.md +++ /dev/null @@ -1,311 +0,0 @@ -# Kushview Licensing & Authorization Architecture - -**Status:** Draft / design -**Scope:** Kushview-owned plugins only. Client-side design complete; server-side deferred (stubbed contracts only). -**Goal:** A subscriber (or standalone purchaser) authenticates once, and every Kushview plugin they're entitled to unlocks silently — no per-plugin user/pass prompt. Cancelling a subscription revokes access on the next refresh. - ---- - -## 1. Summary - -Introduce a dedicated **Kushview Account Manager** application as the single place that logs in and manages entitlements. It writes a **shared per-user store** of auth tokens and signed unlock keys. Element and every Kushview plugin link a small **shared read-side SDK** that reads that store and applies keys through a **common JUCE product-unlocking implementation** — never running an interactive login themselves. - -This mirrors the industry-standard pattern (iLok License Manager, NI Native Access, Waves Central, Steinberg Download Assistant): one privileged writer, many trivial readers. - -### Why this shape - -- **Plugins stay trivial.** No OAuth, no browser, no URL-scheme handling shipped in each plugin — just "read store → apply key → else fall back to the existing manual prompt." Less code, less attack surface, replicated across every product. -- **Auth changes ship once**, in the Manager, decoupled from plugin release cycles. -- **Background subscription refresh.** Subscription keys are short-lived by design; a plugin can only refresh while loaded. The Manager runs as a login-item/agent and keeps every owned plugin's key fresh regardless of whether anything is open — the one place this can be solved cleanly. -- **Graceful degradation.** No token / offline / non-subscriber → plugin shows today's manual unlock UI. Nothing breaks. - ---- - -## 2. Background: JUCE unlocking primer (for the target project) - -Kushview plugins are JUCE products. JUCE's licensing primitives: - -- **RSA key pair.** Generated once per product with `juce::RSAKey::createKeyPair()`. - - **Private key** signs unlock "key files". **Lives only on the server.** Never shipped, never in any client. - - **Public key** is compiled into the product and only *verifies* a signed key file. It cannot forge licenses. It is safe to embed — and is already inside every shipped plugin binary. -- **Key file.** A signed blob (produced server-side by `juce::KeyGeneration::generateKeyFile(...)`) containing the user email, product ID, **machine IDs**, and an **expiry**. Applied client-side with `OnlineUnlockStatus::applyKeyFile(...)`. -- **`juce::OnlineUnlockStatus`.** The client-side state object. We use it in "key file" mode (`applyKeyFile` / `isUnlocked` / `getExpiryTime`) and **bypass its built-in web-authentication flow** entirely — keys come from the shared store, not from OnlineUnlockStatus's own HTTP calls. - -Two protections do the real work: **machine-locking** (a key file only unlocks on the machines whose IDs it was signed for) and **expiry** (subscription semantics). OnlineUnlockStatus's own state obfuscation is weak and is *not* relied on for security. - -### Current state in Element (starting point) - -Element already has the auth foundation this builds on: - -- OAuth2 Authorization-Code + PKCE against WordPress `kv-auth/v1` (`src/auth.cpp`, `src/auth.hpp`, namespace `element::auth`). -- Token response already carries an `entitlements` object (today just `preview_updates`) plus user email/display name. -- An established precedent for "authenticated request → short-lived server-signed artifact": `/appcast-url?plat=` returns a signed, expiring Sparkle feed URL that the client caches and refreshes when expired (`GuiService::checkUpdates`, `auth::isAppcastUrlExpired`). -- **No** `OnlineUnlockStatus`/`RSAKey`/`KeyGeneration` anywhere yet — the unlock layer is net-new. -- Plugin scanning/hosting has no entitlement gate — nothing to unwind. - -The signed-key endpoint is the same shape as the existing appcast-url endpoint. The OAuth/PKCE client is the reusable core that gets lifted into the shared SDK. - ---- - -## 3. Component overview - -``` -┌───────────────────────────────────────────────┐ -│ Kushview Account Manager │ the ONLY app that logs in -│ • interactive OAuth2 + PKCE (browser) │ -│ • URL-scheme callback (kushview://) │ -│ • entitlement sync (tier → product list) │ -│ • key fetch + refresh daemon │ -│ • (future) product download / install │ -└───────────────────────┬───────────────────────┘ - │ writes (atomic) - ▼ - ╔═══════════════════════════════════════╗ - ║ Shared per-user Kushview store ║ - ║ • tokens (OS secure storage) ║ - ║ • keys/.key (machine- ║ - ║ locked, expiring signed blobs) ║ - ║ • entitlements.json ║ - ╚═══════════════════════════════════════╝ - ▲ ▲ - read │ │ read - ┌───────────────┴──┐ ┌────┴──────────────────┐ - │ Element │ │ Kushview plugins │ - │ (host + reader) │ │ ToneGenerator, … │ - └──────────────────┘ └───────────────────────┘ - both link the shared READ-SIDE SDK: - KushviewUnlockStatus + StoreReader + fallback -``` - -### 3.1 Kushview Account Manager (new application) - -The single privileged writer. - -Responsibilities: -- Interactive **OAuth2 + PKCE** login (lifted from `element::auth`), including browser launch and `kushview://auth/callback` URL-scheme handling. -- Fetch and persist **tokens** into the shared store (refresh-token rotation, silent refresh). -- Fetch **entitlements** (tier → owned/subscribed product list) and persist to `entitlements.json`. -- For each entitled product, request a **signed, machine-locked, expiring key file** and write it to `keys/.key`. -- **Refresh daemon:** run on login-item/agent schedule; re-request any key nearing expiry so subscription keys never lapse silently. On the server saying "no longer entitled," delete that product's key. -- Account UI: signed-in identity, tier, owned products, per-product authorization status, manual "refresh all." -- (Future) download & install products and updates; can absorb Element's appcast entitlement flow. - -Cross-platform: one codebase (JUCE app or native), signed/notarized, self-updating. - -### 3.2 Shared read-side SDK (linked by Element + every plugin) - -A small static/shared library. Contains **no interactive login** — read + apply + fall back only. - -Public surface: - -```cpp -namespace kv::lic { - -// Identity a product provides about itself. -struct ProductInfo { - juce::String productId; // stable slug, e.g. "kv.tonegenerator" - juce::String publicKey; // this product's embedded RSA public key - juce::String displayName; // for any UI/prompt fallback -}; - -// Locates and reads the shared per-user store. Read-only. -class StoreReader { -public: - static juce::File storeDir(); // platform-specific (see §5) - juce::File keyFileFor (const juce::String& productId) const; - bool hasTokens() const; // is anyone logged in? - // NOTE: reads tokens for refresh-on-read only if we choose to allow it (see §7, open decision). -}; - -// Common JUCE product-unlocking implementation. One per plugin instance. -class KushviewUnlockStatus : public juce::OnlineUnlockStatus { -public: - explicit KushviewUnlockStatus (ProductInfo); - - // The unlock ladder (see §4.2). Cheap; safe to call on load. - bool authorize(); - - juce::String getProductID() override { return info.productId; } - juce::RSAKey getPublicKey() override { return juce::RSAKey (info.publicKey); } - juce::String getState() override; // persisted applied-key state - void saveState (const juce::String&) override; - // Built-in web auth is intentionally unused: - juce::URL getServerAuthenticationURL() override { return {}; } - juce::String readReplyFromWebserver (const juce::String&, const juce::String&) override { return {}; } - -private: - ProductInfo info; - StoreReader store; -}; - -} // namespace kv::lic -``` - -### 3.3 Common key-writing pattern (the shared contract) - -This is the "common pattern for key writing" — one implementation, honored identically by the Manager (writer) and every plugin (reader). - -- **File per product:** `keys/.key`, containing the raw JUCE key-file text produced by `KeyGeneration::generateKeyFile` server-side. -- **Atomic writes** (Manager): write to `keys/.key.tmp`, `fsync`, rename over the target. Readers never observe a half-written file. -- **Applied-state cache** (plugin): after `applyKeyFile` succeeds, the plugin persists the applied state via `saveState()` into its *own* settings. So an already-authorized plugin keeps working even if the store file is later removed — until the embedded **expiry** lapses. -- **Machine-locked:** every key file carries this machine's IDs; copying it to another machine fails `applyKeyFile`. This is what makes the shared store safe to sync/back-up. -- **Expiry-bearing:** subscription keys are short-lived (e.g. 30 days). The Manager refreshes ahead of expiry; a lapsed entitlement simply stops being refreshed and the plugin falls back to prompting after expiry. - -### 3.4 Per-plugin integration (minimal) - -Each plugin adds only: - -```cpp -static const kv::lic::ProductInfo kToneGenProduct { - "kv.tonegenerator", - EMBEDDED_TONEGEN_PUBLIC_KEY, // build-time constant, safe to embed - "Kushview Tone Generator" -}; - -// On construction / first UI show: -kv::lic::KushviewUnlockStatus unlock (kToneGenProduct); -if (! unlock.authorize()) - showManualUnlockPrompt(); // existing per-plugin fallback UI -``` - -Nothing else. No OAuth, no browser, no URL scheme. - -### 3.5 Element's role - -Element is just another reader of the shared store for the *plugin* keys. Its existing OAuth login for **update entitlements** continues to work as-is short-term. - -- **Short-term:** Element keeps its own login (writes the same shared token store the Manager uses) and additionally reads plugin keys via the SDK. Both apps interoperate through one store. -- **Long-term:** the Manager becomes the canonical account/entitlement hub; Element migrates to a pure reader and drops its embedded login UI. *(Decision — see §7.)* - ---- - -## 4. Data flows - -### 4.1 Login (Manager only) - -``` -User clicks Sign In (Manager) - → PKCE verifier/challenge generated, state stored - → browser opens kushview.net/auth/authorize - → user authenticates on the store - → redirect kushview://auth/callback?code=…&state=… - → OS routes to Manager URL-scheme handler - → validate state, exchange code at kv-auth/v1/token - → receive JWT access token + refresh token + entitlements{tier, products[]} - → write tokens (OS secure storage) + entitlements.json - → for each entitled product: fetch signed key → write keys/.key -``` - -### 4.2 Unlock ladder (Element + plugins, on load) - -``` -authorize(): - 1. Local applied state valid AND not expired? → unlocked, ZERO network - 2. keys/.key present? - applyKeyFile(blob) - machine matches AND not expired? → unlocked, cache state - 3. Otherwise → return false - → caller shows existing manual unlock prompt -``` - -Step 1 makes the common case free (no I/O beyond a settings read). Step 3 guarantees graceful degradation. - -### 4.3 Background subscription refresh (Manager daemon) - -``` -On schedule / login-item wake: - ensure access token fresh (silent refresh; rotate refresh token) - GET entitlements - for each product: - if entitled: - if key missing OR expiry within threshold (e.g. < 7 days): - fetch new signed, machine-locked key → atomic write - else: - delete keys/.key # revoked / downgraded -``` - -This is the mechanism that gives real subscription semantics: cancel → next refresh stops re-signing → key expires → plugin falls back to prompting. - ---- - -## 5. Shared store layout & locations - -``` -/ - tokens → refresh/access tokens (prefer OS secure storage, see below) - entitlements.json - keys/ - kv.tonegenerator.key - kv..key -``` - -Platform base directory: -- **macOS:** `~/Library/Application Support/Kushview/Account/` -- **Windows:** `%APPDATA%\Kushview\Account\` -- **Linux:** `$XDG_CONFIG_HOME/kushview/account/` (fallback `~/.config/kushview/account/`) - -**Token at-rest protection:** store the refresh token in OS secure storage where available — macOS **Keychain**, Windows **Credential Manager (DPAPI)**, Linux **libsecret** — falling back to a restricted-permission file (0600) only if unavailable. Key files themselves are machine-locked, so they are low-sensitivity and can live as plain files. - ---- - -## 6. Security model & invariants - -Non-negotiable: - -1. **Private signing key never leaves the server.** All key-file generation is server-side. No client (Manager, Element, plugin) can mint or re-sign keys. -2. **Public keys are embedded and harmless.** They only verify. Already present in shipped binaries. -3. **Entitlement decisions are server-side.** The client asks; the server returns a signed key or a denial. A patched client cannot grant itself products — it can only replay what the server signed, which is machine-locked and time-limited. -4. **Machine-locked keys.** Every key file is bound to `getLocalMachineIDs()`; it cannot be lifted to another machine. -5. **Expiring keys.** Subscriptions rely on short TTL + refresh. No perpetual key for subscription tiers. -6. **TLS everywhere**; tokens in OS secure storage. -7. **Fail closed to the *prompt*, not to unlocked.** Any failure in the ladder ends at the manual unlock UI, never at an unearned unlock. - -Threats explicitly out of scope of "safe embedding": binary patching to skip the `isUnlocked()` check is possible for any offline-verifiable scheme and is not made worse by this design; machine-locking + expiry + server-side entitlement are the mitigations. - ---- - -## 7. Open decisions - -| # | Decision | Options | Lean | -|---|----------|---------|------| -| 1 | Element long-term auth | (a) keep own login + read keys; (b) delegate all account to Manager, Element becomes pure reader | (a) now → (b) later | -| 2 | Can plugins refresh a token themselves? | Read-only (Manager is sole refresher) vs. plugins allowed silent token refresh when store token is stale | Read-only first; simplest, smallest attack surface | -| 3 | Key TTL & refresh threshold | e.g. 30-day TTL, refresh < 7 days | confirm with subscription cadence | -| 4 | Store token format | OS secure storage vs. encrypted file | OS secure storage, file fallback | -| 5 | Standalone-plugin-without-Manager UX | require Manager once vs. per-plugin lightweight login | require Manager (matches Native Access) | - ---- - -## 8. Server-side (DEFERRED — contract stubs only) - -Not designed here; captured so the client contracts are unambiguous. Extends the existing `kv-auth/v1` namespace. - -- `GET /kv-auth/v1/entitlements` (Bearer) → `{ tier, products: [productId…] }` -- `GET /kv-auth/v1/plugin-key?product=&machine=` (Bearer) - → signed JUCE key file (machine-locked, expiring) or `403` if not entitled. - Same shape as the existing `/appcast-url` precedent. -- Reuse existing `/token`, `/token/refresh`, `/token/revoke`. - -Signing service holds the per-product **private** keys and calls `KeyGeneration::generateKeyFile`. - ---- - -## 9. Phased roadmap - -1. **Shared read-side SDK** — `StoreReader`, `KushviewUnlockStatus`, key-file read/apply/cache, store layout & locations. Unit-testable with a fixture store and a locally generated key pair. -2. **Per-plugin integration in one product** (Tone Generator) — embed public key, wire the ladder + fallback prompt. Prove silent unlock from a hand-placed key file. -3. **Account Manager MVP** — lift `element::auth` OAuth/PKCE into it, URL-scheme handling, write tokens + entitlements + keys to the store. -4. **Refresh daemon** — login-item/agent, expiry-driven re-fetch, revocation delete. -5. **Element as reader** — link the SDK, read plugin keys (keep existing update login). -6. **Roll SDK across remaining plugins.** -7. **Server-side** — entitlements + plugin-key signing endpoints (separate plan). -8. *(Future)* download/install + migrate update entitlement into Manager. - ---- - -## 10. Portability notes (for the other project) - -- The SDK and `KushviewUnlockStatus` are pure JUCE + a store path — no Element dependencies. They drop into any Kushview JUCE product. -- The only per-product inputs are the **productId**, the **embedded public key**, and a **display name**. -- The store layout in §5 and the key-writing pattern in §3.3 are the interop contract between the Manager and all readers — keep them identical on both sides. -- Nothing here requires the server work to exist first: with a locally generated test key pair and a hand-written `keys/.key`, the entire read/apply/ladder path is buildable and testable today. diff --git a/docs/plans/scripting-audit.md b/docs/plans/scripting-audit.md index b447d3de0..0f73f578a 100644 --- a/docs/plans/scripting-audit.md +++ b/docs/plans/scripting-audit.md @@ -28,9 +28,12 @@ out of scope here. installs a searcher `resolve_internal_package` whose `builtins`/`packages` maps are **never filled** (`fill_builtins` is commented out, `addPackage` has no callers). Then `Lua::initializeState` ([bindings.cpp:413-435](../../src/scripting/bindings.cpp#L413)) - opens *all* libraries, installs the real searcher `searchInternalModules` (a 160-line - if/else chain kept in sync by hand with ~30 `extern "C"` declarations), and sets - `package.path`, `package.cpath` (always empty) and the non-standard `package.spath`. + opens *all* libraries, inserts the real searcher `searchInternalModules` (a 160-line + if/else chain kept in sync by hand with ~30 `extern "C"` declarations) at position 2 + of `package.searchers`, and sets `package.path`, `package.cpath` (always empty) and the + non-standard `package.spath`. Both searchers coexist: `resolve_internal_package` is + pushed to position 3, so anything registered through `addPackage` *does* resolve — the + map is simply empty because nothing calls it. - `_G["el.context"]` is a raw `std::ref` ([bindings.cpp:402-405](../../src/scripting/bindings.cpp#L402)), nilled in `~ScriptingEngine`. Nothing guards Lua values that captured it earlier. - No thread assertions anywhere in `src/scripting/`, `src/el/`, `src/scripting.cpp`. @@ -99,7 +102,8 @@ Defects, in priority order: |---|---|---| | `ScriptingEngine::execute (const String&)` | [scripting.hpp:28](../../src/scripting.hpp#L28) | Declared, never defined. | | `lua_State* L` | [scripting.hpp:40](../../src/scripting.hpp#L40) | Never assigned. | -| `State::resolve_internal_package`, `builtins`, `packages`, `addPackage` | [scripting.cpp:84-126](../../src/scripting.cpp#L84) | Searcher can never resolve; `addPackage` has no callers. | +| `State::builtins`, `fill_builtins` | [scripting.cpp:84-126](../../src/scripting.cpp#L84) | Map never filled; the fill call is commented out. Dead. | +| `State::resolve_internal_package`, `packages`, `addPackage` | [scripting.cpp:64-126](../../src/scripting.cpp#L64) | **Live**, not dead: the searcher stays at position 3 of `package.searchers`. No callers yet; [extensions.md](extensions.md) Phase 1 registers extension modules through it. | | `EL_LUA_SPATH` | [scripting.cpp:10](../../src/scripting.cpp#L10) | Defined, never referenced. | | `ScriptManager` | [scriptmanager.cpp](../../src/scripting/scriptmanager.cpp) | Never scans in the app: `Application::setupScripting` is `ignoreUnused (scripts)`; `Impl::scanDefaultLoctaion` (sic) has no callers. Test-only. | | `ScriptInstance::object` | [scriptinstance.hpp](../../src/scripting/scriptinstance.hpp) | Private, no setter, so `cleanup()` is unreachable. | diff --git a/docs/plans/session-proxy.md b/docs/plans/session-proxy.md index 31096b140..995ddf44b 100644 --- a/docs/plans/session-proxy.md +++ b/docs/plans/session-proxy.md @@ -267,7 +267,8 @@ Existing `SessionTests` keep covering the pure-tree path unchanged. Remember: a [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657), writes `tags::active` from an async update) bypass the proxy. If `graph.activated` must cover them, the proxy needs a `ValueTree::Listener` on the `graphs` child with a self-change flag. - Follow-up. + Follow-up, tracked as [session-scripts.md](session-scripts.md) open question 5; the + hook catalogue must say `graph.activated` excludes engine-initiated changes until then. - `PluginProcessor::reloadEngine` → `reload()` must keep the full detach/re-attach behaviour; `prepareExternalPlayback` re-preparation was not traced. - Re-entrant hooks (a handler adding a node from `node.added`) work through the depth diff --git a/docs/plans/session-scripts.md b/docs/plans/session-scripts.md index c9892ea00..38d8075e4 100644 --- a/docs/plans/session-scripts.md +++ b/docs/plans/session-scripts.md @@ -151,6 +151,9 @@ to hooks instead (details in the proxy doc). `juce::Result execute (const juce::String& code, sol::environment env)`: the `"return ;"` compile probe, `sol::protected_function` with a `debug.traceback` handler, byte length from `toRawUTF8()`. Result message carries the traceback. + Asserts the message thread (`JUCE_ASSERT_MESSAGE_THREAD`); the shared state has no + other guard, and this is the entry point every later caller (console, hook scripts, + extension entry scripts) goes through. `LuaConsole::textEntered` becomes a thin caller; drop the `_G.print` swap, the dead `lastError`/`errorHandler`, and the unreachable error branch. - **Persistent environment.** `sol::environment& ScriptingEngine::consoleEnvironment()`, @@ -309,6 +312,16 @@ The same restricted env (with the widget modules `el.View`, `el.Widget`, `el.Sli `ScriptView::setScript` via `loader.call (env)`, closing the "View scripts run in raw globals" hole. +**Compatibility.** View scripts embedded in existing user sessions run with full globals +today, so enforcing the allowlist is a behaviour change that can break a saved session +silently. Before flipping it: audit what the shipped and example View scripts +(`scripts/*.lua`, `docs/`, test snippets) actually `require` and which globals they +touch; then ship one release where `ScriptView` runs the script in the restricted env +with a *reporting* `require`/`__index` that logs each disallowed access to +`ScriptingEngine::logError` but still resolves it; enforce only after that log is quiet. +Anything a View script legitimately needs (e.g. `el.Context` read access) is added to +the allowlist rather than worked around. + ### 9. Trust - `Settings` gains `sessionScriptTrust`: map of *scripts-subtree SHA1* → `allow | deny`. @@ -353,26 +366,37 @@ destructor. Delete: `ScriptingEngine::L`; `State::resolve_internal_package`, `builtins`, `packages` and `EL_LUA_SPATH`; `Impl::scanDefaultLoctaion`; `ScriptInstance::object` and `cleanup()`; `DSPUIScript`; `ScriptSource`/`ValueTreeScriptSource`; `scripts/commands.lua`; -the orphaned `test/snippets/sol3_parent.lua` and `stream_from_c.lua`. Keep `addPackage` -only if it is wired into `searchInternalModules` (extensions will want it); otherwise -delete. Register `el.vector` or delete `vector.c`. Fix the missing comma in -`widget.hpp` `__props`. Make `el/session.lua` resolve the session per call. Leave -`ScriptManager` with a comment that it is test-only until extensions land. Replace the -hard-coded `== 13` in `ScriptManagerTest` with a lower bound. +the orphaned `test/snippets/sol3_parent.lua` and `stream_from_c.lua`. **Keep** +`addPackage`, `State::packages` and `resolve_internal_package`: the searcher is live +(position 3 of `package.searchers`, see the audit § 1) and [extensions.md](extensions.md) +Phase 1 registers extension modules through it; delete only `State::builtins` and the +commented-out `fill_builtins`. Register `el.vector` or delete `vector.c`. Fix the missing +comma in `widget.hpp` `__props`. Make `el/session.lua` resolve the session per call. +Restore the real body of `DSPScript::validate` (currently `#if 0`, returns `ok()` for +any non-empty string) or delete the method and its callers. Leave `ScriptManager` with a +comment that it is test-only until extensions land. Replace the hard-coded `== 13` in +`ScriptManagerTest` with a lower bound. ## Order of work -Each step builds and passes `ctest` before the next. - -1. `HookBus` + `HookBusTests` (no callers yet). -2. `Session::Handle`, `findFor`, `*Data` primitives (no behaviour change). -3. `SessionProxy` + `EngineService` forwards in one PR so all callers keep compiling; +Each step builds and passes `ctest` before the next. Steps 1 and 2 depend on nothing in +[session-proxy.md](session-proxy.md) and land first: they fix what the audit found broken +and give a working REPL for exercising every later step by hand. + +1. Dead-code cleanup (§12) + console foundation (§3) + `el.command` fix; `ConsoleTests`. + Touches `src/scripting*`, `src/ui/luaconsole*`, `src/el/`, `scripts/` only — not + `EngineService`. +2. `HookBus` + `HookBusTests` (no callers yet). +3. `Session::Handle`, `findFor`, `*Data` primitives (no behaviour change). +4. The headless "real graphs through `attach` + `setRootNode`" test first (the untested + path in [session-proxy.md](session-proxy.md) § Risks — a modal alert on that path + hangs the runner, so prove it before moving code). Then `SessionProxy` + + `EngineService` forwards in one PR so all callers keep compiling; `GuiService`/`GraphEditorView` hook handlers; `session.*` events; `SessionProxyTests`. -4. Console foundation + `el.command` fix; `ConsoleTests`. 5. Lua surface: `el.Session`/`el.Graph` verbs, `el.hooks`; `SessionLuaTests`. -6. Session `scripts` tree, `types::Hook`, restricted env, trust, `ScriptingService`, - panel UI; `HookScriptTests`. -7. Dead-code cleanup. +6. View-script `require` audit and the reporting pass (§8 *Compatibility*), then session + `scripts` tree, `types::Hook`, restricted env, trust, `ScriptingService`, panel UI; + `HookScriptTests`. Format with `util/format.py`. Headers under `src/` first; `include/element/` only for the public model API (`hooks.hpp`, `session.hpp`, `graph.hpp`, `tags.hpp`). @@ -426,3 +450,9 @@ Register every suite in `test/CMakeLists.txt` with and `node.displayName`, the latter only once a non-paint call site is identified. 4. Undoable Lua mutations: needs `AppMessage`-based routing or an explicit `UndoManager` transaction API on the proxy. Out of scope for Phase 0. +5. Engine-initiated active-graph changes (MIDI program change, + [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657), writes + `tags::active` from an async update) bypass the proxy, so `graph.activated` does + **not** fire for them in Phase 0. Document it as such in the hook catalogue. Fix is a + `ValueTree::Listener` on the `graphs` child inside `SessionProxy` with a self-change + flag; follow-up after step 4. From 4d69b3024cf8362b4dfc903a64c7e1ddff4ce89a Mon Sep 17 00:00:00 2001 From: Michael Fisher Date: Tue, 22 Sep 2026 23:08:47 -0400 Subject: [PATCH 3/5] scripting: console foundation, DSP script validation, dead-code cleanup Phase 0 step 1 of docs/plans/session-scripts.md. Fixes what the scripting audit found broken on the app side and removes what was never wired up. Console - Implement the declared-but-missing ScriptingEngine::execute: expression probe, protected call with a debug.traceback handler, UTF-8 byte length, message-thread assert, errors returned as a juce::Result. - Give the engine a persistent console environment and history so state survives closing and reopening the console view. - Load the console prelude from the compiled binary data instead of the filesystem search path, which never resolved in dev builds. el.script.exec now raises on load failure instead of returning the message. - Stop swapping the global print per eval; override os.exit on a console-local os table rather than the global one. - Marshal Log messages to the message thread and guard the print buffer. - Fix the persisted console view name, accept the old misspelling, add F3. Bindings - Add Context:commands() to el.Context, resolved per call through the services, so el.command's existing Context.instance():commands() works. - Fix el.command calling a nonexistent strings.valid. - Make el/session.lua resolve the session per call instead of caching it. - Fix the missing comma in widget.hpp __props. DSP scripts - DSPScript::process now uses lua_pcall. Lua is built as C, so an error in a script's process had no handler and aborted the app. The script is disabled and the message kept instead. Restore the stack top after the call. - init/prepare/release go through the same protected path and return false on error; getLastError() exposes the message. - Make DSPScript::validate functional: instantiate in a scratch state and run init/prepare, four process cycles with audio and MIDI, and release; reject scripts that raise or produce non-finite output. Cleanup - Remove ScriptingEngine::L, State::builtins, EL_LUA_SPATH, Impl::scanDefaultLoctaion, scripts/commands.lua, the #if 0 vector.c, and two orphaned test snippets. addPackage and its searcher stay: both searchers coexist and extensions will register modules through it. - Note ScriptManager as test-only until extensions land; loosen the hard-coded script count in ScriptManagerTest to a lower bound. Docs - CLAUDE.md: add a Lua Bindings section (single entry point via el.Context, bind on the owner and resolve per call, no shortcut globals). - Plans: record the addPackage correction, the View-script compatibility pass, the reordered work list, and keep ScriptInstance/ScriptSource. Tests: new ConsoleTests suite; DSPScriptTest gains validate and process-error coverage. --- CLAUDE.md | 20 +++ docs/plans/scripting-audit.md | 4 +- docs/plans/session-scripts.md | 18 ++- scripts/commands.lua | 51 ------- scripts/console.lua | 29 ++-- src/el/Context.cpp | 11 ++ src/el/command.lua | 2 +- src/el/script.lua | 3 +- src/el/session.lua | 8 +- src/el/vector.c | 198 ------------------------ src/el/widget.hpp | 2 +- src/scripting.cpp | 129 ++++++++++++---- src/scripting.hpp | 34 ++++- src/scripting/dspscript.cpp | 218 +++++++++++++++------------ src/scripting/dspscript.hpp | 45 +++--- src/scripting/scriptmanager.hpp | 5 + src/ui/console.cpp | 18 +++ src/ui/console.hpp | 6 + src/ui/luaconsole.cpp | 138 ++++++++--------- src/ui/luaconsole.hpp | 21 ++- src/ui/luaconsoleview.cpp | 13 +- src/ui/luaconsoleview.hpp | 10 +- src/ui/standard.cpp | 3 + test/CMakeLists.txt | 1 + test/scripting/consoletests.cpp | 149 ++++++++++++++++++ test/scripting/dspscripttest.cpp | 84 +++++++++++ test/scripting/scriptmanagertest.cpp | 5 +- test/snippets/sol3_parent.lua | 72 --------- test/snippets/stream_from_c.lua | 18 --- 29 files changed, 708 insertions(+), 607 deletions(-) delete mode 100644 scripts/commands.lua delete mode 100644 src/el/vector.c create mode 100644 test/scripting/consoletests.cpp delete mode 100644 test/snippets/sol3_parent.lua delete mode 100644 test/snippets/stream_from_c.lua diff --git a/CLAUDE.md b/CLAUDE.md index a810155fb..b1aab050b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,26 @@ You are an expert in JUCE and in desktop audio application UI. Element is a JUCE - Shared singletons are reached through `context()` — e.g. `context().session()`, `context().mapping()`, `context().midi()`. - Marshal state changes off the audio/MIDI thread to the message thread with `juce::AsyncUpdater`. Plugin parameter changes must be wrapped in `beginChangeGesture()` / `setValueNotifyingHost()` / `endChangeGesture()`. +## Lua Bindings (`src/el/`) + +- **One entry point.** Lua reaches the application only through `el.Context.instance()` + (backed by `_G["el.context"]`, planted by `Lua::setGlobals`). Never plant additional + globals to reach other app objects. Expose them as methods on the owning usertype instead, + e.g. `Context:commands()` resolves per call via `ctx.services().find()`. +- **Bind on the owner, resolve per call.** A binding returns a reference into the live + object graph; it does not cache it. Lua modules must not cache what they get back either + (a session is replaced on load), so resolve lazily inside functions. +- **Register what you return.** If a method returns a usertype, `require` that module + in the `luaopen_*` of the module that returns it (see `luaopen_el_Context`). +- **Bind less C++, write more Lua.** C++ binds a minimal handle; ergonomics go in a native + Lua module under `src/el/*.lua` (`command.lua`, `object.lua`, `script.lua`). +- **Register modules in both places.** A new `el.*` module goes into + `searchInternalModules` in `src/scripting/bindings.cpp` and, for Lua sources, is picked + up by the `src/el/CMakeLists.txt` glob. +- **No shortcuts.** If reaching an object needs plumbing (an accessor, a service lookup), + add the plumbing. Working around it with a global, a static or a cached pointer is a + defect, not a fix. + ## Testing - Tests use **Boost.Test** and live in `test/` (built into the `test_element` console app). `test/CMakeLists.txt` globs all `*.cpp`, but each suite must ALSO be registered with an explicit `add_test(NAME "MySuite" COMMAND test_element --run_test=MySuite)` line — forgetting this is the usual reason a new test "doesn't run." diff --git a/docs/plans/scripting-audit.md b/docs/plans/scripting-audit.md index 0f73f578a..5312b3128 100644 --- a/docs/plans/scripting-audit.md +++ b/docs/plans/scripting-audit.md @@ -106,8 +106,8 @@ Defects, in priority order: | `State::resolve_internal_package`, `packages`, `addPackage` | [scripting.cpp:64-126](../../src/scripting.cpp#L64) | **Live**, not dead: the searcher stays at position 3 of `package.searchers`. No callers yet; [extensions.md](extensions.md) Phase 1 registers extension modules through it. | | `EL_LUA_SPATH` | [scripting.cpp:10](../../src/scripting.cpp#L10) | Defined, never referenced. | | `ScriptManager` | [scriptmanager.cpp](../../src/scripting/scriptmanager.cpp) | Never scans in the app: `Application::setupScripting` is `ignoreUnused (scripts)`; `Impl::scanDefaultLoctaion` (sic) has no callers. Test-only. | -| `ScriptInstance::object` | [scriptinstance.hpp](../../src/scripting/scriptinstance.hpp) | Private, no setter, so `cleanup()` is unreachable. | -| `DSPUIScript`, `ScriptSource` | `src/scripting/` | Empty class / no users. | +| `ScriptInstance::object` (keep the class) | [scriptinstance.hpp](../../src/scripting/scriptinstance.hpp) | Private, no setter, so `cleanup()` is unreachable. | +| `DSPUIScript`, `ScriptSource` | `src/scripting/` | No users *yet*: intentional scaffolding for the script-type hierarchy and for where script code is sourced from (`ValueTreeScriptSource` → session scripts). Keep. | | `DSPScript::validate` | [dspscript.cpp:434-438](../../src/scripting/dspscript.cpp#L434) | Returns `ok()` for any non-empty string; real body is `#if 0`. | | `el.vector` | `src/el/vector.c` | Compiled, not registered in `searchInternalModules`. | | `widget.hpp` `__props` | `src/el/widget.hpp` | Missing comma fuses `"visible" "opaque"` into `"visibleopaque"`. | diff --git a/docs/plans/session-scripts.md b/docs/plans/session-scripts.md index 38d8075e4..e06bd9d7b 100644 --- a/docs/plans/session-scripts.md +++ b/docs/plans/session-scripts.md @@ -177,10 +177,10 @@ to hooks instead (details in the proxy doc). - **Threading.** `LuaConsoleView::messageLogged` posts to the message thread with `juce::MessageManager::callAsync` and a `Component::SafePointer`; `printMessages` is guarded by a `juce::CriticalSection` (or replaced with an `AsyncUpdater`). -- **`el.command`.** `GuiService::activate()` sets `_G["el.commands"]` to a - `std::ref` (mirroring `el.context`, cleared in `deactivate()`); - `el.Commands.instance()` reads it; `command.lua` uses that instead of - `Context:commands()`. `scripts/commands.lua` is deleted. +- **`el.command`.** `el.Context` gains `commands()`, resolved per call through + `ctx.services().find()->commands()`, so `command.lua`'s existing + `Context.instance():commands()` is correct as written. No new globals: `el.context` is + the single entry point into the app. `scripts/commands.lua` is deleted. - Cosmetic: `EL_VIEW_CONSOLE` becomes `"LuaConsoleView"` (accept the old `"LuaConsoleViw"` when restoring `ContentContainer_lastSecondaryView`); `showConsole` gets a default keypress alongside `showPatchBay`/`showGraphEditor`. @@ -363,9 +363,8 @@ destructor. ### 12. Dead-code cleanup (same PR series) -Delete: `ScriptingEngine::L`; `State::resolve_internal_package`, `builtins`, -`packages` and `EL_LUA_SPATH`; `Impl::scanDefaultLoctaion`; `ScriptInstance::object` and -`cleanup()`; `DSPUIScript`; `ScriptSource`/`ValueTreeScriptSource`; `scripts/commands.lua`; +Delete: `ScriptingEngine::L`; `State::builtins` and `EL_LUA_SPATH`; +`Impl::scanDefaultLoctaion`; `scripts/commands.lua`; the orphaned `test/snippets/sol3_parent.lua` and `stream_from_c.lua`. **Keep** `addPackage`, `State::packages` and `resolve_internal_package`: the searcher is live (position 3 of `package.searchers`, see the audit § 1) and [extensions.md](extensions.md) @@ -375,7 +374,10 @@ comma in `widget.hpp` `__props`. Make `el/session.lua` resolve the session per c Restore the real body of `DSPScript::validate` (currently `#if 0`, returns `ok()` for any non-empty string) or delete the method and its callers. Leave `ScriptManager` with a comment that it is test-only until extensions land. Replace the hard-coded `== 13` in -`ScriptManagerTest` with a lower bound. +`ScriptManagerTest` with a lower bound. **Keep** `ScriptInstance` and `DSPUIScript` (base and +placeholder for the script-type hierarchy: DSP, DSPUI, View, Hook…) and +`ScriptSource`/`ValueTreeScriptSource` (where a script's code comes from; +`ValueTreeScriptSource` is what session `Hook` scripts in §6 read their code through). ## Order of work diff --git a/scripts/commands.lua b/scripts/commands.lua deleted file mode 100644 index e59d6bbea..000000000 --- a/scripts/commands.lua +++ /dev/null @@ -1,51 +0,0 @@ ---- List commands registered in the command manager. --- @script commands --- @type anonymous --- @license GPLv3 --- @author Michael Fisher --- @usage --- > script.exec ('commands') --- SHOW_ABOUT = 256 --- SHOW_ALL_PLUGIN_WINDOWS = 265 --- SHOW_SESSION_CONFIG = 260 --- RECENTS_CLEAR = 4096 --- GRAPH_OPEN = 1793 --- GRAPH_SAVE_AS = 1795 --- UNDO = 4104 --- ... - -local CMD = require ('el.CommandManager') -local strings = require ('el.strings') -local format = select (1, ...) or 'constants' - -if not strings.isslug (format) then - error ("Invalid format: " .. tostring (format)) -end - -local list = { - -- List as constants used with the el.command module - constants = function (cmds) - for k,v in pairs (cmds) do - local out = string.upper (strings.tosnake (k)) - out = out .. " = " .. tostring(v) - print (out) - end - end -} - -local cmds = {} - --- add standard commands -for _, cmd in ipairs (CMD.standard()) do - local str = CMD.tostring (cmd) - if string.len(str) > 0 then - cmds[str] = cmd - end -end - -if type(list[format]) == 'function' then - list[format] (cmds) -end - --- SPDX-FileCopyrightText: Copyright (C) Kushview, LLC. --- SPDX-License-Identifier: GPL-3.0-or-later diff --git a/scripts/console.lua b/scripts/console.lua index 5c98a6fcb..e5340a3aa 100644 --- a/scripts/console.lua +++ b/scripts/console.lua @@ -1,29 +1,32 @@ --- Console init. --- Runs when the console is loaded in the GUI. This anonymous script sets global --- variables, so be careful if you use it directly. +-- Runs once in the persistent console environment when the console is first +-- opened. It sets globals in that environment, so be careful if you use it +-- directly. -- @script console -- @pragma nostrip -- @type Anonymous -io = require ('io') object = require ('el.object') command = require ('el.command') script = require ('el.script') +Context = require ('el.Context') + +--- Returns the active session. +-- A function rather than a value: the session object is replaced when a new +-- session is loaded, so never cache the result. +-- @function session +-- @treturn el.Session +function session() + return Context.instance():session() +end console = { - --- Log to stdout. - -- Calls `tostring` on each argument then prints the combined result to - -- stdout + --- Log to the console. + -- Calls `tostring` on each argument then prints the combined result. -- @function console.log -- @param ... Things to log log = function (...) - local out = "" - for i = 1, select ('#', ...) do - out = out .. tostring (select (i, ...)) .. "\t" - end - if string.len(out) > 0 then - io.stdout:write (out .. "\n") - end + print (...) end } diff --git a/src/el/Context.cpp b/src/el/Context.cpp index 4b472636d..e6457cb7e 100644 --- a/src/el/Context.cpp +++ b/src/el/Context.cpp @@ -8,6 +8,8 @@ #include #include +#include +#include #include #include #include @@ -49,6 +51,14 @@ EL_PLUGIN_EXPORT int luaopen_el_Context (lua_State* L) // @within Instance Methods "session", &Context::session, + /// Returns the application's command manager. + // @function Context:commands + // @treturn el.Commands + // @within Instance Methods + "commands", [] (Context& ctx) -> element::Commands& { + return ctx.services().find()->commands(); + }, + "audio", &Context::audio, "devices", &Context::devices, "mapping", &Context::mapping, @@ -58,6 +68,7 @@ EL_PLUGIN_EXPORT int luaopen_el_Context (lua_State* L) "settings", &Context::settings); lua.script (R"( + require ('el.Commands') require ('el.Node') require ('el.Session') )"); diff --git a/src/el/command.lua b/src/el/command.lua index ea1e9827d..871558855 100644 --- a/src/el/command.lua +++ b/src/el/command.lua @@ -64,7 +64,7 @@ end for _,cmd in ipairs (Commands.standard()) do local strings = require ('el.strings') local s = Commands.toString (cmd) - if string.len(s) > 0 and strings.valid(s) then + if string.len(s) > 0 and strings.issymbol(s) then local k = strings.tosnake (s) M[string.upper (k)] = cmd end diff --git a/src/el/script.lua b/src/el/script.lua index 9d1706c27..f5a0b3e25 100644 --- a/src/el/script.lua +++ b/src/el/script.lua @@ -26,10 +26,11 @@ end -- @tparam table env The environment to use or _ENV -- @tparam any ... Arguments passed to script -- @treturn any Return value from script or no value +-- @raise When the script cannot be found or compiled -- @usage script.exec ('scriptname') function M.exec (path, env, ...) local invoke, err = M.load (path, env) - if err then return err end + if not invoke then error (err, 2) end return invoke (...) end diff --git a/src/el/session.lua b/src/el/session.lua index 4752bad86..82f2d88e1 100644 --- a/src/el/session.lua +++ b/src/el/session.lua @@ -9,10 +9,12 @@ local Node = require ('el.Node') local Graph = require ('el.Graph') local M = {} -local session = Context.instance():session() -function M.toxmlstring() return session:toXmlString() end -function M.name() return session.name end +-- Never cache the session: it is replaced when a new session is loaded. +local function session() return Context.instance():session() end + +function M.toxmlstring() return session():toXmlString() end +function M.name() return session().name end return M diff --git a/src/el/vector.c b/src/el/vector.c deleted file mode 100644 index dde0f0528..000000000 --- a/src/el/vector.c +++ /dev/null @@ -1,198 +0,0 @@ -// Copyright 2023 Kushview, LLC -// SPDX-License-Identifier: GPL-3.0-or-later - -#if 0 - -/// A vector of `kv_sample_t`'s suitable for realtime -// @module el.vector -#include -#include -#include "luainc.h" -#include "vector.h" -#include "util.h" -#include "element/element.h" - -struct kv_vector_impl_t { - kv_sample_t* values; - lua_Integer size; - lua_Integer used; -}; - -typedef struct kv_vector_impl_t Vector; - -//============================================================================= -kv_vector_t* kv_vector_new (lua_State* L, int size) { - Vector* vec = lua_newuserdata (L, sizeof (Vector)); - luaL_setmetatable (L, EL_MT_VECTOR); - - if (size > 0) { - vec->size = vec->used = size; - vec->values = malloc (sizeof (kv_sample_t) * size); - memset (vec->values, 0, sizeof (kv_sample_t) * size); - } else { - vec->size = vec->used = 0; - vec->values = NULL; - } - - return vec; -} - -static void kv_vector_free_values (kv_vector_t* vec) { - if (vec->values != NULL) { - free (vec->values); - vec->values = NULL; - } - vec->size = vec->used = 0; -} - -size_t kv_vector_size (kv_vector_t* vec) { - return (size_t) vec->used; -} - -size_t kv_vector_capacity (kv_vector_t* vec) { - return (size_t) vec->size; -} - -kv_sample_t* kv_vector_values (kv_vector_t* vec) { - return vec->values; -} - -kv_sample_t kv_vector_get (kv_vector_t* vec, int index) { - return vec->values [index]; -} - -void kv_vector_set (kv_vector_t* vec, int index, kv_sample_t value) { - vec->values [index] = value; -} - -void kv_vector_clear (kv_vector_t* vec) { - if (vec->used > 0) { - memset (vec->values, 0, sizeof(kv_sample_t) * vec->used); - } -} - -void kv_vector_resize (kv_vector_t* vec, int size) { - size = MAX (0, size); - if (size <= vec->size) { - vec->used = size; - } else { - vec->used = vec->size = size; - vec->values = realloc (vec->values, sizeof(kv_sample_t) * vec->size); - } -} - -static int vector_gc (lua_State* L) { - kv_vector_free_values (lua_touserdata (L, 1)); - return 0; -} - -static int vector_len (lua_State* L) { - Vector* vec = lua_touserdata (L, 1); - lua_pushinteger (L, vec->size); - return 1; -} - -static int vector_index (lua_State* L) { - Vector* vec = lua_touserdata (L, 1); - lua_Integer i = lua_tointeger (L, 2) - 1; - if (i >= 0 && i < vec->used) { - lua_pushnumber (L, vec->values[i]); - } else { - lua_pushnil (L); - } - return 1; -} - -static int vector_newindex (lua_State* L) { - Vector* vec = lua_touserdata (L, 1); - lua_Integer i = lua_tointeger (L, 2) - 1; - lua_Number v = lua_tonumber (L, 3); - if (i >= 0 && i < vec->used) { - vec->values[i] = v; - } - return 0; -} - -static int vector_tostring (lua_State* L) { - Vector* vec = lua_touserdata (L, 1); - lua_pushfstring (L, "Vector: size=%d capacity=%d", vec->used, vec->size); - return 1; -} - -static const luaL_Reg vector_m[] = { - { "__gc", vector_gc }, - { "__len", vector_len }, - { "__index", vector_index }, - { "__newindex", vector_newindex }, - { "__tostring", vector_tostring }, - { NULL, NULL } -}; - -//============================================================================= - -/// Creates a new vector -// @param size Number of elements -// @function new -static int f_new (lua_State* L) { - if (NULL == kv_vector_new (L, MAX (0, lua_tointeger (L, 1)))) - lua_pushnil (L); - return 1; -} - -/// Clears a vector -// @param vec The vector to clear -// @function clear -static int f_clear (lua_State* L) { - kv_vector_t* vec = luaL_checkudata (L, 1, EL_MT_VECTOR); - if (vec->used <= 0 || vec->values == NULL) { - return 0; - } - - switch (lua_gettop (L)) { - default: { - memset (vec->values, 0, sizeof(kv_sample_t) * vec->used); - break; - } - } - - return 0; -} - -/// Reserve a number of elements -// @param vec The vector to operate on -// @param size Number of elements to allocate memory for -// @function reserve -static int f_reserve (lua_State* L) { - return 0; -} - -/// Resize a vector -// @param vec The vector to clear -// @param size The new number of elements to allocate -// @function resize -static int f_resize (lua_State* L) { - return 0; -} - -static const luaL_Reg vector_f[] = { - { "new", f_new }, - { "clear", f_clear }, - { "reserve", f_reserve }, - { "resize", f_resize }, - { NULL, NULL } -}; - -void kv_vector_metatable (lua_State* L) { - if (0 != luaL_newmetatable (L, EL_MT_VECTOR)) { - luaL_setfuncs (L, vector_m, 0); - } -} - -EL_PLUGIN_EXPORT -int luaopen_el_vector (lua_State* L) { - kv_vector_metatable (L); - luaL_newlib (L, vector_f); - return 1; -} - -#endif diff --git a/src/el/widget.hpp b/src/el/widget.hpp index 19a2587a2..c51fce83b 100644 --- a/src/el/widget.hpp +++ b/src/el/widget.hpp @@ -366,7 +366,7 @@ inline static sol::table defineWidget (lua_State* L, const char* name, Args&&... "y", "width", "height", - "visible" + "visible", "opaque"); T_mt["__methods"] = view.create_table().add ( diff --git a/src/scripting.cpp b/src/scripting.cpp index 6fdac8221..d7bca6b61 100644 --- a/src/scripting.cpp +++ b/src/scripting.cpp @@ -7,10 +7,6 @@ #include "scripting/bindings.hpp" #include -#ifndef EL_LUA_SPATH -#define EL_LUA_SPATH "" -#endif - namespace element { //============================================================================= @@ -29,6 +25,7 @@ class ScriptingEngine::State ~State() { + consoleEnv = sol::environment(); auto& g = state.globals(); g.set (refkey, sol::lua_nil); } @@ -47,8 +44,10 @@ class ScriptingEngine::State friend class ScriptingEngine; ScriptingEngine& owner; sol::state state; - element::lua::PackageLoaderMap builtins; element::lua::PackageLoaderMap packages; + // Declared after `state` so it is released before the state is closed. + sol::environment consoleEnv; + juce::StringArray consoleHistory; /** global table key to state reference */ static constexpr const char* refkey = "__state"; @@ -58,15 +57,13 @@ class ScriptingEngine::State return view.globals()[refkey]; } + /** Inserts the runtime package searcher after Lua's preload searcher. + `Lua::initializeState` later inserts the internal-module searcher ahead + of it, so both stay active. */ static void init_packages (lua_State* L) { sol::state_view view (L); view.open_libraries (sol::lib::package); -#if 0 - // doing this destroys preloading capabilities in lua. - view.clear_package_loaders(); - view.add_package_loader (resolve_internal_package); -#else auto package = view["package"]; const char* skey = LUA_VERSION_NUM < 502 ? "loaders" : "searchers"; @@ -77,29 +74,17 @@ class ScriptingEngine::State for (size_t i = 2; i <= orig_searchers.size(); ++i) // add everything after (file searchers) new_searchers.add (package[skey][i]); // .. package[skey] = new_searchers; // replace them -#endif } - /** Custom lua searcher handler */ + /** Searcher for packages registered at runtime via `addPackage`. */ static int resolve_internal_package (lua_State* L) { sol::state_view view (L); auto& state = getref (view); - // if (state.builtins.empty()) - // element::lua::fill_builtins (state.builtins); - const auto mid = sol::stack::get (L); - auto it = state.builtins.find (mid); - auto end = state.builtins.end(); - std::string msgkey = "builtins"; - if (it == end) - { - it = state.packages.find (mid); - end = state.packages.end(); - msgkey = "packages"; - } - if (it != end) + auto it = state.packages.find (mid); + if (it != state.packages.end()) { if (nullptr != it->second) sol::stack::push (L, it->second); @@ -108,7 +93,7 @@ class ScriptingEngine::State } else { - lua_pushfstring (L, "\n\tno field %s['%s']", msgkey.c_str(), mid.c_str()); + lua_pushfstring (L, "\n\tno field packages['%s']", mid.c_str()); } return 1; @@ -128,8 +113,6 @@ void ScriptingEngine::addPackage (const std::string& name, element::lua::CFuncti std::vector ScriptingEngine::getPackageNames() const noexcept { std::vector v; - for (const auto& pkg : state->builtins) - v.push_back (pkg.first); for (const auto& pkg : state->packages) v.push_back (pkg.first); std::sort (v.begin(), v.end()); @@ -149,11 +132,6 @@ class ScriptingEngine::Impl ~Impl() {} - void scanDefaultLoctaion() - { - manager.scanDefaultLocation(); - } - ScriptManager& getManager() { return manager; } private: @@ -174,6 +152,7 @@ ScriptingEngine::~ScriptingEngine() { if (state != nullptr) { + state->consoleEnv = sol::environment(); Lua::clearGlobals (state->state); state->collectGarbage(); state.reset(); @@ -197,4 +176,88 @@ ScriptManager& ScriptingEngine::getScriptManager() return impl->manager; } +//============================================================================= +sol::environment& ScriptingEngine::consoleEnvironment() +{ + auto& env = state->consoleEnv; + if (! env.valid()) + { + sol::state_view lua (state->state); + env = sol::environment (lua, sol::create, lua.globals()); + } + return env; +} + +juce::StringArray& ScriptingEngine::consoleHistory() +{ + return state->consoleHistory; +} + +juce::Result ScriptingEngine::execute (const juce::String& code, + sol::environment env, + const juce::String& chunkName) +{ + JUCE_ASSERT_MESSAGE_THREAD + sol::state_view lua (state->state); + const auto chunk = "=" + chunkName.toStdString(); + + try + { + bool haveReturn = true; + juce::String buffer; + buffer << "return " << code << ";"; + auto loaded = lua.load_buffer (buffer.toRawUTF8(), buffer.getNumBytesAsUTF8(), chunk, sol::load_mode::text); + + if (! loaded.valid()) + { + haveReturn = false; + loaded = lua.load_buffer (code.toRawUTF8(), code.getNumBytesAsUTF8(), chunk, sol::load_mode::text); + } + + if (! loaded.valid()) + { + sol::error error = loaded; + return juce::Result::fail (error.what()); + } + + sol::protected_function fn = loaded; + if (env.valid()) + sol::set_environment (env, fn); + + sol::protected_function traceback = lua["debug"]["traceback"]; + if (traceback.valid()) + fn.set_error_handler (traceback); + + auto result = fn(); + if (! result.valid()) + { + sol::error error = result; + return juce::Result::fail (error.what()); + } + + if (haveReturn && result.return_count() > 0) + { + sol::protected_function print; + if (env.valid()) + print = env["print"]; + else + print = lua["print"]; + if (print.valid()) + { + auto printed = print (result); + if (! printed.valid()) + { + sol::error error = printed; + return juce::Result::fail (error.what()); + } + } + } + } catch (const std::exception& e) + { + return juce::Result::fail (e.what()); + } + + return juce::Result::ok(); +} + } // namespace element diff --git a/src/scripting.hpp b/src/scripting.hpp index cc79277d0..ecae2b562 100644 --- a/src/scripting.hpp +++ b/src/scripting.hpp @@ -25,7 +25,38 @@ class ScriptingEngine lua_State* getLuaState() const; //========================================================================== - juce::Result execute (const String& code); + /** Compiles and runs a chunk of Lua in the given environment. + + The code is first compiled as an expression (`return ;`). When + that succeeds and the chunk yields values, they are passed to the + environment's `print` function, so a console can echo results. When it + does not compile as an expression the code is run as a plain chunk. + + Runtime errors are caught with a `debug.traceback` handler and never + propagate. Must be called on the message thread. + + @param code The Lua source to run. + @param env The environment to run in. An invalid environment + falls back to the global table. + @param chunkName Name used in error messages and tracebacks. + @return ok on success, otherwise the error message (with traceback + for runtime errors). + */ + juce::Result execute (const juce::String& code, + sol::environment env = {}, + const juce::String& chunkName = "console"); + + /** Returns the persistent console environment. + + Created on first use with a fallback to the globals table, so reads + see everything in `_G` while writes stay local to the console. It + lives as long as the engine, so console state survives the console + view being closed and reopened. + */ + sol::environment& consoleEnvironment(); + + /** Returns the console's command history (persists with the engine). */ + juce::StringArray& consoleHistory(); std::vector getPackageNames() const noexcept; void addPackage (const std::string& name, lua::CFunction loader); @@ -37,7 +68,6 @@ class ScriptingEngine class Impl; std::unique_ptr impl; Context* world = nullptr; - lua_State* L = nullptr; class State; std::unique_ptr state; diff --git a/src/scripting/dspscript.cpp b/src/scripting/dspscript.cpp index 42a6f8e34..236a29bbb 100644 --- a/src/scripting/dspscript.cpp +++ b/src/scripting/dspscript.cpp @@ -1,6 +1,8 @@ // Copyright 2023 Kushview, LLC // SPDX-License-Identifier: GPL-3.0-or-later +#include + #include "el/factories.hpp" #include #include "scripting/dspscript.hpp" @@ -431,86 +433,107 @@ DSPScript::~DSPScript() deref(); } +template +bool DSPScript::callOptional (const char* name, Args&&... args) +{ + sol::protected_function f = DSP[name]; + if (! f.valid()) + return true; + + auto result = f (std::forward (args)...); + if (result.valid()) + return true; + + sol::error error = result; + lastError = error.what(); + std::clog << "[dsp script] " << name << ": " << lastError.toStdString() << std::endl; + return false; +} + +bool DSPScript::init() { return callOptional ("init"); } +bool DSPScript::prepare (double rate, int block) { return callOptional ("prepare", rate, block); } +bool DSPScript::release() { return callOptional ("release"); } + Result DSPScript::validate (const String& script) { if (script.isEmpty()) return Result::fail ("script contains no code"); - return Result::ok(); -#if 0 + sol::state state; element::Lua::initializeState (state); ScriptLoader loader (state.lua_state(), script); if (loader.hasError()) return Result::fail (loader.getErrorMessage()); - - auto ctx = std::make_unique (loader.call()); - if (! ctx->isValid()) - return Result::fail ("could not parse script"); - juce::Result result (juce::Result::fail ("Unknown script problem")); - + auto descriptor = loader.call(); + if (loader.hasError()) + return Result::fail (loader.getErrorMessage()); + if (! descriptor.valid() || descriptor.get_type() != sol::type::table) + return Result::fail ("script did not return a table"); + + auto dsp = std::make_unique (descriptor.as()); + if (! dsp->isValid()) + return Result::fail ("could not instantiate script"); + try { - const int block = 1024; const double rate = 44100.0; + const int block = 512; + const int cycles = 4; + + PortList ports; + dsp->getPorts (ports); + const int nchans = jmax (1, ports.size (PortType::Audio, true), ports.size (PortType::Audio, false)); + const int nmidi = jmax (ports.size (PortType::Midi, true), ports.size (PortType::Midi, false)); + + AudioSampleBuffer audio (nchans, block); + OwnedArray midiBuffers; + Array midiChannels; + for (int i = 0; i < nmidi; ++i) + { + midiBuffers.add (new MidiBuffer()); + midiChannels.add (i); + } + MidiPipe midi (midiBuffers, midiChannels); + + if (! dsp->init()) + return Result::fail ("init: " + dsp->getLastError()); + if (! dsp->prepare (rate, block)) + return Result::fail ("prepare: " + dsp->getLastError()); + + for (int cycle = 0; cycle < cycles; ++cycle) + { + // A quiet test tone so gain-style scripts have something to shape. + for (int c = 0; c < nchans; ++c) + for (int f = 0; f < block; ++f) + audio.setSample (c, f, 0.25f * std::sin (float (f) * 0.05f)); + + for (auto* buffer : midiBuffers) + { + buffer->clear(); + buffer->addEvent (MidiMessage::noteOn (1, 60, 0.8f), 0); + buffer->addEvent (MidiMessage::noteOff (1, 60), block / 2); + } + + dsp->process (audio, midi); + if (! dsp->isValid()) + return Result::fail ("process: " + dsp->getLastError()); + + for (int c = 0; c < audio.getNumChannels(); ++c) + for (int f = 0; f < block; ++f) + if (! std::isfinite (audio.getSample (c, f))) + return Result::fail ("process produced non-finite audio samples"); + } - using PT = PortType; - - // call node_io_ports() and node_params() - PortList validatePorts; - ctx->getPorts (validatePorts); - - // create a dummy audio buffer and midipipe - auto nchans = jmax (validatePorts.size (PT::Audio, true), - validatePorts.size (PT::Audio, false)); - auto nmidi = jmax (validatePorts.size (PT::Midi, true), - validatePorts.size (PT::Midi, false)); - - ctx->prepare (rate, block); - state["__ln_validate_rate"] = rate; - state["__ln_validate_nmidi"] = nmidi; - state["__ln_validate_nchans"] = nchans; - state["__ln_validate_nframes"] = block; - state.script (R"( - function __ln_validate_render() - local AudioBuffer = require ('el.AudioBuffer') - local MidiPipe = require ('el.MidiPipe') - - local a = AudioBuffer (__ln_validate_nchans, __ln_validate_nframes) - local m = MidiPipe (__ln_validate_nmidi) - - for _ = 1,4 do - for i = 0,m:size() - 1 do - local b = m:get(i) - b:insert (0, midi.noteon (1, 60, math.random (1, 127))) - b:insert (10, midi.noteoff (1, 60, 0)) - end - node_render (a, m) - a:clear() - m:clear() - end - - a = nil - m = nil - collectgarbage() - end - - __ln_validate_render() - __ln_validate_render = nil - collectgarbage() - )"); - - ctx->release(); - ctx.reset(); - result = Result::ok(); - } - catch (const std::exception& e) - { - result = Result::fail (e.what()); - } - return result; -#endif + if (! dsp->release()) + return Result::fail ("release: " + dsp->getLastError()); + } catch (const std::exception& e) + { + return Result::fail (e.what()); + } + + return Result::ok(); } void DSPScript::getPorts (PortList& out) @@ -524,48 +547,45 @@ void DSPScript::process (AudioSampleBuffer& a, MidiPipe& m) if (! loaded) return; - if (lua_rawgeti (L, LUA_REGISTRYINDEX, processRef) == LUA_TFUNCTION) + const int top = lua_gettop (L); + + if (lua_rawgeti (L, LUA_REGISTRYINDEX, processRef) == LUA_TFUNCTION + && lua_rawgeti (L, LUA_REGISTRYINDEX, audioRef) == LUA_TUSERDATA + && lua_rawgeti (L, LUA_REGISTRYINDEX, midiRef) == LUA_TUSERDATA + && lua_rawgeti (L, LUA_REGISTRYINDEX, paramsUserData.registry_index()) == LUA_TUSERDATA + && lua_rawgeti (L, LUA_REGISTRYINDEX, controlsUserData.registry_index()) == LUA_TUSERDATA + && lua_rawgeti (L, LUA_REGISTRYINDEX, positionRef) == LUA_TUSERDATA) { - if (lua_rawgeti (L, LUA_REGISTRYINDEX, audioRef) == LUA_TUSERDATA) + (*audio)->setDataToReferTo (a.getArrayOfWritePointers(), + a.getNumChannels(), + a.getNumSamples()); + (*midi)->swapWith (m); + + if (playhead != nullptr) + (*position)->update (playhead->getPosition()); + + // Lua is built as C: an error inside an unprotected lua_call has no + // handler to unwind to and aborts the process. A protected call keeps + // the audio thread alive; the script is disabled instead. The String + // assignment only happens on that error path. + if (lua_pcall (L, 5, 0, 0) != LUA_OK) { - if (lua_rawgeti (L, LUA_REGISTRYINDEX, midiRef) == LUA_TUSERDATA) - { - if (lua_rawgeti (L, LUA_REGISTRYINDEX, paramsUserData.registry_index()) == LUA_TUSERDATA) - { - if (lua_rawgeti (L, LUA_REGISTRYINDEX, controlsUserData.registry_index()) == LUA_TUSERDATA) - { - if (lua_rawgeti (L, LUA_REGISTRYINDEX, positionRef) == LUA_TUSERDATA) - { - (*audio)->setDataToReferTo (a.getArrayOfWritePointers(), - a.getNumChannels(), - a.getNumSamples()); - (*midi)->swapWith (m); - - if (playhead != nullptr) - (*position)->update (playhead->getPosition()); - - try - { - lua_call (L, 5, 0); - } catch (const sol::error& e) - { - std::clog << e.what() << std::endl; - loaded = false; - } - (*midi)->swapWith (m); - - for (int ci = outParams.size(); --ci >= 0;) - outParams.getUnchecked (ci)->update (controlData[ci]); - } - } - } - } + lastError = lua_tostring (L, -1); + std::clog << "[dsp script] process: " << lastError.toStdString() << std::endl; + loaded = false; } + + (*midi)->swapWith (m); + + for (int ci = outParams.size(); --ci >= 0;) + outParams.getUnchecked (ci)->update (controlData[ci]); } else { DBG ("didn't get render function in callback"); } + + lua_settop (L, top); } void DSPScript::save (MemoryBlock& out) diff --git a/src/scripting/dspscript.hpp b/src/scripting/dspscript.hpp index ee7c70d94..f31fc81a6 100644 --- a/src/scripting/dspscript.hpp +++ b/src/scripting/dspscript.hpp @@ -23,31 +23,38 @@ class DSPScript : public ScriptInstance DSPScript (sol::table tbl); ~DSPScript(); - void init() - { - if (sol::function f = DSP["init"]) - f(); - } - + /** Calls the script's optional `init` function. + @return false if the script raised an error (see getLastError) + */ + bool init(); + + /** Compiles and instantiates a DSP script in a scratch Lua state, then + runs a few cycles of prepare/process/release to catch runtime errors. + + @param script The Lua source of the script. + @return ok when the script loads, returns a descriptor table, and + renders without raising or producing non-finite audio. + */ static juce::Result validate (const juce::String& script); void setPlayHead (juce::AudioPlayHead* ph) noexcept { playhead = ph; } - /** Returns true if the script loaded ok */ + /** Returns true if the script loaded ok and has not failed in process */ bool isValid() const noexcept { return loaded; } + /** Returns the message of the last error raised by the script, if any. */ + juce::String getLastError() const { return lastError; } + //========================================================================== - void prepare (double rate, int block) - { - if (sol::function f = DSP["prepare"]) - f (rate, block); - } + /** Calls the script's optional `prepare` function. + @return false if the script raised an error (see getLastError) + */ + bool prepare (double rate, int block); - void release() - { - if (sol::function f = DSP["release"]) - f(); - } + /** Calls the script's optional `release` function. + @return false if the script raised an error (see getLastError) + */ + bool release(); //========================================================================== void process (juce::AudioSampleBuffer& a, element::MidiPipe& m); @@ -83,6 +90,7 @@ class DSPScript : public ScriptInstance lua_State* L = nullptr; bool loaded = false; + juce::String lastError; int numParams = 0, // input params numControls = 0; // output params enum @@ -109,6 +117,9 @@ class DSPScript : public ScriptInstance void addParameterPorts(); void unlinkParams(); void setParameter (int, float, bool); + + template + bool callOptional (const char* name, Args&&... args); }; } // namespace element diff --git a/src/scripting/scriptmanager.hpp b/src/scripting/scriptmanager.hpp index 93854cd14..cccc7360c 100644 --- a/src/scripting/scriptmanager.hpp +++ b/src/scripting/scriptmanager.hpp @@ -9,6 +9,11 @@ namespace element { using ScriptArray = juce::Array; +/** Registry of scripts found on disk. + + Not used by the application yet: nothing scans at startup, so it is only + exercised by tests. Extensions will drive it (see docs/plans/extensions.md). +*/ class ScriptManager final { public: diff --git a/src/ui/console.cpp b/src/ui/console.cpp index d952aa241..652b1859a 100644 --- a/src/ui/console.cpp +++ b/src/ui/console.cpp @@ -127,6 +127,14 @@ class Console::Content : public Component historyPos = history.size(); } + const StringArray& getHistory() const { return history; } + + void setHistory (const StringArray& newHistory) + { + history = newHistory; + historyPos = history.size(); + } + void addText (const String& text, bool prefix) { String line = prefix ? prefixText : String(); @@ -238,6 +246,16 @@ void Console::addText (const String& text, bool prefix) content->addText (text, prefix); } +const StringArray& Console::getHistory() const +{ + return content->getHistory(); +} + +void Console::setHistory (const StringArray& history) +{ + content->setHistory (history); +} + void Console::textEntered (const String& text) { addText (text, true); diff --git a/src/ui/console.hpp b/src/ui/console.hpp index bf86a89d2..65f737751 100644 --- a/src/ui/console.hpp +++ b/src/ui/console.hpp @@ -28,6 +28,12 @@ class Console : public juce::Component /** Show or hide the text prompt */ void setPromptVisible (bool visible); + /** Returns the command history, oldest first. */ + const StringArray& getHistory() const; + + /** Replaces the command history. */ + void setHistory (const StringArray& history); + /** Override this to handle when text is entered on the prompt. The default implementation just adds entered text to the display buffer */ virtual void textEntered (const String& text); diff --git a/src/ui/luaconsole.cpp b/src/ui/luaconsole.cpp index fc5d87782..e9da0b34c 100644 --- a/src/ui/luaconsole.cpp +++ b/src/ui/luaconsole.cpp @@ -8,6 +8,7 @@ #include #include "sol/sol.hpp" +#include "luascripts.hpp" namespace element { using namespace juce; @@ -20,73 +21,63 @@ LuaConsole::LuaConsole() startTimer (200); } -LuaConsole::~LuaConsole() {} - -void LuaConsole::textEntered (const String& text) +LuaConsole::~LuaConsole() { - if (text.isEmpty() || ! env.valid()) - return; - Console::textEntered (text); - auto& e = env; - sol::state_view lua (e.lua_state()); - - auto gprint = lua["print"]; - lua["print"] = e["print"]; + if (engine != nullptr) + engine->consoleHistory() = getHistory(); - try + // Drop anything in the environment that captured `this`. The environment + // outlives this component; reads fall back to the globals. + if (env.valid()) { - bool haveReturn = true; - String buffer = "return "; - buffer << text << ";"; - { - auto loadResult = lua.load_buffer (buffer.toRawUTF8(), buffer.length()); - if (! loadResult.valid() || loadResult.status() != sol::load_status::ok) - { - haveReturn = false; - buffer = text; - } - } - - auto result = lua.script (buffer.toRawUTF8(), e, "console=", sol::load_mode::text); + env["print"] = sol::lua_nil; + env["clear"] = sol::lua_nil; + env["os"] = sol::lua_nil; + } +} - if (result.valid()) - { - if (haveReturn) - e["print"](result); - } - else - { - sol::error error = result; - for (const auto& line : StringArray::fromLines (error.what())) - addText (line); - } +void LuaConsole::initialize (ScriptingEngine& e) +{ + engine = &e; + env = engine->consoleEnvironment(); + setHistory (engine->consoleHistory()); + installEnvironment(); + runPrelude(); +} - if (lastError.isNotEmpty()) - addText (lastError); - } catch (const sol::error& e) - { - addText (e.what()); - } +void LuaConsole::textEntered (const String& text) +{ + if (text.isEmpty() || engine == nullptr || ! env.valid()) + return; + Console::textEntered (text); + addResultText (engine->execute (text, env)); +} - lua["print"] = gprint; - lastError.clear(); +void LuaConsole::addResultText (const juce::Result& result) +{ + if (result.wasOk()) + return; + for (const auto& line : StringArray::fromLines (result.getErrorMessage())) + addText (line); } -void LuaConsole::setEnvironment (const sol::environment& _env) +void LuaConsole::installEnvironment() { - env = _env; - auto& e = env; - jassert (e.valid()); - sol::state_view lua (e.lua_state()); + jassert (env.valid()); + sol::state_view lua (env.lua_state()); - e["os"]["exit"] = sol::overload ( + // A console-local `os` so overriding `exit` does not touch the global table. + sol::table os = lua.create_table(); + os[sol::metatable_key] = lua.create_table_with ("__index", lua.globals()["os"]); + os["exit"] = sol::overload ( [this]() { ViewHelpers::invokeDirectly (this, Commands::quit, true); }, [this] (int code) { JUCEApplication::getInstance()->setApplicationReturnValue (code); ViewHelpers::invokeDirectly (this, Commands::quit, true); }); + env["os"] = os; - e["clear"] = [this] (sol::variadic_args va) { + env["clear"] = [this] (sol::variadic_args va) { if (va.size() == 1 && va.get_type (0) == sol::type::boolean) { clear (va.get (0)); @@ -101,8 +92,8 @@ void LuaConsole::setEnvironment (const sol::environment& _env) } }; - e.set_function ("print", [this] (sol::variadic_args va) { - auto& e = env; + env.set_function ("print", [this] (sol::variadic_args va) { + sol::state_view state (va.lua_state()); String msg; for (auto v : va) { @@ -112,7 +103,7 @@ void LuaConsole::setEnvironment (const sol::environment& _env) continue; } - sol::function ts = e["tostring"]; + sol::function ts = state["tostring"]; if (ts.valid()) { sol::object str = ts ((sol::object) v); @@ -124,38 +115,39 @@ void LuaConsole::setEnvironment (const sol::environment& _env) if (msg.isNotEmpty()) { + const ScopedLock sl (printLock); printMessages.add (msg.trimEnd()); } }); +} - try - { - auto result = lua.safe_script ("require('el.script').exec('console', _ENV)", e); - if (! result.valid()) - { - sol::error error = result; - for (const auto& line : StringArray::fromLines (error.what())) - addText (line); - } - } catch (const sol::error& e) - { - addText (e.what()); - } +void LuaConsole::runPrelude() +{ + // The prelude defines `console`; its presence means it already ran in + // this (persistent) environment. + if (env["console"].valid()) + return; + const auto code = String::fromUTF8 (scripts::console_lua, scripts::console_luaSize); + addResultText (engine->execute (code, env, "console.lua")); } void LuaConsole::timerCallback() { - if (! printMessages.isEmpty()) + StringArray pending; { - const int block = jmax (1, printMessages.size() / 4); - const int count = jmin (block, printMessages.size()); - - if (count > 0) + const ScopedLock sl (printLock); + if (! printMessages.isEmpty()) { - addText (printMessages.joinIntoString ("\n", 0, count)); + const int block = jmax (1, printMessages.size() / 4); + const int count = jmin (block, printMessages.size()); + pending.addArray (printMessages, 0, count); printMessages.removeRange (0, count); } + } + if (! pending.isEmpty()) + { + addText (pending.joinIntoString ("\n")); startTimerHz (50); } else diff --git a/src/ui/luaconsole.hpp b/src/ui/luaconsole.hpp index dbc836f79..10a44713d 100644 --- a/src/ui/luaconsole.hpp +++ b/src/ui/luaconsole.hpp @@ -10,6 +10,9 @@ namespace element { +/** Interactive console that evaluates Lua in the engine's persistent + console environment. +*/ class LuaConsole : public Console, private juce::Timer { @@ -17,15 +20,25 @@ class LuaConsole : public Console, LuaConsole(); virtual ~LuaConsole(); + /** Binds the console to a scripting engine. + + Installs the console's `print`, `clear` and `os.exit` into the engine's + console environment and runs the console prelude the first time the + environment is used. Command history is restored from the engine. + */ + void initialize (ScriptingEngine& engine); + void textEntered (const String&) override; - void setEnvironment (const sol::environment& e); private: - using LuaResult = sol::protected_function_result; + ScriptingEngine* engine = nullptr; sol::environment env; - String lastError; + juce::CriticalSection printLock; StringArray printMessages; - LuaResult errorHandler (lua_State* L, LuaResult pfr); + + void installEnvironment(); + void runPrelude(); + void addResultText (const juce::Result&); friend class juce::Timer; void timerCallback() override; diff --git a/src/ui/luaconsoleview.cpp b/src/ui/luaconsoleview.cpp index 4779a1512..ac86e4e22 100644 --- a/src/ui/luaconsoleview.cpp +++ b/src/ui/luaconsoleview.cpp @@ -20,10 +20,7 @@ LuaConsoleView::~LuaConsoleView() void LuaConsoleView::initializeView (Services& app) { - auto& se = app.context().scripting(); - sol::state_view view (se.getLuaState()); - console.setEnvironment ( - sol::environment (view, sol::create, view.globals())); + console.initialize (app.context().scripting()); log = &app.context().logger(); log->addListener (this); @@ -34,4 +31,12 @@ void LuaConsoleView::initializeView (Services& app) console.addText (buffer.trimEnd(), false); } +void LuaConsoleView::messageLogged (const String& msg) +{ + juce::MessageManager::callAsync ([safe = juce::Component::SafePointer (this), msg]() { + if (safe != nullptr) + safe->console.addText (msg, false); + }); +} + } // namespace element diff --git a/src/ui/luaconsoleview.hpp b/src/ui/luaconsoleview.hpp index b37ad271c..d9147ddb0 100644 --- a/src/ui/luaconsoleview.hpp +++ b/src/ui/luaconsoleview.hpp @@ -7,7 +7,9 @@ #include "ui/luaconsole.hpp" #include "log.hpp" -#define EL_VIEW_CONSOLE "LuaConsoleViw" +#define EL_VIEW_CONSOLE "LuaConsoleView" +/** Misspelled view name persisted to settings by older builds. */ +#define EL_VIEW_CONSOLE_LEGACY "LuaConsoleViw" namespace element { @@ -37,10 +39,8 @@ class LuaConsoleView : public ContentView, console.setBounds (getLocalBounds().reduced (2)); } - void messageLogged (const String& msg) override - { - console.addText (msg, false); - } + /** Called from whichever thread logs the message, inside the Log's lock. */ + void messageLogged (const String& msg) override; private: LuaConsole console; diff --git a/src/ui/standard.cpp b/src/ui/standard.cpp index 1526aabec..e9d73bfa3 100644 --- a/src/ui/standard.cpp +++ b/src/ui/standard.cpp @@ -248,6 +248,8 @@ class ContentContainer : public Component lastSecondaryHeight = jmax (50, lastSecondaryHeight); showAccessoryView = props->getIntValue ("ContentContainer_showAccessoryView", showAccessoryView); auto lastSecondaryName = props->getValue ("ContentContainer_lastSecondaryView"); + if (lastSecondaryName.trim() == EL_VIEW_CONSOLE_LEGACY) + lastSecondaryName = EL_VIEW_CONSOLE; if (showAccessoryView) { owner.setSecondaryView (lastSecondaryName.trim()); @@ -1065,6 +1067,7 @@ void StandardContent::getCommandInfo (CommandID commandID, ApplicationCommandInf int flags = (showAccessoryView() && getAccessoryViewName() == EL_VIEW_CONSOLE) ? Info::isTicked : 0; + result.addDefaultKeypress (KeyPress::F3Key, 0); result.setInfo ("Console", "Show the scripting console", "UI", flags); break; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 8655acaba..b87afcade 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -63,6 +63,7 @@ add_test(NAME "AudioRoutingTests" COMMAND test_element --run_test=AudioRoutingTe add_test(NAME "AuthTests" COMMAND test_element --run_test=AuthTests) add_test(NAME "BytesTest" COMMAND test_element --run_test=BytesTest) add_test(NAME "BusesLayoutTests" COMMAND test_element --run_test=BusesLayoutTests) +add_test(NAME "ConsoleTests" COMMAND test_element --run_test=ConsoleTests) add_test(NAME "JuceIntegrationTests" COMMAND test_element --run_test=JuceIntegrationTests) add_test(NAME "DataPathTests" COMMAND test_element --run_test=DataPathTests) add_test(NAME "DeviceMonitorTests" COMMAND test_element --run_test=DeviceMonitorTests) diff --git a/test/scripting/consoletests.cpp b/test/scripting/consoletests.cpp new file mode 100644 index 000000000..cfedd8a9f --- /dev/null +++ b/test/scripting/consoletests.cpp @@ -0,0 +1,149 @@ +// Copyright 2026 Kushview, LLC +// SPDX-License-Identifier: GPL-3.0-or-later + +#include + +#include + +#include "scripting.hpp" +#include "sol/sol.hpp" +#include "luascripts.hpp" +#include "testutil.hpp" + +using namespace element; +using namespace juce; +namespace et = element::test; + +namespace { + +/** A console environment whose `print` collects into a StringArray. */ +struct ConsoleFixture { + ConsoleFixture() + : engine (et::context()->scripting()), + env (engine.consoleEnvironment()) + { + env.set_function ("print", [this] (sol::variadic_args va) { + sol::state_view lua (va.lua_state()); + sol::function tostring = lua["tostring"]; + String line; + for (auto v : va) + line << tostring (v).get() << " "; + printed.add (line.trimEnd()); + }); + } + + ~ConsoleFixture() + { + env["print"] = sol::lua_nil; + } + + ScriptingEngine& engine; + sol::environment env; + StringArray printed; +}; + +} // namespace + +BOOST_AUTO_TEST_SUITE (ConsoleTests) + +BOOST_AUTO_TEST_CASE (ExpressionIsPrinted) +{ + ConsoleFixture fix; + auto result = fix.engine.execute ("1 + 1", fix.env); + BOOST_REQUIRE_MESSAGE (result.wasOk(), result.getErrorMessage().toStdString()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 1); + BOOST_REQUIRE_EQUAL (fix.printed[0].toStdString(), "2"); +} + +BOOST_AUTO_TEST_CASE (StatementIsNotPrinted) +{ + ConsoleFixture fix; + auto result = fix.engine.execute ("local a = 1", fix.env); + BOOST_REQUIRE (result.wasOk()); + BOOST_REQUIRE (fix.printed.isEmpty()); +} + +BOOST_AUTO_TEST_CASE (EnvironmentPersists) +{ + { + ConsoleFixture fix; + BOOST_REQUIRE (fix.engine.execute ("consoletests_x = 5", fix.env).wasOk()); + } + { + ConsoleFixture fix; + BOOST_REQUIRE (fix.engine.execute ("consoletests_x", fix.env).wasOk()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 1); + BOOST_REQUIRE_EQUAL (fix.printed[0].toStdString(), "5"); + } + + // Writes stay in the environment, not in the globals. + sol::state_view lua (et::context()->scripting().getLuaState()); + BOOST_REQUIRE (! lua["consoletests_x"].valid()); +} + +BOOST_AUTO_TEST_CASE (SyntaxErrorFails) +{ + ConsoleFixture fix; + auto result = fix.engine.execute ("local = ", fix.env); + BOOST_REQUIRE (result.failed()); + BOOST_REQUIRE (result.getErrorMessage().contains ("console")); + BOOST_REQUIRE (fix.printed.isEmpty()); +} + +BOOST_AUTO_TEST_CASE (RuntimeErrorHasTraceback) +{ + ConsoleFixture fix; + auto result = fix.engine.execute ("local function boom() error ('kaboom') end boom()", fix.env); + BOOST_REQUIRE (result.failed()); + const auto msg = result.getErrorMessage(); + BOOST_REQUIRE_MESSAGE (msg.contains ("kaboom"), msg.toStdString()); + BOOST_REQUIRE_MESSAGE (msg.contains ("stack traceback"), msg.toStdString()); +} + +BOOST_AUTO_TEST_CASE (ReturnCodeUsesEnvironmentPrint) +{ + ConsoleFixture fix; + BOOST_REQUIRE (fix.engine.execute ("print ('hello', 2)", fix.env).wasOk()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 1); + BOOST_REQUIRE_EQUAL (fix.printed[0].toStdString(), "hello 2"); +} + +BOOST_AUTO_TEST_CASE (PreludeLoadsFromBinaryData) +{ + ConsoleFixture fix; + const auto prelude = String::fromUTF8 (scripts::console_lua, scripts::console_luaSize); + auto result = fix.engine.execute (prelude, fix.env, "console.lua"); + BOOST_REQUIRE_MESSAGE (result.wasOk(), result.getErrorMessage().toStdString()); + + BOOST_REQUIRE (fix.env["console"].get_type() == sol::type::table); + BOOST_REQUIRE (fix.env["session"].get_type() == sol::type::function); + BOOST_REQUIRE (fix.env["Context"].get_type() == sol::type::table); + BOOST_REQUIRE (fix.env["command"].get_type() == sol::type::table); + // el.command generates SHOW_ABOUT etc. from Commands::toString. + BOOST_REQUIRE (fix.env["command"]["SHOW_ABOUT"].get_type() == sol::type::number); + + // The command manager is reached through el.Context, not a global. + BOOST_REQUIRE (fix.engine.execute ("Context.instance():commands() ~= nil", fix.env).wasOk()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 1); + BOOST_REQUIRE_EQUAL (fix.printed[0].toStdString(), "true"); + fix.printed.clear(); + + // console.log goes to the environment's print, not stdout. + BOOST_REQUIRE (fix.engine.execute ("console.log ('via console')", fix.env).wasOk()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 1); + BOOST_REQUIRE_EQUAL (fix.printed[0].toStdString(), "via console"); + + // session() resolves the live session through el.Context. + BOOST_REQUIRE (fix.engine.execute ("session().name", fix.env).wasOk()); + BOOST_REQUIRE_EQUAL (fix.printed.size(), 2); +} + +BOOST_AUTO_TEST_CASE (ScriptExecRaisesOnMissingScript) +{ + ConsoleFixture fix; + auto result = fix.engine.execute ("require ('el.script').exec ('does-not-exist', _ENV)", fix.env); + BOOST_REQUIRE (result.failed()); + BOOST_REQUIRE (fix.printed.isEmpty()); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/test/scripting/dspscripttest.cpp b/test/scripting/dspscripttest.cpp index 26e422a04..04a708e17 100644 --- a/test/scripting/dspscripttest.cpp +++ b/test/scripting/dspscripttest.cpp @@ -4,6 +4,7 @@ #include #include "luatest.hpp" +#include "luascripts.hpp" #include "scripting/dspscript.hpp" #include "scripting/scriptloader.hpp" #include "testutil.hpp" @@ -81,4 +82,87 @@ BOOST_AUTO_TEST_CASE (Basics) expect (Amp.get_or ("released", false) == true); } +static juce::String dspScriptWithProcess (const char* body) +{ + return juce::String (R"( +--- Validate fixture. +-- @script validate_fixture +-- @type DSP +local function layout() return { audio = { 2, 2 }, midi = { 1, 1 } } end +local function process (a, m, p, c, t) +)") + body + + R"( +end +return { type = 'DSP', layout = layout, process = process } +)"; +} + +BOOST_AUTO_TEST_CASE (ValidateShippedAmp) +{ + const auto amp = String::fromUTF8 (scripts::amp_lua, scripts::amp_luaSize); + auto result = DSPScript::validate (amp); + BOOST_REQUIRE_MESSAGE (result.wasOk(), result.getErrorMessage().toStdString()); +} + +BOOST_AUTO_TEST_CASE (ValidateRejectsEmpty) +{ + BOOST_REQUIRE (DSPScript::validate ("").failed()); +} + +BOOST_AUTO_TEST_CASE (ValidateRejectsSyntaxError) +{ + BOOST_REQUIRE (DSPScript::validate ("return {").failed()); +} + +BOOST_AUTO_TEST_CASE (ValidateRejectsNonTable) +{ + BOOST_REQUIRE (DSPScript::validate ("return 42").failed()); +} + +BOOST_AUTO_TEST_CASE (ValidateRendersMidiAndAudio) +{ + auto result = DSPScript::validate (dspScriptWithProcess (R"( + local buf = m:get (1) + assert (buf:size() > 0, 'expected midi events') + a:fade (1.0, 0.5) + )")); + BOOST_REQUIRE_MESSAGE (result.wasOk(), result.getErrorMessage().toStdString()); +} + +BOOST_AUTO_TEST_CASE (ValidateRejectsProcessError) +{ + auto result = DSPScript::validate (dspScriptWithProcess ("error ('process exploded')")); + BOOST_REQUIRE (result.failed()); + BOOST_REQUIRE_MESSAGE (result.getErrorMessage().contains ("process exploded"), + result.getErrorMessage().toStdString()); +} + +BOOST_AUTO_TEST_CASE (ValidateRejectsNonFiniteOutput) +{ + auto result = DSPScript::validate (dspScriptWithProcess ("a:fade (math.huge, math.huge)")); + BOOST_REQUIRE (result.failed()); + BOOST_REQUIRE_MESSAGE (result.getErrorMessage().contains ("non-finite"), + result.getErrorMessage().toStdString()); +} + +BOOST_AUTO_TEST_CASE (ProcessErrorDisablesScriptInsteadOfAborting) +{ + LuaFixture fix; + sol::state_view lua (fix.luaState()); + ScriptLoader loader (lua, dspScriptWithProcess ("error ('boom')")); + BOOST_REQUIRE_MESSAGE (! loader.hasError(), loader.getErrorMessage().toStdString()); + sol::table descriptor = loader.call(); + DSPScript dsp (descriptor); + BOOST_REQUIRE (dsp.isValid()); + + AudioSampleBuffer audio (2, 64); + MidiPipe midi; + dsp.process (audio, midi); + BOOST_REQUIRE (! dsp.isValid()); + BOOST_REQUIRE (dsp.getLastError().contains ("boom")); + + // A disabled script is a no-op afterwards, not a crash. + dsp.process (audio, midi); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/test/scripting/scriptmanagertest.cpp b/test/scripting/scriptmanagertest.cpp index 81c5917db..f6c9452e0 100644 --- a/test/scripting/scriptmanagertest.cpp +++ b/test/scripting/scriptmanagertest.cpp @@ -17,8 +17,9 @@ BOOST_AUTO_TEST_CASE (ScanDirectory) scripts.scanDirectory (d); // Counts every script that parses with a non-empty @type (ScriptInfo::valid), - // regardless of type — not just dsp/dspui. - BOOST_REQUIRE_EQUAL (scripts.getNumScripts(), 13); + // regardless of type — not just dsp/dspui. A lower bound so adding or + // removing a shipped script does not break the test. + BOOST_REQUIRE_GE (scripts.getNumScripts(), 10); } BOOST_AUTO_TEST_SUITE_END() diff --git a/test/snippets/sol3_parent.lua b/test/snippets/sol3_parent.lua deleted file mode 100644 index 8f9e70293..000000000 --- a/test/snippets/sol3_parent.lua +++ /dev/null @@ -1,72 +0,0 @@ --- SPDX-FileCopyrightText: Copyright (C) Kushview, LLC. --- SPDX-License-Identifier: GPL-3.0-or-later - -local object = require ('el.object') - -local function dumpt (obj) - for k,v in pairs (obj) do print (k,v) end -end - -local function dumpmt (obj) - dumpt (getmetatable (obj)) -end - -local function runtest() - begintest ("object.new (Parent)") - local parent = object.new (Parent) - expect (parent.name == "Parent Object") - parent.name = "Parent" - expect (parent.name == "Parent") - - local Derived = object (Parent, { - name = { - get = function (self) - return rawget (self, '_name') or "Derived" - end - } - }) - - function Derived:init() - Parent.init (self) - end - - function Derived:get() - local impl = getmetatable(self).__impl - return impl:get() * 2.0 - end - - local d = object.new (Derived) - local impl = getmetatable(d).__impl - - begintest ("Derived: rawset") - d.name2 = "Alternate Name" - expect (d.name2 == "Alternate Name") - expect (d.name == "Derived", tostring (d.name)) - - begintest ("readonly att") - local r,e = pcall (function() - d.name = d.name2 - end) - expect (r == false) - expect (d.name ~= d.name2, d.name) - - begintest ("Derived:get") - - expect (type(d.get) == 'function', "wrong type: " .. type(d.get)) - expect (impl:get() == 100, impl:get()) - expect (d:get() == 200, d:get()) - - begintest ("changed") - expect (impl.changed == nil) - d.changed = function() - print ("value changed: " .. tostring (impl:get())) - end - - -- expect (impl.changed ~= nil) - begintest ("Derived:set") - d:set (50) - expect (d:get() == 100, d:get()) -end - -runtest() -collectgarbage() diff --git a/test/snippets/stream_from_c.lua b/test/snippets/stream_from_c.lua deleted file mode 100644 index d4599fc87..000000000 --- a/test/snippets/stream_from_c.lua +++ /dev/null @@ -1,18 +0,0 @@ --- SPDX-FileCopyrightText: Copyright (C) Kushview, LLC. --- SPDX-License-Identifier: GPL-3.0-or-later - -result = "" -local function read_and_print_input() - result = io.read ("a") - print ("data read: " .. result) -end - -currentpos = stream:seek ("cur") -local oi = io.input() -io.input (stream) - -read_and_print_input() - -io.input (oi) -stream:close() -stream = nil From e40f61908cbb519bd87ed86166b2b366be6e9d48 Mon Sep 17 00:00:00 2001 From: Michael Fisher Date: Tue, 22 Sep 2026 23:44:36 -0400 Subject: [PATCH 4/5] docs: plans: working out proxy/delegate system between model/engine layers. --- docs/plans/extensions.md | 12 +- docs/plans/session-proxy.md | 338 ++++++++++++++++++++-------------- docs/plans/session-scripts.md | 47 +++-- 3 files changed, 228 insertions(+), 169 deletions(-) diff --git a/docs/plans/extensions.md b/docs/plans/extensions.md index b59c5e29e..675c7717a 100644 --- a/docs/plans/extensions.md +++ b/docs/plans/extensions.md @@ -1,17 +1,17 @@ # `.element` Extension Format + App-Side Lua Runtime -> **Phase 0 prerequisites:** [session-proxy.md](session-proxy.md) — the `SessionProxy` +> **Phase 0 prerequisites:** [session-proxy.md](session-proxy.md) — the `GraphController` > through which every topology mutation (UI, undo, Lua, session load) flows and from which > hook events are dispatched — and [session-scripts.md](session-scripts.md) — console, > `HookBus`, `el.hooks`, mutation verbs on `el.Session`/`el.Graph`, the > restricted-environment/capability model, and `ScriptingService`. Those are built first; -> Phase 2 below reduces to graph providers over the model verbs and Phase 3 to +> Phase 2 below reduces to graph providers over the `el.Session`/`el.Graph` bindings and Phase 3 to > extension-owned registration. [scripting-audit.md](scripting-audit.md) records the > starting state. See also [luajit.md](luajit.md) for the LuaJIT assessment. ## Context -Element has a mature Lua substrate (embedded Lua 5.4, sol2 bindings under `src/el/`, a shared `ScriptingEngine` on `Context`) but the app-side surface is thin (see [scripting-audit.md](scripting-audit.md)). Phase 0 supplies startup script execution (`ScriptingEngine::execute`, user `init.lua`), the Lua graph-mutation API (model verbs) and lifecycle hooks; GUI scripting remains limited to embedded View scripts until Phase 4. The goal is a new **`*.element` extension format** — a directory (like `.lv2`/`.vst3`) acting as a package/extension that can carry Lua modules, hook scripts, views/panels, DSP scripts, graphs (serialized data **or** Lua builder scripts — manifest decides), presets, resources, and bundled plugins (CLAP first) — plus the app-side Lua runtime it requires: graph building, lifecycle hooks, and GUI extensibility. +Element has a mature Lua substrate (embedded Lua 5.4, sol2 bindings under `src/el/`, a shared `ScriptingEngine` on `Context`) but the app-side surface is thin (see [scripting-audit.md](scripting-audit.md)). Phase 0 supplies startup script execution (`ScriptingEngine::execute`, user `init.lua`), the Lua graph-mutation API (`el.Session`/`el.Graph` over `GraphController`) and lifecycle hooks; GUI scripting remains limited to embedded View scripts until Phase 4. The goal is a new **`*.element` extension format** — a directory (like `.lv2`/`.vst3`) acting as a package/extension that can carry Lua modules, hook scripts, views/panels, DSP scripts, graphs (serialized data **or** Lua builder scripts — manifest decides), presets, resources, and bundled plugins (CLAP first) — plus the app-side Lua runtime it requires: graph building, lifecycle hooks, and GUI extensibility. **Decisions made with user:** - Terminology: **Extension** everywhere (classes, service, Lua modules). Directory suffix stays `.element`. @@ -68,14 +68,14 @@ C++ side: `struct ExtensionManifest` — plain struct parsed once from the sol t | `ExtensionManager` (scan/load/unload registry) | `src/scripting/extensionmanager.hpp/.cpp` | | `ScriptingService` (`Service`, **Phase 0**) | `src/services/scriptingservice.hpp/.cpp` | | `HookBus` (C++ event dispatcher, **Phase 0**, owned by `Context`) | `include/element/hooks.hpp`, `src/hooks.cpp` | -| `SessionProxy` (**Phase 0**, see session-proxy.md) | `src/engine/sessionproxy.hpp/.cpp` | +| `GraphController` (**Phase 0**, see session-proxy.md) | `src/engine/graphcontroller.hpp/.cpp` | | `el.hooks` (**Phase 0**), `el.ui` Lua modules | `src/el/Hooks.cpp` + `hooks.lua`, `src/el/UI.cpp` | | `ViewFactory` (slug → ContentView registry) | `src/ui/viewfactory.hpp/.cpp`, owned by `GuiService` | | `ScriptContentView` + `MissingExtensionView` | `src/ui/scriptcontentview.hpp/.cpp` | Key existing seams to reuse (verified): - `ScriptingEngine::addPackage(name, loader)` runtime package registry ([scripting.cpp:118](../../src/scripting.cpp#L118)) — extension Lua modules register here (namespaced; no global `package.path` pollution). -- Mutation verbs on the models (`Session::addGraph/removeGraph/moveGraph/setActiveGraph`, `Graph::addNode/addPlugin/removeNode/connect/disconnect/connectChannels`), backed by `SessionProxy` ([session-proxy.md](session-proxy.md)) and already bound as `el.Session`/`el.Graph` in Phase 0. `EngineService` keeps its public signatures as thin forwards for UI/undo callers. +- `el.Session`/`el.Graph` mutation bindings (`addGraph/removeGraph/moveGraph/setActiveGraph`, `addNode/addPlugin/removeNode/connect/disconnect/connectChannels`) over `GraphController` ([session-proxy.md](session-proxy.md)), resolved per call through the services, landed in Phase 0. The model itself carries no engine verbs. `EngineService` keeps its public signatures as thin forwards for UI/undo callers. - `StandardContent::createContentView(const String&)` virtual, consulted first by `setMainView`/`setSecondaryView` ([standard.hpp:88](../../include/element/ui/standard.hpp#L88), [standard.cpp:541,633](../../src/ui/standard.cpp#L541)). - `NavigationConcertinaPanel::addPanel(desc, factory, header)` public ([navigation.hpp:73](../../include/element/ui/navigation.hpp#L73)); panel state keys by name — extension slugs must be stable. - `ScriptView` per-view `sol::environment` + descriptor pattern ([scriptview.cpp](../../src/ui/scriptview.cpp)) — the sandbox precedent for all extension script execution. @@ -95,7 +95,7 @@ Ownership/lifetime rule: `ExtensionManager` is owned by `ScriptingEngine::Impl` ### Phase 2 — Graph providers (the Lua graph API comes from Phase 0) - New: `test/scripting/graphprovidertests.cpp`. - Modified: `src/services/scriptingservice.cpp` (provider instantiation), `src/ui/mainmenu.cpp` + commands (File → New Graph From Extension ▸ submenu). -- Builder scripts use the Phase 0 model verbs; no `el.engine` module exists. Lua two-value `nil, "message"` error convention: +- Builder scripts use the Phase 0 `el.Session`/`el.Graph` bindings; no `el.engine` module exists. Lua two-value `nil, "message"` error convention: ```lua local s = require ('el.Context').instance():session() local g = s:addGraph ("My Rig") diff --git a/docs/plans/session-proxy.md b/docs/plans/session-proxy.md index 995ddf44b..785bf3bb0 100644 --- a/docs/plans/session-proxy.md +++ b/docs/plans/session-proxy.md @@ -1,18 +1,17 @@ -# Session Proxy — model-initiated engine changes +# Graph Controller — the engine-side mutation path -Architectural decision behind [session-scripts.md](session-scripts.md): how Lua (and -anything else) mutates the live session, and where hook events are dispatched from. +Architectural decision behind [session-scripts.md](session-scripts.md): where topology +changes to the live session are performed, and where hook events are dispatched from. See [scripting-audit.md](scripting-audit.md) for the state of the code this builds on. -## Motivation +Formerly "SessionProxy". Renamed because the object is not a proxy for the session: it +is the engine-side controller of the session's graphs. The file name is kept so links +elsewhere stay valid. -Direction: any modification to engine-layer state is **initiated in the model** -(`Session`, `Graph`, `Node`). Model objects hold a shared internal object — the -*proxy* — that performs the engine-side work and syncs state back. This is explicitly -not "the model emits a signal and hopes a service reacts". +## Motivation -Today `EngineService` is the only coordination point and it keeps model and engine -aligned by ordering and by index: +Today `EngineService` is the only coordination point between model and engine, and it +keeps them aligned by ordering and by index: - Root graphs: `EngineService::addGraph (const Node&, bool)` ([engineservice.cpp:347-386](../../src/services/engineservice.cpp#L347)) creates a @@ -23,82 +22,74 @@ aligned by ordering and by index: `nodes.addChild`. - `EngineService::moveGraph` calls `AudioEngine::moveGraph` then `Session::moveGraph`. That "pure model primitive + pure engine primitive, coordinated by the service" split - (#1184) was done deliberately so a proxy could call both. + (#1184) was done deliberately so a single coordinator could call both. + +Two things are wanted from the change: -The proxy is also the natural **single dispatch point for hooks**: if every topology -change passes through it, `graph.added` / `node.removed` / `connection.added` fire once -regardless of source (menu, undoable action, Lua, session load). +1. **One mutation path.** Every topology change — menu, undoable action, Lua, session + load — goes through one object, so `graph.added` / `node.removed` / + `connection.added` fire exactly once regardless of source. That object is the single + `HookBus` dispatch point for those events. +2. **Separation of concerns.** The model (`Session`, `Graph`, `Node` in + `include/element`) stays a data layer: value types over a `ValueTree`, no engine + headers, callable from anywhere. The engine layer owns engine changes. The binding + layer (`src/el/`), which already knows both, wires Lua to the controller. Dependencies + point one way: engine → model, never model → engine. ## Options considered -**A — proxy object stored in the tree** (a ref-counted object under a `tags::proxy` -property, the `tags::updater` idiom). Rejected on lifetime grounds: - -- `Context::Impl::freeAll()` frees `services` first and `session` later - ([context.cpp:94-106](../../src/context.cpp#L94)); `PluginProcessor::~PluginProcessor` - deactivates services and *then* calls `session->clear()` - ([pluginprocessor.cpp:373-390](../../src/pluginprocessor.cpp#L373)). A proxy living in - the tree would hold pointers into a dead `EngineService`/`RootGraphs` unless it is - disarmed on every deactivate path. -- `Session::clear()` (`setMissingProperties (true)`) and `Session::loadData()` (whole-tree - swap, [session.cpp:150-160](../../src/session.cpp#L150)) strip or replace the tree. -- A `var`-held object serializes as `"Object 0x…"` unless added to - `Node::sanitizeProperties`. -- It moves all of `RootGraphs` into an object whose lifetime is governed by `ValueTree` - refcounts. - -**B — service facade** (a `GraphService`/`EngineService` vocabulary plus an `el.engine` -Lua module resolving the service per call). Least change, but the models stay inert, -`EngineService` stays the god object, and model-initiated mutation is not achieved. - -**C — service-owned proxy, registered on the Session, discoverable from the tree.** -Chosen. Lifetime follows the service; the model holds only a `weak_ptr`; a tiny handle -in the tree lets any `Node` copy find its `Session`. +**A — controller object stored in the tree** (a ref-counted object under a +`tags::proxy` property, the `tags::updater` idiom). Rejected on lifetime grounds: +`Context::Impl::freeAll()` frees `services` first and `session` later +([context.cpp:94-106](../../src/context.cpp#L94)); `PluginProcessor::~PluginProcessor` +deactivates services and *then* calls `session->clear()`; `Session::clear()` and +`loadData()` strip or replace the tree; a `var`-held object serializes as +`"Object 0x…"` unless stripped. + +**B — leave `EngineService` as the facade.** Least change, but `RootGraphs` stays inside +the service, the hook dispatch is smeared across it, and it stays the god object. + +**C — controller registered on the `Session`, discoverable from any `Node` copy via a +`Session::Handle` planted on the root tree, with engine verbs on the model.** Rejected +after review: + +- It makes `include/element` depend on `src/engine`, makes value types + message-thread-only, and lets a `Graph` copy instantiate plugins. +- The "detached copies are inert" claim does not hold. [session.cpp:170](../../src/session.cpp#L170), + [:338](../../src/session.cpp#L338) and [:373](../../src/session.cpp#L373) copy the + *root* tree, and a root copy carries the handle; `findFor` on that copy would resolve + the live session and mutate the running engine. +- `Session::addGraph` behaving differently depending on whether a controller happens to + be installed is a hidden mode. + +**D — `EngineService`-owned controller, reached through the services.** Chosen. Same +object as C (it absorbs `RootGraphs` and is the hook dispatch point), but the model is +not involved: callers are `EngineService` forwards, the undo actions (unchanged, they +call `EngineService`), and the Lua bindings, which resolve it per call through +`Context` → services → `EngineService`, per the *Lua Bindings* rules in CLAUDE.md. ## Design ### Ownership and lifetime -- `SessionProxy` (`src/engine/sessionproxy.hpp/.cpp`) becomes the home of +- `GraphController` (`src/engine/graphcontroller.hpp/.cpp`) becomes the home of `RootGraphHolder` and `RootGraphs`, moved verbatim from [engineservice.cpp:78-305](../../src/services/engineservice.cpp#L78). The constructor - captures `Context&`, `AudioEnginePtr`, `SessionPtr` and `HookBus&`. The destructor - never calls `context()` (services may already be gone). -- `EngineService` owns `std::shared_ptr proxy`: created in `activate()` - after `engine->activate()` and before `sessionReloaded()`, followed by - `session->setProxy (proxy)`; in `deactivate()` after `session->saveGraphState()`: - `session->setProxy (nullptr)` then `proxy.reset()` (replaces `graphs->clear()`). -- `Session::Impl` ([session.cpp:78-90](../../src/session.cpp#L78), currently empty) holds - `std::weak_ptr proxy`. Mis-ordered teardown degrades to "no proxy → - pure tree op", never a dangling call. -- The proxy survives session reload for free: `loadData()`/`clear()` touch - `objectData`, not `Impl`. Holders are rebuilt by `sessionReloaded()` → `proxy->reload()`. - -### How a `Node` copy finds the proxy - -`Node` is a value type; copies share only the tree. So the tree must lead back to the -`Session`: - -- `Session::Handle : juce::ReferenceCountedObject { Session* session; }` is planted on - the session root tree under a new `tags::sessionObject` — the same idiom as - `GraphManager` planting `NodeModelUpdater` under `tags::updater` - ([graphmanager.cpp:651-668](../../src/engine/graphmanager.cpp#L651)). -- `static SessionPtr Session::findFor (const juce::ValueTree& any)` reads - `any.getRoot()[tags::sessionObject]` → handle → `Session*` → `SessionPtr`. -- Plant: `Session` ctor, `loadData`, `setMissingProperties (reset = true)`. - Null: `~Session()` before `clear()`. - Ignore: `Session::valueTreePropertyChanged` - ([session.cpp:262](../../src/session.cpp#L262), alongside `object`/`updater`). - Strip: `Node::sanitizeProperties` ([node.cpp:349](../../src/node.cpp#L349)), which - `createXml`/`writeToFile` already call on the session copy. -- Detached copies (`.elg` export, `RemoveNodeAction::nodeData`, `createCopy()` of a - graph subtree) carry no root handle → no proxy → pure tree ops. That is exactly the - headless/undo-friendly behaviour wanted. - -### `SessionProxy` API - -All methods return `bool`/`Node`, never show an `AlertWindow`, and assert the message -thread. + takes `Context&`, `AudioEnginePtr`, `SessionPtr` and `HookBus&` (injected, never + looked up). The destructor never calls `context()` (services may already be gone). +- `EngineService` owns `std::unique_ptr controller`: created in + `activate()` after `engine->activate()` and before `sessionReloaded()`; reset in + `deactivate()` after `session->saveGraphState()` (replaces `graphs->clear()`). + `GraphController* EngineService::controller()` returns null when inactive. +- The controller survives session reload for free: `loadData()`/`clear()` touch the + tree, not the controller. Holders are rebuilt by `sessionReloaded()` → `reload()`. +- The model is untouched: no `Session::Handle`, no `findFor`, no weak pointer in + `Session::Impl`, no engine verbs on `Session`/`Graph`. `Session::addGraph`, + `moveGraph`, `setActiveGraph` stay what they are today: pure tree operations. + +### `GraphController` API + +All methods return `bool`/`Node`, never show UI, and assert the message thread. ```cpp // root graphs @@ -106,7 +97,7 @@ bool addGraph (const Node& graph, bool makeActive); Node addGraph (const juce::String& name); // default ports from engine->getNumChannels bool removeGraph (int index); bool moveGraph (const Node& graph, int newIndex); -bool setActiveGraph (int index); // today's setRootNode + Session::setActiveGraphData +bool setActiveGraph (int index); // today's setRootNode + Session::setActiveGraph void reload(); // today's sessionReloaded, under HookBus::ScopedSuspend void clear(); void syncModels(); // nodes @@ -120,53 +111,43 @@ bool addConnection (const Node& graph, uint32 s, uint32 sp, uint32 d, uint32 dp) bool removeConnection (const Node& graph, uint32 s, uint32 sp, uint32 d, uint32 dp); bool connectChannels (const Node& graph, const Node& src, int sc, const Node& dst, int dc, PortType = PortType::Audio, int count = 1); -void disconnectNode (const Node&, bool inputs, bool outputs, bool audio, bool midi); // per-arc removeConnection +struct DisconnectOptions { bool inputs = true, outputs = true, audio = true, midi = true; }; +void disconnectNode (const Node&, DisconnectOptions = {}); // per-arc removeConnection // lookup -GraphManager* findGraphManagerFor (const Node&) const; +GraphManager* findGraphManagerFor (const Node&) const; // by graph tree identity RootGraphManager* findActiveRootGraphManager() const; ``` -`addNode`/`addPlugin` pre-validate through `PluginManager::findDescriptionFor` / -`getKnownPlugins()` so a bad identifier from Lua returns an invalid `Node` instead of -reaching the modal `AlertWindow` inside `GraphManager::addNode` -([graphmanager.cpp:380,408](../../src/engine/graphmanager.cpp#L380)). - -### Model verbs - -`Session` (`include/element/session.hpp`, `src/session.cpp`): - -- `setProxy (std::shared_ptr)`, `proxy()`, `static findFor (ValueTree)`. -- Proxy-aware, existing names keep working: `addGraph (const Node&, bool)`, - `moveGraph (int, int)`, `setActiveGraph (int)`; new `removeGraph (int)`, - `Node addGraph (const juce::String& name)`. Each is - `if (auto p = proxy()) return p->X (...); return XData (...);`. -- Pure primitives stay public, named like `loadData`: `addGraphData`, `removeGraphData`, - `moveGraphData`, `setActiveGraphData`. -- `SessionService::loadNewSessionData` - ([sessionservice.cpp:316](../../src/services/sessionservice.cpp#L316)) switches to - `addGraphData`; with a proxy installed at that point the default graph would attach - immediately and then be detached/re-attached by `refreshOtherControllers()`. - -`Graph` (`include/element/graph.hpp`, `src/graph.cpp`): - -- `Node addNode (const Node& nodeTemplate)`, `Node addNode (const juce::String& id, const juce::String& format = EL_NODE_FORMAT_NAME)`, - `Node addPlugin (const juce::PluginDescription&)`, `bool removeNode (const Node&)`, - `bool connect (uint32, uint32, uint32, uint32)`, `bool disconnect (…)`, - `bool connectChannels (const Node& src, int sc, const Node& dst, int dc, PortType = Audio)`. -- Fallback without a proxy: copy the template into `nodes` with ids reset (what - `SessionLoadBenchTests::makeGraphModel` does by hand), append/remove an `Arc` in - `arcs`. -- `graph.cpp` includes `` and `"engine/sessionproxy.hpp"`; - `graph.hpp` forward-declares only. +- `addNode`/`addPlugin` pre-validate through `PluginManager::findDescriptionFor` / + `getKnownPlugins()` so a bad identifier from Lua returns an invalid `Node`. +- `DisconnectOptions` replaces today's four positional booleans; no new method on the + controller takes more than one `bool`. +- `findGraphManagerFor` matches on the graph's underlying tree, so any `Node` copy of a + live graph resolves; a copy of a detached subtree resolves to null and the call fails. + +### `GraphManager` never shows UI + +`GraphManager` shows modal alerts at +[graphmanager.cpp:29-31](../../src/engine/graphmanager.cpp#L29), [:380](../../src/engine/graphmanager.cpp#L380), +[:408](../../src/engine/graphmanager.cpp#L408) and [:418](../../src/engine/graphmanager.cpp#L418). +Pre-validation in the controller catches "unknown plugin" but not "found but failed to +instantiate", and a modal in that path hangs a headless test. `GraphManager::addNode` +and friends return an invalid `Node`/`false` and log; the alert moves to +`EngineService`, which is where UI policy lives after this change. This is a +prerequisite for the controller and lands as its own step (see *Order of work* in +session-scripts.md). ### `EngineService` afterwards Every public signature in [engine.hpp](../../include/element/engine.hpp) stays, as a -thin forward to the proxy plus the UI/plugin-list policy that does not belong in the -proxy: `AlertWindow` messages, `presentPluginWindow` (only on the entry points that show -it today, so undo/redo never pops windows), `detail::verifyPlugin` / `saveUserPlugins` / -`addToKnownPlugins`, `addMidiDeviceNode` (rewritten over `proxy->addPlugin` + -`Node::getObject()`), `stabilizeViews` after `replace`/`changeBusesLayout`. +thin forward to the controller plus the UI/plugin-list policy that does not belong in +the controller: the alerts (including those moved out of `GraphManager`), +`presentPluginWindow` (only on the entry points that show it today, so undo/redo never +pops windows), `detail::verifyPlugin` / `saveUserPlugins` / `addToKnownPlugins`, +`addMidiDeviceNode` (rewritten over `controller->addPlugin` + `Node::getObject()`), +`stabilizeViews` after `replace`/`changeBusesLayout`. Group the alert and plugin-list +policy in a `detail::` helper inside `engineservice.cpp` so the service itself is +forwards plus policy calls. Callers that keep compiling unchanged: `src/messages.cpp` undo actions, `src/services.cpp:224-320`, `guiservice.cpp:1070-1073`, @@ -187,13 +168,40 @@ Moves out of `EngineService` entirely: `RootGraphHolder`, `RootGraphs`, the bodi ([grapheditorview.cpp:80,158](../../src/ui/grapheditorview.cpp#L80)) subscribes to `graph.removed` on `context().hooks()` instead. +### Lua reaches the controller through the services + +Per CLAUDE.md *Lua Bindings*: one entry point, resolve per call, no globals. The +`el.Session`/`el.Graph` methods in [session-scripts.md](session-scripts.md) § 4 are +implemented in `src/el/Session.cpp` / `Graph.cpp` as: + +```cpp +static GraphController* controllerFor (lua_State* L) +{ + auto& ctx = element::lua::contextFrom (L); // reads _G["el.context"] like el.Context.instance() + auto* engine = ctx.services().find(); + return engine != nullptr ? engine->controller() : nullptr; +} +// el.Graph:addNode +"addNode", [] (Graph& self, const std::string& id, sol::optional format, sol::this_state L) + -> std::tuple { + auto* c = controllerFor (L); + if (c == nullptr) return { nil, "engine not running" }; + auto node = c->addNode (self, Node::makeTemplate (id, format...)); + return node.isValid() ? { make_object (node), nil } : { nil, "could not add " + id }; +} +``` + +Nothing is cached: no controller pointer in a userdata, no session cached at require +time. A `Graph` userdata is only a tree handle; the controller decides whether it +refers to a live graph. + ### Undo No structural change. `AddPluginAction`, `RemoveNodeAction`, `AddConnectionAction`, `RemoveConnectionAction` ([messages.cpp](../../src/messages.cpp)) keep calling -`EngineService`; each `perform()`/`undo()` is exactly one proxy mutation and therefore -exactly one hook. Lua-initiated mutations are **not undoable** in this phase: they bypass -`GuiService::handleMessage` +`EngineService`; each `perform()`/`undo()` is exactly one controller mutation and +therefore exactly one hook. Lua-initiated mutations are **not** undoable in this phase: +they bypass `GuiService::handleMessage` ([guiservice.cpp:1235-1245](../../src/services/guiservice.cpp#L1235)). Making them undoable later means posting `AppMessage`s, which is asynchronous and cannot return the created node — a deliberate non-goal for now. @@ -208,20 +216,20 @@ created node — a deliberate non-goal for now. [sessiontreepanel.cpp:618-627](../../src/ui/sessiontreepanel.cpp#L618) and [guiservice.cpp:171-175](../../src/services/guiservice.cpp#L171)). - Direct writes of `tags::active` in `sessiontreepanel.cpp:624-626`, `:758` and - `guiservice.cpp:173-174` become `session->setActiveGraph (index)`; otherwise + `guiservice.cpp:173-174` become `EngineService::setActiveGraph (index)`; otherwise Lua-initiated activation and hooks diverge from the UI path. ### Hook dispatch points -Only `SessionProxy` fires topology actions: `graph.added`, `graph.removing`, +Only `GraphController` fires topology actions: `graph.added`, `graph.removing`, `graph.removed`, `graph.moved`, `graph.activated`, `node.added`, `node.removing`, `node.removed`, `connection.added`, `connection.removed`. Pre-hooks (`*.removing`) exist so the GUI can close plugin windows *before* the processor dies regardless of who initiated the removal. - `reload()` runs under `HookBus::ScopedSuspend`, so session load fires nothing per node - (`GraphManager::setNodeModel` and `IONodeEnforcer` operate below the proxy anyway) and - the internal `setActiveGraph` during load does not leak `graph.activated`. + (`GraphManager::setNodeModel` and `IONodeEnforcer` operate below the controller + anyway) and the internal `setActiveGraph` during load does not leak `graph.activated`. - `session.loaded` is fired once from a new `SessionService::notifySessionLoaded()`, which also emits `sigSessionLoaded()`; called from `refreshOtherControllers()` and from [pluginprocessor.cpp:800](../../src/pluginprocessor.cpp#L800) in place of the @@ -233,23 +241,76 @@ initiated the removal. default-session open frozen, which would wrongly swallow a legitimate `graph.added` from an `.elg` import. +**Coverage gaps, decided:** + +- `ConnectionBuilder` connections made while adding a node currently happen inside + `GraphManager::addNode` ([graphmanager.cpp:801](../../src/engine/graphmanager.cpp#L801)), + below the controller. The controller applies the builder itself through its own + `addConnection` after the node exists, so each auto-connection fires + `connection.added`; `GraphManager::addNode` loses its builder parameter. +- `Processor` re-applies channel connections after a bus-layout change at + [processor.cpp:972-977](../../src/engine/processor.cpp#L972), also below the + controller. Phase 0 fires no `connection.*` for these; `changeBusesLayout` is + documented as "ports and their connections may change" and a `node.portsChanged` + action is a candidate follow-up. +- Engine-initiated active-graph changes (MIDI program change, + [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657)) bypass the + controller; `graph.activated` excludes them in Phase 0. Tracked as + session-scripts.md open question 5. + +## Future: model-initiated mutation, if ever wanted + +If a later phase wants `Session::addGraph (...)` to drive the engine, the path is a +delegate interface **declared by the model and implemented by the controller** — not a +handle in the tree, and not engine headers in `include/element`: + +```cpp +// include/element/session.hpp +class Session { +public: + struct Delegate { + virtual ~Delegate() = default; + virtual bool addGraph (Session&, const Node& graph, bool makeActive) = 0; + virtual bool removeGraph (Session&, int index) = 0; + // one method per controller verb that should be model-initiated + }; + void setDelegate (Delegate*); // EngineService installs in activate(), clears in deactivate() +}; +``` + +`Session::addGraph` calls the delegate synchronously: the controller attaches the +engine side first, then performs the tree op, then fires the hook, and the result is +returned to the caller. That is the ordering `EngineService::addGraph` already uses +today. Graph-level verbs take the graph as an argument on the session +(`session->addNode (graph, template)`) so `Graph`/`Node` copies stay inert. A +"model first, engine follows via `ValueTree::Listener`" design is explicitly not the +path: the caller cannot learn about engine failure, and reload/undo/`loadData` all have +to suspend the listener. + ## Tests -`test/SessionProxyTests.cpp`, suite `SessionProxyTests`, using `element::test::context()` -([TestMain.cpp:15-23](../../test/TestMain.cpp#L15), which activates services so the proxy -is installed) and a test node provider extracted from `CountingNodeProvider` -([SessionLoadBenchTests.cpp:58-82](../../test/SessionLoadBenchTests.cpp#L58)) into -`test/fixture/TestNodeProvider.h`: +`test/GraphControllerTests.cpp`, suite `GraphControllerTests`, using +`element::test::context()` ([TestMain.cpp:15-23](../../test/TestMain.cpp#L15), which +activates services so the controller exists) and a test node provider extracted from +`CountingNodeProvider` ([SessionLoadBenchTests.cpp:58-82](../../test/SessionLoadBenchTests.cpp#L58)) +into `test/fixture/TestNodeProvider.h`: -- `ProxyInstalledAndCleared` — own `Context`, `services().activate()/deactivate()`. -- `AddGraphAttachesEngine` — `session->addGraph` → `getObject()` non-null, `graph.added` once. +- `ControllerInstalledAndCleared` — own `Context`, `services().activate()/deactivate()`, + `controller()` non-null only while active. +- `AddGraphAttachesEngine` — `controller->addGraph` → `getObject()` non-null, + `graph.added` once; the session tree gained the child. - `AddNodeCreatesProcessor`, `ConnectDisconnect` (arc present in `arcs`, hooks fired). +- `BuilderConnectionsFireHooks` — `addNode` with a `ConnectionBuilder` → one + `connection.added` per built arc. - `RemoveNodeOrdering` — `node.removing` before `node.removed`. - `RemoveGraphFixesActive`, `SetActiveGraphSyncsEngine` (`engine->getActiveGraphIndex()`). - `ReloadFiresNoPerNodeHooks` — `loadData` + `sessionReloaded()` → zero `node.added`. -- `FallbackWithoutProxy` — bare `Context`, `Graph::addNode` adds a tree child, `getObject()` null. +- `InstantiationFailureIsSilent` — a node type whose provider returns null → invalid + `Node`, no modal, test does not hang (proves the `GraphManager` alert move). +- `DetachedGraphIsRejected` — `addNode` on a `Graph` copied out of the session → invalid + `Node`, no hook. -Register with `add_test (NAME "SessionProxyTests" COMMAND test_element --run_test=SessionProxyTests)`. +Register with `add_test (NAME "GraphControllerTests" COMMAND test_element --run_test=GraphControllerTests)`. Existing `SessionTests` keep covering the pure-tree path unchanged. Remember: a `GraphNode` driven by `GraphManager` must be heap-allocated (`ProcessorPtr keep (new GraphNode (ctx))`). @@ -259,23 +320,14 @@ Existing `SessionTests` keep covering the pure-tree path unchanged. Remember: a `AudioEngine::addGraph` works without a device and `test::context()` runs `sessionReloaded()` with zero graphs, but the full `attach` + `setRootNode` (`setPlayConfigFor (devices)`) path with real graphs has no current test; sample - rate/block size may be 0 headless. -- `GraphManager::addNode` shows *modal* alerts on instantiation failure; the proxy's - pre-validation must cover every path or a headless test hangs. What `createGraphNode` - does with unknown identifiers was not traced. -- Engine-initiated active-graph changes (MIDI program change, - [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657), writes `tags::active` - from an async update) bypass the proxy. If `graph.activated` must cover them, the - proxy needs a `ValueTree::Listener` on the `graphs` child with a self-change flag. - Follow-up, tracked as [session-scripts.md](session-scripts.md) open question 5; the - hook catalogue must say `graph.activated` excludes engine-initiated changes until then. + rate/block size may be 0 headless. Write this test before moving code. - `PluginProcessor::reloadEngine` → `reload()` must keep the full detach/re-attach behaviour; `prepareExternalPlayback` re-preparation was not traced. - Re-entrant hooks (a handler adding a node from `node.added`) work through the depth guard but interleave `GraphManager::changed()` broadcasts. Untested territory. - `RootGraphHolder` touches `Processor` internals as `friend class EngineService` - ([processor.hpp:539](../../include/element/processor.hpp#L539)); `SessionProxy` may - need adding to the friend list. + ([processor.hpp:539](../../include/element/processor.hpp#L539)); `GraphController` + needs adding to the friend list. - `new_nodetype`'s index metamethod already returns non-graph children as `Graph` today; the `Graph (const Node&)` constructor asserts type == Graph in debug builds. The `sol::make_object` split in session-scripts.md is required, not optional. diff --git a/docs/plans/session-scripts.md b/docs/plans/session-scripts.md index e06bd9d7b..fc2c5d06f 100644 --- a/docs/plans/session-scripts.md +++ b/docs/plans/session-scripts.md @@ -8,11 +8,12 @@ scripts **embedded in the session file**, where DSP/DSPUI scripts already live t Prerequisites and companions: - [scripting-audit.md](scripting-audit.md) — what exists today and what is broken. -- [session-proxy.md](session-proxy.md) — the `SessionProxy`: model-initiated engine - mutation and the single hook dispatch point. **Read it first**; this document assumes - it. Consequences here: hooks fire from the proxy, and Lua mutates the session through - model verbs on `el.Session`/`el.Graph`. The `el.engine` facade module previously - planned is dropped. +- [session-proxy.md](session-proxy.md) — the `GraphController`: the engine-side + mutation path and the single hook dispatch point. **Read it first**; this document + assumes it. Consequences here: hooks fire from the controller; `el.Session`/`el.Graph` + reach it per call through `Context` → services → `EngineService` (CLAUDE.md *Lua + Bindings*); the model stays a data layer with no engine verbs. The `el.engine` facade + module previously planned is dropped. ## Context @@ -45,7 +46,8 @@ change graph topology from Lua. There is no hook/event registry of any kind. `script.lua`. New surface (`el.hooks`) is Lua-first with a thin C core. 4. **One mutation path.** Every topology change — UI, undo, Lua, session load — goes - through `SessionProxy`. Hooks are dispatched there and nowhere else for those events. + through `GraphController`. Hooks are dispatched there and nowhere else for those + events. ## What gets built @@ -127,7 +129,7 @@ subscribe in `activate()`). In `freeAll()` before `lua`: Lua-backed handlers hold `sol::protected_function`s and must die while the state is alive. -Proof-of-shape filter: `graph.defaultName`, applied in `SessionProxy::addGraph (name)` +Proof-of-shape filter: `graph.defaultName`, applied in `GraphController::addGraph (name)` to the generated `"Graph N"` string. Single call site, no UI paint-path cost. Filters like `node.displayName` are deferred until a call site that is not in a paint loop is chosen. @@ -135,14 +137,14 @@ chosen. ### 2. Dispatch sites See [session-proxy.md](session-proxy.md) § *Hook dispatch points*. Summary: every -`SessionProxy` mutation fires its action; `reload()` runs under `ScopedSuspend`; +`GraphController` mutation fires its action; `reload()` runs under `ScopedSuspend`; `SessionService::notifySessionLoaded()` fires `session.loaded` (replacing the direct `sigSessionLoaded()` at `pluginprocessor.cpp:800` too); `session.saving` beside `sigWillSave`; `session.closed` in `closeSession()`; `app.started` at the end of `Services::launch`; `app.shutdown` in `ScriptingService::deactivate()`. `EngineService::sigNodeRemoved` is deleted; `GuiService` and `GraphEditorView` subscribe -to hooks instead (details in the proxy doc). +to hooks instead (details in the controller doc). ### 3. Console foundation (`src/ui/luaconsole.*`, `luaconsoleview.*`, `src/scripting.*`) @@ -187,8 +189,10 @@ to hooks instead (details in the proxy doc). ### 4. Lua surface for mutation (`src/el/Session.cpp`, `Graph.cpp`, `nodetype.hpp`) -All of these forward to the model verbs defined in the proxy doc and therefore fire -hooks and work headless (pure tree) when no proxy is installed. +All of these resolve the `GraphController` per call through +`Context` → services → `EngineService` (see the controller doc § *Lua reaches the +controller through the services*) and therefore fire hooks. With no active engine they +return `nil, "engine not running"`; nothing falls back to silently editing the tree. `el.Session`: @@ -389,12 +393,14 @@ and give a working REPL for exercising every later step by hand. Touches `src/scripting*`, `src/ui/luaconsole*`, `src/el/`, `scripts/` only — not `EngineService`. 2. `HookBus` + `HookBusTests` (no callers yet). -3. `Session::Handle`, `findFor`, `*Data` primitives (no behaviour change). +3. `GraphManager` stops showing alerts: failures return an invalid `Node`/`false`, the + alerts move to `EngineService`. Prerequisite for headless controller tests. 4. The headless "real graphs through `attach` + `setRootNode`" test first (the untested path in [session-proxy.md](session-proxy.md) § Risks — a modal alert on that path - hangs the runner, so prove it before moving code). Then `SessionProxy` + + hangs the runner, so prove it before moving code). Then `GraphController` + `EngineService` forwards in one PR so all callers keep compiling; - `GuiService`/`GraphEditorView` hook handlers; `session.*` events; `SessionProxyTests`. + `GuiService`/`GraphEditorView` hook handlers; `session.*` events; + `GraphControllerTests`. 5. Lua surface: `el.Session`/`el.Graph` verbs, `el.hooks`; `SessionLuaTests`. 6. View-script `require` audit and the reporting pass (§8 *Compatibility*), then session `scripts` tree, `types::Hook`, restricted env, trust, `ScriptingService`, panel UI; @@ -411,7 +417,7 @@ Register every suite in `test/CMakeLists.txt` with - `HookBusTests` (`test/HookBusTests.cpp`): priority order; filter value threading; `remove`/`removeOwner`; auto-disable after 3 errors and re-enable after success; nested `doAction` queued not recursed; `ScopedSuspend`; off-thread `doAction` asserts. -- `SessionProxyTests` — see [session-proxy.md](session-proxy.md). +- `GraphControllerTests` — see [session-proxy.md](session-proxy.md). - `ConsoleTests` (`test/scripting/consoletests.cpp`): `execute ("1+1")` prints `2`; `execute ("x = 5")` then `execute ("x")` prints `5` in the persistent env; a syntax error yields a failed `Result` whose message contains a traceback line; prelude loads @@ -420,7 +426,8 @@ Register every suite in `test/CMakeLists.txt` with `test/snippets/session_mutate.lua`): state initialised with `test::context()`; `session():addGraph ('lua')`, `g:addNode ('element.volume')`, `hooks.action ('node.removed', …)`, `g:removeNode (n)` → handler ran once; `g:addNode ('bogus')` - returns `nil, msg`. + returns `nil, msg`; with services deactivated, `session():addGraph ('x')` returns + `nil, "engine not running"` and the tree is unchanged. - `HookScriptTests` (`test/scripting/hookscripttests.cpp`): session `scripts` tree round-trips through XML; descriptor with `requires = {'session'}` not granted → `attach` never called and no error spam; granted → `ctx.session` present and nothing @@ -436,7 +443,7 @@ Register every suite in `test/CMakeLists.txt` with - `HookBus`, `el.hooks`, the restricted-env + capability machinery, `ScriptingService` and the mutation verbs all land here. [extensions.md](extensions.md) Phase 2 becomes - "reuse the model verbs"; Phase 3 reduces to wiring extension ownership and a + "reuse the `el.Session`/`el.Graph` bindings"; Phase 3 reduces to wiring extension ownership and a friendlier default grant; extension entry scripts reuse the same descriptor and capability conventions. - The capability model is also the answer to "extensions are trusted-but-isolated" — @@ -451,10 +458,10 @@ Register every suite in `test/CMakeLists.txt` with 3. Filter catalogue beyond `graph.defaultName`: candidates are `session.saving` payload and `node.displayName`, the latter only once a non-paint call site is identified. 4. Undoable Lua mutations: needs `AppMessage`-based routing or an explicit - `UndoManager` transaction API on the proxy. Out of scope for Phase 0. + `UndoManager` transaction API on the controller. Out of scope for Phase 0. 5. Engine-initiated active-graph changes (MIDI program change, [audioengine.cpp:657-665](../../src/engine/audioengine.cpp#L657), writes - `tags::active` from an async update) bypass the proxy, so `graph.activated` does + `tags::active` from an async update) bypass the controller, so `graph.activated` does **not** fire for them in Phase 0. Document it as such in the hook catalogue. Fix is a - `ValueTree::Listener` on the `graphs` child inside `SessionProxy` with a self-change + `ValueTree::Listener` on the `graphs` child inside `GraphController` with a self-change flag; follow-up after step 4. From c8d0b45bf62bb59b781a5b1218ab308df64f6f2b Mon Sep 17 00:00:00 2001 From: Michael Fisher Date: Tue, 22 Sep 2026 23:48:19 -0400 Subject: [PATCH 5/5] ldoc: update config to account for stale file removal. --- docs/config.ld | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/config.ld b/docs/config.ld index 2f7cee9b4..d8b40c27f 100644 --- a/docs/config.ld +++ b/docs/config.ld @@ -8,7 +8,6 @@ file = { exclude = { '../src/el/midi_buffer.hpp', '../src/el/sol_helpers.hpp', - '../src/el/vector.c', '../src/el/color.lua', '../src/el/session.lua', '../src/el/Parameter.cpp'