diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala index 945a9fd8..e5b27d8c 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala @@ -41,7 +41,7 @@ object Args: if name.isEmpty then Left("empty flag: '--'") else if name.contains('=') then val (k, v) = name.span(_ != '=') - Right(()).flatMap(_ => loop(tail, positional, values + (k -> v.drop(1)), switches)) + loop(tail, positional, values + (k -> v.drop(1)), switches) else if ValueFlags.contains(name) then tail match case v :: more if !v.startsWith("--") => diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala index 5d7d1ef5..4537ba6c 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala @@ -20,6 +20,24 @@ import org.jpablo.graphexplorer.gxcore.rpc.ChannelError import java.nio.file.{Path, Paths} import scala.util.control.NonFatal +/** Why a reference resolved to no single diagram. Two cases, not one: "you + * typed a name nothing has" and "you typed a name several things share" need + * different answers from the user. + */ +private enum RefError derives CanEqual: + case NotFound(ref: String) + case Ambiguous(ref: String, matches: Vector[Diagram]) + +/** What one reconciliation did. + * + * `failure` is a store write that did NOT land. It used to be discarded — + * `env.store.save(updated)` with the Either thrown away at three sites — so + * `gx sync` printed Behind/Ahead and exited 0 after failing to persist the + * record it had just reconciled. The next run then redid the same work from + * the same stale baseline, silently, forever. + */ +private case class SyncOutcome(diagram: Diagram, state: SyncState, failure: Option[String]) + /** What a reference on the command line turned out to mean. */ private enum Target derives CanEqual: case InLibrary(diagram: Diagram) @@ -29,7 +47,7 @@ private enum Target derives CanEqual: * writing a file needs no registration first. v1 could not do this — `get` * failed with "path is not currently watched" until you had called `watch`. */ - case OnDisk(path: Path, origin: OriginUri) + case OnDisk(path: Path) /** The `gx` command surface (§8). * @@ -218,13 +236,10 @@ object Cli: createdAt = env.now(), updatedAt = env.now() ) - env.store.initialize() - env.store.save(d) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(saved) => - env.audit.record(AuditEvent.Allowed(resolved.toString, "import")) - printDiagram(saved, args, env) - ExitCode.Ok + persist(d, env): saved => + env.audit.record(AuditEvent.Allowed(resolved.toString, "import")) + printDiagram(saved, args, env) + ExitCode.Ok // ----------------------------------------------------------------- ls @@ -250,7 +265,7 @@ object Cli: case Target.InLibrary(d) => if args.json then env.out(summaryJson(d).render(indent = 2)) else env.out(d.text) ExitCode.Ok - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => Documents.read(path) match case Left(err) => env.err(s"gx: ${describe(err)}") @@ -282,7 +297,7 @@ object Cli: */ private def applyText(target: Target, text: String, args: Args, env: CliEnv, source: String): Int = target match - case Target.OnDisk(path, _) => writeFile(path, text, args, env, source) + case Target.OnDisk(path) => writeFile(path, text, args, env, source) case Target.InLibrary(d) => val updated = d.copy(text = text, updatedAt = env.now()) d.binding.filter(b => b.mode.pushes) match @@ -291,9 +306,8 @@ object Cli: // never writes them back (§5.3). Saving the record is the whole // operation, and the divergence it creates is reported by // `gx sync` rather than resolved behind the user's back. - env.store.save(updated) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => reportSet(updated, wroteOrigin = false, args, env); ExitCode.Ok + persist(updated, env): _ => + reportSet(updated, wroteOrigin = false, args, env); ExitCode.Ok case Some(binding) => binding.origin.filePath.map(Paths.get(_)) match case None => @@ -311,12 +325,10 @@ object Cli: text = doc.text, binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now())) ) - env.store.save(synced) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => - env.audit.record(AuditEvent.Written(path.toString, doc.hash, source)) - reportSet(synced, wroteOrigin = true, args, env) - ExitCode.Ok + persist(synced, env): _ => + env.audit.record(AuditEvent.Written(path.toString, doc.hash, source)) + reportSet(synced, wroteOrigin = true, args, env) + ExitCode.Ok private def writeFile(path: Path, text: String, args: Args, env: CliEnv, source: String): Int = checkPolicy(path, env) match @@ -352,11 +364,9 @@ object Cli: modeOf(args, default = SyncMode.Pull) match case Left(why) => env.err(s"gx: $why"); ExitCode.Usage case Right(mode) => - findInLibrary(ref, env) match - case None => - env.err(s"gx: no diagram matches '$ref'") - ExitCode.InvalidPathOrPolicy - case Some(d) => + resolveRef(ref, env) match + case Left(err) => reportRef(err, env) + case Right(d) => val path = env.cwd.resolve(rawPath) checkPolicy(path, env) match case Left(code) => code @@ -376,73 +386,105 @@ object Cli: binding = Some(Binding(origin, mode, base, env.now())), updatedAt = env.now() ) - env.store.save(bound) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => printDiagram(bound, args, env); ExitCode.Ok + persist(bound, env): _ => + printDiagram(bound, args, env); ExitCode.Ok case _ => env.err("gx: bind needs a diagram and a path") ExitCode.Usage private def unbind(args: Args, env: CliEnv): Int = - args.positionalAt(0).flatMap(findInLibrary(_, env)) match + args.positionalAt(0) match case None => env.err("gx: unbind needs a diagram") ExitCode.InvalidPathOrPolicy - case Some(d) => - val detached = d.copy(binding = None, updatedAt = env.now()) - env.store.save(detached) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => printDiagram(detached, args, env); ExitCode.Ok + case Some(ref) => + resolveRef(ref, env) match + case Left(err) => reportRef(err, env) + case Right(d) => + val detached = d.copy(binding = None, updatedAt = env.now()) + persist(detached, env): _ => + printDiagram(detached, args, env); ExitCode.Ok // --------------------------------------------------------------- sync private def sync(args: Args, env: CliEnv): Int = - val targets = - if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) - else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) + val (unknown, found) = selectedRefs(args, env) + // Report every bad ref, not just the first: a script passing ten names + // wants all the typos at once. + if unknown.nonEmpty then unknown.map(reportRef(_, env)).last + else syncTargets(found.filter(_.isBound), args, env) + private def syncTargets(targets: Vector[Diagram], args: Args, env: CliEnv): Int = if targets.isEmpty then env.out(if args.json then "[]" else "(nothing bound to sync)") ExitCode.Ok else val results = targets.map(syncOne(_, env)) - val diverged = results.count(_._2 == SyncState.Diverged) + val diverged = results.count(_.state == SyncState.Diverged) + val failures = results.flatMap(o => o.failure.map(o.diagram.id.value -> _)) if args.json then env.out( ujson.Arr - .from(results.map((d, state) => - ujson.Obj("id" -> d.id.value, "state" -> state.toString) + .from(results.map(o => + ujson.Obj("id" -> o.diagram.id.value, "state" -> o.state.toString) )) .render(indent = 2) ) - else for (d, state) <- results do env.out(f"${d.id.value}%-28s $state") - // Divergence is a state, not an error (§5.2) — but a script that just - // pushed and wants to know whether it landed deserves a non-zero code. - if diverged > 0 then ExitCode.Conflict else ExitCode.Ok + else for o <- results do env.out(f"${o.diagram.id.value}%-28s ${o.state}") + + for (id, why) <- failures do env.err(s"gx: $id reconciled but could not be saved: $why") + + // A write that did not land outranks divergence: divergence is a state + // the user can act on, an unsaved record is one they have not been told + // about. Divergence is a state, not an error (§5.2) — but a script that + // just pushed and wants to know whether it landed deserves a non-zero code. + if failures.nonEmpty then ExitCode.Unknown + else if diverged > 0 then ExitCode.Conflict + else ExitCode.Ok + + private def syncOne(d: Diagram, env: CliEnv): SyncOutcome = + // Save, and KEEP the answer. Every early return below goes through here. + def store(updated: Diagram, state: SyncState): SyncOutcome = + env.store.save(updated) match + case Left(e) => SyncOutcome(updated, state, Some(e.toString)) + case Right(saved) => SyncOutcome(saved, state, None) - private def syncOne(d: Diagram, env: CliEnv): (Diagram, SyncState) = d.binding match - case None => (d, SyncState.InSync) + case None => SyncOutcome(d, SyncState.InSync, None) case Some(binding) => val path = binding.origin.filePath.map(Paths.get(_)) - val remote = path.flatMap(Documents.hashOf) - val local = Hashing.ofText(d.text, LineEnding.Lf) - val state = SyncState.of(binding.baseHash, local, remote) + // ONE read of the origin: it answers both questions below, and the Pull + // branch reuses it rather than reading and re-hashing the same bytes. + val origin = path.flatMap(Documents.read(_).toOption) + val remote = origin.map(_.hash) + + // `base` and `remote` are hashes of FILE BYTES, so `local` has to be + // measured the same way: the record's text as it would be written into + // THIS file, using the convention that file already uses (V-04). + // + // Hashing with a fixed LF made every CRLF-authored origin read `Ahead` + // forever — nothing had been edited, the bytes simply could not agree — + // and made a byte-identical regeneration land on `Diverged` instead of + // `Converged`, which is the conflict machine SyncState.Converged exists + // to prevent. Hashing.ofText demands the convention explicitly for this + // exact reason; see its scaladoc and V-16. + // + // With no origin on disk the state is OriginMissing whatever `local` + // says, and Lf is what Documents.create would write if it reappears. + val local = Hashing.ofText(d.text, origin.map(_.lineEnding).getOrElse(LineEnding.Lf)) + val state = SyncState.of(binding.baseHash, local, remote) binding.mode.autoAction(state) match case Some(SyncAction.Pull) => - (for - p <- path - doc <- Documents.read(p).toOption + (for doc <- origin yield val updated = d.copy( text = doc.text, binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now())), updatedAt = env.now() ) - env.store.save(updated) - (updated, state) - ).getOrElse((d, state)) + store(updated, state) + ).getOrElse(SyncOutcome(d, state, None)) case Some(SyncAction.Push) => (for @@ -451,25 +493,24 @@ object Cli: yield val updated = d.copy(binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now()))) - env.store.save(updated) env.audit.record(AuditEvent.Written(p.toString, doc.hash, "sync")) - (updated, state) - ).getOrElse((d, state)) + store(updated, state) + ).getOrElse(SyncOutcome(d, state, None)) case Some(SyncAction.AdvanceBase) => // Converged: both sides moved to the same content, so only the // agreed baseline is stale. No I/O — this is what stops a // byte-identical regeneration from looking like a change. val updated = d.copy(binding = Some(binding.copy(baseHash = local, lastSyncAt = env.now()))) - env.store.save(updated) - (updated, state) + store(updated, state) case None => if state == SyncState.Diverged then env.audit.record( AuditEvent.Conflict(binding.origin.value, binding.baseHash, local, "sync") ) - (d, state) + // Nothing was written, so there is nothing that could fail to save. + SyncOutcome(d, state, None) // -------------------------------------------------------------- watch @@ -482,43 +523,53 @@ object Cli: * discarded"). */ private def watch(args: Args, env: CliEnv): Int = - val diagrams = - if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) - else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) - - val fromPaths = args.positional.filter(r => findInLibrary(r, env).isEmpty).map: raw => - FileOrigins.originOf(env.cwd.resolve(raw), env.cwd) - - val origins = (diagrams.flatMap(_.binding.map(_.origin)) ++ fromPaths).distinct - - if origins.isEmpty then - env.err("gx: nothing to watch") - ExitCode.Usage + // Resolve each ref ONCE. This used to run findInLibrary twice per argument + // — once to collect the diagrams, once to test emptiness — and each call + // scans the whole library. + val (unresolved, found) = selectedRefs(args, env) + val diagrams = found.filter(_.isBound) + + // Only "nothing matched" can mean "this is a path"; an ambiguous ref is a + // ref, and watching a FILE of that name is not what was asked for. + val (ambiguous, missing) = unresolved.partitionMap: + case err: RefError.Ambiguous => Left(err) + case RefError.NotFound(ref) => Right(ref) + + // A ref that is not in the library is a PATH, and every other path-taking + // command runs it past the access policy first. watch did not, which made + // it the one way to point gx at a file the policy forbids. + val (denied, allowed) = missing.partitionMap(raw => checkPolicy(env.cwd.resolve(raw), env)) + + if ambiguous.nonEmpty then ambiguous.map(reportRef(_, env)).last + else if denied.nonEmpty then denied.head else - if args.has("open") && !env.desktopRunning() then - env.err("gx: --open needs a running desktop; watching anyway") - - val interval = args.value("interval").flatMap(_.toLongOption).getOrElse(50L) - val registry = WatchRegistry(env.audit, debounceMs = interval) - origins.foreach(registry.watch) - for o <- origins do env.err(s"watching ${o.value}") + val fromPaths = allowed.map(FileOrigins.originOf(_, env.cwd)) + val origins = (diagrams.flatMap(_.binding.map(_.origin)) ++ fromPaths).distinct - while env.keepWatching() do - for event <- registry.poll() do emitWatchEvent(event, args, env) - env.sleep(interval) - ExitCode.Ok + if origins.isEmpty then + env.err("gx: nothing to watch") + ExitCode.Usage + else + if args.has("open") && !env.desktopRunning() then + env.err("gx: --open needs a running desktop; watching anyway") + + val interval = args.value("interval").flatMap(_.toLongOption).getOrElse(50L) + val registry = WatchRegistry(env.audit, debounceMs = interval) + origins.foreach(registry.watch) + for o <- origins do env.err(s"watching ${o.value}") + + while env.keepWatching() do + for event <- registry.poll() do emitWatchEvent(event, args, env) + env.sleep(interval) + ExitCode.Ok private def emitWatchEvent(event: WatchEvent, args: Args, env: CliEnv): Unit = val (kind, uri, hash) = event match - case WatchEvent.Changed(u, h) => ("changed", u, Some(h)) - case WatchEvent.Restored(u, h) => ("restored", u, Some(h)) - case WatchEvent.Deleted(u, h) => ("deleted", u, Some(h)) + case WatchEvent.Changed(u, h) => ("changed", u, h) + case WatchEvent.Restored(u, h) => ("restored", u, h) + case WatchEvent.Deleted(u, h) => ("deleted", u, h) if args.json then - env.out( - ujson - .Obj("event" -> kind, "origin" -> uri.value, "hash" -> hash.map(_.hex).getOrElse("")) - .render() - ) + env.out(ujson.Obj("event" -> kind, "origin" -> uri.value, "hash" -> hash.hex).render()) else env.out(s"$kind\t${uri.value}") // ---------------------------------------------------------------- run @@ -534,9 +585,7 @@ object Cli: if args.has("list") then // Discoverability is part of the vocabulary being a vocabulary: a name // nobody can enumerate is not addressable in any useful sense. - if args.json then env.out(ujson.Arr.from(AnyCommand.names.map(ujson.Str(_))).render(indent = 2)) - else AnyCommand.names.foreach(env.out) - ExitCode.Ok + listNames(AnyCommand.names, args, env) else (args.positionalAt(0), args.positionalAt(1)) match case (Some(_), Some(commandName)) => @@ -572,7 +621,7 @@ object Cli: */ private def executeRecord(target: Target, command: RecordCommand, args: Args, env: CliEnv): Int = target match - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => env.err(s"gx: '${path.getFileName}' is not in the library, so it has no record to change") env.err(s"gx: import it first: gx import ${path.getFileName}") ExitCode.InvalidPathOrPolicy @@ -591,12 +640,10 @@ object Cli: // Metadata only: the record is saved, and the ORIGIN is untouched // whatever the sync mode says. That is §5.3.1's split doing its job // — hiding a node must never make a regenerating origin conflict. - env.store.save(updated.copy(updatedAt = env.now())) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(saved) => - if args.json then env.out(summaryJson(saved).render(indent = 2)) - else env.out(s"${saved.id.value} ${command.name}") - ExitCode.Ok + persist(updated.copy(updatedAt = env.now()), env): saved => + if args.json then env.out(summaryJson(saved).render(indent = 2)) + else env.out(s"${saved.id.value} ${command.name}") + ExitCode.Ok private def executeDocument(target: Target, command: DocumentCommand, args: Args, env: CliEnv): Int = textOf(target, env) match @@ -654,7 +701,7 @@ object Cli: private def textOf(target: Target, env: CliEnv): Either[Int, String] = target match case Target.InLibrary(d) => Right(d.text) - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => Documents.read(path) match case Right(doc) => Right(doc.text) case Left(err) => @@ -690,9 +737,7 @@ object Cli: */ private def sessionCommand(args: Args, env: CliEnv): Int = if args.has("list") then - if args.json then env.out(ujson.Arr.from(SessionCommand.names.map(ujson.Str(_))).render(indent = 2)) - else SessionCommand.names.foreach(env.out) - ExitCode.Ok + listNames(SessionCommand.names, args, env) else args.positionalAt(0) match case None => @@ -804,16 +849,17 @@ object Cli: */ private def pathToShow(args: Args, env: CliEnv): Either[Int, Path] = val ref = args.positional.head - findInLibrary(ref, env) match - case Some(d) => + resolveRef(ref, env) match + case Left(err: RefError.Ambiguous) => Left(reportRef(err, env)) + case Right(d) => d.binding.flatMap(_.origin.filePath).map(Paths.get(_)) match case Some(path) => Right(path) case None => env.err(s"gx: '${d.name}' is not bound to a file, so there is nothing to open") env.err(s"gx: bind it first: gx bind ${d.id.value} ") Left(ExitCode.InvalidPathOrPolicy) - case None => - checkPolicy(env.cwd.resolve(ref), env).left.map(identity) + case Left(_: RefError.NotFound) => + checkPolicy(env.cwd.resolve(ref), env) // -------------------------------------------------------------- skill @@ -891,34 +937,93 @@ object Cli: env.err("gx: this command needs a diagram or a path") ExitCode.Usage case Some(ref) => - findInLibrary(ref, env) match - case Some(d) => f(Target.InLibrary(d)) - case None => + resolveRef(ref, env) match + case Right(d) => f(Target.InLibrary(d)) + // Only "nothing matched" can mean "this is a path". An ambiguous ref + // falling through here is how `gx set ` ended up writing + // to a FILE of that name instead of refusing. + case Left(err: RefError.Ambiguous) => reportRef(err, env) + case Left(_: RefError.NotFound) => val path = env.cwd.resolve(ref) checkPolicy(path, env) match case Left(code) => code - case Right(resolved) => f(Target.OnDisk(resolved, FileOrigins.originOf(resolved, env.cwd))) + case Right(resolved) => f(Target.OnDisk(resolved)) /** id, then exact name, then the origin path. Ambiguity is reported rather * than resolved by picking one, because "gx set" on the wrong diagram is not * a mistake the user can see happening. + * + * That promise used to be unkept: this returned None for "nothing matched" + * AND for "several matched", so every caller rendered both as "no diagram + * matches" — and the ones that fall back to treating the ref as a PATH did + * so on an ambiguous name, which is the wrong-diagram write the paragraph + * above is about. */ - private def findInLibrary(ref: String, env: CliEnv): Option[Diagram] = + private def resolveRef(ref: String, env: CliEnv): Either[RefError, Diagram] = val all = env.store.list() - all - .find(_.id.value == ref) - .orElse: - all.filter(_.name == ref) match - case Vector(one) => Some(one) - case _ => None - .orElse: - val origin = FileOrigins.originOf(env.cwd.resolve(ref), env.cwd) - all.filter(_.binding.exists(_.origin == origin)) match - case Vector(one) => Some(one) - case _ => None + lazy val byName = all.filter(_.name == ref) + lazy val byOrigin = + val origin = FileOrigins.originOf(env.cwd.resolve(ref), env.cwd) + all.filter(_.binding.exists(_.origin == origin)) + + all.find(_.id.value == ref) match + case Some(d) => Right(d) + case None => + // Tier order is preserved: a unique name beats an origin match, and an + // origin match still answers when the name tier found nothing usable. + (byName, byOrigin) match + case (Vector(one), _) => Right(one) + case (_, Vector(one)) => Right(one) + case (Vector(), Vector()) => Left(RefError.NotFound(ref)) + case (many, others) => Left(RefError.Ambiguous(ref, (many ++ others).distinct)) + + /** True when the ref names something in the library. Callers that fall back + * to a path use this; an AMBIGUOUS ref is not a path and must not fall + * through, so they check [[resolveRef]] rather than this. + */ + private def findInLibrary(ref: String, env: CliEnv): Option[Diagram] = + resolveRef(ref, env).toOption + + private def reportRef(err: RefError, env: CliEnv): Int = err match + case RefError.NotFound(ref) => + env.err(s"gx: no diagram matches '$ref'") + ExitCode.InvalidPathOrPolicy + case RefError.Ambiguous(ref, matches) => + env.err(s"gx: '$ref' matches ${matches.size} diagrams — name one by id:") + for d <- matches do env.err(s"gx: ${d.id.value} ${d.name}") + ExitCode.InvalidPathOrPolicy // ------------------------------------------------------------- helpers + /** Save, or report the failure the same way everywhere. A store write that + * fails is `Unknown`, not a success with a warning — the six call sites that + * spelled this out by hand could each have answered differently. + */ + private def persist(d: Diagram, env: CliEnv)(onSaved: Diagram => Int): Int = + env.store.save(d) match + case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown + case Right(saved) => onSaved(saved) + + /** The vocabulary of a command tier, for `--list`. Both tiers print it the + * same way; two copies is how the two `--list` flags start differing. + */ + private def listNames(names: Vector[String], args: Args, env: CliEnv): Int = + if args.json then env.out(ujson.Arr.from(names.map(ujson.Str(_))).render(indent = 2)) + else names.foreach(env.out) + ExitCode.Ok + + /** `--all`, or no arguments at all, means the whole library; otherwise the + * positional refs, resolved. `sync` and `watch` are documented as sharing + * that rule, so they read it from here rather than each restating it. + * + * Failures are RETURNED, not dropped. Flat-mapping them away is what made + * `gx sync typo` print "(nothing bound to sync)" and exit 0 — a script that + * mistyped a name was told it had succeeded. + */ + private def selectedRefs(args: Args, env: CliEnv): (Vector[RefError], Vector[Diagram]) = + if args.has("all") || args.positional.isEmpty then (Vector.empty, env.store.list()) + else args.positional.partitionMap(resolveRef(_, env)) + private def checkPolicy(path: Path, env: CliEnv): Either[Int, Path] = env.policy.evaluate(path, env.cwd) match case Right(resolved) => Right(resolved) diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala index 9c48e2bc..5251dc42 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala @@ -38,10 +38,15 @@ object Main: if debug then message => System.err.println(s"gx[protocol] $message") else _ => () + // One clock for the whole process: the audit log stamps its lines from the + // same source as every `updatedAt` and `lastSyncAt`, so a record and the + // line describing it cannot disagree about when it happened. + val clock: () => Long = () => System.currentTimeMillis() + val env = CliEnv( store = LibraryStore.default(gxHome), policy = AccessPolicy.fromEnv(), - audit = Audit(runtime.resolve("audit.log.jsonl")), + audit = Audit(runtime.resolve("audit.log.jsonl"), clock), // The user's shell, not the process's idea of it. v1 learned this the hard // way: the desktop's working directory is an artifact of how it was // launched, so paths must be resolved where the human typed them. @@ -49,7 +54,7 @@ object Main: out = println, err = System.err.println, stdin = () => String(System.in.readAllBytes(), StandardCharsets.UTF_8), // V-16 - now = () => System.currentTimeMillis(), + now = clock, desktopRunning = () => Main.desktopRunning(control, trace), rpc = (method, params) => Main.call(control, trace, method, params) ) diff --git a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala index 91871233..d5fa7b8c 100644 --- a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala +++ b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala @@ -6,6 +6,7 @@ import org.jpablo.graphexplorer.gxcore.rpc.ChannelError import org.jpablo.graphexplorer.gxcore.store.LibraryStore import java.nio.file.{Files, Path, Paths} +import java.nio.file.attribute.PosixFilePermissions import scala.jdk.CollectionConverters.* @@ -54,7 +55,9 @@ class CliSpec extends FunSuite: val env: CliEnv = CliEnv( store = store, policy = AccessPolicy(Nil, Nil), - audit = Audit(dir.resolve("audit.jsonl")), + // Same clock the CLI uses, so an audit line's timestamp is a fact a test + // can assert rather than whatever the wall clock said. + audit = Audit(dir.resolve("audit.jsonl"), () => clock), cwd = dir, out = s => out.append(s).append('\n'), err = s => err.append(s).append('\n'), @@ -256,6 +259,148 @@ class CliSpec extends FunSuite: assert(r.stdout.contains("nothing bound"), r.stdout) } + /** A ref that resolves to nothing is a typo, not an empty selection. This + * used to be flat-mapped away: "(nothing bound to sync)" and exit 0, which + * tells a script that mistyped a name that it succeeded. + */ + tmp.test("sync on a ref that matches nothing fails, it does not report success") { dir => + dot(dir, "gen.dot", "v1") + val i = Run(dir) + i("import", "gen.dot", "--mode", "sync") + + val s = Run(dir) + assertNotEquals(s("sync", "typo"), ExitCode.Ok, s.stdout) + assert(s.stderr.contains("no diagram matches"), s.stderr) + assert(!s.stdout.contains("nothing bound"), s.stdout) + } + + /** A reconciliation that cannot be persisted is not a success. The three + * saves inside syncOne discarded their Either, so gx printed `Behind`, + * exited 0, and left the record on its old baseline — and the next run + * redid the same work from the same stale state, silently. + */ + tmp.test("a sync that cannot save the record reports it and does not exit 0") { dir => + val f = dot(dir, "gen.dot", "v1") + val i = Run(dir) + i("import", "gen.dot", "--mode", "pull") + + Files.writeString(f, "v2") // the origin moves, so Pull has something to save + + // Readable and listable, but nothing new can be created in it — so the + // scan still finds the record and only the WRITE fails. + val diagrams = dir.resolve("library").resolve("diagrams") + val restore = Files.getPosixFilePermissions(diagrams) + Files.setPosixFilePermissions(diagrams, PosixFilePermissions.fromString("r-xr-xr-x")) + try + // Root ignores the mode bits; there is nothing to assert on such a box. + assume( + scala.util.Try(Files.createTempFile(diagrams, "probe", null)).isFailure, + "this user can write to a read-only directory" + ) + + val s = Run(dir) + assertNotEquals(s("sync", "--all"), ExitCode.Ok, s.stdout) + assert(s.stderr.contains("could not be saved"), s.stderr) + finally Files.setPosixFilePermissions(diagrams, restore) + } + + // ------------------------------------------------------- ambiguous refs + // + // findInLibrary's own doc says ambiguity is reported rather than resolved by + // picking one. It returned None for "nothing matched" AND "several matched", + // so callers rendered both as "no diagram matches" — and the ones that fall + // back to a PATH did so on an ambiguous name. + + /** Two records deliberately sharing a name; only their ids differ. */ + private def twoNamed(dir: Path, name: String): Run = + dot(dir, "a.dot", "digraph { a }") + dot(dir, "b.dot", "digraph { b }") + val i = Run(dir) + i("import", "a.dot", "--name", name) + i("import", "b.dot", "--name", name) + i + + tmp.test("an ambiguous ref is reported as ambiguous, with the ids to pick from") { dir => + twoNamed(dir, "Shared Thing") + val r = Run(dir) + assertNotEquals(r("get", "Shared Thing"), ExitCode.Ok, r.stdout) + assert(r.stderr.contains("matches 2 diagrams"), r.stderr) + assert(!r.stderr.contains("no diagram matches"), "ambiguity reported as absence") + } + + /** The wrong-diagram write the doc warns about, in its real form: with no + * ambiguity case, `set` fell through to treating the ref as a path and + * created a FILE called `shared` instead of refusing. + */ + tmp.test("an ambiguous ref does not fall through to being a path") { dir => + twoNamed(dir, "Shared Thing") + val r = Run(dir, stdinText = "digraph { c }") + assertNotEquals(r("set", "Shared Thing", "--stdin"), ExitCode.Ok, r.stdout) + assert(r.stderr.contains("matches 2 diagrams"), r.stderr) + assert(!Files.exists(dir.resolve("Shared Thing")), "set wrote a file named after an ambiguous ref") + } + + tmp.test("an unambiguous id still resolves when its name is shared") { dir => + val i = twoNamed(dir, "Shared Thing") + val id = i.store.list().head.id + val r = Run(dir) + assertEquals(r("get", id.value), ExitCode.Ok, r.stderr) + } + + // ------------------------------------------------- sync × line endings + // + // `base` and `remote` are hashes of file BYTES, so `local` has to be measured + // the same way. Hashing the record's text with a fixed LF made every one of + // these read as a local edit that never happened. + // + // Note every OTHER sync test above uses text with no newline in it, where LF + // and CRLF are the same bytes — which is exactly how this survived. + + private def crlf: String = "digraph G {\r\n a -> b\r\n}\r\n" + + tmp.test("a CRLF origin nobody has touched is InSync, not Ahead") { dir => + dot(dir, "win.dot", crlf) + val i = Run(dir) + assertEquals(i("import", "win.dot", "--mode", "sync"), ExitCode.Ok, i.stderr) + + // Nothing at all happens here. Both sides are exactly as imported. + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr) + assert(s.stdout.contains("InSync"), s.stdout) + assertEquals(Files.readString(dir.resolve("win.dot")), crlf, "sync rewrote an untouched origin") + } + + tmp.test("a byte-identical CRLF regeneration is Converged, not Diverged") { dir => + val f = dot(dir, "win.dot", crlf) + val i = Run(dir) + i("import", "win.dot", "--mode", "sync") + val id = i.store.list().head.id + + // The generator rewrites the same bytes; the store independently agrees. + val d = i.store.get(id).fold(x => fail(s"$x"), identity) + val next = "digraph G {\r\n a -> c\r\n}\r\n" + i.store.save(d.copy(text = next)) + Files.writeString(f, next) + + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stdout) + assert(s.stdout.contains("Converged"), s.stdout) + } + + tmp.test("a CRLF origin that moves is Behind, and pull follows it") { dir => + val f = dot(dir, "win.dot", crlf) + val i = Run(dir) + i("import", "win.dot", "--mode", "pull") + + val next = "digraph G {\r\n a -> b\r\n b -> c\r\n}\r\n" + Files.writeString(f, next) + + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr) + assert(s.stdout.contains("Behind"), s.stdout) + assertEquals(s.store.list().head.text, next) + } + // --------------------------------------------------------------- watch /** v1 had no way to observe changes without a window. This is the primitive a @@ -476,6 +621,38 @@ class CliSpec extends FunSuite: assert(r.stderr.contains("denied root"), r.stderr) } + /** The audit log is the only place `source` is recorded, so WHEN an event + * happened has to come from the same clock as the record it describes. + * Audit stamped its own `System.currentTimeMillis()` instead — the one hole + * in a seam every other timestamp goes through. + */ + tmp.test("an audit line is stamped from the injected clock, not the wall clock") { dir => + dot(dir, "a.dot") + val r = Run(dir) + assertEquals(r("import", "a.dot"), ExitCode.Ok, r.stderr) + + val line = r.env.audit.entries.headOption.getOrElse(fail("nothing was audited")) + val stamp = ujson.read(line).obj("timestampMs").num.toLong + // The fixture's clock starts at 1000 and ticks by one per read, so a real + // wall-clock stamp is thirteen digits and this assertion is unmissable. + assert(stamp > 1000L && stamp < 2000L, s"audit used a clock the test does not control: $stamp") + } + + /** The guardrail has to hold on EVERY path-taking command, not most of them. + * `watch` took a ref that was not in the library, turned it straight into an + * origin, and started following it — the one way to point gx at a file the + * policy forbids. + */ + tmp.test("watch refuses a denied path too, like every other command") { dir => + val secret = Files.createDirectories(dir.resolve("secrets")) + Files.writeString(secret.resolve("a.dot"), "x") + val r = Run(dir) + val env = r.env.copy(policy = AccessPolicy(Nil, List(secret))) + assertEquals(Cli.run(Vector("watch", "secrets/a.dot"), env), ExitCode.InvalidPathOrPolicy) + assert(r.stderr.contains("denied root"), r.stderr) + assert(!r.stderr.contains("watching "), "watch followed a denied path anyway") + } + // ----------------------------------------------------------------- run /** The document tier, headless (D7.2). These are V-09's point restated for diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala index 726bbbc1..ad7c8c06 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala @@ -28,21 +28,31 @@ enum AuditEvent derives CanEqual: * Writes are best-effort by construction. Failing to record an action must * never fail the action: a full disk should not make the editor read-only. */ -final class Audit(path: Path): +/** @param now the clock, injected for the same reason every other timestamp in + * the CLI is: an event stamped from a hidden `currentTimeMillis` + * cannot be pinned by a test, and this was the one hole left in an + * otherwise complete seam. + */ +final class Audit(path: Path, now: () => Long = () => System.currentTimeMillis()): private val lock = Object() def record(event: AuditEvent): Unit = lock.synchronized: try - Option(path.getParent).foreach(Files.createDirectories(_)) - val line = Audit.toJson(event) + "\n" + // Only a file we are about to CREATE needs its directory made and its + // mode set. Doing both per line cost two extra syscalls on every event, + // which `watch` pays once per change for as long as it runs — and + // testing existence still re-restricts a log deleted underneath us. + val existed = Files.isRegularFile(path) + if !existed then Option(path.getParent).foreach(Files.createDirectories(_)) + val line = Audit.toJson(event, now()) + "\n" Files.write( path, line.getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE, StandardOpenOption.APPEND ) - Audit.restrictToOwner(path) + if !existed then Audit.restrictToOwner(path) catch case NonFatal(_) => () // never fail the operation being audited def entries: Vector[String] = @@ -74,7 +84,7 @@ object Audit: private def obj(fields: (String, String)*): String = fields.map((k, v) => s""""$k":"${escape(v)}"""").mkString("{", ",", "}") - private[fs] def toJson(event: AuditEvent): String = + private[fs] def toJson(event: AuditEvent, timestampMs: Long): String = import AuditEvent.* val base = event match case Allowed(path, action) => obj("event" -> "allowed", "path" -> path, "action" -> action) @@ -93,4 +103,4 @@ object Audit: case WatchRemoved(uri) => obj("event" -> "watch.removed", "uri" -> uri) case OriginMissing(path) => obj("event" -> "origin.missing", "path" -> path) // Timestamp is prepended rather than threaded through every case. - s"""{"timestampMs":${System.currentTimeMillis()},${base.drop(1)}""" + s"""{"timestampMs":$timestampMs,${base.drop(1)}""" diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala index 8073370f..4738c9d3 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala @@ -20,11 +20,6 @@ enum ChannelError derives CanEqual: case Io(message: String) case Rpc(code: String, message: String, details: ujson.Obj) - def describe: String = this match - case NoDesktop(_) => "no desktop is running" - case Io(message) => message - case Rpc(_, message, _) => message - /** The desktop's control channel: a unix socket carrying one JSON object per * line, request then response (D4). * diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala index c2861c99..c6dc8501 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala @@ -110,9 +110,6 @@ final class LibraryStore(val root: Path) extends DiagramSink: def findByOrigin(origin: OriginUri): Vector[Diagram] = list().filter(_.binding.exists(_.origin == origin)) - def inFolder(folder: FolderPath): Vector[Diagram] = - list().filter(_.folder == folder) - // ------------------------------------------------------------ folders /** The tree, including folders that hold nothing. diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala index 797bc2cf..0bb0173b 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala @@ -25,9 +25,7 @@ trait DiagramSelectionOps: val editingElementV = Var[Option[ElementId]](None) object selection: - val signal = selectionV.signal - .distinct -// .tapEach(sel => println(s"[selection] $sel")) + val signal = selectionV.signal.distinct val selectionChanges: EventStream[(toUnselect: ElementIds, toSelect: ElementIds)] = selection.signal @@ -41,24 +39,20 @@ trait DiagramSelectionOps: .distinct .changes - val _selectSuccessors = - selectRelated(outgoing = true, transitive = true)((graph, nodes) => graph.allSuccessorsGraph(nodes.nodeIds)) - val _selectPredecessors = - selectRelated(outgoing = false, transitive = true)((graph, nodes) => graph.allPredecessorsGraph(nodes.nodeIds)) - val _selectDirectSuccessors = - selectRelated(outgoing = true, transitive = false)((graph, nodes) => graph.directSuccessorsGraph(nodes.nodeIds)) - val _selectDirectPredecessors = - selectRelated(outgoing = false, transitive = false)((graph, nodes) => graph.directPredecessorsGraph(nodes.nodeIds)) - /** `outgoing` and `transitive` describe the same operation the selector * already performs; they exist because the ROW-scoped path has to take the * first hop itself (only that hop is constrained by the port) and therefore * needs to know which way it points and whether to keep going. */ - private def selectRelated(outgoing: Boolean, transitive: Boolean)( - selector: (ViewerGraph, Selection) => ViewerGraph - )(fullGraph: ViewerGraph, hiddenNodes: HiddenElements): Unit = - val visibleSubGraph: ViewerGraph = fullGraph.removeElements(hiddenNodes) + private def selectRelated(outgoing: Boolean, transitive: Boolean): Unit = + def selector(graph: ViewerGraph, nodes: Selection): ViewerGraph = + (outgoing, transitive) match + case (true, true) => graph.allSuccessorsGraph(nodes.nodeIds) + case (true, false) => graph.directSuccessorsGraph(nodes.nodeIds) + case (false, true) => graph.allPredecessorsGraph(nodes.nodeIds) + case (false, false) => graph.directPredecessorsGraph(nodes.nodeIds) + + val visibleSubGraph: ViewerGraph = fullGraphNow().removeElements(hiddenElements.now()) recordCells.selectedCellHop(visibleSubGraph, outgoing) match // A row of a record/table is the subject: only the arrows attached at // its port count, so take that hop by hand. A PORT constrains the first @@ -99,14 +93,14 @@ trait DiagramSelectionOps: def toggle(ss: ElementId*): Unit = selectionV.update(ss.foldLeft(_)(_.toggle(_))) - def set(ss: Selection)(using name: sourcecode.FullName): Unit = + def set(ss: Selection): Unit = selectionV.set(ss) @targetName("setElementIds") - def set1(ss: Set[? <: ElementId])(using name: sourcecode.FullName): Unit = + def set1(ss: Set[? <: ElementId]): Unit = set(ElementIds(ss)) - def set2(ss: ElementId*)(using name: sourcecode.FullName): Unit = + def set2(ss: ElementId*): Unit = set1(ss.toSet) @targetName("addElementIds") @@ -130,7 +124,7 @@ trait DiagramSelectionOps: def keepOnly(p: ElementId => Boolean): Unit = selectionV.update(_.filter(p)) - def clear()(using name: sourcecode.FullName): Unit = + def clear(): Unit = set(ElementIds()) def contains(id: ElementId) = @@ -181,17 +175,10 @@ trait DiagramSelectionOps: // Keep the original groups/clusters in the selection and add all members set(s ++ memberNodeIds) - def selectSuccessors() = - _selectSuccessors(fullGraphNow(), hiddenElements.now()) - - def selectPredecessors() = - _selectPredecessors(fullGraphNow(), hiddenElements.now()) - - def selectDirectSuccessors() = - _selectDirectSuccessors(fullGraphNow(), hiddenElements.now()) - - def selectDirectPredecessors() = - _selectDirectPredecessors(fullGraphNow(), hiddenElements.now()) + def selectSuccessors() = selectRelated(outgoing = true, transitive = true) + def selectPredecessors() = selectRelated(outgoing = false, transitive = true) + def selectDirectSuccessors() = selectRelated(outgoing = true, transitive = false) + def selectDirectPredecessors() = selectRelated(outgoing = false, transitive = false) def addToGroup() = val classified = now().classify @@ -361,10 +348,14 @@ trait DiagramSelectionOps: set(ElementIds.from(nodeId)) if now().contains(nodeId) then navCursorSet(nodeId) + /** `elementsFromRectEnd` is BY-NAME: it is a document-wide hit test that + * forces layout, and only the click branch below reads it — this runs on + * every mouse-move of a rubber-band drag. + */ def selectExtendSelectionOverlappingElements( - rect: MouseActionRect, - selectableElements: Seq[SelectableElement], - elementsFromRectEnd: js.Array[dom.Element] + rect: MouseActionRect, + selectableElements: Seq[SelectableElement], + elementsFromRectEnd: => js.Array[dom.Element] ) = if rect.isEmpty then // Equivalent to an onClick event diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala index 0b14082d..4cb0618c 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala @@ -231,13 +231,10 @@ trait KeyboardNavOps: boxes: Map[ElementId, NavBox] ): Option[NodeId] = boxes.get(n).flatMap: o => - // Strictly past this node's edge, so an overlapping neighbour never - // counts as "beside" — and this node can never be its own answer. - def beyond(b: NavBox) = dir match - case NavDirection.NavLeft => b.r <= o.l - case NavDirection.NavRight => b.l >= o.r - case NavDirection.NavUp => b.b <= o.t - case NavDirection.NavDown => b.t >= o.b + // Signed distance from this node's edge to the candidate's facing edge. + // Non-negative means strictly past that edge, so an overlapping + // neighbour never counts as "beside" — and this node is never its own + // answer. One definition, so the two uses cannot drift apart. def gap(b: NavBox) = dir match case NavDirection.NavLeft => o.l - b.r case NavDirection.NavRight => b.l - o.r @@ -246,7 +243,7 @@ trait KeyboardNavOps: visibleGraphNow().nodeIds.iterator .filter(_ != n) .flatMap(id => boxes.get(id).map(id -> _)) - .filter((_, b) => overlaps(o, b, dir) && beyond(b)) + .filter((_, b) => overlaps(o, b, dir) && gap(b) >= 0) .minByOption((_, b) => gap(b)) .map(_._1) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala index 1d58f2fe..f8cdeeb2 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala @@ -1,6 +1,5 @@ package org.jpablo.graphexplorer.viewer.state -import com.raquo.airstream.core.Signal import com.raquo.airstream.ownership.Owner import com.raquo.airstream.state.Var import com.softwaremill.quicklens.* @@ -10,17 +9,11 @@ import org.jpablo.graphexplorer.viewer.models.GroupId */ case class ProjectOps(project: Var[Project])(using Owner): - // export project.{signal, update, updater} - val signal = project.signal - val update = project.update - val updater = project.updater + val signal = project.signal val name: Var[String] = project.zoomLazy(_.name)((p, n) => p.copy(name = n)).distinct - val page: Var[Page] = - project.zoomLazy(_.page)((p, page) => p.copy(page = page)).distinct - val hiddenElements: Var[HiddenElements] = project .zoomLazy(_.page.hiddenElements)((p, s) => p.modify(_.page.hiddenElements).setTo(s)) @@ -31,16 +24,4 @@ case class ProjectOps(project: Var[Project])(using Owner): .zoomLazy(_.page.collapsedGroups)((p, s) => p.modify(_.page.collapsedGroups).setTo(s)) .distinct -// hiddenElements.signal.foreach: hidden => -// dom.console.debug(s"hidden elements changed: $hidden") - - val basePaths: Signal[List[String]] = - project.signal.map(_.projectSettings.basePaths).distinct - - val projectSettings: Signal[ProjectSettings] = - project.signal.map(_.projectSettings).distinct - - val diagramOptions: Signal[DiagramOptions] = - project.signal.map(_.page.diagramOptions) - end ProjectOps diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala index 69ea5452..a48d5fac 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala @@ -5,11 +5,13 @@ import com.raquo.airstream.state.Var import org.jpablo.graphexplorer.graphviz.html.{HtmlTable, HtmlTableLayout} import org.jpablo.graphexplorer.graphviz.layout.RecordLabel import org.jpablo.graphexplorer.viewer.components.selection.SelectableElement +import org.jpablo.graphexplorer.viewer.components.toSvgPoint +import org.jpablo.graphexplorer.viewer.utils.{ClientPoint, SvgPoint} import org.jpablo.graphexplorer.viewer.components.svgCanvas.RecordCellOverlay import org.jpablo.graphexplorer.viewer.domUtils.querySelectorAllT import org.jpablo.graphexplorer.viewer.formats.dot.{HtmlLabelOps, HtmlLabels, RecordTree} import org.jpablo.graphexplorer.viewer.formats.dot.ast.{AttrEq, AttrValue} -import org.jpablo.graphexplorer.viewer.formats.dot.attributes.Rankdir +import org.jpablo.graphexplorer.viewer.formats.dot.attributes.{FontSize, Height, Width} import org.jpablo.graphexplorer.viewer.graph.ViewerGraph import org.jpablo.graphexplorer.viewer.models.* @@ -62,11 +64,13 @@ trait RecordCellOps: val editingCellV = Var[Option[SelectedCell]](None) object recordCells: - // gv const.h defaults — private in NodeSize, mirrored here (stable). - private val DefFontSize = 14.0 + // gv const.h defaults. The numeric three come from the shared attribute + // objects; DefFontName does NOT — FontName.default is the UI spelling + // ("Times New Roman"), while layout needs the gv font name. + private val DefFontSize = FontSize.default private val DefFontName = "Times-Roman" - private val DefWidthIn = 0.75 - private val DefHeightIn = 0.5 + private val DefWidthIn = Width.default + private val DefHeightIn = Height.default private val PointsPerInch = 72.0 private enum CellKind derives CanEqual: @@ -97,25 +101,21 @@ trait RecordCellOps: def selectedCellIsHtml: Boolean = selectedCellV.now().exists(c => kindOf(c.nodeId).contains(CellKind.Html)) - private def rankdirNow(): Rankdir = - fullGraphNow().elements.graphAttributes.values - .get(Rankdir.attrId) - .flatMap(attr => Rankdir.values.find(_.toString == attr.toString)) - .getOrElse(Rankdir.TB) - - def topLRNow(): Boolean = RecordTree.topLRFor(rankdirNow()) + def topLRNow(): Boolean = RecordTree.topLRFor(graphRankDirNow()) private def getNodeNow(nodeId: NodeId): Option[ViewerNode] = fullGraphNow().getNode(nodeId) /** The record tree of a RECORD node (record ops only). */ def cellTreeOf(nodeId: NodeId): Option[RecordTree.Group] = - Option.when(kindOf(nodeId).contains(CellKind.Record))(()).flatMap: _ => + if kindOf(nodeId).contains(CellKind.Record) then getNodeNow(nodeId).map(node => RecordTree.parse(node.label.toString)) + else None private def htmlTableOf(nodeId: NodeId): Option[HtmlTable] = - Option.when(kindOf(nodeId).contains(CellKind.Html))(()).flatMap: _ => + if kindOf(nodeId).contains(CellKind.Html) then getNodeNow(nodeId).flatMap(node => HtmlLabelOps.parseTable(node.label.toString)) + else None /** Node-local cell boxes, from the SAME layout the engine used. */ def cellBoxes(nodeId: NodeId): Vector[RecordCellBox] = @@ -181,11 +181,11 @@ trait RecordCellOps: if !isCellEditable(nodeId) then None else for - group <- nodeGroupInDom(nodeId) - (lx, ly) <- clientToLocal(group, clientX, clientY) + group <- nodeGroupInDom(nodeId) + local <- clientToLocal(group, clientX, clientY) path <- { val bbox = RecordCellOverlay.ownGeometryBBox(group) - cellNearestLocalPoint(nodeId, lx - (bbox.x + bbox.width / 2), (bbox.y + bbox.height / 2) - ly) + cellNearestLocalPoint(nodeId, local.x - (bbox.x + bbox.width / 2), (bbox.y + bbox.height / 2) - local.y) } yield path @@ -196,16 +196,8 @@ trait RecordCellOps: .filterNot(_.closest(s".${SelectableElement.exitGhostClass}") != null) .collectFirst { case g: dom.svg.G => g } - private def clientToLocal(group: dom.svg.G, clientX: Double, clientY: Double): Option[(Double, Double)] = - for - svgEl <- Option(group.ownerSVGElement) - ctm <- Option(group.getScreenCTM()) - yield - val pt = svgEl.createSVGPoint() - pt.x = clientX - pt.y = clientY - val local = pt.matrixTransform(ctm.inverse()) - (local.x, local.y) + private def clientToLocal(group: dom.svg.G, clientX: Double, clientY: Double): Option[SvgPoint] = + Option(group.getScreenCTM()).map(ctm => ClientPoint(clientX, clientY).toSvgPoint(ctm)) /** The port of the cell at `path`, MINTED into the label when the cell has * none (a fresh `f`). Pure on the given graph, so arrow ops can compose diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala index 58b1e48f..20c5fe46 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala @@ -72,25 +72,21 @@ object ThumbnailRenderer: telemetryContext: Seq[(String, Any)] = Nil )(using ExecutionContext): Signal[ReactiveSvgElement[SVG]] = val format = DiagramFormat.detect(dot.value) + + // Every event from here carries the caller's context; going through one + // place is what keeps a site from silently dropping the project id. + def log(name: String, extra: (String, Any)*): Unit = + Telemetry.log(name, (telemetryContext ++ extra)*) + + def cacheFields = Seq("format" -> format.toString, "cacheSize" -> ThumbnailSvgCache.size) + ThumbnailSvgCache.get(format, dot.value) match case Some(proto) => - Telemetry.log( - "thumb.cache.hit", - (telemetryContext ++ Seq( - "format" -> format.toString, - "cacheSize" -> ThumbnailSvgCache.size - ))* - ) + log("thumb.cache.hit", cacheFields*) Signal.fromValue(ThumbnailSvgCache.cloneSvg(proto)) case None => - Telemetry.log( - "thumb.cache.miss", - (telemetryContext ++ Seq( - "format" -> format.toString, - "cacheSize" -> ThumbnailSvgCache.size - ))* - ) + log("thumb.cache.miss", cacheFields*) // Adopt an SVG string (from the persistent cache, or fresh off a // render) as this card's element, refilling the in-memory cache on the @@ -98,10 +94,7 @@ object ThumbnailRenderer: def adopt(svgHtml: String): ReactiveSvgElement[SVG] = val proto = parseSVG(svgHtml).ref ThumbnailSvgCache.put(format, dot.value, proto) - Telemetry.log( - "thumb.cache.store", - (telemetryContext ++ Seq("format" -> format.toString, "cacheSize" -> ThumbnailSvgCache.size))* - ) + log("thumb.cache.store", cacheFields*) ThumbnailSvgCache.cloneSvg(proto) // ONE svg-only render (`textToSvgOnly`) straight from the source text. @@ -116,14 +109,8 @@ object ThumbnailRenderer: onIdle { () => val svgStartedAt = Telemetry.nowMs() val resultTry = graphviz.textToSvgOnly(dot) - Telemetry.log( - "thumb.dot.textToSvg", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - svgStartedAt), "ok" -> resultTry.isSuccess))* - ) - Telemetry.log( - "thumb.dot.total", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> resultTry.isSuccess))* - ) + log("thumb.dot.textToSvg", "dtMs" -> (Telemetry.nowMs() - svgStartedAt), "ok" -> resultTry.isSuccess) + log("thumb.dot.total", "dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> resultTry.isSuccess) resultTry.get.outerHTML } @@ -135,15 +122,12 @@ object ThumbnailRenderer: // containment reduces it. Not rendering at all is the only real cure, // which is what the persistent cache buys on every visit after the first. def renderMermaid(startedAt: Double): Future[String] = - Telemetry.log("thumb.mermaid.start", (telemetryContext ++ Seq("sourceChars" -> dot.value.length))*) + log("thumb.mermaid.start", "sourceChars" -> dot.value.length) val backend = MermaidBackend() onIdle(() => backend.textToSvg(dot.value)) .flatMap(identity) .map: r => - Telemetry.log( - "thumb.mermaid.done", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> true))* - ) + log("thumb.mermaid.done", "dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> true) r.svg.ref.outerHTML val startedAt = Telemetry.nowMs() @@ -156,13 +140,10 @@ object ThumbnailRenderer: val html: Future[String] = ThumbnailDiskCache.get(format, dot.value).flatMap: case Some(stored) => - Telemetry.log( - "thumb.disk.hit", - (telemetryContext ++ Seq("format" -> format.toString, "bytes" -> stored.length))* - ) + log("thumb.disk.hit", "format" -> format.toString, "bytes" -> stored.length) Future.successful(stored) case None => - Telemetry.log("thumb.disk.miss", (telemetryContext ++ Seq("format" -> format.toString))*) + log("thumb.disk.miss", "format" -> format.toString) val rendered = format match case DiagramFormat.DOT => renderDot(startedAt) case DiagramFormat.Mermaid => renderMermaid(startedAt) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala index c66f0fca..8f460179 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala @@ -4,11 +4,9 @@ import com.raquo.airstream.core.Signal import com.raquo.airstream.eventbus.EventBus import com.raquo.airstream.state.Var -enum RightPanelSection(idx: Int) derives CanEqual: - case none extends RightPanelSection(-1) - case diagramAttributes extends RightPanelSection(0) - case elements extends RightPanelSection(1) - case sources extends RightPanelSection(2) +/** Persisted by `ordinal` (see Persistence), NOT by any hand-assigned number. */ +enum RightPanelSection derives CanEqual: + case none, diagramAttributes, elements, sources def isVisible = this != none diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala index 3337bd64..e16ed1e6 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala @@ -285,9 +285,6 @@ case class ViewerState( // If true, prompt for label before creating a new node (default: true) val promptLabelBeforeNewNode: Var[Boolean] = Var(true) - // If true, prompt for label before creating a new group (default: true) - val promptLabelBeforeNewGroup: Var[Boolean] = Var(true) - // ------------- New node flow ------------- case class PendingNewNode(attributes: Attributes, direction: ArrowDirection) val pendingNewNodeV: Var[Option[PendingNewNode]] = Var(None) @@ -306,12 +303,9 @@ case class ViewerState( else addNodeWithSmartConnection(attributes, direction) - /** Creates a new group from the current selection, optionally prompting for the label before creation based on settings. */ + /** Creates a new group from the current selection, prompting for its label. */ def createGroupMaybePrompt(elementIds: ElementIds): Unit = - if promptLabelBeforeNewGroup.now() then - pendingNewGroupV.set(Some(PendingNewGroup(elementIds))) - else - createGroupWithLabel(elementIds, "") + pendingNewGroupV.set(Some(PendingNewGroup(elementIds))) /** Creates a new group with the specified elements and label. */ def createGroupWithLabel(elementIds: ElementIds, label: String): Unit = @@ -326,9 +320,6 @@ case class ViewerState( // -------- storage ------------ initializePersistence() - def nodeById(ids: Seq[NodeId]): Seq[ViewerNode] = - ids.flatMap(fullGraphNow().getNode) - def allNodeIds(): Set[NodeId] = fullGraphNow().nodeIds @@ -369,7 +360,7 @@ case class ViewerState( to: NodeId, fromCell: Option[List[Int]] = None, toCell: Option[List[Int]] = None - )(using name: sourcecode.FullName) = + ) = phases.fullGraphV.update: g => val (g1, fromPort) = recordCells.resolvePortIn(g, from, fromCell) val (g2, toPort) = recordCells.resolvePortIn(g1, to, toCell) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala index aa856bdb..5c4d8f95 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala @@ -20,9 +20,6 @@ trait VisibilityOps: val signal = _hiddenElements.signal - def toggle(s: NodeId): Unit = - _hiddenElements.update(_.toggle(s)) - def add(ss: Set[NodeId]): Unit = _hiddenElements.update(_ ++ ss) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala index 5e0111ce..fd33637d 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala @@ -68,8 +68,10 @@ trait AddNewArrowOps: def handleNewArrowControls(parent: dom.svg.G, selection: Option[SelectableElement], action: MouseAction): Unit = // Read the badge model ONCE, and read the SAME one CountBadges drew from: // a control decides where to stand by which edges carry a count, so the two - // must never disagree about that. - val concealed = concealedCountsNow() + // must never disagree about that. LAZY because this runs on every + // mouse-move of every drag, where no control is built and the whole + // O(visible nodes) count would be computed and thrown away. + lazy val concealed = concealedCountsNow() val controls = for elem <- selection.toSeq