From 349f921671414a8ce520fdeb1564ddb2a6c32535 Mon Sep 17 00:00:00 2001 From: xtof Date: Sun, 1 Nov 2026 01:04:13 +0100 Subject: [PATCH 1/2] Add netlist review threads to the Python host A review is a directory committed with the design (docs/review_format.md): threads anchored on netlist objects by naja ids only, one new file per write so branches merge without conflicts, and the netlist each thread was written on stored as a content-addressed naja-if snapshot. - review.py: the format, anchor location by ids (names only at write time), resolution (exact / other_snapshot / unresolved), snapshots. - ViewerSession pushes review_response after each root load and every write; review_add_thread / review_add_event requests. - naja-schematic --review DIR [--author NAME]; --review alone opens the latest snapshot (--snapshot ID picks one). review= on show()/Schematic. - Requires najaeda 0.7.34 (SNLDesign.getNetByID, SNLNet.getID). --- REUSE.toml | 3 + docs/review_format.md | 282 ++++++++++++++++ python/naja_schematic/__init__.py | 7 +- python/naja_schematic/protocol.py | 3 +- python/naja_schematic/review.py | 522 ++++++++++++++++++++++++++++++ python/naja_schematic/server.py | 72 ++++- python/naja_schematic/session.py | 53 ++- python/naja_schematic/widget.py | 13 +- python/tests/test_review.py | 270 ++++++++++++++++ 9 files changed, 1202 insertions(+), 23 deletions(-) create mode 100644 docs/review_format.md create mode 100644 python/naja_schematic/review.py create mode 100644 python/tests/test_review.py diff --git a/REUSE.toml b/REUSE.toml index b98ff6b..434939c 100644 --- a/REUSE.toml +++ b/REUSE.toml @@ -21,12 +21,14 @@ path = [ "CMakePresets.json", "README.md", "docs/demo.gif", + "docs/review_format.md", "docs/source_information.md", "python/README.md", "python/naja_schematic/__init__.py", "python/naja_schematic/__main__.py", "python/naja_schematic/_bundle.py", "python/naja_schematic/protocol.py", + "python/naja_schematic/review.py", "python/naja_schematic/server.py", "python/naja_schematic/session.py", "python/naja_schematic/static/index.html", @@ -42,6 +44,7 @@ path = [ "python/tests/test_design_generations.py", "python/tests/test_notebook.py", "python/tests/test_protocol.py", + "python/tests/test_review.py", "python/tests/test_source_references.py", "python/tests/test_transports.py", "python/tests/test_viewer_server.py", diff --git a/docs/review_format.md b/docs/review_format.md new file mode 100644 index 0000000..bc4c447 --- /dev/null +++ b/docs/review_format.md @@ -0,0 +1,282 @@ +# Netlist reviews in naja-schematic + +How people (and agents) leave findings on a netlist and pass them to each +other, asynchronously, through git. Status: threads are implemented in the +Python host (`python/naja_schematic/review.py`, najaeda >= 0.7.34 for net ids); the C++ viewer and native +provider, and saved views, are not yet. This document is the contract the two backends +(`src/LocalSNLProvider.cpp`, `python/naja_schematic/protocol.py`) and any +external writer (scripts, naja-agent) follow. + +## Goals + +- A finding outlives the window it was made in: anchored on a netlist object, + stored in a file, read back by anyone who opens the same design. +- Sharing is git: the review sits next to the design, is committed, diffed and + reviewed in pull requests like the RTL. No server, no database, nothing + leaves the network. +- **No merge conflicts by construction**: every write creates a new file, + no file is ever modified. Two people commenting on the same thread on two + branches merge cleanly. +- Humans and tools are equal authors: a kepler-formal diagnosis is a thread a + human can reply to, confirm or dismiss. +- **Objects are identified by naja ids only, never by names.** A thread is + pinned to the exact netlist it was written on (its snapshot); on another + netlist it isn't guessed at by name -- a name match can land a comment on + the wrong gate. Names are stored only as labels for people. + +## Concepts + +- **Thread**: a discussion anchored on one netlist object (or on a saved + view). Its history is an ordered list of **events**: comments, and changes + of status/severity/labels. +- **Saved view**: a named, replayable schematic (focus instance, traces, + equipotentials, hierarchy toggle, camera), stored as what builds it, never + as geometry, so it re-lays out on a newer netlist. +- **Anchor**: the object, by its naja ids (instance id path, term or net id, + bus bit), with its names alongside as labels. +- **Snapshot**: the netlist a thread was written on, stored in the review as + a naja-if dump and named by its content hash. A thread's ids mean + something only on its snapshot; anyone can open that exact netlist, with + no design flow (RTL, defines, Liberty, synthesis) at hand. + +## Directory layout + +A review is a directory, `.naja-review/` by convention, committed with the +design: + +``` +.naja-review/ + review.json {"format": 1} + .gitattributes snapshots/** binary + snapshots/ + / a naja-if dump, written once per netlist + threads/ + / + thread.json created once, never modified + -.json one per comment / status change + views/ + .json created once, never modified +``` + +- Thread, event and view ids are lowercase hex UUID4s (32 characters, no + dashes); snapshot ids are lowercase hex sha256s (64 characters). +- `` is UTC with milliseconds, `YYYYMMDDTHHMMSSmmmZ`, so a + directory listing is in chronological order. A writer keeps its own + timestamps strictly increasing; the event id breaks ties between writers. +- Dates inside files are ISO 8601 UTC with milliseconds, + `2026-10-31T14:25:30.123Z`. +- Files are UTF-8 JSON, 2-space indented, keys sorted, trailing newline, so a + writer's output is stable and diffs are minimal. +- Readers ignore unknown keys and unknown files (forward compatibility), and + skip a malformed file with a warning rather than failing the whole review. +- "Editing" a view means saving a new one; deleting a view or a thread is + `git rm`, outside the tool's file model (the viewer offers it, as a + file deletion, never as a rewrite). + +## Anchor + +```json +{ + "kind": "term", // "instance" | "term" | "net" | "view" + // identity, in the thread's snapshot: + "id_path": [3, 7], // instance ids (SNLInstance::getID()), root excluded, [] = top + "term_id": 2, // "term" only: SNLTerm id in that instance's model + "net_id": 5, // "net" only: SNLNet id in that instance's model + "bit": 3, // "term"/"net", optional: one bus bit, by its bit number + "view": "", // "view" only + // labels, for people only, never used to locate anything: + "path": ["u1", "u2"], // instance names, same levels ("" = anonymous) + "terminal": "Q", // "term": the term's name + "net": "data" // "net": the net's name ("" = anonymous) +} +``` + +For `instance` the id path is the instance itself; for `term`/`net` it is +the *containing* instance (`[]` = a top-level port or a top design net), +and the term/net id is looked up in its model (the top design for `[]`). Ids +reach what names can't: anonymous instances and anonymous nets (e.g. +naja's unnamed constant tie-offs). + +Writers store ids and labels together, read from the design. A writer may +*name* the object instead (`path`, `terminal`, `net`), e.g. an agent or a +log adapter that only knows hierarchical names: the host locates it once, +at write time, in the loaded design (same rule as `resolve_request_path()`: +never through an anonymous `""` level), and stores its ids. Readers never +use the labels to locate anything. + +## Snapshots + +The host identifies the loaded design by dumping the whole universe as +naja-if (`netlist.dump_naja_if()`, `SNLCapnP::dump()`) and hashing the dump: + +``` +sha256( join(sorted(relative_posix_path + " " + sha256_hex(file) for file in dump), "\n") ) +``` + +naja-if dumps are deterministic and keep instance ids, so the same netlist +gets the same id wherever and however it was loaded (Verilog, SystemVerilog, +VHDL, naja-if, or built in a notebook), and a reloaded snapshot answers the +same `id_path`s. The id covers everything naja built -- `--top`, defines, +Liberty cells, and the naja version's id assignment -- not the input files. + +- The dump is made once per loaded design (again after each root load or + `design_changed`), and stored as `snapshots//` the first time a thread + is opened on it, before the thread file naming it. It's staged under a + dot-name and renamed into place, so a snapshot directory is always + complete; two writers storing the same id write the same bytes. +- `naja-schematic --review DIR` with no design opens the latest snapshot a + thread was written on (`--snapshot ID` picks one). +- A snapshot is required: a thread's ids mean nothing without it. A thread + whose snapshot isn't in the review (deleted, or not pulled through Git + LFS) can still be read, but not shown on a schematic. +- Snapshots are binary and don't diff; a large netlist can be big. The + review writes `snapshots/** binary` in its `.gitattributes`; teams with + large designs should track them with Git LFS + (`git lfs track ".naja-review/snapshots/**"`). +- Source files are not in a snapshot (only naja's references to them): Show + Sources still needs the RTL checkout. + +## Resolving an anchor + +When a review is loaded against a design, each anchor resolves to one +`state`: + +1. **`exact`** — the thread's snapshot is the loaded design's, and its ids + resolve there (`resolve_request_path()` on `id_path`, then the term/net + id and bit). The thread is drawn on that object. +2. **`other_snapshot`** — the thread was written on another netlist. Ids + aren't comparable across netlists (a re-synthesized one can reuse an id + for an unrelated instance), and names aren't trusted: the thread is + listed, with its labels, and the viewer offers to open its snapshot + (`snapshot_stored` says whether the review has it). +3. **`unresolved`** — its own snapshot is loaded but the ids don't resolve + (a hand-edited or corrupt file). Listed, not drawn. + +A `view` anchor is `exact` while the view file exists, else `unresolved`. + +Carrying threads from one netlist to the next is deliberately left out: it +needs a real correspondence between the two (e.g. kepler-formal's +equivalence matching, or a naja diff mapping one snapshot's ids to the +other's), not a name heuristic. + +## `thread.json` + +```json +{ + "format": 1, + "id": "", + "created": "2026-10-31T14:25:30Z", + "author": "Jane Doe", // a person or a tool ("kepler-formal") + "source": "human", // "human" | free-form tool name + "snapshot": "", // the netlist the anchor's ids refer to + "anchor": { ... }, + "view": "", // optional: a view illustrating it + "title": "Reset not synchronized" +} +``` + +## Events: `-.json` + +```json +{ + "format": 1, + "id": "", + "date": "2026-10-31T14:25:30Z", + "author": "John Roe", + "text": "Markdown body", // optional: a comment + "status": "resolved", // optional: "open" | "resolved" | "wontfix" + "severity": "error", // optional: "info" | "warning" | "error" + "labels": ["cdc"] // optional: replaces the label set +} +``` + +The first event of a thread is written together with `thread.json` and +normally carries the opening comment, `status: "open"` and the severity. A +thread's current status/severity/labels is the value from the latest event +(by `date`, then `id`) that sets it; a thread with no status event is +`open`. Comments are shown in that same order. + +## Saved view: `views/.json` + +```json +{ + "format": 1, + "id": "", + "name": "reset cone", + "author": "Jane Doe", + "created": "2026-10-31T14:25:30Z", + "snapshot": "", + "focus": { "kind": "instance", ... }, // optional + "traces": [ + { "start": { "kind": "term", ..., "bit": 0 }, + "color": "#d62728", // optional, else palette + "visible": true } + ], + "equipotentials": [ { "start": { "kind": "term", ... } } ], + "show_hierarchy": true, + "camera": { "center": [120.0, 40.0], "zoom": 1.5 } // optional hint +} +``` + +Opening a view clears the schematic and replays it in order: focus first +(`resolve_instance`), then each trace (`trace_driver`) and equipotential +(`load_equipotential`). Their starting terminals are resolved by name to the +provider's term ids by the host. An unresolvable item is skipped and +reported; the rest still shows. The camera is applied only if the +snapshot matches (geometry may differ otherwise; the view fits instead). + +## Author identity + +Taken, in order, from: `--author`, `NAJA_SCHEMATIC_AUTHOR`, `git config +user.name` in the review directory's repository. With none, writing is +refused (the review stays readable). + +## Wire protocol + +The host owns the files, as with `load_source` (a WASM viewer can't write +disk). + +- push `review_response`: + ```json + { "response": "review_response", + "author": "Jane Doe", "writable": true, // false: no author, read-only + "snapshot": "", // the loaded design's, or null + "views": [ ... ], // the view files, as stored + "threads": [ { "id": "...", "title": "...", "author": "...", "source": "human", + "created": "...", "view": null, + "snapshot": "...", "snapshot_stored": true, // its netlist, in the review? + "anchor": { ... }, // as stored + "resolution": { "state": "exact", // | "other_snapshot" | "unresolved" + "id_path": [0, 0], "path": ["u1", "u2"] }, // exact only: names now + "status": "open", "severity": "error", "labels": [], + "comments": [ {"id": "...", "author": "...", "date": "...", "text": "..."} ] } ] } + ``` + Threads are folded (current status/severity/labels, comments in order). + Sent after each `root_response` when a review directory is configured, and + after every write, to every viewer. +- requests `review_add_thread {anchor, text, severity?, labels?, view?, title?}` + (the host locates the anchor in the loaded design -- by its ids, or by + names for a writer that has no ids -- and stores its ids with name labels; + it must exist) and + `review_add_event {thread, text?, status?, severity?, labels?}` -> the host + writes the file(s) and pushes the updated `review_response`. A refused + write is answered `{"response": "error", "request": , "message": ...}` + to that viewer only. (`review_save_view` comes with saved views.) +- All carry `generation` like every other message; a stale write is dropped + unanswered (`ViewerSession.answer()`). +- Without a review directory, or without an author, write requests fail and + the viewer hides its write actions. + +The viewer anchors by ids it already has: instance `id_path`s everywhere, +and a term's `child_id` (`terms_response`, `expand_instance_terms`). Nets +carry no id on the wire yet: `nets_response` entries need a `net_id` +(both backends) before the tree can open a thread on a net. + +Shared cases for both backends: `tests/data/review_protocol_cases.json`. + +## Diagnoses + +`diagnosis_response` items are shown as unsaved threads (author = the +item's `source`). "Keep" writes one to the review as a real thread, after +which humans can reply to it. A tool can also write threads directly into +the directory, with no viewer running. diff --git a/python/naja_schematic/__init__.py b/python/naja_schematic/__init__.py index 9501b10..76b2557 100644 --- a/python/naja_schematic/__init__.py +++ b/python/naja_schematic/__init__.py @@ -12,15 +12,16 @@ __version__ = "0.2.0" from .protocol import diagnosis_response, handle_request +from .review import Review from .server import ViewerServer -def show(instance=None, *, height=600, diagnosis=None): +def show(instance=None, *, height=600, diagnosis=None, review=None): """Display a view of the loaded design in the current notebook; see naja_schematic.widget.show().""" # Imported lazily: the CLI/server path doesn't need anywidget/IPython. from .widget import show as _show - return _show(instance, height=height, diagnosis=diagnosis) + return _show(instance, height=height, diagnosis=diagnosis, review=review) -__all__ = ["__version__", "show", "handle_request", "diagnosis_response", "ViewerServer"] +__all__ = ["__version__", "show", "handle_request", "diagnosis_response", "ViewerServer", "Review"] diff --git a/python/naja_schematic/protocol.py b/python/naja_schematic/protocol.py index bfc937e..6d2bdb1 100644 --- a/python/naja_schematic/protocol.py +++ b/python/naja_schematic/protocol.py @@ -20,7 +20,8 @@ # 0.7.31: load_gate_level_verilog/GateLevelVerilogConfig (load_verilog and # VerilogConfig now warn). # 0.7.34: word-level operator checks (isAddSub, isMultiply, isShift, isDivMod, -# ...) and loadSystemVerilog's frontend_options. +# ...) and loadSystemVerilog's frontend_options; nets by id +# (SNLDesign.getNetByID, SNLNet.getID), for review anchors. MIN_NAJAEDA_VERSION = (0, 7, 34) # keep in step with pyproject.toml diff --git a/python/naja_schematic/review.py b/python/naja_schematic/review.py new file mode 100644 index 0000000..f445a2b --- /dev/null +++ b/python/naja_schematic/review.py @@ -0,0 +1,522 @@ +"""Netlist reviews: threads and saved views kept in a directory in git. + +The format is specified in docs/review_format.md. Every write creates a new +file and no file is ever modified, so reviews made on two branches merge +without conflicts. This module reads and writes that directory and resolves +anchors against the loaded design; ViewerSession wires it to the viewer. +""" + +import hashlib +import json +import logging +import os +import shutil +import subprocess +import tempfile +import threading +import uuid +from datetime import datetime, timedelta, timezone +from pathlib import Path + +from najaeda import naja, netlist + +from . import protocol + +log = logging.getLogger(__name__) + +FORMAT = 1 +STATUSES = ("open", "resolved", "wontfix") +SEVERITIES = ("info", "warning", "error") +ANCHOR_KINDS = ("instance", "term", "net", "view") + + +class ReviewError(Exception): + """A write the review refuses (bad request, no author, ...).""" + + +def new_id(): + return uuid.uuid4().hex + + +def is_review_id(value): + # Ids name files: only what new_id() makes, never a path. + return (isinstance(value, str) and len(value) == 32 + and all(c in "0123456789abcdef" for c in value)) + + +def now(): + when = datetime.now(timezone.utc) + return when.replace(microsecond=when.microsecond // 1000 * 1000) + + +def iso_date(when): + return when.strftime("%Y-%m-%dT%H:%M:%S.") + f"{when.microsecond // 1000:03d}Z" + + +def file_stamp(when): + return when.strftime("%Y%m%dT%H%M%S") + f"{when.microsecond // 1000:03d}Z" + + +def dump(path, document): + """Write `document` as the format wants it (sorted keys, 2-space indent, + trailing newline), refusing to overwrite: review files are never + modified.""" + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "x", encoding="utf-8") as f: + json.dump(document, f, indent=2, sort_keys=True, ensure_ascii=False) + f.write("\n") + + +def _load(path): + try: + with open(path, encoding="utf-8") as f: + document = json.load(f) + except (OSError, ValueError) as e: + log.warning("Skipping unreadable review file %s: %s", path, e) + return None + if not isinstance(document, dict): + log.warning("Skipping review file %s: not an object", path) + return None + return document + + +def file_sha256(path): + digest = hashlib.sha256() + with open(path, "rb") as f: + for block in iter(lambda: f.read(1 << 20), b""): + digest.update(block) + return digest.hexdigest() + + +def snapshot_hash(directory): + """The id of a naja-if dump (see "Snapshots" in docs/review_format.md): + a sha256 over its files' relative paths and contents. naja-if dumps are + deterministic, so the same netlist always gets the same id.""" + directory = Path(directory) + lines = [f"{p.relative_to(directory).as_posix()} {file_sha256(p)}" + for p in sorted(directory.rglob("*")) if p.is_file()] + return hashlib.sha256("\n".join(lines).encode("utf-8")).hexdigest() + + +def is_snapshot_id(value): + return (isinstance(value, str) and len(value) == 64 + and all(c in "0123456789abcdef" for c in value)) + + +def default_author(directory, explicit=None): + """`explicit`, else $NAJA_SCHEMATIC_AUTHOR, else `git config user.name` + in `directory`'s repository; None when there's none.""" + if explicit: + return explicit + env = os.environ.get("NAJA_SCHEMATIC_AUTHOR") + if env: + return env + cwd = Path(directory) + while not cwd.is_dir() and cwd != cwd.parent: + cwd = cwd.parent # the review directory may not exist yet + try: + out = subprocess.run(["git", "config", "user.name"], cwd=cwd, capture_output=True, + text=True, timeout=5) + except (OSError, subprocess.SubprocessError): + return None + name = out.stdout.strip() + return name or None + + +def _is_id(value): + return isinstance(value, int) and not isinstance(value, bool) and value >= 0 + + +def _check_ids(anchor, key): + value = anchor.get(key) + if value is not None and not (isinstance(value, list) and all(_is_id(i) for i in value)): + raise ReviewError(f"anchor: {key} must be a list of instance ids") + return value + + +def check_anchor(anchor): + """The anchor with only the keys its kind uses; raises ReviewError if + it's malformed. Ids and names are both accepted here: a writer may name + the object either way (see locate()).""" + if not isinstance(anchor, dict) or anchor.get("kind") not in ANCHOR_KINDS: + raise ReviewError("anchor: kind must be one of " + ", ".join(ANCHOR_KINDS)) + kind = anchor["kind"] + if kind == "view": + if not is_review_id(anchor.get("view")): + raise ReviewError("anchor: a view anchor needs a view id") + return {"kind": "view", "view": anchor["view"]} + clean = {"kind": kind} + if _check_ids(anchor, "id_path") is not None: + clean["id_path"] = list(anchor["id_path"]) + path = anchor.get("path") + if path is not None: + if not (isinstance(path, list) and all(isinstance(n, str) for n in path)): + raise ReviewError("anchor: path must be a list of instance names") + clean["path"] = list(path) + if "id_path" not in clean and "path" not in clean: + raise ReviewError("anchor: id_path or path is required") + if kind in ("term", "net"): + id_key, name_key = ("term_id", "terminal") if kind == "term" else ("net_id", "net") + if anchor.get(id_key) is not None: + if not _is_id(anchor[id_key]): + raise ReviewError(f"anchor: {id_key} must be an id") + clean[id_key] = anchor[id_key] + if isinstance(anchor.get(name_key), str): + clean[name_key] = anchor[name_key] + if id_key not in clean and not clean.get(name_key): + raise ReviewError(f"anchor: a {kind} anchor needs a {id_key} (or a {name_key} name)") + bit = anchor.get("bit") + if bit is not None: + if not isinstance(bit, int) or isinstance(bit, bool): + raise ReviewError("anchor: bit must be an integer") + clean["bit"] = bit + return clean + + +def locate(top, anchor, by_name=False): + """The object `anchor` names in `top`, as a stored anchor: its ids + (`id_path`, `term_id`/`net_id`, `bit`), which identify it, plus its + names (`path`, `terminal`/`net`), which only label it. None if it isn't + there. Stored anchors are only ever located by their ids; `by_name` + lets a writer that knows names (an agent, a log adapter) name the object + once, at write time, against the loaded design -- by the usual request + rule, never through an anonymous "" level.""" + kind = anchor["kind"] + lookup = anchor if by_name else {"id_path": anchor.get("id_path")} + if not by_name and not isinstance(lookup["id_path"], list): + return None + design, chain = protocol.resolve_request_path(top, lookup) + if design is None: + return None + found = {"kind": kind, "id_path": [i.getID() for i in chain], + "path": [i.getName() for i in chain]} + bit = anchor.get("bit") + if kind == "term": + if "term_id" in anchor: + term = design.getTermByID(anchor["term_id"]) + else: + term = design.getTerm(anchor["terminal"]) if by_name else None + if term is None: + return None + if bit is not None and (not isinstance(term, naja.SNLBusTerm) + or term.getBusTermBit(bit) is None): + return None + found.update(term_id=term.getID(), terminal=term.getName()) + elif kind == "net": + if "net_id" in anchor: + net = design.getNetByID(anchor["net_id"]) + else: + net = design.getNet(anchor["net"]) if by_name and anchor.get("net") else None + if net is None: + return None + if bit is not None and (not isinstance(net, naja.SNLBusNet) or net.getBit(bit) is None): + return None + found.update(net_id=net.getID(), net=net.getName()) # "" when anonymous + if bit is not None: + found["bit"] = bit + return found + + +def resolve_anchor(top, anchor, thread_snapshot, design_snapshot, views=()): + """Where a stored anchor lands in the loaded design (see "Resolving an + anchor" in docs/review_format.md): {"state": "exact", "id_path", "path"} + (the object's names now) when the thread was written on this very + netlist and its ids resolve; {"state": "other_snapshot"} when it was + written on another one (never guessed across netlists: open that + snapshot to see it); else {"state": "unresolved"}.""" + if anchor["kind"] == "view": + return {"state": "exact" if anchor["view"] in views else "unresolved"} + if thread_snapshot is None or thread_snapshot != design_snapshot: + return {"state": "other_snapshot"} + found = locate(top, anchor) if top is not None else None + if found is None: + return {"state": "unresolved"} + return {"state": "exact", "id_path": found["id_path"], "path": found["path"]} + + +class Thread: + def __init__(self, document, events): + self.document = document + self.events = sorted(events, key=lambda e: (str(e.get("date", "")), str(e.get("id", "")))) + + @property + def id(self): + return self.document["id"] + + def latest(self, key, default): + value = default + for event in self.events: + if key in event: + value = event[key] + return value + + def to_json(self, resolution, stored): + return { + "id": self.id, + "title": self.document.get("title", ""), + "author": self.document.get("author", ""), + "source": self.document.get("source", "human"), + "created": self.document.get("created", ""), + "view": self.document.get("view"), + "snapshot": self.document.get("snapshot"), + "snapshot_stored": stored, + "anchor": self.document["anchor"], + "resolution": resolution, + "status": self.latest("status", "open"), + "severity": self.latest("severity", "info"), + "labels": self.latest("labels", []), + "comments": [ + {"id": e.get("id", ""), "author": e.get("author", ""), "date": e.get("date", ""), + "text": e["text"]} + for e in self.events if isinstance(e.get("text"), str) + ], + } + + +class Review: + """One review directory. `author` writes new threads and events (None: + read-only). The loaded design is dumped as naja-if to identify it, and + stored in the review the first time a thread is opened on it, so the + review carries every netlist its threads' ids refer to. + Reads always come from disk, so files pulled from git while the viewer + runs show at the next load. + + The universe must not change between two calls without + design_changed() (ViewerSession calls it on every root load): the dump + is reused until then.""" + + def __init__(self, directory, author=None): + self.directory = Path(directory) + self.author = author + self._lock = threading.Lock() + self._snapshot = None # (id, TemporaryDirectory holding its dump) + self._last_write = None + + def _now(self): + # Strictly increasing per writer, so its own events keep their order + # even within one millisecond (ties across writers fall back to ids). + with self._lock: + when = now() + if self._last_write is not None and when <= self._last_write: + when = self._last_write + timedelta(milliseconds=1) + self._last_write = when + return when + + def design_changed(self): + """Forget the loaded design's snapshot: the next call dumps it again.""" + with self._lock: + self._drop_snapshot() + + def _drop_snapshot(self): + if self._snapshot is not None: + self._snapshot[1].cleanup() + self._snapshot = None + + def design_snapshot(self, top): + """The loaded design's snapshot id (None: no design). Dumps the + universe once per design (see the class docstring); call with + exclusive access to it.""" + if top is None: + return None + with self._lock: + if self._snapshot is None: + scratch = tempfile.TemporaryDirectory(prefix="naja-review-") + dump_dir = Path(scratch.name) / "snapshot" + netlist.dump_naja_if(str(dump_dir)) + self._snapshot = (snapshot_hash(dump_dir), scratch) + return self._snapshot[0] + + def snapshot_path(self, snapshot): + return self.directory / "snapshots" / snapshot + + def _store_snapshot(self, snapshot): + target = self.snapshot_path(snapshot) + with self._lock: + if target.is_dir() or self._snapshot is None or self._snapshot[0] != snapshot: + return + dump_dir = Path(self._snapshot[1].name) / "snapshot" + staging = target.with_name(f".{snapshot}-{new_id()}") + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(dump_dir, staging) + try: + staging.rename(target) # atomic: never a half-written snapshot + except OSError: + shutil.rmtree(staging, ignore_errors=True) # another writer stored it + if not target.is_dir(): + raise + + def _is_stored(self, snapshot): + return is_snapshot_id(snapshot) and self.snapshot_path(snapshot).is_dir() + + def latest_snapshot(self): + """The stored snapshot of the most recent thread that has one, to + open a review with no design given; None if there's none.""" + for thread in reversed(self.threads()): + snapshot = thread.document.get("snapshot") + if self._is_stored(snapshot): + return snapshot + return None + + # -- reading ------------------------------------------------------------- + + def threads(self): + threads = [] + root = self.directory / "threads" + if not root.is_dir(): + return threads + for folder in sorted(root.iterdir()): + if not folder.is_dir(): + continue + document = _load(folder / "thread.json") + if document is None: + continue + if document.get("id") != folder.name: + log.warning("Skipping review thread %s: its id doesn't match", folder) + continue + try: + document["anchor"] = check_anchor(document.get("anchor")) + except ReviewError as e: + log.warning("Skipping review thread %s: %s", folder, e) + continue + events = [e for e in (_load(p) for p in sorted(folder.glob("*.json")) + if p.name != "thread.json") if e is not None] + threads.append(Thread(document, events)) + threads.sort(key=lambda t: (str(t.document.get("created", "")), t.id)) + return threads + + def views(self): + views = [] + root = self.directory / "views" + if root.is_dir(): + for path in sorted(root.glob("*.json")): + document = _load(path) + if document is not None and document.get("id") == path.stem: + views.append(document) + views.sort(key=lambda v: (str(v.get("created", "")), v["id"])) + return views + + def response(self, top): + """The review_response message for the loaded design `top`.""" + design_snapshot = self.design_snapshot(top) + views = self.views() + view_ids = {v["id"] for v in views} + threads = [ + t.to_json(resolve_anchor(top, t.document["anchor"], t.document.get("snapshot"), + design_snapshot, view_ids), + self._is_stored(t.document.get("snapshot"))) + for t in self.threads() + ] + return { + "response": "review_response", + "author": self.author or "", + "writable": bool(self.author), + "snapshot": design_snapshot, + "threads": threads, + "views": views, + } + + # -- writing ------------------------------------------------------------- + + def _require_author(self): + if not self.author: + raise ReviewError("no author: pass --author or set NAJA_SCHEMATIC_AUTHOR") + return self.author + + def _ensure_root(self): + meta = self.directory / "review.json" + if not meta.exists(): + try: + dump(meta, {"format": FORMAT}) + except FileExistsError: + pass # another writer made it first + # Snapshots are binary: no text diffs or merges (see + # docs/review_format.md for keeping them in Git LFS). + attributes = self.directory / ".gitattributes" + if not attributes.exists(): + try: + with open(attributes, "x", encoding="utf-8") as f: + f.write("snapshots/** binary\n") + except FileExistsError: + pass + + @staticmethod + def _event(author, when, text=None, status=None, severity=None, labels=None): + event = {"format": FORMAT, "id": new_id(), "date": iso_date(when), "author": author} + if text is not None: + if not isinstance(text, str): + raise ReviewError("text must be a string") + event["text"] = text + if status is not None: + if status not in STATUSES: + raise ReviewError("status must be one of " + ", ".join(STATUSES)) + event["status"] = status + if severity is not None: + if severity not in SEVERITIES: + raise ReviewError("severity must be one of " + ", ".join(SEVERITIES)) + event["severity"] = severity + if labels is not None: + if not (isinstance(labels, list) and all(isinstance(x, str) for x in labels)): + raise ReviewError("labels must be a list of strings") + event["labels"] = list(labels) + return event + + def _write_event(self, thread_id, event, when): + dump(self.directory / "threads" / thread_id / f"{file_stamp(when)}-{event['id']}.json", + event) + + def add_thread(self, top, anchor, text, severity="info", labels=None, title="", view=None, + author=None, source="human"): + """Open a thread on `anchor`, an object of the loaded design `top` + named by ids or, failing those, names (see locate()); it's stored by + its ids, with its names as labels. `author`/`source` override the + review's author, for a tool writing its findings. Returns the + thread id.""" + author = author or self._require_author() + anchor = check_anchor(anchor) + if anchor["kind"] != "view": + by_name = "id_path" not in anchor or (anchor["kind"] == "term" + and "term_id" not in anchor) or ( + anchor["kind"] == "net" and "net_id" not in anchor) + found = locate(top, anchor, by_name=by_name) if top is not None else None + if found is None: + raise ReviewError("anchor: no such object in the loaded design") + anchor = found + snapshot = self.design_snapshot(top) if anchor["kind"] != "view" else None + when = self._now() + thread_id = new_id() + document = { + "format": FORMAT, + "id": thread_id, + "created": iso_date(when), + "author": author, + "source": source, + "snapshot": snapshot, + "anchor": anchor, + "title": title if isinstance(title, str) else "", + } + if view is not None: + if not is_review_id(view): + raise ReviewError("view: not a view id") + document["view"] = view + event = self._event(author, when, text=text, status="open", severity=severity, + labels=labels) + self._ensure_root() + if snapshot is not None: + self._store_snapshot(snapshot) # before the thread that names it + dump(self.directory / "threads" / thread_id / "thread.json", document) + self._write_event(thread_id, event, when) + return thread_id + + def add_event(self, thread_id, text=None, status=None, severity=None, labels=None, + author=None): + """Reply to a thread and/or change its status, severity or labels.""" + author = author or self._require_author() + if not is_review_id(thread_id) or not (self.directory / "threads" / thread_id + / "thread.json").is_file(): + raise ReviewError(f"no thread {thread_id!r}") + if text is None and status is None and severity is None and labels is None: + raise ReviewError("nothing to add: give a text, status, severity or labels") + when = self._now() + event = self._event(author, when, text, status, severity, labels) + self._write_event(thread_id, event, when) + return event["id"] diff --git a/python/naja_schematic/server.py b/python/naja_schematic/server.py index 6249be5..2e7d9e6 100644 --- a/python/naja_schematic/server.py +++ b/python/naja_schematic/server.py @@ -32,6 +32,7 @@ from . import protocol from ._bundle import static_file +from .review import Review, default_author from .session import ViewerSession, lock_runner log = logging.getLogger("naja_schematic") @@ -75,13 +76,13 @@ def _static_response(connection, request): async def serve(host="localhost", port=DEFAULT_PORT, diagnosis=None, open_browser=False, - source_roots=(), source_maps=()): + source_roots=(), source_maps=(), review=None): """Serve the viewer page and its WebSocket on host:port, forever.""" from websockets.asyncio.server import serve as ws_serve from websockets.exceptions import ConnectionClosed session = ViewerSession(diagnosis=diagnosis, source_roots=source_roots, - source_maps=source_maps) + source_maps=source_maps, review=review) async def handle_connection(websocket): log.info("Client connected") @@ -154,12 +155,13 @@ class ViewerServer: The viewer can only read source files the design's source references name (Show Sources); relative ones are looked up in `source_roots`, then the current directory, and `source_maps` rewrites (FROM, TO) path - prefixes for files recorded on another machine. + prefixes for files recorded on another machine. `review`, a + review.Review, is shown to and written by every viewer. """ def __init__(self, host="127.0.0.1", port=0, *, lock=None, run=None, token=True, allowed_origins=(), diagnosis=None, source_roots=(), - source_maps=()): + source_maps=(), review=None): if lock is not None and run is not None: raise ValueError("pass lock or run, not both") if run is None: @@ -177,7 +179,8 @@ def __init__(self, host="127.0.0.1", port=0, *, lock=None, run=None, thread_name_prefix="naja-schematic-callbacks") self._session = ViewerSession(push=self._broadcast, run=run, diagnosis=diagnosis, dispatch=self._dispatch_callback, - source_roots=source_roots, source_maps=source_maps) + source_roots=source_roots, source_maps=source_maps, + review=review) self._thread = None self._loop = None self._stopping = None @@ -453,13 +456,14 @@ def _send_all(self, message): # stdio server # --------------------------------------------------------------------------- -def serve_stdio(diagnosis=None, stdin=None, stdout=None, source_roots=(), source_maps=()): +def serve_stdio(diagnosis=None, stdin=None, stdout=None, source_roots=(), source_maps=(), + review=None): """Answer one JSON request per stdin line with JSON lines on stdout, until stdin closes. Logging must not go to stdout in this mode.""" stdin = stdin or sys.stdin stdout = stdout or sys.stdout session = ViewerSession(diagnosis=diagnosis, source_roots=source_roots, - source_maps=source_maps) + source_maps=source_maps, review=review) for line in stdin: line = line.strip() if not line: @@ -507,6 +511,16 @@ def build_arg_parser(): help="Rewrite source paths starting with FROM to start with TO " "(repeatable): for files recorded on another machine, e.g. " "Yosys src paths of a synthesis server") + parser.add_argument("--review", metavar="DIR", + help="Review directory (threads and saved views, see " + "docs/review_format.md) to show and write, e.g. .naja-review") + parser.add_argument("--author", metavar="NAME", + help="Author of the review comments written (default: " + "$NAJA_SCHEMATIC_AUTHOR, else git config user.name)") + parser.add_argument("--snapshot", metavar="ID", + help="With --review and no design: open this snapshot of the " + "review (default: the latest one a thread was written on)") + parser.add_argument("--verbose", "-v", action="store_true", help="Log every request (to stderr)") parser.add_argument("--xilinx", action="store_true", @@ -594,6 +608,24 @@ def load_design(args): return top +def load_review_snapshot(review, snapshot=None): + """Load one of the review's stored designs (by default the latest one a + thread was written on) into the NLUniverse; returns the top.""" + from najaeda import netlist + + from .review import is_snapshot_id + + if snapshot is None: + snapshot = review.latest_snapshot() + if snapshot is None: + raise SystemExit(f"{review.directory}: no stored snapshot to open; " + "give the design (--verilog, --systemverilog, ...)") + elif not is_snapshot_id(snapshot) or not review.snapshot_path(snapshot).is_dir(): + raise SystemExit(f"{review.directory}: no snapshot {snapshot!r}") + log.info("Loading review snapshot %s", snapshot) + return netlist.load_naja_if(str(review.snapshot_path(snapshot))) + + def load_diagnosis_file(path): with open(path) as f: return json.load(f) @@ -604,9 +636,13 @@ def main(argv=None): args = parser.parse_args(argv) is_sv = bool(args.systemverilog or args.flist) - if sum(map(bool, (args.verilog, is_sv, args.vhdl))) != 1: + designs = sum(map(bool, (args.verilog, is_sv, args.vhdl))) + from_review = not designs and args.review is not None + if designs > 1 or not (designs or from_review): parser.error("provide one design: --verilog, --systemverilog and/or --flist, " - "or --vhdl") + "or --vhdl (or --review alone, to open its snapshot)") + if args.snapshot and not from_review: + parser.error("--snapshot only applies with --review and no design") if args.top and not (is_sv or args.vhdl): parser.error("--top only applies to --systemverilog/--flist/--vhdl") if args.define and not is_sv: @@ -620,6 +656,8 @@ def main(argv=None): parser.error("--yosys-src only applies to --verilog") if args.stdio and args.open: parser.error("--open does not apply to --stdio") + if args.author and not args.review: + parser.error("--author only applies with --review") # stderr only: stdout is the protocol channel in --stdio mode. logging.basicConfig(stream=sys.stderr, format="%(message)s", @@ -642,15 +680,25 @@ def main(argv=None): os.dup2(2, 1) diagnosis = load_diagnosis_file(args.diagnosis) if args.diagnosis else None - load_design(args) + review = None + if args.review: + author = default_author(args.review, args.author) + if author is None: + log.warning("No review author (--author, $NAJA_SCHEMATIC_AUTHOR or git " + "user.name): the review is read-only") + review = Review(args.review, author) + if from_review: + load_review_snapshot(review, args.snapshot) + else: + load_design(args) if args.stdio: serve_stdio(diagnosis, stdout=protocol_out, source_roots=args.source_roots, - source_maps=args.source_maps) + source_maps=args.source_maps, review=review) return try: asyncio.run(serve(args.host, args.port, diagnosis, args.open, args.source_roots, - args.source_maps)) + args.source_maps, review)) except KeyboardInterrupt: pass diff --git a/python/naja_schematic/session.py b/python/naja_schematic/session.py index b265735..a96fc21 100644 --- a/python/naja_schematic/session.py +++ b/python/naja_schematic/session.py @@ -30,6 +30,7 @@ from najaeda import naja from . import protocol +from .review import ReviewError log = logging.getLogger("naja_schematic") @@ -135,6 +136,9 @@ def run(fn): return run +_REVIEW_WRITES = ("review_add_thread", "review_add_event") + + class ViewerSession: """The Python side of one viewer (or of several showing the same view). @@ -147,6 +151,8 @@ class ViewerSession: `source_roots` lists the directories relative source file references are looked up in, `source_maps` (FROM, TO) path prefixes to rewrite for files recorded on another machine (see protocol.resolve_source_file()). + `review`, a review.Review, is sent to the viewer after every root load + and written by its review_* requests (see docs/review_format.md). Design generations: the session numbers the designs it has shown, starting at 0; design_changed() moves to the next one. Every message to @@ -162,8 +168,9 @@ class ViewerSession: """ def __init__(self, push=None, run=None, diagnosis=None, dispatch=None, source_roots=(), - source_maps=()): + source_maps=(), review=None): self._push = push + self.review = review # Stamped now and at each design_changed(): the design the viewer # shows was loaded by then, so a source file modified later is stale. self._source_paths = protocol.SourcePaths( @@ -228,7 +235,16 @@ def handle(): if kind == "svg_exported": svg = request.get("svg") return generation, svg if isinstance(svg, str) else "", pushes - return generation, protocol.handle_request(request, self._source_paths), pushes + if kind in _REVIEW_WRITES: + return generation, self._review_write(kind, request), pushes + replies = protocol.handle_request(request, self._source_paths) + if self.review is not None and any(r.get("response") == "root_response" + for r in replies): + # Under `run`: anchors resolve against the universe. A root + # load is where an edited design shows: identify it afresh. + self.review.design_changed() + pushes = pushes + [self._review_response()] + return generation, replies, pushes generation, result, pushes = self._run(handle) if result is None: @@ -240,6 +256,11 @@ def handle(): if kind == "svg_exported": self._commit_svg(generation, result) return [] + if kind in _REVIEW_WRITES and self._push is not None and result[0].get( + "response") == "review_response": + # Every viewer shows the review, not just the one that wrote. + self._send(self._stamp(result[0], generation)) + return [] replies = [] for reply in result: replies.append(self._stamp(reply, generation)) @@ -249,6 +270,32 @@ def handle(): replies.extend(self._stamp(p, generation) for p in pushes) return replies + def _review_response(self): + return self.review.response(naja.NLUniverse.get().getTopDesign() + if naja.NLUniverse.get() else None) + + def _review_write(self, kind, request): + # Under `run`. A refused write is an error reply to that viewer only. + if self.review is None: + return [{"response": "error", "request": kind, + "message": "no review directory: start with --review DIR"}] + try: + if kind == "review_add_thread": + u = naja.NLUniverse.get() + self.review.add_thread( + u.getTopDesign() if u else None, request.get("anchor"), request.get("text"), + severity=request.get("severity", "info"), labels=request.get("labels"), + title=request.get("title", ""), view=request.get("view")) + else: + self.review.add_event(request.get("thread"), text=request.get("text"), + status=request.get("status"), + severity=request.get("severity"), + labels=request.get("labels")) + except (ReviewError, OSError) as e: + log.warning("%s refused: %s", kind, e) + return [{"response": "error", "request": kind, "message": str(e)}] + return [self._review_response()] + @staticmethod def _resolve_selection(request): path, ids = request.get("path"), request.get("id_path") @@ -364,6 +411,8 @@ def design_changed(self, diagnosis=None, instance=None): if instance is not None: names, ids = instance_paths(instance) focus_push = protocol.focus_instance(names, ids) + if self.review is not None: + self.review.design_changed() with self._state_lock: self._generation += 1 generation = self._generation diff --git a/python/naja_schematic/widget.py b/python/naja_schematic/widget.py index 97740ea..ac815e9 100644 --- a/python/naja_schematic/widget.py +++ b/python/naja_schematic/widget.py @@ -46,7 +46,7 @@ class Schematic(anywidget.AnyWidget): # Kernel-side only, like the selection. svg = traitlets.Unicode(default_value=None, allow_none=True) - def __init__(self, source_roots=(), source_maps=(), **kwargs): + def __init__(self, source_roots=(), source_maps=(), review=None, **kwargs): # Read before AnyWidget.__init__, which turns it into a synced trait. # Not a class attribute: that would read the bundle at import time # and fail the import when no bundle is installed. @@ -55,7 +55,7 @@ def __init__(self, source_roots=(), source_maps=(), **kwargs): # Requests are answered on the kernel thread, like the cells that # edit the design, so the session's default runner is enough. self._session = ViewerSession(push=self._send_json, source_roots=source_roots, - source_maps=source_maps) + source_maps=source_maps, review=review) self._session.on_select(self._on_viewer_selection) self.on_msg(self._on_viewer_message) @@ -149,7 +149,8 @@ def handler(_change): return handler -def show(instance=None, *, height=600, diagnosis=None, source_roots=(), source_maps=()): +def show(instance=None, *, height=600, diagnosis=None, source_roots=(), source_maps=(), + review=None): """Display a view of the design currently loaded with najaeda. Evaluate it as the last expression of a cell (or pass it to @@ -160,10 +161,12 @@ def show(instance=None, *, height=600, diagnosis=None, source_roots=(), source_m `source_roots` lists the directories relative source file references (Show Sources) are looked up in, before the current directory; `source_maps`, (FROM, TO) path prefixes to rewrite for files recorded on - another machine, e.g. [("/home/ci/flow", "/work/flow")]. + another machine, e.g. [("/home/ci/flow", "/work/flow")]. `review`, a + naja_schematic.Review, shows its threads and lets the viewer add to it. """ # (anywidget itself turns on Colab's custom widget manager.) - view = Schematic(height=height, source_roots=source_roots, source_maps=source_maps) + view = Schematic(height=height, source_roots=source_roots, source_maps=source_maps, + review=review) if diagnosis is not None: view.annotate(diagnosis) if instance is not None: diff --git a/python/tests/test_review.py b/python/tests/test_review.py new file mode 100644 index 0000000..afe580b --- /dev/null +++ b/python/tests/test_review.py @@ -0,0 +1,270 @@ +"""Netlist reviews (docs/review_format.md): the directory format, anchor +resolution and the viewer's review_* requests.""" +import json + +import pytest +from najaeda import naja + +from naja_schematic.review import Review, ReviewError, resolve_anchor, snapshot_hash +from naja_schematic.session import ViewerSession + +# What a viewer sends: ids, plus names it happens to know. +U_AND = {"kind": "instance", "id_path": [0, 0], "path": ["u_sub", "u_and"]} +# What an agent knowing only names sends: located once, at write time. +PIN_A = {"kind": "term", "path": ["u_sub"], "terminal": "a"} +TOP_D1 = {"kind": "term", "path": [], "terminal": "d", "bit": 1} + + +def design(): + return naja.NLUniverse.get().getTopDesign() + + +def sub(): + return design().getInstanceByID(0).getModel() + + +def anonymous_nets(owner): + return [n for n in owner.getNets() if not n.getName()] + + +@pytest.fixture +def review(top, tmp_path): + return Review(tmp_path / ".naja-review", "Jane") + + +def files(directory): + return sorted(str(p.relative_to(directory)) for p in directory.rglob("*") if p.is_file()) + + +def test_thread_files_and_folding(review): + tid = review.add_thread(design(), U_AND, "LUT looks wrong", severity="error", + labels=["lut"], title="u_and") + review.add_event(tid, text="Agreed", author="John") + review.add_event(tid, status="resolved") + + names = files(review.directory) + assert "review.json" in names + assert ".gitattributes" in names + assert f"threads/{tid}/thread.json" in names + assert len([n for n in names if n.startswith(f"threads/{tid}/")]) == 4 + assert any(n.startswith("snapshots/") and n.endswith("snl.mf") for n in names) + + [thread] = review.response(design())["threads"] + assert thread["status"] == "resolved" + assert thread["severity"] == "error" + assert thread["labels"] == ["lut"] + assert [(c["author"], c["text"]) for c in thread["comments"]] == [ + ("Jane", "LUT looks wrong"), ("John", "Agreed")] + assert thread["snapshot_stored"] is True + assert thread["resolution"] == {"state": "exact", "id_path": [0, 0], + "path": ["u_sub", "u_and"]} + + +def test_files_are_never_rewritten(review): + tid = review.add_thread(design(), PIN_A, "first") + before = {n: (review.directory / n).read_bytes() for n in files(review.directory)} + review.add_event(tid, text="second") + after = {n: (review.directory / n).read_bytes() for n in files(review.directory)} + assert {n: after[n] for n in before} == before + assert len(after) == len(before) + 1 + + +def test_two_writers_never_share_a_file(top, tmp_path): + # Two clones writing on two branches: their files never collide, so the + # merge is a union of the two directories. + a = Review(tmp_path / "a", "A") + b = Review(tmp_path / "b", "B") + a.add_thread(design(), U_AND, "from A") + b.add_thread(design(), U_AND, "from B") + shared = set(files(a.directory)) & set(files(b.directory)) + # Only the review's metadata, and the snapshot of the design both wrote + # on: same netlist, same content-addressed files, byte for byte. + assert {n for n in shared if not n.startswith("snapshots/")} == {"review.json", + ".gitattributes"} + assert all((a.directory / n).read_bytes() == (b.directory / n).read_bytes() + for n in shared) + + +def test_stored_anchor_is_ids_with_name_labels(review): + tid = review.add_thread(design(), PIN_A, "x") + stored = json.loads((review.directory / "threads" / tid / "thread.json").read_text()) + assert stored["anchor"] == {"kind": "term", "id_path": [0], "path": ["u_sub"], + "term_id": sub().getTerm("a").getID(), "terminal": "a"} + snapshot = stored["snapshot"] + assert snapshot == snapshot_hash(review.snapshot_path(snapshot)) + assert review.latest_snapshot() == snapshot + + +def test_stored_names_are_only_labels(review): + # A stored anchor is located by its ids: wrong labels don't matter... + tid = review.add_thread(design(), PIN_A, "x") + path = review.directory / "threads" / tid / "thread.json" + document = json.loads(path.read_text()) + document["anchor"].update(path=["renamed"], terminal="renamed") + path.write_text(json.dumps(document)) + [thread] = review.response(design())["threads"] + assert thread["resolution"] == {"state": "exact", "id_path": [0], "path": ["u_sub"]} + # ...and names alone never locate a stored anchor. + del document["anchor"]["term_id"] + document["anchor"]["terminal"] = "a" + path.write_text(json.dumps(document)) + [thread] = review.response(design())["threads"] + assert thread["resolution"] == {"state": "unresolved"} + + +def test_bus_bit_anchor(review): + review.add_thread(design(), TOP_D1, "d[1]") + [thread] = review.response(design())["threads"] + assert thread["anchor"]["term_id"] == design().getTerm("d").getID() + assert thread["anchor"]["bit"] == 1 + assert thread["resolution"]["state"] == "exact" + with pytest.raises(ReviewError): + review.add_thread(design(), {**TOP_D1, "bit": 7}, "no such bit") + + +def test_anonymous_net_by_id(review): + # The unnamed tie-off nets can't be named: only an id reaches one. + net = anonymous_nets(design())[1] + tid = review.add_thread(design(), {"kind": "net", "id_path": [], "net_id": net.getID()}, + "tie-off") + [thread] = review.response(design())["threads"] + assert thread["id"] == tid + assert thread["anchor"] == {"kind": "net", "id_path": [], "path": [], + "net_id": net.getID(), "net": ""} + assert thread["resolution"]["state"] == "exact" + + +def test_anonymous_instance_by_id(review): + # u_sub's assign is an anonymous instance (id 1): no name path reaches it. + assert sub().getInstanceByID(1).getName() == "" + review.add_thread(design(), {"kind": "instance", "id_path": [0, 1]}, "assign") + [thread] = review.response(design())["threads"] + assert thread["anchor"]["path"] == ["u_sub", ""] + assert thread["resolution"] == {"state": "exact", "id_path": [0, 1], "path": ["u_sub", ""]} + with pytest.raises(ReviewError): + review.add_thread(design(), {"kind": "instance", "path": ["u_sub", ""]}, "x") + + +@pytest.mark.parametrize("anchor", [ + {"kind": "instance", "path": ["nope"]}, + {"kind": "instance", "id_path": [9]}, + {"kind": "term", "path": ["u_sub"], "terminal": "nope"}, + {"kind": "term", "id_path": [0], "term_id": 99}, + {"kind": "net", "id_path": [], "net_id": 99}, + {"kind": "net", "id_path": []}, + {"kind": "bogus", "path": []}, + {"kind": "view", "view": "../../etc"}, +]) +def test_bad_anchor_refused(review, anchor): + with pytest.raises(ReviewError): + review.add_thread(design(), anchor, "x") + assert not (review.directory / "threads").exists() + + +def test_bad_thread_id_refused(review): + with pytest.raises(ReviewError): + review.add_event("../../review", text="x") + + +def test_no_author_is_read_only(top, tmp_path): + review = Review(tmp_path / "r", None) + with pytest.raises(ReviewError): + review.add_thread(design(), U_AND, "x") + assert review.response(design())["writable"] is False + + +def test_resolution_rules(top): + snap = "a" * 64 + assert resolve_anchor(design(), U_AND, snap, snap) == { + "state": "exact", "id_path": [0, 0], "path": ["u_sub", "u_and"]} + # Written on another netlist: never guessed, by ids or by names. + assert resolve_anchor(design(), U_AND, "b" * 64, snap) == {"state": "other_snapshot"} + assert resolve_anchor(design(), U_AND, None, snap) == {"state": "other_snapshot"} + assert resolve_anchor(design(), {**U_AND, "id_path": [5]}, snap, snap) == { + "state": "unresolved"} + + +def test_thread_on_another_snapshot(review): + tid = review.add_thread(design(), U_AND, "x") + # As if written on another netlist, whose snapshot isn't in the review. + other = "f" * 64 + path = review.directory / "threads" / tid / "thread.json" + path.write_text(path.read_text().replace(review.design_snapshot(design()), other)) + [thread] = review.response(design())["threads"] + assert thread["snapshot"] == other + assert thread["snapshot_stored"] is False + assert thread["resolution"] == {"state": "other_snapshot"} + + +def test_snapshot_identifies_the_netlist(review): + first = review.design_snapshot(design()) + review.design_changed() + assert review.design_snapshot(design()) == first # deterministic dump + top = design() + net = naja.SNLScalarNet.create(top, "review_test_extra") + try: + review.design_changed() + assert review.design_snapshot(top) != first + finally: + net.destroy() + review.design_changed() + + +def test_malformed_files_are_skipped(review): + review.add_thread(design(), U_AND, "kept") + broken = review.directory / "threads" / ("0" * 32) + broken.mkdir() + (broken / "thread.json").write_text("{ not json") + (review.directory / "threads" / "stray.txt").write_text("ignored") + assert [t["comments"][0]["text"] for t in review.response(design())["threads"]] == ["kept"] + + +# -- through the session ----------------------------------------------------- + +def answer(session, **request): + return [json.loads(r) for r in session.answer(json.dumps(request))] + + +def test_review_pushed_after_root(review): + review.add_thread(design(), U_AND, "x") + replies = answer(ViewerSession(review=review), request="load_root") + assert [r["response"] for r in replies] == ["root_response", "review_response"] + assert len(replies[1]["threads"]) == 1 + assert replies[1]["writable"] is True + + +def test_viewer_writes(review): + pushed = [] + session = ViewerSession(push=pushed.append, review=review) + assert answer(session, request="review_add_thread", anchor=PIN_A, text="hello", + severity="warning") == [] + [thread] = json.loads(pushed[-1])["threads"] + assert thread["severity"] == "warning" + assert answer(session, request="review_add_event", thread=thread["id"], + text="reply", status="resolved") == [] + [thread] = json.loads(pushed[-1])["threads"] + assert thread["status"] == "resolved" + assert [c["text"] for c in thread["comments"]] == ["hello", "reply"] + + +def test_viewer_write_without_push_replies(review): + [reply] = answer(ViewerSession(review=review), request="review_add_thread", anchor=U_AND, + text="x") + assert reply["response"] == "review_response" + + +def test_refused_writes_are_errors(top, review): + session = ViewerSession(review=review) + [reply] = answer(session, request="review_add_thread", anchor={"kind": "instance", + "path": ["nope"]}, text="x") + assert reply["response"] == "error" and reply["request"] == "review_add_thread" + [reply] = answer(ViewerSession(), request="review_add_event", thread="0" * 32, text="x") + assert reply["response"] == "error" + + +def test_stale_write_is_dropped(review): + session = ViewerSession(review=review) + session.design_changed() + assert answer(session, request="review_add_thread", anchor=U_AND, text="x", + generation=0) == [] + assert not (review.directory / "threads").exists() From 3aab2c1ca5c25263b80c42231bc8af8b4f6c851c Mon Sep 17 00:00:00 2001 From: xtof Date: Sun, 1 Nov 2026 01:17:10 +0100 Subject: [PATCH 2/2] Show and write review threads in the viewer - ReviewStore parses the host's review_response and matches threads to objects by ids only (instance id_path, term/net id, bit), and only those the host resolved exact on the loaded design. - Review tab: open/all filter, a thread's comments, Reply, Resolve / Won't Fix / Reopen, Select / Show in Schematic, the snapshot id of a thread written on another netlist. Selecting a thread keeps the tab in front. - "Add Comment..." in the tree's instance/pin/net menus and the schematic's box/pin menus (writable hosts only); [n] open threads after tree rows and a numbered badge on schematic boxes, drawn through the painter. - nets_response entries carry net_id (both backends), so a net row can anchor a thread. --- CLAUDE.md | 36 ++- CMakeLists.txt | 4 + README.md | 10 +- REUSE.toml | 6 + docs/review_format.md | 15 +- python/naja_schematic/protocol.py | 2 +- python/tests/test_protocol.py | 3 + src/AppLogic.cpp | 31 ++- src/AppLogic.h | 4 + src/EquipotentialView.cpp | 25 ++ src/LocalSNLProvider.cpp | 3 +- src/NetlistTree.cpp | 63 ++++- src/NetlistTree.h | 25 +- src/ReviewStore.cpp | 253 ++++++++++++++++++ src/ReviewStore.h | 119 ++++++++ src/ReviewView.cpp | 235 ++++++++++++++++ src/ReviewView.h | 36 +++ src/SchematicView.cpp | 28 +- src/Types.cpp | 3 + src/Types.h | 6 + tests/NetlistTreeTest.cpp | 24 ++ tests/ReviewStoreTest.cpp | 126 +++++++++ tests/ReviewViewTest.cpp | 76 ++++++ .../provider/LocalSNLProviderProtocolTest.cpp | 1 + 24 files changed, 1109 insertions(+), 25 deletions(-) create mode 100644 src/ReviewStore.cpp create mode 100644 src/ReviewStore.h create mode 100644 src/ReviewView.cpp create mode 100644 src/ReviewView.h create mode 100644 tests/ReviewStoreTest.cpp create mode 100644 tests/ReviewViewTest.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 8b49257..da7a4c7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -382,7 +382,9 @@ next to `Terms` under an instance node) with one deliberate asymmetry: a net entry carries no `child_id` and offers no "Show Equipotential" action — a net has no direction, and get_properties identifies it by name (`"net"` field) rather than by the provider-specific numeric id a term's -`load_equipotential` request needs. See `NetlistTreeNetNode`/ +`load_equipotential` request needs. It does carry its `net_id` +(`SNLNet::getID()`), only to anchor review threads (`getNetID()`; absent +from older hosts). See `NetlistTreeNetNode`/ `NetlistTreeBusNetBitNode` in `NetlistTree.h/.cpp`. Both `nets_response` and the `has_nets` flag filter out anonymous scalar @@ -646,6 +648,19 @@ covers delayed selections, requests queued behind the swap and several viewers deterministically; the netlist there is "replaced" by itself, the worst case where every stale id still resolves. +**Reviews** (full design: `docs/review_format.md`): threads on netlist +objects, shared asynchronously through a directory committed with the +design. The host owns the files (Python: `review.py`, `--review DIR`, +`review=`; not yet `LocalSNLProvider`) and pushes `review_response` after +each `root_response` and every write; the viewer sends `review_add_thread` +(an anchor by ids: instance `id_path`, `term_id`, `net_id` from +`nets_response`, optional `bit`) and `review_add_event` (reply, status), and +a refused write comes back as `{"response":"error","request":"review_..."}`. +Objects are identified by naja ids only, never by names: a thread is pinned +to the netlist it was written on, stored in the review as a naja-if +snapshot, and only drawn when the host resolves it `exact` on the loaded +design (`other_snapshot` threads are listed, not drawn). + `get_properties`/`properties_response` is a general name/value inspector for whatever object the UI asks about — an instance (including the top design itself), a term/pin, or a net — answered by both `LocalSNLProvider` @@ -792,6 +807,25 @@ path the host read, used by the native "Open in Editor". `TraceStore::setColor()` refuses a near-black one (the convergence color, so shared gates stay unambiguous) and a near-white one (invisible on the white canvas). +- **`ReviewStore`** — global static store (same pattern) for the review + threads the host last pushed (`review_response`; see "Reviews" under Wire + protocol): parsed threads, `writable`/`author`, the last refused write. + `instanceThreads()`/`termThreads()`/`netThreads()` match an object by + ids only (`id_path` + term/net id, optional bit), and only threads the + host resolved `exact`; `openCount()`/`openColor()` give the badges. + Builds the `review_add_thread`/`review_add_event` requests and their + anchors. Cleared on every fresh `root_response`/`root_loaded` (the host + pushes the review again after it). +- **`ReviewView`** — the "Review" bottom-panel tab (open/all filter; a + thread's comments, Reply, Resolve/Won't Fix/Reopen, Select and Show in + Schematic for an exact one, the snapshot id for one on another netlist) + and the modal Add Comment dialog (`openComposer()`, drawn by + `renderComposer()` at the top level each frame), opened by + `commentMenuItem()` from the tree's instance/term/net menus and the + schematic's box and pin menus -- only shown when the host is writable. + The tree shows `[n]` open threads after a row, the schematic a numbered + disc on a box's corner (`InstanceShape::reviewOpen`, drawn through the + painter); hovering a tree row lists them. - **`SelectionStore`** — global static store (same pattern) for the one selection: an instance, by `InstancePath` (`{}` = top), or a schematic net, by `SchematicNetRef` (the pin its wire tree starts from: instance diff --git a/CMakeLists.txt b/CMakeLists.txt index 6d73c55..777a47a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -103,6 +103,8 @@ set(CORE_SOURCES src/SelectionStore.cpp src/TraceStore.cpp src/TraceView.cpp + src/ReviewStore.cpp + src/ReviewView.cpp src/AppLogic.cpp ${IMGUI_SOURCES} # Read-only code view of the Source tab (SourceView). @@ -265,6 +267,8 @@ if(NOT EMSCRIPTEN_BUILD) src/PropertiesStore.cpp src/SelectionStore.cpp src/TraceStore.cpp + src/ReviewStore.cpp + src/ReviewView.cpp src/GenerationProvider.cpp src/SchematicLayout.cpp src/SchematicInteraction.cpp diff --git a/README.md b/README.md index fc08b5f..411e2c9 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,8 @@ plus options, in any order. Their flags differ slightly: | `--yosys-src` | `--yosys-src` | Read Yosys `(* src *)` attributes, so Show Sources leads back to the RTL (Verilog only) | | `--source-root dir` | `--source-root dir` | Where to look for relative source paths (repeatable) | | `--source-map FROM=TO` | `--source-map FROM=TO` | Rewrite source paths recorded on another machine (repeatable) | +| | `--review DIR`, `--author NAME` | Read and write review threads in DIR, a directory kept in git with the design ([format](docs/review_format.md)) | +| | `--review DIR` alone, `--snapshot ID` | Open the netlist a review thread was written on, stored in the review | | | `--open` | Open the viewer in a browser | | | `--port N`, `--host H` | Where to serve (default `localhost:8081`) | @@ -139,7 +141,13 @@ Cmd in the macOS desktop app (Ctrl in a browser, on any OS). | Click a box | Select the instance (also in the design tree) | | Click a wire | Select (highlight) its net | | Click a box's hierarchy glyph | Show / hide what is inside it | -| Right-click | Menu: trace to driver, sources, properties, fit, clear | +| Right-click | Menu: trace to driver, sources, properties, comment, fit, clear | + +In the design tree, right-click a row (an instance, a pin or a net) for its +menu. With a review loaded (`--review DIR`), **Add Comment...** in these +menus starts a review thread on that object; the **Review** tab lists the +threads, to reply to them and resolve them. A tree row shows `[n]`, and a +schematic box a numbered disc, for its open threads. | Keys (mouse over the schematic) | Action | |---|---| diff --git a/REUSE.toml b/REUSE.toml index 434939c..23698a7 100644 --- a/REUSE.toml +++ b/REUSE.toml @@ -95,6 +95,10 @@ path = [ "src/SchematicView.h", "src/SelectionStore.cpp", "src/SelectionStore.h", + "src/ReviewStore.cpp", + "src/ReviewStore.h", + "src/ReviewView.cpp", + "src/ReviewView.h", "src/SourceStore.cpp", "src/SourceStore.h", "src/SourceView.cpp", @@ -120,6 +124,8 @@ path = [ "tests/FakeNetlistProvider.h", "tests/GUIDataTest.cpp", "tests/NetlistTreeTest.cpp", + "tests/ReviewStoreTest.cpp", + "tests/ReviewViewTest.cpp", "tests/GenerationProviderTest.cpp", "tests/SchematicInteractionTest.cpp", "tests/SelectionTest.cpp", diff --git a/docs/review_format.md b/docs/review_format.md index bc4c447..cbe0840 100644 --- a/docs/review_format.md +++ b/docs/review_format.md @@ -2,8 +2,10 @@ How people (and agents) leave findings on a netlist and pass them to each other, asynchronously, through git. Status: threads are implemented in the -Python host (`python/naja_schematic/review.py`, najaeda >= 0.7.34 for net ids); the C++ viewer and native -provider, and saved views, are not yet. This document is the contract the two backends +Python host (`python/naja_schematic/review.py`, najaeda >= 0.7.34 for net ids) +and the viewer (`src/ReviewStore.cpp`, `src/ReviewView.cpp`), so the browser +and notebook viewers read and write reviews; the native provider (desktop +app) and saved views are not yet. This document is the contract the two backends (`src/LocalSNLProvider.cpp`, `python/naja_schematic/protocol.py`) and any external writer (scripts, naja-agent) follow. @@ -267,10 +269,11 @@ disk). - Without a review directory, or without an author, write requests fail and the viewer hides its write actions. -The viewer anchors by ids it already has: instance `id_path`s everywhere, -and a term's `child_id` (`terms_response`, `expand_instance_terms`). Nets -carry no id on the wire yet: `nets_response` entries need a `net_id` -(both backends) before the tree can open a thread on a net. +The viewer anchors by ids it already has: instance `id_path`s everywhere, a +term's `child_id` (`terms_response`, `expand_instance_terms`) and a net's +`net_id` (`nets_response`, both backends; a net row from an older host has +none and offers no comment). It matches threads to tree rows and schematic +boxes the same way, by ids, and only `exact` ones. Shared cases for both backends: `tests/data/review_protocol_cases.json`. diff --git a/python/naja_schematic/protocol.py b/python/naja_schematic/protocol.py index 6d2bdb1..6efb25f 100644 --- a/python/naja_schematic/protocol.py +++ b/python/naja_schematic/protocol.py @@ -554,7 +554,7 @@ def _handle_design_request(u, request): for net in design.getNets(): if is_anonymous_constant_net(net): continue - entry = {"name": net.getName()} + entry = {"name": net.getName(), "net_id": net.getID()} if isinstance(net, naja.SNLBusNet): entry["msb"] = net.getMSB() entry["lsb"] = net.getLSB() diff --git a/python/tests/test_protocol.py b/python/tests/test_protocol.py index fde70fe..469b603 100644 --- a/python/tests/test_protocol.py +++ b/python/tests/test_protocol.py @@ -47,6 +47,9 @@ def test_design_requests(top): assert "s" in names # The implicit 1'b0/1'b1 tie-offs on r.CE/r.R are filtered out. assert "" not in names + # Each net carries its id, what a review thread anchors it by. + design = naja.NLUniverse.get().getTopDesign() + assert all(n["net_id"] == design.getNet(n["name"]).getID() for n in nets["children"]) def test_instance_requests_echo_the_instance_path(top): diff --git a/src/AppLogic.cpp b/src/AppLogic.cpp index d429b97..da31794 100644 --- a/src/AppLogic.cpp +++ b/src/AppLogic.cpp @@ -27,6 +27,8 @@ using json = nlohmann::json; #include "TraceStore.h" #include "TraceView.h" #include "PropertiesView.h" +#include "ReviewStore.h" +#include "ReviewView.h" #include "CousineFont.h" #include "DroidSansFont.h" @@ -157,7 +159,8 @@ static void renderShortcutsWindow(bool& open) { {"Click a box", "Select the instance (also in the tree)"}, {"Click a wire", "Select (highlight) its net"}, {"Click a box's hierarchy glyph","Show / hide what is inside it"}, - {"Right-click", "Context menu: trace, sources, properties, fit, clear"}, + {"Right-click", "Context menu: trace, sources, properties, comment, fit, clear"}, + {"Right-click a tree row", "Its menu; Add Comment... starts a review thread (with --review)"}, }; auto table = [](const char* id, const Row* rows, size_t n) { if (!ImGui::BeginTable(id, 2, ImGuiTableFlags_RowBg | ImGuiTableFlags_SizingFixedFit)) return; @@ -270,6 +273,7 @@ void setupProvider(AppState& state) { PropertiesStore::clear(); // stale properties reference the old design SourceStore::clear(); // so do source references SelectionStore::clear(); // so does a stale selection + ReviewStore::clear(); // the host pushes the review again for this design const auto& root = j["root"]; if (root.contains("has_terms") || root.contains("has_primitives") || root.contains("has_instances")) { InstanceResponseJson data = root.get(); @@ -358,7 +362,7 @@ void setupProvider(AppState& state) { } parent->createChildren(); for (auto& net : data.children) { - parent->createNetNode(net.name, net.msb, net.lsb, net.has_sources); + parent->createNetNode(net.name, net.msb, net.lsb, net.has_sources, net.net_id); } } else if (resp == "equipotential_response") { // A trace_id (echoed from a tree "Show Equipotential") makes the net a @@ -495,7 +499,7 @@ void setupProvider(AppState& state) { std::string subject = j.value("subject", std::string("")); Console::Log("Properties received: " + std::to_string(data.properties.size()) + " item(s)"); PropertiesStore::setProperties(subject, std::move(data.properties)); - state.focusPropertiesTab = true; + state.focusPropertiesTab = !state.reviewTabShown; } else if (resp == "diagnosis_response") { std::vector items; if (j.contains("items") && j["items"].is_array()) { @@ -506,6 +510,8 @@ void setupProvider(AppState& state) { Console::Log("Diagnosis received: " + std::to_string(items.size()) + " item(s)"); DiagnosisStore::setDiagnostics(std::move(items)); state.focusDiagnosisTab = true; + } else if (resp == "review_response") { + ReviewStore::setResponse(j); } else if (resp == "export_svg") { state.svgExportRequested = true; } else if (resp == "focus_instance") { @@ -541,6 +547,13 @@ void setupProvider(AppState& state) { SelectionStore::select(*path, SelectionStore::Origin::Host); } else if (resp == "error") { std::cerr << "Backend error: " << j["message"] << std::endl; + // A refused review write: shown in the Review tab. + const std::string request = j.value("request", std::string()); + if (request.rfind("review_", 0) == 0) { + const std::string message = j.value("message", std::string("refused")); + Console::Error(request + ": " + message); + ReviewStore::setError(message); + } } }); @@ -947,6 +960,17 @@ bool appFrame(AppState& state) { // DiagnosisView::render(); // ImGui::EndTabItem(); // } + // "Review (3)": the open threads. A fixed ID, so the tab stays put + // as the count changes. + size_t open = 0; + for (const auto& t : ReviewStore::threads()) open += t.isOpen(); + std::string reviewTab = open ? "Review (" + std::to_string(open) + ")###Review" + : std::string("Review###Review"); + state.reviewTabShown = ImGui::BeginTabItem(reviewTab.c_str()); + if (state.reviewTabShown) { + ReviewView::render(state.provider); + ImGui::EndTabItem(); + } ImGuiTabItemFlags srcFlags = state.focusSourceTab ? ImGuiTabItemFlags_SetSelected : 0; if (ImGui::BeginTabItem("Source", nullptr, srcFlags)) { SourceView::render(state.provider); @@ -970,6 +994,7 @@ bool appFrame(AppState& state) { ImGui::End(); renderShortcutsWindow(shortcutsOpen); + ReviewView::renderComposer(state.provider); // === About modal === if (aboutOpen) { ImGui::OpenPopup("About naja-schematic"); aboutOpen = false; } diff --git a/src/AppLogic.h b/src/AppLogic.h index afc85df..e0ed17a 100644 --- a/src/AppLogic.h +++ b/src/AppLogic.h @@ -24,6 +24,10 @@ struct AppState { bool growSourcePanel {false}; bool focusDiagnosisTab {false}; bool focusPropertiesTab {false}; + // The Review tab is the one shown: selecting a thread selects its + // instance, whose properties_response then doesn't take the panel away + // from the thread being read. + bool reviewTabShown {false}; // How many of the most recently added equipotentials the bottom // "Equipotential" table lists: 1 after a single load_equipotential, every // net of the cone after a trace_driver_response. diff --git a/src/EquipotentialView.cpp b/src/EquipotentialView.cpp index 7b369d8..557fc20 100644 --- a/src/EquipotentialView.cpp +++ b/src/EquipotentialView.cpp @@ -15,6 +15,8 @@ #include "DiagnosisStore.h" #include "SchematicLayout.h" #include "SchematicInteraction.h" +#include "ReviewStore.h" +#include "ReviewView.h" #include "SelectionStore.h" #include "TraceStore.h" @@ -72,6 +74,13 @@ static int g_ctxInstanceId = -1; // box the pin sits on. -1 = none. static int g_ctxPortId = -1; +// The review badge of an instance box or module frame (ReviewStore). +static void setReviewBadge(InstanceShape& shape) { + const auto threads = ReviewStore::instanceThreads(shape.path); + shape.reviewOpen = int(ReviewStore::openCount(threads)); + shape.reviewColor = ReviewStore::openColor(threads); +} + // What a box's context menu needs. `hasSources`: offer "Show Sources" -- // a module frame always does (the provider sent no flag for it). struct OccurrenceInfo { InstancePath path; DesignRef designRef; bool hasSources = false; }; @@ -294,6 +303,7 @@ static HierEmitResult emitInstanceInternals(InstanceShape& parent, int& nextInst cs.hasChildren = c.hasInstances; cs.hierExpanded = c.hasInstances && g_hierExpanded.count(cs.path) > 0; cs.diagOutline = DiagnosisStore::instanceColor(cs.path); + setReviewBadge(cs); childIdToShapeId[c.childId] = cs.id; g_occInfoByShapeId[cs.id] = { cs.path, c.designRef, c.hasSources }; children.push_back(std::move(cs)); @@ -623,6 +633,17 @@ void EquipotentialView::renderSchematic(const std::vector& equip req["trace_id"] = TraceStore::begin(label); g_provider->send(req.dump()); } + // A pin's term, by its ids (a top-level port stub's path is {}). + for (auto& inst : g_schematic.instances) { + const Port* p = g_schematic.findPortById(inst, g_ctxPortId); + if (!p) continue; + const std::string label = inst.modelName == "port" ? p->name + : traceLabel(inst.path, p->name); + ReviewView::commentMenuItem( + ReviewStore::termAnchor(inst.path, portIt->second.termId, p->name, + portIt->second.bit), label); + break; + } auto busIt = g_expandedBusGroupByPortId.find(g_ctxPortId); if (busIt != g_expandedBusGroupByPortId.end() && ImGui::MenuItem("Collapse Bus")) g_expandedBuses.erase(busIt->second); @@ -641,6 +662,8 @@ void EquipotentialView::renderSchematic(const std::vector& equip writePath(req, occIt->second.path); g_provider->send(req.dump()); } + ReviewView::commentMenuItem(ReviewStore::instanceAnchor(occIt->second.path), + displayPath(occIt->second.path)); // A module frame covers a lot of canvas, so right-clicking its // empty area also offers the canvas-level entries. auto* shape = g_schematic.findInstanceById(g_ctxInstanceId); @@ -903,6 +926,7 @@ void EquipotentialView::renderSchematic(const std::vector& equip if (inst.diagOutline == 0 && traceIdsByInstance[key].size() >= 2) inst.diagOutline = TraceStore::convergenceColor(); inst.selected = SelectionStore::isSelected(key); + setReviewBadge(inst); g_occInfoByShapeId[inst.id] = { key, mi.designRef, mi.hasSources }; pathToInstId[key] = inst.id; @@ -1144,6 +1168,7 @@ void EquipotentialView::renderSchematic(const std::vector& equip for (auto& f : frames) { f.shape.diagOutline = DiagnosisStore::instanceColor(f.shape.path); f.shape.selected = SelectionStore::isSelected(f.shape.path); + setReviewBadge(f.shape); g_occInfoByShapeId[f.shape.id] = { f.shape.path, DesignRef{}, true }; frameShapes.push_back(std::move(f.shape)); } diff --git a/src/LocalSNLProvider.cpp b/src/LocalSNLProvider.cpp index 924b335..0db452a 100644 --- a/src/LocalSNLProvider.cpp +++ b/src/LocalSNLProvider.cpp @@ -809,12 +809,13 @@ std::string LocalSNLProvider::buildNetsResponse( // itself and expands individual bits lazily. json n = { {"name", bn->getName().getString()}, + {"net_id", bn->getID()}, {"msb", static_cast(bn->getMSB())}, {"lsb", static_cast(bn->getLSB())} }; children.push_back(std::move(withSources(n, busHasSources(bn)))); } else if (auto* bit = dynamic_cast(net)) { - json n = {{"name", bit->getName().getString()}}; + json n = {{"name", bit->getName().getString()}, {"net_id", bit->getID()}}; children.push_back(std::move(withSources(n, hasSources(bit)))); } } diff --git a/src/NetlistTree.cpp b/src/NetlistTree.cpp index bd8ad03..d5bb0b0 100644 --- a/src/NetlistTree.cpp +++ b/src/NetlistTree.cpp @@ -5,6 +5,8 @@ #include "Console.h" #include "DiagnosisStore.h" #include "INetlistProvider.h" +#include "ReviewStore.h" +#include "ReviewView.h" #include "SelectionStore.h" #include "TraceStore.h" @@ -31,8 +33,43 @@ void inspectMenuItems(INetlistProvider* provider, json address, bool hasSources) } } +// What a review thread on this row anchors to, and the threads already on +// it (ReviewStore, matched by ids). A net row whose host sent no net_id has +// neither. `label` names the object in the Add Comment dialog. +struct ReviewTarget { + std::optional anchor; + std::vector threads; + std::string label; +}; + } // namespace +static ReviewTarget reviewTarget(const NetlistTreeNode& node) { + ReviewTarget target; + const InstancePath path = node.getInstancePath(); + const std::optional bit = node.isBusBit() ? std::optional(node.getBusBit()) + : std::nullopt; + if (node.isInstanceNode()) { + target.anchor = ReviewStore::instanceAnchor(path); + target.threads = ReviewStore::instanceThreads(path); + target.label = displayPath(path); + } else if (node.isBitTerm() || node.isBus()) { + target.anchor = ReviewStore::termAnchor(path, node.getChildID(), node.getTermBaseName(), bit); + target.threads = ReviewStore::termThreads(path, node.getChildID(), bit); + target.label = node.getLabel(); + } else if ((node.isBitNet() || node.isBusNet()) && node.getNetID()) { + target.anchor = ReviewStore::netAnchor(path, *node.getNetID(), node.getNetBaseName(), bit); + target.threads = ReviewStore::netThreads(path, *node.getNetID(), bit); + target.label = node.getLabel(); + } + if (target.anchor) { + std::string where = displayPath(path); + if (!node.isInstanceNode() && !where.empty()) target.label = where + "/" + target.label; + if (target.label.empty()) target.label = "the top design"; + } + return target; +} + void NetlistTree::createRootNode( const std::string& name, const DesignRef& design_ref, @@ -201,15 +238,17 @@ void NetlistTreeNode::render() { if (isInstanceNode() && ImGui::IsItemClicked(ImGuiMouseButton_Left) && !ImGui::IsItemToggledOpen()) { SelectionStore::select(getInstancePath(), SelectionStore::Origin::Tree); } + const ReviewTarget review = reviewTarget(*this); if (ImGui::IsItemHovered()) { auto diagnostics = getDiagnostics(); - if (!diagnostics.empty()) { + if (!diagnostics.empty() || !review.threads.empty()) { ImGui::BeginTooltip(); for (const auto* d : diagnostics) { ImGui::TextColored(ImColor(DiagnosisStore::colorForSeverity(d->severity)).Value, "[%s] %s", toString(d->severity), d->message.c_str()); if (!d->source.empty()) ImGui::TextDisabled("source: %s", d->source.c_str()); } + ReviewView::threadsTooltip(review.threads); ImGui::EndTooltip(); } } @@ -249,6 +288,7 @@ void NetlistTreeNode::render() { req["terminal"] = getTermBaseName(); if (isBusBit()) req["bit"] = getBusBit(); inspectMenuItems(getTree()->getProvider(), req, hasSources()); + if (review.anchor) ReviewView::commentMenuItem(*review.anchor, review.label); ImGui::EndPopup(); } } else if (isBus()) { @@ -260,6 +300,7 @@ void NetlistTreeNode::render() { writePath(req, getInstancePath()); req["terminal"] = getTermBaseName(); inspectMenuItems(getTree()->getProvider(), req, hasSources()); + if (review.anchor) ReviewView::commentMenuItem(*review.anchor, review.label); ImGui::EndPopup(); } } else if (isBitNet() || isBusNet()) { @@ -270,6 +311,7 @@ void NetlistTreeNode::render() { req["net"] = getNetBaseName(); if (isBusBit()) req["bit"] = getBusBit(); inspectMenuItems(getTree()->getProvider(), req, hasSources()); + if (review.anchor) ReviewView::commentMenuItem(*review.anchor, review.label); ImGui::EndPopup(); } } else if (isInstanceNode()) { @@ -284,9 +326,17 @@ void NetlistTreeNode::render() { req["kind"] = "instance"; writePath(req, getInstancePath()); inspectMenuItems(getTree()->getProvider(), req, hasSources()); + if (review.anchor) ReviewView::commentMenuItem(*review.anchor, review.label); ImGui::EndPopup(); } } + // Open review threads on this object: their count, in the worst + // severity's color (the popups and tooltip above are other windows, so + // this still follows the row's label). + if (const size_t open = ReviewStore::openCount(review.threads)) { + ImGui::SameLine(); + ImGui::TextColored(ImColor(ReviewStore::openColor(review.threads)).Value, "[%zu]", open); + } if (isOpen and not isLeaf()) { if (children_ != nullptr) { for (auto child : *children_) { @@ -529,8 +579,10 @@ NetlistTreeNetNode::NetlistTreeNetNode( const std::string& name, std::optional msb, std::optional lsb, - bool hasSources): - NetlistTreeNode(parent), name_(name), msb_(msb), lsb_(lsb), hasSources_(hasSources) { + bool hasSources, + std::optional netID): + NetlistTreeNode(parent), name_(name), msb_(msb), lsb_(lsb), hasSources_(hasSources), + netID_(netID) { if (not msb_.has_value() or not lsb_.has_value()) { //scalar children_ = new Children(); } @@ -627,9 +679,10 @@ void NetlistTreeNode::createNetNode( const std::string& name, std::optional msb, std::optional lsb, - bool hasSources) { + bool hasSources, + std::optional netID) { //this should be a group node of type Nets - children_->push_back(new NetlistTreeNetNode(this, name, msb, lsb, hasSources)); + children_->push_back(new NetlistTreeNetNode(this, name, msb, lsb, hasSources, netID)); } void NetlistTreeNode::createInstanceNode( diff --git a/src/NetlistTree.h b/src/NetlistTree.h index ee502df..90a9e68 100644 --- a/src/NetlistTree.h +++ b/src/NetlistTree.h @@ -113,7 +113,8 @@ class NetlistTreeNode { const std::string& name, std::optional msb, std::optional lsb, - bool hasSources = false); + bool hasSources = false, + std::optional netID = std::nullopt); void createInstanceNode( const std::string& name, const std::string& modelName, @@ -169,6 +170,12 @@ class NetlistTreeNode { virtual std::string getNetBaseName() const { return std::string(); } + // SNLNet id in the containing instance's model, for net/bus-net-bit + // nodes whose host sent one (nets_response's net_id); what a review + // thread anchors a net by. + virtual std::optional getNetID() const { + return std::nullopt; + } // True for a whole-bus term node (msb/lsb both set) — its menu has only // Show Properties; equipotentials and traces are per bit. virtual bool isBus() const { @@ -356,7 +363,8 @@ class NetlistTreeNetNode : public NetlistTreeNode { const std::string& name, std::optional msb, std::optional lsb, - bool hasSources = false); + bool hasSources = false, + std::optional netID = std::nullopt); virtual std::string getLabel() const override; virtual void expand() override; @@ -371,13 +379,15 @@ class NetlistTreeNetNode : public NetlistTreeNode { return msb_.has_value() && lsb_.has_value(); } virtual std::string getNetBaseName() const override { return name_; } + virtual std::optional getNetID() const override { return netID_; } size_t getWidth() const; virtual std::vector busBits() const override; private: - std::string name_; - std::optional msb_; - std::optional lsb_; - bool hasSources_; + std::string name_; + std::optional msb_; + std::optional lsb_; + bool hasSources_; + std::optional netID_; }; class NetlistTreeBusNetBitNode : public NetlistTreeNode { @@ -400,6 +410,9 @@ class NetlistTreeBusNetBitNode : public NetlistTreeNode { virtual std::string getNetBaseName() const override { return getParent()->getNetBaseName(); } + virtual std::optional getNetID() const override { + return getParent()->getNetID(); + } virtual bool hasSources() const override { return getParent()->hasSources(); } private: int bit_; diff --git a/src/ReviewStore.cpp b/src/ReviewStore.cpp new file mode 100644 index 0000000..189d75d --- /dev/null +++ b/src/ReviewStore.cpp @@ -0,0 +1,253 @@ +#include "ReviewStore.h" + +#include + +#include "DiagnosisStore.h" + +namespace { +bool g_hasReview = false; +bool g_writable = false; +std::string g_author; +std::string g_error; +std::vector g_threads; +unsigned g_revision = 0; + +std::string text(const json& j, const char* key) { + auto it = j.find(key); + return it != j.end() && it->is_string() ? it->get() : std::string(); +} + +std::optional unsignedField(const json& j, const char* key) { + auto it = j.find(key); + if (it == j.end() || !it->is_number_unsigned()) return std::nullopt; + return it->get(); +} + +DiagnosisSeverity severityFrom(const std::string& s) { + if (s == "error") return DiagnosisSeverity::Error; + if (s == "warning") return DiagnosisSeverity::Warning; + return DiagnosisSeverity::Info; +} + +// The wire's severity names (toString() gives the display form). +const char* severityName(DiagnosisSeverity s) { + switch (s) { + case DiagnosisSeverity::Error: return "error"; + case DiagnosisSeverity::Warning: return "warning"; + case DiagnosisSeverity::Info: return "info"; + } + return "info"; +} + +ReviewAnchorKind kindFrom(const std::string& s) { + if (s == "term") return ReviewAnchorKind::Term; + if (s == "net") return ReviewAnchorKind::Net; + if (s == "view") return ReviewAnchorKind::View; + return ReviewAnchorKind::Instance; +} + +ReviewState stateFrom(const std::string& s) { + if (s == "exact") return ReviewState::Exact; + if (s == "other_snapshot") return ReviewState::OtherSnapshot; + return ReviewState::Unresolved; +} + +ReviewAnchor parseAnchor(const json& j) { + ReviewAnchor a; + if (!j.is_object()) return a; + a.kind = kindFrom(text(j, "kind")); + if (j.contains("id_path") && j["id_path"].is_array()) + for (const auto& id : j["id_path"]) + if (id.is_number_unsigned()) a.idPath.push_back(id.get()); + if (j.contains("path") && j["path"].is_array()) + for (const auto& name : j["path"]) + a.path.push_back(name.is_string() ? name.get() : std::string()); + a.termId = unsignedField(j, "term_id"); + a.netId = unsignedField(j, "net_id"); + if (j.contains("bit") && j["bit"].is_number_integer()) a.bit = j["bit"].get(); + a.view = text(j, "view"); + a.terminal = text(j, "terminal"); + a.net = text(j, "net"); + return a; +} + +ReviewThread parseThread(const json& j) { + ReviewThread t; + t.id = text(j, "id"); + t.title = text(j, "title"); + t.author = text(j, "author"); + t.source = text(j, "source"); + t.created = text(j, "created"); + t.snapshot = text(j, "snapshot"); + t.snapshotStored = j.value("snapshot_stored", false); + if (j.contains("anchor")) t.anchor = parseAnchor(j["anchor"]); + if (j.contains("resolution") && j["resolution"].is_object()) { + const auto& r = j["resolution"]; + t.state = stateFrom(text(r, "state")); + if (t.state == ReviewState::Exact) t.resolvedPath = readPath(r); + } + if (j.contains("status") && j["status"].is_string()) t.status = j["status"].get(); + t.severity = severityFrom(text(j, "severity")); + if (j.contains("labels") && j["labels"].is_array()) + for (const auto& label : j["labels"]) + if (label.is_string()) t.labels.push_back(label.get()); + if (j.contains("comments") && j["comments"].is_array()) + for (const auto& c : j["comments"]) + t.comments.push_back({text(c, "id"), text(c, "author"), text(c, "date"), text(c, "text")}); + return t; +} + +// Exact threads of one kind on one object: same instance ids and, for a +// term/net, same id; `bit` nullopt matches every bit (and the whole bus). +std::vector find(ReviewAnchorKind kind, const InstancePath& path, + std::optional objectId, std::optional bit) { + std::vector found; + const auto ids = pathIds(path); + for (const auto& t : g_threads) { + if (t.state != ReviewState::Exact || t.anchor.kind != kind) continue; + if (t.anchor.idPath != ids) continue; + if (kind == ReviewAnchorKind::Term && t.anchor.termId != objectId) continue; + if (kind == ReviewAnchorKind::Net && t.anchor.netId != objectId) continue; + if (bit && t.anchor.bit != bit) continue; + found.push_back(&t); + } + return found; +} + +json withBit(json anchor, std::optional bit) { + if (bit) anchor["bit"] = *bit; + return anchor; +} +} // namespace + +void ReviewStore::setResponse(const json& response) { + g_hasReview = true; + g_writable = response.value("writable", false); + g_author = text(response, "author"); + g_error.clear(); + g_threads.clear(); + if (response.contains("threads") && response["threads"].is_array()) + for (const auto& t : response["threads"]) { + if (!t.is_object()) continue; + g_threads.push_back(parseThread(t)); + } + ++g_revision; +} + +void ReviewStore::clear() { + g_hasReview = false; + g_writable = false; + g_author.clear(); + g_error.clear(); + g_threads.clear(); + ++g_revision; +} + +bool ReviewStore::hasReview() { return g_hasReview; } +bool ReviewStore::writable() { return g_hasReview && g_writable; } +const std::string& ReviewStore::author() { return g_author; } +const std::vector& ReviewStore::threads() { return g_threads; } +unsigned ReviewStore::revision() { return g_revision; } + +std::vector ReviewStore::instanceThreads(const InstancePath& path) { + return find(ReviewAnchorKind::Instance, path, std::nullopt, std::nullopt); +} + +std::vector ReviewStore::termThreads(const InstancePath& path, + unsigned termId, + std::optional bit) { + return find(ReviewAnchorKind::Term, path, termId, bit); +} + +std::vector ReviewStore::netThreads(const InstancePath& path, + unsigned netId, + std::optional bit) { + return find(ReviewAnchorKind::Net, path, netId, bit); +} + +size_t ReviewStore::openCount(const std::vector& threads) { + size_t n = 0; + for (const auto* t : threads) + if (t->isOpen()) ++n; + return n; +} + +ImU32 ReviewStore::openColor(const std::vector& threads) { + std::optional worst; + for (const auto* t : threads) + if (t->isOpen() && (!worst || t->severity > *worst)) worst = t->severity; + return worst ? DiagnosisStore::colorForSeverity(*worst) : 0; +} + +void ReviewStore::setError(const std::string& message) { + g_error = message; + ++g_revision; +} + +const std::string& ReviewStore::error() { return g_error; } + +json ReviewStore::addThreadRequest(const json& anchor, const std::string& text, + DiagnosisSeverity severity, const std::string& title) { + json req = {{"request", "review_add_thread"}, {"anchor", anchor}, {"text", text}, + {"severity", severityName(severity)}}; + if (!title.empty()) req["title"] = title; + return req; +} + +json ReviewStore::addEventRequest(const std::string& thread, const std::string& text, + const std::string& status) { + json req = {{"request", "review_add_event"}, {"thread", thread}}; + if (!text.empty()) req["text"] = text; + if (!status.empty()) req["status"] = status; + return req; +} + +json ReviewStore::instanceAnchor(const InstancePath& path) { + json anchor = {{"kind", "instance"}}; + writePath(anchor, path); + return anchor; +} + +json ReviewStore::termAnchor(const InstancePath& path, unsigned termId, + const std::string& terminal, std::optional bit) { + json anchor = {{"kind", "term"}, {"term_id", termId}, {"terminal", terminal}}; + writePath(anchor, path); + return withBit(std::move(anchor), bit); +} + +json ReviewStore::netAnchor(const InstancePath& path, unsigned netId, const std::string& net, + std::optional bit) { + json anchor = {{"kind", "net"}, {"net_id", netId}, {"net", net}}; + writePath(anchor, path); + return withBit(std::move(anchor), bit); +} + +const char* toString(ReviewState state) { + switch (state) { + case ReviewState::Exact: return "exact"; + case ReviewState::OtherSnapshot: return "other netlist"; + case ReviewState::Unresolved: return "unresolved"; + } + return ""; +} + +std::string anchorLabel(const ReviewAnchor& anchor) { + if (anchor.kind == ReviewAnchorKind::View) return "view " + anchor.view; + std::string label; + for (size_t i = 0; i < anchor.path.size(); ++i) { + const auto& name = anchor.path[i]; + if (i) label += "/"; + label += name.empty() && i < anchor.idPath.size() + ? "<#" + std::to_string(anchor.idPath[i]) + ">" : name; + } + std::string object; + if (anchor.kind == ReviewAnchorKind::Term) object = anchor.terminal; + if (anchor.kind == ReviewAnchorKind::Net) + object = anchor.net.empty() && anchor.netId + ? "" : anchor.net; + if (!object.empty()) { + if (anchor.bit) object += "[" + std::to_string(*anchor.bit) + "]"; + label += label.empty() ? object : "/" + object; + } + return label.empty() ? std::string("(top)") : label; +} diff --git a/src/ReviewStore.h b/src/ReviewStore.h new file mode 100644 index 0000000..3556f3c --- /dev/null +++ b/src/ReviewStore.h @@ -0,0 +1,119 @@ +#pragma once + +#include +#include +#include + +#include + +#include "Types.h" + +// The review threads the host last pushed (review_response, see +// docs/review_format.md) -- same static/global state pattern as +// DiagnosisStore/TraceStore. The host owns the review files: the viewer only +// shows what it is sent and asks for writes (review_add_thread, +// review_add_event), answered by a fresh review_response. +// +// Threads are matched to netlist objects by ids only: a thread is drawn on +// an object only when the host resolved it "exact" on the loaded design, and +// then by its anchor's id_path and term/net id, never by name. + +enum class ReviewAnchorKind { Instance, Term, Net, View }; + +// docs/review_format.md "Resolving an anchor". +enum class ReviewState { Exact, OtherSnapshot, Unresolved }; + +struct ReviewAnchor { + ReviewAnchorKind kind = ReviewAnchorKind::Instance; + std::vector idPath; // the instance, or the one containing the term/net + std::optional termId; // Term only + std::optional netId; // Net only + std::optional bit; // Term/Net, optional: one bus bit + std::string view; // View only + // Labels, for display only. + std::vector path; + std::string terminal; + std::string net; +}; + +struct ReviewComment { + std::string id; + std::string author; + std::string date; + std::string text; +}; + +struct ReviewThread { + std::string id; + std::string title; + std::string author; + std::string source; // "human" or a tool name + std::string created; + std::string snapshot; + bool snapshotStored = false; + ReviewAnchor anchor; + ReviewState state = ReviewState::Unresolved; + // Exact only: the anchor's instance as it is now (ids and names). + std::optional resolvedPath; + std::string status = "open"; // "open" | "resolved" | "wontfix" + DiagnosisSeverity severity = DiagnosisSeverity::Info; + std::vector labels; + std::vector comments; + + bool isOpen() const { return status == "open"; } +}; + +class ReviewStore { + public: + // A review_response push replaces everything. + static void setResponse(const json& response); + // Forget the review (a fresh design: the host pushes the review again + // after its root_response). + static void clear(); + + // A review directory is configured on the host (a review_response came). + static bool hasReview(); + // The host has an author to write as. + static bool writable(); + static const std::string& author(); + static const std::vector& threads(); + // Bumped on every change, for views caching anything derived. + static unsigned revision(); + + // Exact threads on an object, every status. `path`: the instance + // (Instance), or the instance containing the term/net, as in + // NetlistTree::getInstancePath() and InstanceShape::path. `bit`: one bus + // bit, or nullopt for the whole object (all its bits included). + static std::vector instanceThreads(const InstancePath& path); + static std::vector termThreads(const InstancePath& path, unsigned termId, + std::optional bit = std::nullopt); + static std::vector netThreads(const InstancePath& path, unsigned netId, + std::optional bit = std::nullopt); + + // Severity color of the worst open thread, 0 when none is open. + static ImU32 openColor(const std::vector& threads); + static size_t openCount(const std::vector& threads); + + // The last refused write (an "error" reply to a review_* request), shown + // in the Review tab until the next review_response. + static void setError(const std::string& message); + static const std::string& error(); + + // Requests. `anchor` is the review_add_thread anchor JSON (ids, plus the + // name labels the viewer knows). + static json addThreadRequest(const json& anchor, const std::string& text, + DiagnosisSeverity severity, const std::string& title); + static json addEventRequest(const std::string& thread, const std::string& text, + const std::string& status); + + // Anchor JSON for an instance, or a term/net of an instance. + static json instanceAnchor(const InstancePath& path); + static json termAnchor(const InstancePath& path, unsigned termId, const std::string& terminal, + std::optional bit); + static json netAnchor(const InstancePath& path, unsigned netId, const std::string& net, + std::optional bit); +}; + +const char* toString(ReviewState state); +// A thread's object for people, e.g. "u1/u2/Q[3]" (labels only). +std::string anchorLabel(const ReviewAnchor& anchor); diff --git a/src/ReviewView.cpp b/src/ReviewView.cpp new file mode 100644 index 0000000..3556311 --- /dev/null +++ b/src/ReviewView.cpp @@ -0,0 +1,235 @@ +#include "ReviewView.h" + +#include + +#include "DiagnosisStore.h" +#include "INetlistProvider.h" +#include "ReviewStore.h" +#include "SelectionStore.h" + +namespace { +// What the Review tab lists. +enum class Filter { Open, All }; +Filter g_filter = Filter::Open; +std::string g_selected; // thread id +std::string g_replyFor; // thread the reply box was typed for +char g_reply[4096] = ""; + +// The Add Comment dialog. +bool g_composerRequested = false; +json g_composerAnchor; +std::string g_composerLabel; +char g_composerText[4096] = ""; +char g_composerTitle[256] = ""; +int g_composerSeverity = 0; // DiagnosisSeverity + +const char* kComposerTitle = "Add Comment"; +std::optional g_commentButton; + +ImVec4 severityColor(DiagnosisSeverity severity) { + return ImColor(DiagnosisStore::colorForSeverity(severity)).Value; +} + +const ReviewThread* findThread(const std::string& id) { + for (const auto& t : ReviewStore::threads()) + if (t.id == id) return &t; + return nullptr; +} + +std::string firstLine(const std::string& s) { + auto end = s.find('\n'); + return end == std::string::npos ? s : s.substr(0, end) + " ..."; +} + +std::string summary(const ReviewThread& t) { + if (!t.title.empty()) return t.title; + return t.comments.empty() ? std::string() : firstLine(t.comments.front().text); +} + +void sendEvent(INetlistProvider* provider, const ReviewThread& t, const std::string& text, + const std::string& status) { + provider->send(ReviewStore::addEventRequest(t.id, text, status).dump()); +} + +void renderDetail(INetlistProvider* provider, const ReviewThread& t) { + ImGui::SeparatorText(anchorLabel(t.anchor).c_str()); + ImGui::TextColored(severityColor(t.severity), "%s", toString(t.severity)); + ImGui::SameLine(); + ImGui::Text("%s", t.status.c_str()); + for (const auto& label : t.labels) { + ImGui::SameLine(); + ImGui::TextDisabled("#%s", label.c_str()); + } + if (t.state == ReviewState::OtherSnapshot) { + ImGui::TextWrapped("Written on another netlist: not shown on this design.%s", + t.snapshotStored ? "" : " Its snapshot isn't in the review."); + if (t.snapshotStored) { + ImGui::TextDisabled("naja-schematic --review DIR --snapshot %s", t.snapshot.c_str()); + ImGui::SameLine(); + if (ImGui::SmallButton("Copy Snapshot ID")) ImGui::SetClipboardText(t.snapshot.c_str()); + } + } else if (t.state == ReviewState::Unresolved) { + ImGui::TextWrapped("Its ids don't resolve in this netlist (an edited review file?)."); + } else if (t.resolvedPath && t.anchor.kind != ReviewAnchorKind::View) { + if (ImGui::SmallButton("Select")) SelectionStore::select(*t.resolvedPath, + SelectionStore::Origin::Host); + if (!t.resolvedPath->empty()) { + ImGui::SameLine(); + if (ImGui::SmallButton("Show in Schematic")) { + json req = {{"request", "resolve_instance"}}; + writePath(req, *t.resolvedPath); + provider->send(req.dump()); + } + } + } + + for (const auto& c : t.comments) { + ImGui::Spacing(); + ImGui::TextDisabled("%s %s", c.author.c_str(), c.date.c_str()); + if (!c.text.empty()) ImGui::TextWrapped("%s", c.text.c_str()); + } + + if (!ReviewStore::writable()) return; + ImGui::Spacing(); + if (g_replyFor != t.id) { + g_replyFor = t.id; + g_reply[0] = '\0'; + } + ImGui::InputTextMultiline("##reply", g_reply, sizeof(g_reply), + ImVec2(-1, ImGui::GetTextLineHeight() * 4)); + const bool hasText = g_reply[0] != '\0'; + ImGui::BeginDisabled(!hasText); + if (ImGui::Button("Reply")) { + sendEvent(provider, t, g_reply, ""); + g_reply[0] = '\0'; + } + ImGui::EndDisabled(); + ImGui::SameLine(); + // A status change carries the typed text, if any, as its comment. + if (t.isOpen()) { + if (ImGui::Button("Resolve")) { sendEvent(provider, t, g_reply, "resolved"); g_reply[0] = '\0'; } + ImGui::SameLine(); + if (ImGui::Button("Won't Fix")) { sendEvent(provider, t, g_reply, "wontfix"); g_reply[0] = '\0'; } + } else if (ImGui::Button("Reopen")) { + sendEvent(provider, t, g_reply, "open"); + g_reply[0] = '\0'; + } +} +} // namespace + +void ReviewView::render(INetlistProvider* provider) { + if (!ReviewStore::hasReview()) { + ImGui::TextWrapped("No review loaded. Start the host with --review DIR to read and " + "write review threads (docs/review_format.md)."); + return; + } + if (ReviewStore::writable()) + ImGui::Text("Commenting as %s", ReviewStore::author().c_str()); + else + ImGui::TextDisabled("Read-only: the host has no author (--author, NAJA_SCHEMATIC_AUTHOR " + "or git config user.name)"); + ImGui::SameLine(); + int filter = static_cast(g_filter); + ImGui::SetNextItemWidth(ImGui::GetFontSize() * 7); + if (ImGui::Combo("##filter", &filter, "Open\0All\0")) g_filter = static_cast(filter); + if (!ReviewStore::error().empty()) + ImGui::TextColored(severityColor(DiagnosisSeverity::Error), "%s", + ReviewStore::error().c_str()); + + const auto& threads = ReviewStore::threads(); + const ImGuiTableFlags flags = ImGuiTableFlags_RowBg | ImGuiTableFlags_BordersInnerV | + ImGuiTableFlags_Resizable | ImGuiTableFlags_ScrollY; + const float listH = ImGui::GetTextLineHeightWithSpacing() * 8; + if (ImGui::BeginTable("##threads", 5, flags, ImVec2(0, listH))) { + ImGui::TableSetupScrollFreeze(0, 1); + ImGui::TableSetupColumn("Severity", ImGuiTableColumnFlags_WidthFixed); + ImGui::TableSetupColumn("Object"); + ImGui::TableSetupColumn("Comment", ImGuiTableColumnFlags_WidthStretch, 2.0f); + ImGui::TableSetupColumn("Author"); + ImGui::TableSetupColumn("Status", ImGuiTableColumnFlags_WidthFixed); + ImGui::TableHeadersRow(); + for (const auto& t : threads) { + if (g_filter == Filter::Open && !t.isOpen()) continue; + ImGui::TableNextRow(); + ImGui::TableNextColumn(); + ImGui::PushID(t.id.c_str()); + if (ImGui::Selectable("##row", g_selected == t.id, + ImGuiSelectableFlags_SpanAllColumns)) { + g_selected = t.id; + if (t.resolvedPath) SelectionStore::select(*t.resolvedPath, SelectionStore::Origin::Host); + } + ImGui::PopID(); + ImGui::SameLine(); + ImGui::TextColored(severityColor(t.severity), "%s", toString(t.severity)); + ImGui::TableNextColumn(); + if (t.state == ReviewState::Exact) ImGui::TextUnformatted(anchorLabel(t.anchor).c_str()); + else ImGui::TextDisabled("%s (%s)", anchorLabel(t.anchor).c_str(), toString(t.state)); + ImGui::TableNextColumn(); + ImGui::TextUnformatted(summary(t).c_str()); + if (t.comments.size() > 1) { + ImGui::SameLine(); + ImGui::TextDisabled("+%zu", t.comments.size() - 1); + } + ImGui::TableNextColumn(); + ImGui::TextUnformatted(t.author.c_str()); + ImGui::TableNextColumn(); + ImGui::TextUnformatted(t.status.c_str()); + } + ImGui::EndTable(); + } + if (const auto* t = findThread(g_selected)) renderDetail(provider, *t); +} + +void ReviewView::openComposer(const json& anchor, const std::string& label) { + g_composerRequested = true; + g_composerAnchor = anchor; + g_composerLabel = label; + g_composerText[0] = '\0'; + g_composerTitle[0] = '\0'; + g_composerSeverity = static_cast(DiagnosisSeverity::Info); +} + +void ReviewView::renderComposer(INetlistProvider* provider) { + if (g_composerRequested) { + ImGui::OpenPopup(kComposerTitle); + g_composerRequested = false; + } + g_commentButton.reset(); + ImGui::SetNextWindowSize(ImVec2(ImGui::GetFontSize() * 30, 0), ImGuiCond_Appearing); + if (!ImGui::BeginPopupModal(kComposerTitle, nullptr)) return; + ImGui::TextWrapped("On %s", g_composerLabel.c_str()); + ImGui::InputText("Title", g_composerTitle, sizeof(g_composerTitle)); + ImGui::Combo("Severity", &g_composerSeverity, "info\0warning\0error\0"); + if (ImGui::IsWindowAppearing()) ImGui::SetKeyboardFocusHere(); + ImGui::InputTextMultiline("##text", g_composerText, sizeof(g_composerText), + ImVec2(-1, ImGui::GetTextLineHeight() * 8)); + ImGui::BeginDisabled(g_composerText[0] == '\0'); + const bool comment = ImGui::Button("Comment"); + const ImVec2 bmin = ImGui::GetItemRectMin(), bmax = ImGui::GetItemRectMax(); + g_commentButton = ImVec2((bmin.x + bmax.x) * 0.5f, (bmin.y + bmax.y) * 0.5f); + if (comment) { + provider->send(ReviewStore::addThreadRequest( + g_composerAnchor, g_composerText, + static_cast(g_composerSeverity), g_composerTitle).dump()); + ImGui::CloseCurrentPopup(); + } + ImGui::EndDisabled(); + ImGui::SameLine(); + if (ImGui::Button("Cancel") || ImGui::IsKeyPressed(ImGuiKey_Escape)) ImGui::CloseCurrentPopup(); + ImGui::EndPopup(); +} + +std::optional ReviewView::commentButtonForTesting() { return g_commentButton; } + +void ReviewView::commentMenuItem(const json& anchor, const std::string& label) { + if (!ReviewStore::writable()) return; + if (ImGui::MenuItem("Add Comment...")) openComposer(anchor, label); +} + +void ReviewView::threadsTooltip(const std::vector& threads) { + for (const auto* t : threads) { + ImGui::TextColored(severityColor(t->severity), "[%s] %s", t->status.c_str(), + summary(*t).c_str()); + ImGui::TextDisabled("%s, %zu comment(s)", t->author.c_str(), t->comments.size()); + } +} diff --git a/src/ReviewView.h b/src/ReviewView.h new file mode 100644 index 0000000..2764097 --- /dev/null +++ b/src/ReviewView.h @@ -0,0 +1,36 @@ +#pragma once + +#include +#include +#include + +#include + +#include "ReviewStore.h" + +class INetlistProvider; + +// The "Review" bottom-panel tab (the threads in ReviewStore: filter, read, +// reply, resolve) and the "Add Comment" dialog the tree and schematic menus +// open. Writes go to the host as review_* requests; what shows is always +// the host's next review_response. +class ReviewView { + public: + static void render(INetlistProvider* provider); + + // Opens the Add Comment dialog for an object: `anchor` is a + // ReviewStore::*Anchor(), `label` names the object in the dialog. + static void openComposer(const json& anchor, const std::string& label); + // The dialog, drawn once per frame at the top level (a modal popup). + static void renderComposer(INetlistProvider* provider); + + // "Add Comment..." menu item for a context menu: shown only when the + // host can write; opens the dialog when clicked. + static void commentMenuItem(const json& anchor, const std::string& label); + // Hover tooltip listing these threads (their first comment each). + static void threadsTooltip(const std::vector& threads); + + // Screen center of the dialog's Comment button in the last frame drawn + // (tests click it), or nullopt while the dialog is closed. + static std::optional commentButtonForTesting(); +}; diff --git a/src/SchematicView.cpp b/src/SchematicView.cpp index 77dbbe6..add091c 100644 --- a/src/SchematicView.cpp +++ b/src/SchematicView.cpp @@ -343,6 +343,28 @@ float instanceNameWidth(const std::string& name) { return ImGui::GetFont()->CalcTextSizeA(kInstanceLabelBaseSize, FLT_MAX, 0.0f, name.c_str()).x; } +// Open review threads on an instance: a disc with their count on the box's +// top-right corner, in the worst open severity's color. Drawn through the +// painter, so it reaches SVG exports; it hides with the labels when zoomed +// too far out to read. +static void drawReviewBadge(SchematicPainter* dl, const InstanceShape& inst, + const SchematicView& sv, + const ImVec2& canvasPos, const ImVec2& canvasSize) { + if (inst.reviewOpen <= 0 || inst.w <= 0.0f || inst.h <= 0.0f) return; + float fontSize = labelFontSize(kInstanceLabelBaseSize, sv.transform.scale); + if (fontSize <= 0.0f) return; + ImVec2 rmin, rmax; + sv.worldRectToScreen(inst.x, inst.y, inst.w, inst.h, canvasPos, canvasSize, rmin, rmax); + const std::string count = std::to_string(inst.reviewOpen); + ImFont* font = ImGui::GetFont(); + ImVec2 ts = font->CalcTextSizeA(fontSize, FLT_MAX, 0.0f, count.c_str()); + const float r = std::max(ts.x, ts.y) * 0.5f + 2.0f; + const ImVec2 c(rmax.x, rmin.y); + dl->AddCircleFilled(c, r, inst.reviewColor); + dl->AddText(font, fontSize, ImVec2(c.x - ts.x * 0.5f, c.y - ts.y * 0.5f), + contrastingTextColor(inst.reviewColor), count.c_str()); +} + // Instance name above its symbol: a box's inside holds its pin // names, and a gate symbol has no room for text. The layout reserved this // exact (possibly shortened) text's row, so it never overlaps anything. @@ -1186,7 +1208,10 @@ void SchematicView::render(SchematicPainter& painter, const ImVec2& canvasPos, c // which module the traced leaves belong to, and are listed parent-first // so nested frames paint over their enclosing one. for (const auto& inst : instances) { - if (inst.isHierGroup) drawInstance(dl, inst, canvasPos, canvasSize); + if (inst.isHierGroup) { + drawInstance(dl, inst, canvasPos, canvasSize); + drawReviewBadge(dl, inst, *this, canvasPos, canvasSize); + } } // Top-level nets draw next (so top-level instances render on top), as @@ -1203,6 +1228,7 @@ void SchematicView::render(SchematicPainter& painter, const ImVec2& canvasPos, c // wire under a box's fill or a child under its own internal wiring. std::function drawSubtree = [&](const InstanceShape& inst) { drawInstance(dl, inst, canvasPos, canvasSize); + drawReviewBadge(dl, inst, *this, canvasPos, canvasSize); for (const auto& net : nets) if (net.containerShapeId == inst.id) drawNet(dl, net, canvasPos, canvasSize); for (const auto& child : instances) diff --git a/src/Types.cpp b/src/Types.cpp index 998c653..4972bf1 100644 --- a/src/Types.cpp +++ b/src/Types.cpp @@ -121,6 +121,9 @@ void from_json(const json& j, NetsResponseJson& n) { if (child.contains("name") && !child["name"].is_null()) { net.name = child["name"].get(); } + if (child.contains("net_id") && child["net_id"].is_number_unsigned()) { + net.net_id = child["net_id"].get(); + } if (child.contains("msb") && !child["msb"].is_null()) { net.msb = child["msb"].get(); diff --git a/src/Types.h b/src/Types.h index 2b97eb3..ab264ca 100644 --- a/src/Types.h +++ b/src/Types.h @@ -294,6 +294,7 @@ struct TermsResponseJson { // NetlistTreeNetNode -- nets don't offer a "Show Equipotential" action). struct NetResponseJson { std::string name; + std::optional net_id; // SNLNet id in its design; absent from older hosts std::optional msb; std::optional lsb; bool has_sources = false; // the net, or any bit of a bus net @@ -482,6 +483,11 @@ struct InstanceShape { // The instance (or module frame) selected in the viewer -- see // SelectionStore. Drawn with a selection outline. bool selected = false; + // Open review threads on this instance (ReviewStore), drawn as a + // numbered badge on the box's top-right corner in `reviewColor` (the + // worst open severity's color). 0 = no badge. + int reviewOpen = 0; + ImU32 reviewColor = 0; }; // A net in the schematic (one routed wire tree), named by the pin its wires diff --git a/tests/NetlistTreeTest.cpp b/tests/NetlistTreeTest.cpp index 2c1d407..36bab0c 100644 --- a/tests/NetlistTreeTest.cpp +++ b/tests/NetlistTreeTest.cpp @@ -179,6 +179,30 @@ TEST(NetlistTree, ScalarNetIsBitNetNotBus) { EXPECT_TRUE(net->busBits().empty()); } +// nets_response's net_id, what a review thread anchors a net by; a bit row +// has its bus's. An older host sends none. +TEST(NetlistTree, NetNodesKeepTheHostsNetId) { + FakeNetlistProvider provider; + NetlistTree tree(&provider); + tree.createRootNode("top", someDesign(), false, false, false, true); + auto* root = tree.getRoot(); + + root->createChildren(); + root->createNetNode("bus", 1, 0, false, 5u); + root->createNetNode("old", std::nullopt, std::nullopt); + auto* bus = tree.getNode(1); + auto* old = tree.getNode(2); + ASSERT_NE(bus, nullptr); + ASSERT_NE(old, nullptr); + EXPECT_EQ(bus->getNetID(), 5u); + EXPECT_FALSE(old->getNetID()); + bus->expand(); + auto* bit = tree.getNode(3); + ASSERT_NE(bit, nullptr); + EXPECT_TRUE(bit->isBusBit()); + EXPECT_EQ(bit->getNetID(), 5u); +} + TEST(NetlistTree, SendLoadEquipotentialFormatsRequestAndFiresCallback) { FakeNetlistProvider provider; NetlistTree tree(&provider); diff --git a/tests/ReviewStoreTest.cpp b/tests/ReviewStoreTest.cpp new file mode 100644 index 0000000..3f6a66e --- /dev/null +++ b/tests/ReviewStoreTest.cpp @@ -0,0 +1,126 @@ +#include "DiagnosisStore.h" +#include "ReviewStore.h" + +#include + +namespace { + +// What python/naja_schematic/review.py pushes (docs/review_format.md): an +// instance thread, a term thread on bus bit d[1], a net thread on an +// anonymous net, a resolved one, and one written on another netlist. +const char* kResponse = R"({ + "response": "review_response", "author": "Jane", "writable": true, + "snapshot": "aaaa", "views": [], + "threads": [ + {"id": "t1", "title": "", "author": "Jane", "source": "human", "created": "2026-10-31T14:25:30.123Z", + "snapshot": "aaaa", "snapshot_stored": true, "view": null, + "anchor": {"kind": "instance", "id_path": [0, 0], "path": ["u_sub", "u_and"]}, + "resolution": {"state": "exact", "id_path": [0, 0], "path": ["u_sub", "u_and"]}, + "status": "open", "severity": "error", "labels": ["lut"], + "comments": [{"id": "e1", "author": "Jane", "date": "2026-10-31T14:25:30.123Z", "text": "LUT looks wrong\nsee INIT"}, + {"id": "e2", "author": "John", "date": "2026-10-31T14:26:00.000Z", "text": "Agreed"}]}, + {"id": "t2", "author": "kepler-formal", "source": "kepler-formal", "snapshot": "aaaa", + "anchor": {"kind": "term", "id_path": [], "path": [], "term_id": 2, "terminal": "d", "bit": 1}, + "resolution": {"state": "exact", "id_path": [], "path": []}, + "status": "open", "severity": "warning", "comments": []}, + {"id": "t3", "author": "Jane", "snapshot": "aaaa", + "anchor": {"kind": "net", "id_path": [0], "path": ["u_sub"], "net_id": 5, "net": ""}, + "resolution": {"state": "exact", "id_path": [0], "path": ["u_sub"]}, + "status": "resolved", "severity": "error", "comments": []}, + {"id": "t4", "author": "Jane", "snapshot": "bbbb", "snapshot_stored": false, + "anchor": {"kind": "instance", "id_path": [0, 0], "path": ["u_sub", "u_and"]}, + "resolution": {"state": "other_snapshot"}, + "status": "open", "severity": "error", "comments": []} + ]})"; + +InstancePath uAnd() { return {{0, "u_sub"}, {0, "u_and"}}; } + +class Review : public ::testing::Test { + protected: + void SetUp() override { ReviewStore::setResponse(json::parse(kResponse)); } + void TearDown() override { ReviewStore::clear(); } +}; + +TEST_F(Review, ParsesTheHostsResponse) { + EXPECT_TRUE(ReviewStore::hasReview()); + EXPECT_TRUE(ReviewStore::writable()); + EXPECT_EQ(ReviewStore::author(), "Jane"); + const auto& threads = ReviewStore::threads(); + ASSERT_EQ(threads.size(), 4u); + const auto& t1 = threads[0]; + EXPECT_EQ(t1.state, ReviewState::Exact); + EXPECT_EQ(t1.severity, DiagnosisSeverity::Error); + EXPECT_EQ(t1.labels, std::vector{"lut"}); + ASSERT_EQ(t1.comments.size(), 2u); + EXPECT_EQ(t1.comments[1].author, "John"); + ASSERT_TRUE(t1.resolvedPath); + EXPECT_EQ(*t1.resolvedPath, uAnd()); + EXPECT_EQ(threads[1].anchor.termId, 2u); + EXPECT_EQ(threads[1].anchor.bit, 1); + EXPECT_EQ(threads[3].state, ReviewState::OtherSnapshot); + EXPECT_FALSE(threads[3].resolvedPath); +} + +TEST_F(Review, MatchesObjectsByIdsOnlyAndOnlyExactThreads) { + // t4 has the same ids but was written on another netlist: never matched. + auto onAnd = ReviewStore::instanceThreads(uAnd()); + ASSERT_EQ(onAnd.size(), 1u); + EXPECT_EQ(onAnd[0]->id, "t1"); + // Names don't matter, ids do. + EXPECT_EQ(ReviewStore::instanceThreads({{0, "renamed"}, {0, "x"}}).size(), 1u); + EXPECT_TRUE(ReviewStore::instanceThreads({{0, "u_sub"}, {1, "u_and"}}).empty()); + + EXPECT_EQ(ReviewStore::termThreads({}, 2, 1).size(), 1u); + EXPECT_TRUE(ReviewStore::termThreads({}, 2, 0).empty()); + EXPECT_EQ(ReviewStore::termThreads({}, 2).size(), 1u); // the bus row: any bit + EXPECT_TRUE(ReviewStore::termThreads({}, 3).empty()); + + EXPECT_EQ(ReviewStore::netThreads({{0, "u_sub"}}, 5).size(), 1u); + EXPECT_TRUE(ReviewStore::netThreads({}, 5).empty()); +} + +TEST_F(Review, BadgesCountOpenThreadsInTheWorstSeverityColor) { + auto net = ReviewStore::netThreads({{0, "u_sub"}}, 5); + EXPECT_EQ(ReviewStore::openCount(net), 0u); // resolved + EXPECT_EQ(ReviewStore::openColor(net), 0u); + auto bus = ReviewStore::termThreads({}, 2); + EXPECT_EQ(ReviewStore::openCount(bus), 1u); + EXPECT_EQ(ReviewStore::openColor(bus), DiagnosisStore::colorForSeverity(DiagnosisSeverity::Warning)); +} + +TEST_F(Review, LabelsNameTheObjectForPeople) { + const auto& threads = ReviewStore::threads(); + EXPECT_EQ(anchorLabel(threads[0].anchor), "u_sub/u_and"); + EXPECT_EQ(anchorLabel(threads[1].anchor), "d[1]"); + EXPECT_EQ(anchorLabel(threads[2].anchor), "u_sub/"); +} + +TEST_F(Review, BuildsRequestsWithIdsAndLabels) { + json anchor = ReviewStore::termAnchor({{3, "u1"}, {7, ""}}, 4, "Q", 2); + EXPECT_EQ(anchor, json::parse(R"({"kind": "term", "id_path": [3, 7], "path": ["u1", ""], + "term_id": 4, "terminal": "Q", "bit": 2})")); + EXPECT_EQ(ReviewStore::netAnchor({}, 9, "", std::nullopt), + json::parse(R"({"kind": "net", "id_path": [], "path": [], "net_id": 9, "net": ""})")); + json req = ReviewStore::addThreadRequest(ReviewStore::instanceAnchor(uAnd()), "look", + DiagnosisSeverity::Warning, ""); + EXPECT_EQ(req["request"], "review_add_thread"); + EXPECT_EQ(req["severity"], "warning"); + EXPECT_FALSE(req.contains("title")); + EXPECT_EQ(req["anchor"]["id_path"], json::parse("[0, 0]")); + EXPECT_EQ(ReviewStore::addEventRequest("t1", "", "resolved"), + json::parse(R"({"request": "review_add_event", "thread": "t1", "status": "resolved"})")); +} + +TEST_F(Review, ClearedForAFreshDesignAndReadOnlyWithoutAuthor) { + ReviewStore::clear(); + EXPECT_FALSE(ReviewStore::hasReview()); + EXPECT_TRUE(ReviewStore::threads().empty()); + ReviewStore::setResponse(json::parse(R"({"response": "review_response", "writable": false, + "threads": [{"id": "x", "anchor": 3}]})")); + EXPECT_TRUE(ReviewStore::hasReview()); + EXPECT_FALSE(ReviewStore::writable()); + ASSERT_EQ(ReviewStore::threads().size(), 1u); // malformed parts are defaulted + EXPECT_EQ(ReviewStore::threads()[0].state, ReviewState::Unresolved); +} + +} // namespace diff --git a/tests/ReviewViewTest.cpp b/tests/ReviewViewTest.cpp new file mode 100644 index 0000000..dcf78f9 --- /dev/null +++ b/tests/ReviewViewTest.cpp @@ -0,0 +1,76 @@ +// Headless ImGui test of the Add Comment dialog (same harness as +// EquipotentialViewTest): what it sends to the host. +#include "ReviewStore.h" +#include "ReviewView.h" + +#include +#include + +#include + +#include "FakeNetlistProvider.h" + +namespace { + +class CommentDialog : public ::testing::Test { + protected: + void SetUp() override { + ImGui::CreateContext(); + ImGuiIO& io = ImGui::GetIO(); + io.DisplaySize = ImVec2(1200.f, 800.f); + io.DeltaTime = 1.f / 60.f; + io.IniFilename = nullptr; + unsigned char* px; int w, h; + io.Fonts->GetTexDataAsRGBA32(&px, &w, &h); + } + void TearDown() override { + ReviewStore::clear(); + ImGui::DestroyContext(); + } + void frame(int n = 1) { + for (int i = 0; i < n; ++i) { + ImGui::NewFrame(); + ReviewView::renderComposer(&provider); + ImGui::Render(); + } + } + void click(ImVec2 at) { + ImGui::GetIO().AddMousePosEvent(at.x, at.y); + frame(); + ImGui::GetIO().AddMouseButtonEvent(ImGuiMouseButton_Left, true); + frame(); + ImGui::GetIO().AddMouseButtonEvent(ImGuiMouseButton_Left, false); + frame(); + } + FakeNetlistProvider provider; +}; + +TEST_F(CommentDialog, SendsTheThreadAnchoredByIds) { + const json anchor = ReviewStore::termAnchor({{0, "u_sub"}}, 2, "a", std::nullopt); + ReviewView::openComposer(anchor, "u_sub/a"); + frame(2); + ASSERT_TRUE(ReviewView::commentButtonForTesting()); + // The text box has the keyboard focus when the dialog opens. + ImGui::GetIO().AddInputCharactersUTF8("glitch here"); + frame(); + click(*ReviewView::commentButtonForTesting()); + + ASSERT_EQ(provider.sent.size(), 1u); + json req = json::parse(provider.sent[0]); + EXPECT_EQ(req["request"], "review_add_thread"); + EXPECT_EQ(req["anchor"], anchor); + EXPECT_EQ(req["text"], "glitch here"); + EXPECT_EQ(req["severity"], "info"); + frame(); + EXPECT_FALSE(ReviewView::commentButtonForTesting()); // closed +} + +TEST_F(CommentDialog, SendsNothingWithoutText) { + ReviewView::openComposer(ReviewStore::instanceAnchor({}), "the top design"); + frame(2); + ASSERT_TRUE(ReviewView::commentButtonForTesting()); + click(*ReviewView::commentButtonForTesting()); // disabled + EXPECT_TRUE(provider.sent.empty()); +} + +} // namespace diff --git a/tests/provider/LocalSNLProviderProtocolTest.cpp b/tests/provider/LocalSNLProviderProtocolTest.cpp index a47249b..ee53bf8 100644 --- a/tests/provider/LocalSNLProviderProtocolTest.cpp +++ b/tests/provider/LocalSNLProviderProtocolTest.cpp @@ -284,6 +284,7 @@ TEST_F(ProviderProtocol, ListsTheLegacyLocationWhenNoTypedReference) { "provider": "", "language": "", "representation": ""}])")); EXPECT_FALSE(nets["children"][0].contains("has_sources")); // net i EXPECT_EQ(nets["children"][1]["has_sources"], true); // net o + EXPECT_EQ(nets["children"][1]["net_id"], net->getID()); // what reviews anchor by } // A big file (a gate-level netlist) only sends the lines around the reference.