From f5d0f6b2f6c69b20e817a63a739c081a8d9d56f4 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Thu, 17 Sep 2026 03:29:28 -0400 Subject: [PATCH] Carry a patch for the engine dropping depth on a config update ViewConfig::apply_update in perspective-client applies ten fields and neither group_by_depth nor split_by_depth is among them. A depth therefore works when a view is created -- table.view({ group_by_depth: 1 }), which is what the fork's own depth_test.mjs exercises -- and is silently discarded by restore(), which is how a viewer changes its own configuration. That is why the EXPAND buttons did nothing however the value was sent. group_by_depth and the omission are both upstream; the fork mirrored split_by_depth alongside it faithfully, including the omission. So the column axis expand/collapse the fork added has the same hole, and pf_app only avoids it by collapsing columns through a truncated split_by instead. The patch adds an Option-aware sibling to _apply and applies both fields. It cannot be built here -- protoc is absent, so the generated protobuf modules are missing and the crate does not compile for unrelated reasons -- but cargo check reports nothing against view_config.rs. It sits in ui/vendor with the rebuild instructions, beside the tarballs it is not yet in. Meanwhile applyDepth reads the config back after restoring and falls back to view.set_depth() when the value did not stick, so the buttons work on the current build. The fallback loses depth on a view rebuild, as it always did; what is not back is the shim and listeners that used to chase that. The check clears itself once the engine honours the field. Co-Authored-By: Claude Opus 5 (1M context) --- ui/src/views/Forecast.jsx | 20 ++++++++ ...-apply-depth-fields-on-config-update.patch | 45 ++++++++++++++++++ ui/vendor/README.md | 46 +++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 ui/vendor/0001-apply-depth-fields-on-config-update.patch diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index 4238363..f3c6832 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -844,6 +844,26 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio // name in a restored config fails the lookup. const { table: _t, ...cfg } = await viewer.save() await viewer.restore({ ...cfg, group_by_depth: d + 1 }) + + // Until the engine carries the patch in ui/vendor, restore() drops the depth: + // ViewConfig::apply_update applies ten fields and neither depth is among + // them, so the value arrives, deserializes, and is discarded before the + // engine sees it. Read it back to find out which build we are on, and fall + // back to the imperative call when it did not stick. + // + // The fallback is the old behaviour, warts and all: set_depth lives on the + // view, so it is lost whenever the viewer rebuilds one. What is deliberately + // not back is the machinery that used to chase that -- no observer shim, no + // focus listeners, no retries. Depth is simply re-applied when you next press + // a button, which is predictable, and it becomes durable for free once the + // engine is rebuilt. + const after = await viewer.save() + if (after.group_by_depth !== d + 1) { + const view = await viewer.getView() + await view.set_depth(d) + const plugin = await viewer.getPlugin() + await plugin.draw(view) + } setExpandDepth(d) } diff --git a/ui/vendor/0001-apply-depth-fields-on-config-update.patch b/ui/vendor/0001-apply-depth-fields-on-config-update.patch new file mode 100644 index 0000000..a460fd6 --- /dev/null +++ b/ui/vendor/0001-apply-depth-fields-on-config-update.patch @@ -0,0 +1,45 @@ +diff --git a/rust/perspective-client/src/rust/config/view_config.rs b/rust/perspective-client/src/rust/config/view_config.rs +index fa4f36e..360b816 100644 +--- a/rust/perspective-client/src/rust/config/view_config.rs ++++ b/rust/perspective-client/src/rust/config/view_config.rs +@@ -526,6 +526,23 @@ impl ViewConfig { + } + } + ++ /// `_apply` for a field which is itself `Option`, where `None` in the update ++ /// means "not mentioned" rather than "clear it". `Option>` would be ++ /// needed to express both, and the wire format cannot carry the difference: ++ /// these fields are `skip_serializing_if = "Option::is_none"`, so an absent ++ /// field and an explicit null arrive identically. To lift a depth, send the ++ /// number of levels on that axis rather than clearing it. ++ fn _apply_optional(field: &mut Option, update: Option) -> bool { ++ match update { ++ None => false, ++ Some(_) if *field == update => false, ++ Some(_) => { ++ *field = update; ++ true ++ }, ++ } ++ } ++ + pub fn reset(&mut self, reset_expressions: bool) { + let mut config = Self::default(); + if !reset_expressions { +@@ -568,6 +585,16 @@ impl ViewConfig { + changed = Self::_apply(&mut self.windows, update.windows) || changed; + changed = Self::_apply(&mut self.group_rollup_mode, update.group_rollup_mode) || changed; + changed = Self::_apply(&mut self.split_rollup_mode, update.split_rollup_mode) || changed; ++ ++ // Without these two, a depth can be set when a view is created -- ++ // `table.view({ group_by_depth })` -- but never through `restore()`, which ++ // merges a `ViewConfigUpdate` onto the live config. The field arrives, ++ // deserializes, and is then dropped here, so the viewer's config and the ++ // engine never see it and nothing happens. That makes expand/collapse ++ // unreachable for anything driven by `restore()`, which is how a viewer ++ // changes its own configuration. ++ changed = Self::_apply_optional(&mut self.group_by_depth, update.group_by_depth) || changed; ++ changed = Self::_apply_optional(&mut self.split_by_depth, update.split_by_depth) || changed; + if self.group_rollup_mode == GroupRollupMode::Total && !self.group_by.is_empty() { + tracing::info!("`total` incompatible with `group_by`"); + changed = true; diff --git a/ui/vendor/README.md b/ui/vendor/README.md index e5901f4..da630e4 100644 --- a/ui/vendor/README.md +++ b/ui/vendor/README.md @@ -48,3 +48,49 @@ This is a fork, with the maintenance that implies. The exit is upstream taking the change — the patch is small and additive, and the engine work is already theirs. When a release ships it, delete this directory and put normal version ranges back in `ui/package.json`. + +## Pending patch: depth fields on a config update + +`0001-apply-depth-fields-on-config-update.patch` is not in the vendored +tarballs yet. It fixes an upstream bug in +`rust/perspective-client/src/rust/config/view_config.rs`: +`ViewConfig::apply_update` applies ten fields and **neither `group_by_depth` +nor `split_by_depth` is among them**. So a depth can be set when a view is +created (`table.view({ group_by_depth: 1 })` — which is what the fork's own +`depth_test.mjs` exercises) but never through `restore()`, which is how a +viewer changes its own configuration. The field arrives, deserializes, and is +discarded before the engine sees it. + +`group_by_depth` and the omission are both upstream; the fork mirrored +`split_by_depth` alongside it faithfully, including the omission. + +Note the semantics, from `server.cpp`: + +```cpp +ctx1->set_depth(row_pivot_depth - 1); // one-sided +ctx2->set_depth(t_header::HEADER_ROW, row_pivot_depth - 1); // two-sided +``` + +The config field counts **levels to show**; `view.set_depth()` counts the +boundary below them. So `group_by_depth: n` equals `set_depth(n - 1)`, and +`Forecast.jsx` sends `d + 1`. + +To apply: + +```bash +cd $PSP_DIR # default ~/perspective +git apply /path/to/pf_app/ui/vendor/0001-apply-depth-fields-on-config-update.patch +./ui/vendor/rebuild-perspective.sh +cd ui && npm install +``` + +`cargo check` on this patch was clean — the 125 errors it reports without +`protoc` installed are unresolved generated protobuf modules, none of them in +`view_config.rs`. + +Until the rebuild lands, `applyDepth()` in `Forecast.jsx` reads the config back +after restoring it and falls back to the imperative `view.set_depth()` when the +value did not stick. That fallback loses the depth whenever the viewer rebuilds +its view; the declarative path does not, which is the point of the patch. The +check is cheap and self-clearing — once the engine honours the field, the +fallback stops running on its own.