diff --git a/crates/moon-core/src/feed/folder_tree.rs b/crates/moon-core/src/feed/folder_tree.rs new file mode 100644 index 00000000..acabac0b --- /dev/null +++ b/crates/moon-core/src/feed/folder_tree.rs @@ -0,0 +1,185 @@ +//! Edits to a core's folder tree, applied where the newest tree is known. +//! +//! The protocol takes a folder edit as the COMPLETE desired set — the core deletes every empty +//! folder the list omits — so what an edit is worth depends entirely on the list it was built from. +//! Built from a stale one it silently deletes whatever arrived in between, which is exactly the +//! trap moonproto's own guidance names ("building the next edit from an older confirmed snapshot +//! can undo your own pending folder changes", `docs/strats.md`). +//! +//! So the window states its INTENT — add this folder, move that subtree — and the base list is +//! chosen here, on the feed thread, where both the core's confirmed tree and the one this terminal +//! last sent are known. Nothing upstream assembles a complete tree, because nothing upstream can. + +/// Remove one folder and everything under it from a tree. +/// +/// Removal IS omission — the wire form is the complete desired set — so this is the whole of it. A +/// folder still holding a strategy is kept by the core regardless of this list, which is the safety +/// the caller relies on rather than a check made here. +/// +/// Args: +/// paths: The tree to remove from. +/// path: Canonical path of the folder to remove. +/// +/// Returns: +/// The tree without that folder or any of its descendants. +pub fn without(paths: &[String], path: &str) -> Vec { + let folded = fold(path); + let under = format!("{folded}/"); + paths + .iter() + .filter(|seen| { + let seen = fold(seen); + seen != folded && !seen.starts_with(&under) + }) + .cloned() + .collect() +} + +/// Whether every path in a submission would pass moonproto's own folder validation. +/// +/// MIRRORS `validate_strategy_folder_paths` (moonproto `client/active_runtime/handles.rs`), which +/// rejects the WHOLE submission on one bad path. That matters because bundling a folder tree with +/// strategies puts every STRATEGY path through the same check, and this terminal knowingly holds +/// paths it would refuse: MoonBot allows a `/` inside a folder name — `"EMA / ORGANIC"` is one +/// folder there — while the validator splits on every `/` and refuses a segment with surrounding +/// whitespace. Asking first is what keeps a folder tree from turning a working rename into a +/// refusal that moves nothing at all. +/// +/// A hand-kept mirror, like the other cross-crate constants in this workspace: moonproto exposes no +/// validator, and being wrong here costs a skipped folder tree rather than a wrong edit. +/// +/// Args: +/// paths: Every folder path the submission would carry — the tree AND each strategy's own. +/// +/// Returns: +/// Whether moonproto would accept the set. +pub fn sendable<'a>(paths: impl Iterator) -> bool { + let mut folders = std::collections::HashSet::new(); + for path in paths { + if path.len() > usize::from(u8::MAX) || path.contains(['\r', '\n', '\0', '"']) { + return false; + } + let mut prefix = String::new(); + for part in path.split('/').filter(|_| !path.is_empty()) { + if part.is_empty() || part.trim() != part { + return false; + } + if !prefix.is_empty() { + prefix.push('/'); + } + prefix.push_str(part); + folders.insert(prefix.to_lowercase()); + } + } + folders.len() < usize::from(u16::MAX) +} + +/// Fold one path into the form two spellings of the same folder share. +/// +/// Lowercased because the core compares folder paths case-insensitively, and `\` read as `/` +/// because a strategy's stored path may carry either while the tree the core reports uses `/`. Two +/// spellings that fold alike name one folder, and an edit that missed that would look for a folder +/// the core has under a name it does not. +/// +/// Args: +/// path: A folder path in any of those spellings. +/// +/// Returns: +/// Its comparison form. +fn fold(path: &str) -> String { + path.replace('\\', "/").to_lowercase() +} + +/// Add one folder to a tree, or return it unchanged when the tree already holds it. +/// +/// Args: +/// paths: The tree to add to. +/// path: Canonical path of the folder to add. +/// +/// Returns: +/// The tree with that folder present exactly once. +pub fn with_added(paths: &[String], path: &str) -> Vec { + let mut out = paths.to_vec(); + let folded = fold(path); + if !out.iter().any(|seen| fold(seen) == folded) { + out.push(path.to_string()); + } + out +} + +/// Split one path into the segments the CORE reads it as. +/// +/// On every separator, which is the core's own rule for the tree it reports — deliberately NOT the +/// Strategies window's rule, which keeps a `/` with whitespace beside it inside a folder name. The +/// two disagree only for a name the core could never accept an edit to anyway, and on such a core +/// folder editing is switched off whole (`CoreFolders::editable`). +fn segments(path: &str) -> Vec<&str> { + path.split(['/', '\\']) + .filter(|part| !part.is_empty()) + .collect() +} + +/// Rewrite a tree for a subtree that was renamed or moved. +/// +/// The half a strategy edit cannot carry: rewriting the rows' paths moves the strategies, but the +/// OLD folder survives in the tree as a folder of its own, now holding nothing. Only a tree that +/// omits it removes it, and the protocol wants both halves in one snapshot. +/// +/// A path is rewritten when it IS the moved folder or lies under it. "Under" is a segment +/// boundary, never a character prefix — `Research2` is not inside `Research` — and the comparison +/// is case-insensitive because the core's own is; the surviving spelling is the one the caller +/// asked for, since that is what the operator typed, with the untouched tail kept as the core +/// spelled it. +/// +/// Compared segment by segment rather than by slicing the raw path at the length of a folded +/// prefix. That shortcut is wrong twice over: `to_lowercase` does not preserve byte length for +/// every character — `İ` grows — so the offset can land mid-character and panic, on a string this +/// terminal received from a core. +/// +/// Args: +/// paths: The tree to rewrite. +/// old_key: Canonical path of the folder that moved. +/// new_key: Canonical path it moved to. +/// +/// Returns: +/// The complete desired tree, in the order given, with no path listed twice. +pub fn rebase(paths: &[String], old_key: &str, new_key: &str) -> Vec { + let old_parts: Vec = segments(old_key) + .into_iter() + .map(|part| part.to_lowercase()) + .collect(); + if old_parts.is_empty() || fold(old_key) == fold(new_key) { + return paths.to_vec(); + } + let mut out: Vec = Vec::with_capacity(paths.len()); + // A rename onto an existing name merges the two folders, and the tree must say so once: the core + // reads a repeated path as one folder either way, but a list that repeats itself is one nobody + // can check against what they asked for. Through a set rather than a scan of what is already + // out — the tree runs to thousands of paths, and a scan would fold each one per comparison. + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + for path in paths { + let parts = segments(path); + let under = parts.len() >= old_parts.len() + && parts + .iter() + .zip(&old_parts) + .all(|(part, want)| part.to_lowercase() == *want); + let rebased = match under { + false => path.clone(), + true => { + let tail = &parts[old_parts.len()..]; + match tail.is_empty() { + true => new_key.to_string(), + false => format!("{new_key}/{}", tail.join("/")), + } + } + }; + if seen.insert(fold(&rebased)) { + out.push(rebased); + } + } + out +} + +#[cfg(test)] +mod tests; diff --git a/crates/moon-core/src/feed/folder_tree/tests.rs b/crates/moon-core/src/feed/folder_tree/tests.rs new file mode 100644 index 00000000..260e09a0 --- /dev/null +++ b/crates/moon-core/src/feed/folder_tree/tests.rs @@ -0,0 +1,151 @@ +//! Unit tests for folder-tree edits. + +use super::{rebase, sendable, with_added, without}; + +/// The rewrite covers the folder and everything under it. The tree is submitted as the COMPLETE +/// desired set, so a path left un-rewritten is a folder the core is told to keep at its old name, +/// with the renamed one arriving beside it. +#[test] +fn a_rename_rewrites_the_folder_and_its_whole_subtree() { + let tree = vec![ + "Research".to_string(), + "Research/Deep".to_string(), + "Live".to_string(), + ]; + assert_eq!( + rebase(&tree, "Research", "Archive"), + vec!["Archive", "Archive/Deep", "Live"] + ); +} + +/// "Under" is a segment boundary, never a character prefix. Rewriting a folder that merely starts +/// with the same letters would rename something nobody touched — and, the tree being complete, +/// delete the original by omission in the same breath. +#[test] +fn a_folder_sharing_a_prefix_is_left_alone() { + let tree = vec!["Research".to_string(), "Research2".to_string()]; + assert_eq!( + rebase(&tree, "Research", "Archive"), + vec!["Archive", "Research2"] + ); +} + +/// The core compares folder paths case-insensitively, so the rewrite must too: a tree that kept +/// `RESEARCH/Deep` while renaming `Research` would ask the core to hold one folder under two names, +/// and the one it dropped would be the operator's. +#[test] +fn the_match_ignores_case_while_the_new_spelling_is_kept() { + let tree = vec!["research".to_string(), "RESEARCH/Deep".to_string()]; + assert_eq!( + rebase(&tree, "Research", "Archive"), + vec!["Archive", "Archive/Deep"] + ); +} + +/// Renaming onto a name that already exists merges the two folders; the list must say so once. +#[test] +fn a_rename_onto_an_existing_folder_lists_it_once() { + let tree = vec!["Old".to_string(), "New".to_string()]; + assert_eq!(rebase(&tree, "Old", "New"), vec!["New"]); +} + +/// Nothing to rewrite leaves the tree exactly as it stands, including the case-only rename the +/// protocol states is not a distinct folder-tree change at all. +#[test] +fn a_tree_with_nothing_to_rebase_comes_back_unchanged() { + let tree = vec!["Alpha".to_string(), "Alpha/Deep".to_string()]; + assert_eq!(rebase(&tree, "Alpha", "alpha"), tree); + assert_eq!(rebase(&tree, "", "Beta"), tree); + assert_eq!(rebase(&tree, "Missing", "X"), tree); +} + +/// A move is the same rewrite with a longer destination — one operation, so a drag and a rename +/// cannot drift apart. +#[test] +fn a_move_reparents_the_subtree() { + let tree = vec![ + "Live".to_string(), + "Live/Deep".to_string(), + "Box".to_string(), + ]; + assert_eq!( + rebase(&tree, "Live", "Box/Live"), + vec!["Box/Live", "Box/Live/Deep", "Box"] + ); +} + +/// Adding is idempotent, case-insensitively: the same folder asked for twice is one folder, and a +/// second spelling of it would ask the core to create it again. +#[test] +fn adding_a_folder_the_tree_already_holds_changes_nothing() { + let tree = vec!["Research".to_string()]; + assert_eq!( + with_added(&tree, "Research/New"), + vec!["Research", "Research/New"] + ); + assert_eq!(with_added(&tree, "research"), tree); +} + +/// The mirror of moonproto's validator on the case this terminal actually holds: MoonBot allows a +/// `/` inside a folder name, the validator does not, and it refuses the WHOLE submission on one +/// such path — including the strategy moves bundled with it. A core reporting one of those cannot +/// have its folders edited at all, which is what `CoreFolders::editable` is for. +#[test] +fn a_moonbot_folder_name_containing_a_slash_is_not_sendable() { + assert!(sendable(["Research", "Research/Deep"].into_iter())); + assert!(!sendable(["EMA / ORGANIC"].into_iter())); + // The parent moonproto's own state derives from that name by splitting it. + assert!(!sendable(["EMA "].into_iter())); + assert!(!sendable([" leading"].into_iter())); +} + +/// The validator's remaining rules, so a submission cannot be refused for a reason this mirror does +/// not know about. +#[test] +fn quotes_control_characters_and_overlong_paths_are_not_sendable() { + assert!(!sendable(["say \"no\""].into_iter())); + assert!(!sendable(["two\nlines"].into_iter())); + assert!(!sendable(["a/"].into_iter())); + // 255 BYTES, not characters: a Cyrillic name half that long already exceeds it. + let long = "я".repeat(128); + assert!(!sendable([long.as_str()].into_iter())); + let fits = "я".repeat(127); + assert!(sendable([fits.as_str()].into_iter())); + // An empty path is the root, which is always acceptable. + assert!(sendable([""].into_iter())); +} + +/// Removal is omission, and it takes the whole subtree with it — a descendant left in the desired +/// tree would ask the core to keep the folder it was just told to drop. +#[test] +fn removing_a_folder_takes_its_subtree_and_nothing_else() { + let tree = vec![ + "Research".to_string(), + "Research/Deep".to_string(), + "Research2".to_string(), + "Live".to_string(), + ]; + assert_eq!(without(&tree, "Research"), vec!["Research2", "Live"]); + assert_eq!(without(&tree, "Missing"), tree); +} + +/// The two spellings of one folder fold together everywhere: a path stored with `\` names the same +/// folder as one with `/`, and the core's own compare ignores case. An edit that missed either +/// would look for a folder the core has under a name it does not. +#[test] +fn one_folder_spelled_two_ways_is_one_folder() { + let tree = vec![r"Deep\Inner".to_string()]; + assert_eq!(without(&tree, "deep/inner"), Vec::::new()); + assert_eq!(with_added(&tree, "DEEP/inner"), tree); + assert_eq!(rebase(&tree, "deep", "Box"), vec!["Box/Inner"]); +} + +/// A folder name whose lowercase form is LONGER than the name itself. `İ` (U+0130) lowercases to +/// two characters, so a rewrite that sliced the raw path at the folded prefix's byte length would +/// cut mid-character and panic — on a string this terminal received from a core. +#[test] +fn a_name_that_changes_length_when_folded_is_still_rebased() { + let tree = vec!["İ".to_string(), "İ/Deep".to_string(), "Other".to_string()]; + assert_eq!(rebase(&tree, "İ", "Box"), vec!["Box", "Box/Deep", "Other"]); + assert_eq!(without(&tree, "İ"), vec!["Other"]); +} diff --git a/crates/moon-core/src/feed/live/commands.rs b/crates/moon-core/src/feed/live/commands.rs index 58eacd23..df7112c9 100644 --- a/crates/moon-core/src/feed/live/commands.rs +++ b/crates/moon-core/src/feed/live/commands.rs @@ -107,17 +107,128 @@ fn strategy_placements_unchanged( /// changes: a conditional delete is allowed only when both views match the caller's evidence. pub(super) struct StrategyPlacementGuard { queued_sync: Option>, + queued_order: Option, + queued_folders: Option, +} + +/// The strategy sequence the last accepted sync carried, and the confirmed order it was built on. +/// +/// Kept because `strats.snapshots()` is the CORE-CONFIRMED order and moonproto rewrites it only +/// from the core's own Full echo. Between a reorder and that echo, every other strategy command — +/// a checkbox, a field edit, a move — rebuilds the outgoing list from the confirmed order and would +/// hand the core back the arrangement the operator had just replaced. So the queued sequence is +/// applied to every outgoing list until the core has answered. +/// The folder tree the last accepted folder edit carried, and the confirmed tree it was built on. +/// +/// The same shape as [`QueuedOrder`] and for the same reason: `folder_paths()` is what the CORE has +/// confirmed, and between an edit and its echo a second edit built on that list would drop the +/// first. The version is the folder tree's own — moonproto advances it only when the core publishes +/// one — so once it moves, the core has spoken and this is retired. +struct QueuedFolders { + /// Paths in the tree last sent. + paths: Vec, + /// `StratsState::folders_last_modified` at that moment. + base_modified: i64, +} + +struct QueuedOrder { + /// Strategy ids in the order last sent. + ids: Vec, + /// `StratsState::last_modified` at that moment: the version moonproto advances ONLY in + /// `apply_server_order` — that is, only when the core publishes a full snapshot. Once it moves, + /// the core has ruled, whether it accepted this order or overruled it, and re-asserting ours + /// past that point would be an argument with no end. + /// + /// KNOWN LIMIT: that version belongs to the snapshot, not to the order, and moonproto exposes + /// no order-specific one. So an unrelated full snapshot racing the send retires the sequence + /// before the core has applied it, and the reorder is lost — the window goes on drawing it + /// until its own confirmation window closes. The alternative, ignoring the version, is the + /// endless argument above. + base_modified: u64, } impl StrategyPlacementGuard { /// Create an empty guard before the feed thread has queued any full-list synchronization. pub(super) fn new() -> Self { - Self { queued_sync: None } + Self { + queued_sync: None, + queued_order: None, + queued_folders: None, + } } - /// Remember the placements in a full-list synchronization accepted by MoonProto's queue. - fn note_queued_sync(&mut self, placements: Vec<(u64, String)>) { + /// The newest folder tree this terminal knows: the one it last sent, or the core's own. + /// + /// Args: + /// confirmed: The core's confirmed tree. + /// last_modified: That tree's version. + /// + /// Returns: + /// The base a folder edit must be applied to. + fn folder_base(&mut self, confirmed: Vec, last_modified: i64) -> Vec { + if self + .queued_folders + .as_ref() + .is_some_and(|queued| queued.base_modified != last_modified) + { + self.queued_folders = None; + } + match &self.queued_folders { + Some(queued) => queued.paths.clone(), + None => confirmed, + } + } + + /// Remember a folder tree accepted by MoonProto's queue. + fn note_queued_folders(&mut self, paths: Vec, base_modified: i64) { + self.queued_folders = Some(QueuedFolders { + paths, + base_modified, + }); + } + + /// Remember what a full-list synchronization accepted by MoonProto's queue carried. + /// + /// Args: + /// placements: `(strategy id, raw folder path)` for every row in that list. + /// order: The same rows' ids, in the sequence they were sent. + /// base_modified: The confirmed order's version at the moment of sending. + fn note_queued_sync( + &mut self, + placements: Vec<(u64, String)>, + order: Vec, + base_modified: u64, + ) { self.queued_sync = Some(placements); + self.queued_order = Some(QueuedOrder { + ids: order, + base_modified, + }); + } + + /// The sequence still owed to the core, or `None` once the core has published its own. + /// + /// Args: + /// last_modified: The confirmed order's current version. + /// + /// Returns: + /// Ids in the order last sent, while that send is still the newest word on the subject. + fn pending_order(&mut self, last_modified: u64) -> Option<&[u64]> { + if self + .queued_order + .as_ref() + .is_some_and(|queued| queued.base_modified != last_modified) + { + self.queued_order = None; + } + self.queued_order + .as_ref() + .map(|queued| queued.ids.as_slice()) + } + + /// Drop the queued sequence, once something has established that the core no longer owes it. + fn retire_order(&mut self) { + self.queued_order = None; } /// Return whether live and still-pending placement views both match the caller's snapshot. @@ -300,6 +411,9 @@ impl LocalStratEdits { /// dropping a timed-out desired value here would convert a lost echo into a real revert or a /// real disappearance. /// +/// Returns the list, and whether the queued order it was given has been satisfied and can be +/// retired. +/// /// Appended entries are sorted by `(submitted_at, strategy_id)` because `strategy_edits()` is a /// `HashMap` iterator with no stable order — an unsorted append would make the outgoing list /// order vary between runs. @@ -307,7 +421,10 @@ impl LocalStratEdits { /// One accepted side effect: re-staging resets `submitted_at` and `deadline` for every still- /// open edit, so an unrelated edit EXTENDS another's 45 s confirmation window. It can only ever /// extend, never cause a false `TimedOut`. It is not fixed here — the fix belongs upstream. -fn overlay_pending_edits(strats: &StratsState) -> Vec { +fn overlay_pending_edits( + strats: &StratsState, + order: Option<&[u64]>, +) -> (Vec, bool) { let mut full: Vec = strats .snapshots() .map( @@ -326,7 +443,185 @@ fn overlay_pending_edits(strats: &StratsState) -> Vec { unconfirmed.sort_by_key(|(submitted_at, snapshot)| (*submitted_at, snapshot.strategy_id)); full.extend(unconfirmed.into_iter().map(|(_, snapshot)| snapshot)); - full + // The order this terminal last sent and the core has not answered yet. Without it every command + // here would rebuild the list in the CONFIRMED order and quietly undo a reorder still in + // flight — see [`QueuedOrder`]. + if let Some(order) = order { + let ranks: std::collections::HashMap = order + .iter() + .enumerate() + .map(|(rank, id)| (*id, rank)) + .collect(); + if crate::feed::strategy_order::resequence(&mut full, |sc| { + ranks.get(&sc.strategy_id).copied() + }) == 0 + { + // The confirmed list already holds this sequence, so there is nothing left to owe. The + // second retirement rule, and the one that covers a core whose Full carries no order + // version at all: `last_modified` then never moves, and the version test alone would + // keep re-asserting a sequence the core had already applied. + return (full, true); + } + } + + (full, false) +} + +/// Apply one folder-tree edit and send it, choosing the base and refusing what cannot be sent. +/// +/// The base is the newest tree this terminal knows: the one it last sent while the core has not +/// answered, otherwise the core's confirmed one. That is the whole reason folder edits arrive here +/// as intents — a tree assembled upstream is assembled from a snapshot that may already be stale, +/// and the wire form deletes every folder it omits. +/// +/// Refuses to send a tree moonproto would reject rather than discovering it as an error: some real +/// MoonBot folder names cannot survive its validator at all (see [`crate::feed::CoreFolders`]), and +/// on such a core a submission would be refused whole. +/// +/// Args: +/// client: The core's client. +/// server_id: Core id, for the log. +/// action: Log label. +/// strategy_placements: Guard holding the tree this terminal last sent. +/// edit: Rewrites the base into the desired tree. +/// +/// Returns: +/// Nothing; every refusal is logged where it happens. +fn folder_edit( + client: &MoonClient, + server_id: u64, + action: &str, + subject: &str, + strategy_placements: &mut StrategyPlacementGuard, + edit: impl FnOnce(&[String]) -> Vec, +) { + let Some(snap) = client.snapshot() else { + log::warn!( + "core {} {action} folder {subject:?} skipped: strategy state is not ready", + crate::feed::core_label(server_id) + ); + return; + }; + let strats = snap.strats(); + let last_modified = strats.folders_last_modified(); + if last_modified == 0 { + log::info!( + "core {} {action} folder {subject:?} skipped: this core keeps no folder tree", + crate::feed::core_label(server_id) + ); + return; + } + let confirmed: Vec = strats.folder_paths().map(str::to_string).collect(); + let base = strategy_placements.folder_base(confirmed, last_modified); + let desired = edit(&base); + // Validated the way moonproto validates it: a folder submission carries the tree, and the + // library checks every current strategy path alongside it. + let rows = strats.snapshots().map(|sc| sc.path.as_ref()); + if !crate::feed::folder_tree::sendable(desired.iter().map(String::as_str).chain(rows)) { + log::warn!( + "core {} {action} folder {subject:?} skipped: this core's tree holds a path MoonProto refuses", + crate::feed::core_label(server_id) + ); + return; + } + let count = desired.len(); + match client.strategies().sync_local_folders(desired.clone()) { + Ok(()) => { + strategy_placements.note_queued_folders(desired, last_modified); + log::info!( + "core {} {action} folder {subject:?}, {count} in the tree", + crate::feed::core_label(server_id) + ); + } + Err(error) => log::warn!( + "core {} {action} folder {subject:?} failed: {error}", + crate::feed::core_label(server_id) + ), + } +} + +/// Join every relocated row to the run its destination folder ALREADY occupies. +/// +/// Called after a move has rewritten `path`, so a folder the operator dropped rows into stays one +/// contiguous group rather than two — the core asks for that (moonproto `docs/strats.md`, "Strategy +/// Order"), and the tree places a folder where its first strategy appears, so a row left at its old +/// index can drag a whole folder to a new place from a gesture that named neither. +/// +/// Three rules keep it from moving anything it was not asked to: +/// +/// * Only rows whose path actually CHANGED are considered. A drag that includes rows already in +/// the destination leaves those exactly where they are. +/// * The anchor is a row that was NOT part of this move. So a folder RENAME — where every row +/// carrying the new name is one of the renamed ones — relocates nothing at all, and neither +/// does a move into a folder that does not exist yet. +/// * Rows joining the same run are placed in the order they were given, one after another. +/// +/// Built as one rebuilding pass rather than a sequence of `remove`/`insert` calls. That is not a +/// matter of cost: every removal shifts every later index, so a plan expressed in positions goes +/// stale the moment two destinations interleave — which `ops::move_folder` and `ops::rename_folder` +/// both produce — and the rows then land one slot early, splitting the very runs this repairs. +/// Positions here are only ever read from the ORIGINAL list, and each row is emitted exactly once. +/// +/// Args: +/// full: The complete strategy set, already carrying the new paths. +/// relocated: `(strategy id, new folder path)` for the rows whose folder actually changed. +/// +/// Returns: +/// Nothing; a row with no existing destination run to join is left untouched. +fn regroup_moved(full: &mut Vec, relocated: &[(u64, String)]) { + let moved: std::collections::HashSet = relocated.iter().map(|(id, _)| *id).collect(); + let index_of: std::collections::HashMap = full + .iter() + .enumerate() + .map(|(at, sc)| (sc.strategy_id, at)) + .collect(); + + // Per anchor row, the ids that follow it. The anchor is the LAST row of that destination this + // move did not touch; without one there is no run to join and the row is left alone. + let mut following: std::collections::HashMap> = + std::collections::HashMap::new(); + let mut joining: std::collections::HashSet = std::collections::HashSet::new(); + for (id, path) in relocated { + if !index_of.contains_key(id) { + continue; + } + let anchor = full + .iter() + .rposition(|sc| sc.path.as_ref() == path && !moved.contains(&sc.strategy_id)); + if let Some(anchor) = anchor { + following.entry(anchor).or_default().push(*id); + joining.insert(*id); + } + } + if joining.is_empty() { + return; + } + + let mut slots: Vec> = + std::mem::take(full).into_iter().map(Some).collect(); + let mut rebuilt: Vec = Vec::with_capacity(slots.len()); + for at in 0..slots.len() { + let Some(id) = slots[at].as_ref().map(|sc| sc.strategy_id) else { + continue; + }; + if joining.contains(&id) { + // Emitted behind its anchor instead, wherever that sits. + continue; + } + let Some(row) = slots[at].take() else { + continue; + }; + rebuilt.push(row); + let Some(ids) = following.get(&at) else { + continue; + }; + for id in ids { + if let Some(row) = index_of.get(id).and_then(|from| slots[*from].take()) { + rebuilt.push(row); + } + } + } + *full = rebuilt; } /// Shared strategy-sync path: load the COMPLETE current set, let `build` edit it (patch fields, @@ -342,22 +637,43 @@ fn rebuild_sync( server_id: u64, action: &str, strategy_placements: &mut StrategyPlacementGuard, + folders: Option>, build: impl FnOnce(&mut Vec, Option<&StrategySchema>, u64) -> usize, ) -> bool { if let Some(snap) = client.snapshot() { let strats = snap.strats(); let schema = strats.strategy_schema(); let now = now_ms() as u64; - let mut full: Vec = overlay_pending_edits(strats); + // Read before the list is built: it is both the baseline the queued order is judged against + // and the one recorded with the next send. + let last_modified = strats.last_modified(); + let (mut full, order_satisfied) = + overlay_pending_edits(strats, strategy_placements.pending_order(last_modified)); + if order_satisfied { + strategy_placements.retire_order(); + } let changed = build(&mut full, schema, now); - if changed > 0 { + // A folder tree is worth a snapshot on its own: a rename whose rows all vanished between + // queueing and here still has to take the emptied folder with it. + if changed > 0 || folders.is_some() { let placements = full .iter() .map(|strategy| (strategy.strategy_id, strategy.path.to_string())) .collect(); - match client.strategies().sync_local_strategies(full) { + let sequence: Vec = full.iter().map(|strategy| strategy.strategy_id).collect(); + // One snapshot for both when a folder tree comes along: the core applies the + // strategy changes first and the newer tree second, which is what removes a folder the + // strategies have just left. Sent as two commands they could arrive the other way + // round, and the core would then refuse to drop a folder that still held rows. + let queued = match folders { + Some(paths) => client + .strategies() + .sync_local_strategies_with_folders(full, paths), + None => client.strategies().sync_local_strategies(full), + }; + match queued { Ok(()) => { - strategy_placements.note_queued_sync(placements); + strategy_placements.note_queued_sync(placements, sequence, last_modified); log::info!( "core {} {action} {changed} strategies", crate::feed::core_label(server_id) @@ -467,6 +783,8 @@ pub(super) fn drain_commands( server.id, "edit", strategy_placements, + // No folder tree: these edit rows, never the set of folders. + None, |full, schema, now| { let mut edited = 0usize; for sc in full.iter_mut() { @@ -632,6 +950,8 @@ pub(super) fn drain_commands( server.id, "create", strategy_placements, + // No folder tree: these edit rows, never the set of folders. + None, |full, schema, now| { let mut next_id = full.iter().map(|s| s.strategy_id).max().unwrap_or(0) + 1; // Plan the whole batch before insertion because each insertion shifts every @@ -681,6 +1001,8 @@ pub(super) fn drain_commands( server.id, "restore", strategy_placements, + // No folder tree: these edit rows, never the set of folders. + None, |full, schema, now| { // It is already live (double-click in the menu or an echo), so do not duplicate it. if full.iter().any(|s| s.strategy_id == id) { @@ -708,28 +1030,122 @@ pub(super) fn drain_commands( local_strat_edits.mark(id); } } - Ok(CoreCmd::MoveStrategies { moves }) => { + Ok(CoreCmd::MoveStrategies { moves, rebase }) => { + // The folder half, built HERE from the newest tree this terminal knows. Declined + // whole — leaving the strategies to travel alone — when the result is something + // MoonProto would refuse, because it validates the bundle as one: a tree it will + // not take would otherwise turn a working rename into a refusal that moves nothing. + let planned = rebase.and_then(|(old_key, new_key)| { + let snap = client.snapshot()?; + let strats = snap.strats(); + let last_modified = strats.folders_last_modified(); + if last_modified == 0 { + return None; + } + let confirmed: Vec = + strats.folder_paths().map(str::to_string).collect(); + let base = strategy_placements.folder_base(confirmed, last_modified); + let desired = crate::feed::folder_tree::rebase(&base, &old_key, &new_key); + // Every path the submission carries goes through the same validator as the + // tree — the folders, the rows as they stand, and the paths this move is about + // to give them, which is where a name the operator just typed shows up. + let rows = strats.snapshots().map(|sc| sc.path.as_ref()); + let targets = moves.iter().map(|(_, path)| path.as_str()); + let sendable = crate::feed::folder_tree::sendable( + desired + .iter() + .map(String::as_str) + .chain(rows) + .chain(targets), + ); + if !sendable { + log::warn!( + "core {} move {old_key:?} -> {new_key:?}: folder tree left out, a path MoonProto refuses", + crate::feed::core_label(server.id) + ); + return None; + } + Some((desired, last_modified)) + }); + let folders = planned.as_ref().map(|(desired, _)| desired.clone()); // Change `path` and increment `last_date` for the selected strategies in one sync. - // Nothing is recorded on the strength of this send, so its answer is not needed. - let _ = rebuild_sync( + let queued = rebuild_sync( client, server.id, "move", strategy_placements, + folders, |full, _schema, now| { let mut changed = 0usize; + let mut relocated: Vec<(u64, String)> = Vec::new(); for sc in full.iter_mut() { if let Some((_, new_path)) = moves.iter().find(|(id, _)| *id == sc.strategy_id) { + if sc.path.as_ref() != new_path.as_str() { + relocated.push((sc.strategy_id, new_path.clone())); + } sc.path = new_path.as_str().into(); sc.last_date = now.max(sc.last_date + 1); changed += 1; } } + regroup_moved(full, &relocated); changed }, ); + // Recorded only once the queue has taken it. A tree noted before the send would + // become the base of the NEXT folder edit while the core never received it. + if let (true, Some((desired, base))) = (queued, planned) { + strategy_placements.note_queued_folders(desired, base); + } + } + Ok(CoreCmd::AddFolder { path }) => { + folder_edit( + client, + server.id, + "add", + &path, + strategy_placements, + |base| crate::feed::folder_tree::with_added(base, &path), + ); + } + Ok(CoreCmd::RemoveFolder { path }) => { + folder_edit( + client, + server.id, + "remove", + &path, + strategy_placements, + |base| crate::feed::folder_tree::without(base, &path), + ); + } + Ok(CoreCmd::ReorderStrategies { order }) => { + // The new SEQUENCE is the whole edit: no field is patched and no `last_date` moves, + // because moonproto versions strategy order separately from per-strategy edit + // dates and reads the order off the row sequence of the Full snapshot this sends. + let _ = rebuild_sync( + client, + server.id, + "reorder", + strategy_placements, + None, + |full, _schema, _now| { + let ranks: std::collections::HashMap = order + .iter() + .enumerate() + .map(|(rank, id)| (*id, rank)) + .collect(); + // Counted against the list as this terminal last left it — `full` + // arrives already carrying any order still owed to the core — so pressing + // Down and then Up inside one round trip is seen for what it is: a real + // change back, rather than a no-op against a confirmed order the core is + // no longer holding. + crate::feed::strategy_order::resequence(full, |sc| { + ranks.get(&sc.strategy_id).copied() + }) + }, + ); } Ok(CoreCmd::TransferAsset { asset, diff --git a/crates/moon-core/src/feed/live/commands/tests.rs b/crates/moon-core/src/feed/live/commands/tests.rs index 714de79c..57c7deae 100644 --- a/crates/moon-core/src/feed/live/commands/tests.rs +++ b/crates/moon-core/src/feed/live/commands/tests.rs @@ -1,7 +1,10 @@ //! Placement of newly created strategies plus snapshot guards for destructive strategy commands. +use moonproto::StrategySnapshot; + use super::{ - StrategyPlacementGuard, anchor_on_core, plan_insert_positions, strategy_placements_unchanged, + StrategyPlacementGuard, anchor_on_core, plan_insert_positions, regroup_moved, + strategy_placements_unchanged, }; /// An anchor is honoured only on the core it names. @@ -127,7 +130,7 @@ fn conditional_deletes_require_live_and_queued_placements_to_agree() { let original = vec![(1, "alpha".to_string())]; let moved = vec![(1, "beta".to_string())]; let mut guard = StrategyPlacementGuard::new(); - guard.note_queued_sync(moved.clone()); + guard.note_queued_sync(moved.clone(), vec![1], 0); assert!(!guard.allows_snapshot(Some(original.clone()), original)); assert!(guard.allows_snapshot(Some(moved.clone()), moved.clone())); @@ -178,7 +181,7 @@ fn every_full_list_sync_updates_the_placement_shadow() { .find("client.strategies().sync_local_strategies(full)") .expect("the rebuild path must queue its full list"); let shadow = body - .find("strategy_placements.note_queued_sync(placements)") + .find("strategy_placements.note_queued_sync(placements,") .expect("accepted full-list syncs must update the synchronous shadow"); assert!( @@ -275,3 +278,173 @@ fn only_an_applied_edit_claims_local_origin() { "the claim must follow the rebuild that decides what was actually edited" ); } + +/// `StrategyPlacementGuard::pending_order`: a reorder is owed to the core until the core itself +/// publishes an order. Every other strategy command in that window rebuilds its outgoing list from +/// the CONFIRMED order, so without this the next checkbox or field edit would hand the core back +/// the arrangement the operator had just replaced. +#[test] +fn a_queued_order_is_owed_to_the_core_until_it_publishes_one() { + let mut guard = StrategyPlacementGuard::new(); + assert_eq!(guard.pending_order(7), None); + + guard.note_queued_sync(vec![(1, String::new())], vec![3, 1, 2], 7); + // The confirmed order has not moved, so this terminal's sequence is still the newest word. + assert_eq!(guard.pending_order(7), Some([3, 1, 2].as_slice())); + assert_eq!(guard.pending_order(7), Some([3, 1, 2].as_slice())); +} + +/// The other half of the same rule, and the one that makes it terminate: once the core has +/// published an order — accepting ours or overruling it — the queued sequence is dropped. Kept, it +/// would be re-asserted on every later sync forever, against a core that had already answered. +#[test] +fn the_cores_own_published_order_retires_the_queued_one() { + let mut guard = StrategyPlacementGuard::new(); + guard.note_queued_sync(vec![(1, String::new())], vec![3, 1, 2], 7); + assert_eq!(guard.pending_order(9), None); + // ... and it stays retired, including for a later call that repeats the old version. + assert_eq!(guard.pending_order(7), None); +} + +/// Builds a snapshot carrying only what [`regroup_moved`] reads: its id and its folder path. +fn placed(id: u64, path: &str) -> StrategySnapshot { + StrategySnapshot::new( + id, + 1, + 0, + false, + moonproto::StrategyKind::from_ordinal(0), + path, + Default::default(), + ) +} + +/// Names the folder of each row in order, which is what the contiguity rule is about. +fn paths(full: &[StrategySnapshot]) -> Vec<(u64, String)> { + full.iter() + .map(|sc| (sc.strategy_id, sc.path.to_string())) + .collect() +} + +/// A drag into a folder that already holds strategies joins that folder's run, so the destination +/// stays ONE group. Left split, the tree — which places a folder where its first strategy appears — +/// can hoist that whole folder somewhere nobody asked for. +#[test] +fn a_move_joins_the_run_its_destination_already_occupies() { + let mut full = vec![ + placed(1, "X"), + placed(2, "X"), + placed(3, "Z"), + placed(4, "X"), + ]; + // 4 was relabelled to X by the caller and now has to reach it. + regroup_moved(&mut full, &[(4, "X".to_string())]); + assert_eq!( + paths(&full), + vec![ + (1, "X".into()), + (2, "X".into()), + (4, "X".into()), + (3, "Z".into()) + ] + ); +} + +/// Several rows moved at once queue up behind each other in the order they were given, instead of +/// all taking the same slot — which would reverse them — or anchoring on each other and splitting +/// the run they are trying to join. +#[test] +fn rows_moved_together_land_in_the_order_they_were_given() { + let mut full = vec![ + placed(1, "X"), + placed(2, "X"), + placed(3, "Z"), + placed(4, "X"), + ]; + regroup_moved(&mut full, &[(2, "X".to_string()), (4, "X".to_string())]); + assert_eq!( + paths(&full), + vec![ + (1, "X".into()), + (2, "X".into()), + (4, "X".into()), + (3, "Z".into()) + ] + ); +} + +/// A folder RENAME reaches this through the same command, and it must move nothing: every row +/// carrying the new name is one of the renamed ones, so there is no existing run to join. Relocating +/// on a rename would silently change the folder's place in the tree. +#[test] +fn a_rename_relocates_nothing() { + let mut full = vec![placed(1, "B"), placed(2, "C"), placed(3, "B")]; + regroup_moved(&mut full, &[(1, "B".to_string()), (3, "B".to_string())]); + assert_eq!( + paths(&full), + vec![(1, "B".into()), (2, "C".into()), (3, "B".into())] + ); +} + +/// A move into a folder that does not exist yet has nothing to join either, so the row stays where +/// it is and the new folder is created around it. The alternative — appending to the end of the +/// whole list — is the placement this module exists to avoid. +#[test] +fn a_move_into_a_new_folder_leaves_the_row_in_place() { + let mut full = vec![placed(1, "X"), placed(2, "NEW"), placed(3, "X")]; + regroup_moved(&mut full, &[(2, "NEW".to_string())]); + assert_eq!( + paths(&full), + vec![(1, "X".into()), (2, "NEW".into()), (3, "X".into())] + ); +} + +/// A row that sits BEFORE its destination run still joins it. The case is worth its own test +/// because a plan expressed in positions gets this one wrong in the opposite direction from the +/// backward move above. +#[test] +fn a_row_ahead_of_its_destination_still_joins_it() { + let mut full = vec![placed(1, "X"), placed(2, "Z"), placed(3, "Z")]; + regroup_moved(&mut full, &[(1, "Z".to_string())]); + assert_eq!( + paths(&full), + vec![(2, "Z".into()), (3, "Z".into()), (1, "X".into())] + ); +} + +/// Two destinations interleaved in one move — what a folder move or rename produces whenever it +/// merges into folders that already exist. Each run comes out whole: a plan carried as absolute +/// positions goes stale as soon as the first relocation crosses another destination's slot, and +/// then both folders end up split. +#[test] +fn two_destinations_in_one_move_each_come_out_contiguous() { + let mut full = vec![ + placed(101, "D2"), + placed(1, "D1"), + placed(11, "D1"), + placed(21, "D2"), + placed(22, "D2"), + placed(12, "D1"), + ]; + regroup_moved( + &mut full, + &[ + (11, "D1".to_string()), + (21, "D2".to_string()), + (22, "D2".to_string()), + (12, "D1".to_string()), + ], + ); + let order = paths(&full); + let at = |id: u64| order.iter().position(|(row, _)| *row == id).expect("row"); + // Every D1 row adjacent to the others, and likewise every D2 row. + let mut d1 = [at(1), at(11), at(12)]; + let mut d2 = [at(101), at(21), at(22)]; + d1.sort_unstable(); + d2.sort_unstable(); + assert_eq!(d1[2] - d1[0], 2, "D1 must be one contiguous run: {order:?}"); + assert_eq!(d2[2] - d2[0], 2, "D2 must be one contiguous run: {order:?}"); + // ... and the rows joining each run keep the order they were given. + assert!(at(11) < at(12), "D1 joiners keep their order: {order:?}"); + assert!(at(21) < at(22), "D2 joiners keep their order: {order:?}"); +} diff --git a/crates/moon-core/src/feed/live/convert.rs b/crates/moon-core/src/feed/live/convert.rs index ee53af53..0c969e55 100644 --- a/crates/moon-core/src/feed/live/convert.rs +++ b/crates/moon-core/src/feed/live/convert.rs @@ -1568,5 +1568,95 @@ pub(super) fn engine_action_result(e: &moonproto::EngineActionEvent) -> EngineAc } } +/// Longest folder path kept from the core's tree, in characters. +/// +/// A DISPLAY guard and nothing more: this projection is never the source of an outgoing tree — the +/// feed builds those from the core's own untouched paths — so clamping here cannot ask the core to +/// rename anything. Whether a path could be sent back at all is a separate question, answered once +/// per core by `CoreFolders::editable`. +const FOLDER_PATH_MAX_CHARS: usize = 255; + +/// How many folders one core's tree is kept to. +/// +/// moonproto's own ceiling is 65 534 dictionary entries; this sits far below it because the list is +/// retained per core and drawn as rows. Truncating is safe for the same reason the clamp above is: +/// nothing sends this list back. +const FOLDER_PATHS_MAX: usize = 5_000; + +/// Project the core's folder tree, empty folders included. +/// +/// `supported` is `folders_last_modified() > 0` and nothing else: the protocol states plainly that +/// zero means either "no versioned tree has arrived" or "this core does not synchronize folders", +/// and the two are indistinguishable from here. Both readings mean the same thing to a caller — an +/// empty folder cannot be sent to this core — so nothing is gained by guessing which one holds. +/// +/// Paths are cleaned like every other inbound string, and the caller does the path SPLITTING: which +/// slashes separate folders is decided in one place in the window, and a second rule here would +/// disagree with the tree it feeds. +/// +/// Args: +/// strats: The core's retained strategy state. +/// +/// Returns: +/// The reported tree, or a `supported: false` value with no paths. +pub(super) fn folders_from_proto( + strats: &moonproto::state::StratsState, +) -> crate::feed::CoreFolders { + let supported = strats.folders_last_modified() > 0; + if !supported { + return crate::feed::CoreFolders::default(); + } + // Asked of the paths as the CORE spells them, before any cleaning: whether an edit can be sent + // depends on what the core holds, not on what this projection made of it. + let editable = crate::feed::folder_tree::sendable(strats.folder_paths()); + let mut paths: Vec = strats + .folder_paths() + .map(|path| wire_text(path, FOLDER_PATH_MAX_CHARS)) + .filter(|path| !path.is_empty()) + .collect(); + // Sorted because the source order is explicitly meaningless — moonproto iterates a map and says + // so — and an unstable order would republish an unchanged tree on every rehash, waking the + // window and rebuilding its whole strategy tree for nothing. + // + // BEFORE the cap, so that which folders survive it is decided by the paths themselves rather + // than by where a rehash happened to put them. + paths.sort_unstable(); + paths.dedup(); + paths.truncate(FOLDER_PATHS_MAX); + crate::feed::CoreFolders { + supported, + editable, + paths, + } +} + +/// Fold the strategy set into the signature that decides whether the UI is told about it. +/// +/// ORDER-SENSITIVE, and that is the load-bearing property rather than an artefact of the fold. The +/// core's strategy list is an arrangement the operator made, moonproto synchronizes it, and the +/// terminal's whole confirmation path — the tree's unconfirmed-order overlay, which draws a sent +/// arrangement until the core answers — can only be retired by seeing the answer arrive. Folded +/// commutatively, as a sibling signature in `tree::cache` deliberately is, a reorder echo would be +/// invisible: the overlay would sit out its whole window and then expire onto a list it had already +/// been given. `a_reordered_set_is_a_different_signature` pins it. +/// +/// Args: +/// rows: Per strategy, `(id, revision, last edit date, checked)`, in the core's own order. +/// +/// Returns: +/// A signature that changes whenever the contents OR the sequence do. +pub(super) fn strategies_publish_sig(rows: impl Iterator) -> u64 { + let mut sig = 0u64; + for (id, ver, last_date, checked) in rows { + sig = sig + .wrapping_mul(1099511628211) + .wrapping_add(id) + .wrapping_add((ver as u32 as u64).wrapping_shl(1)) + .wrapping_add(last_date) + .wrapping_add(checked as u64); + } + sig +} + #[cfg(test)] mod tests; diff --git a/crates/moon-core/src/feed/live/convert/tests.rs b/crates/moon-core/src/feed/live/convert/tests.rs index 47af3a88..438431e7 100644 --- a/crates/moon-core/src/feed/live/convert/tests.rs +++ b/crates/moon-core/src/feed/live/convert/tests.rs @@ -421,3 +421,34 @@ fn a_delivered_finding_counts_as_support_before_the_first_list() { "an answered core with no findings is not silent either" ); } + +/// The publish gate is the ONLY way a reorder echo reaches the window: the tree draws a sent +/// arrangement until the core answers, and it recognises the answer by the strategy set arriving +/// again. A commutative fold here — the shape a sibling signature in `tree::cache` uses on purpose +/// — would make an order-only change invisible and strand that overlay for its whole window. +#[test] +fn a_reordered_set_is_a_different_signature() { + let rows = [(1u64, 1i32, 100u64, true), (2, 1, 200, false)]; + let forward = super::strategies_publish_sig(rows.iter().copied()); + let reversed = super::strategies_publish_sig(rows.iter().rev().copied()); + assert_ne!(forward, reversed); + // ... while the same set in the same order still says nothing changed. + assert_eq!(forward, super::strategies_publish_sig(rows.iter().copied())); +} + +/// The contents still matter as much as the sequence: one flag or one edit date moving on its own +/// must republish, which is what every non-order change relies on. Only that — the fold sums its +/// four per-row fields, so two changes that cancel out numerically are not separated, which is the +/// ordinary trade of a cheap signature and not something this asserts otherwise. +#[test] +fn a_changed_field_is_a_different_signature() { + let base = [(1u64, 1i32, 100u64, true)]; + let checked_off = [(1u64, 1i32, 100u64, false)]; + let edited = [(1u64, 1i32, 101u64, true)]; + let sig = super::strategies_publish_sig(base.iter().copied()); + assert_ne!( + sig, + super::strategies_publish_sig(checked_off.iter().copied()) + ); + assert_ne!(sig, super::strategies_publish_sig(edited.iter().copied())); +} diff --git a/crates/moon-core/src/feed/live/mod.rs b/crates/moon-core/src/feed/live/mod.rs index 376fecdf..0f246e06 100644 --- a/crates/moon-core/src/feed/live/mod.rs +++ b/crates/moon-core/src/feed/live/mod.rs @@ -574,6 +574,13 @@ pub(super) fn run( // changes because strategy fields are expensive and need not be sent every second. let mut last_schema_rev: u64 = u64::MAX; let mut last_strat_sig: u64 = u64::MAX; + // Folder-tree cursors. Two of them because the tree moves for two independent reasons: the core + // versions it whenever a folder is created or deleted, and the folders the STRATEGIES imply + // change whenever a strategy's path does — which the version does not see. + let mut last_folders_version: i64 = i64::MIN; + // Seeded with the digest of an EMPTY tree, which is what a core reports before it has said + // anything: `u64::MAX` would make the first pass publish that nothing-yet as a change. + let mut last_folders_digest: u64 = 0; // Strategy-edit publish cadence, independent of the 1 Hz strategies gate below. The retained // latch is set on ANY edit event and cleared only once a publish actually goes out, so a // resolution arriving inside the 250 ms shadow of the previous publish is delayed, never @@ -2200,15 +2207,11 @@ pub(super) fn run( } // Publish contents/values when the signature changes (id/ver/last_date/checked). - let mut sig = 0u64; - for s in strats.snapshots() { - sig = sig - .wrapping_mul(1099511628211) - .wrapping_add(s.strategy_id) - .wrapping_add((s.strategy_ver as u32 as u64).wrapping_shl(1)) - .wrapping_add(s.last_date) - .wrapping_add(s.checked as u64); - } + let sig = convert::strategies_publish_sig( + strats + .snapshots() + .map(|s| (s.strategy_id, s.strategy_ver, s.last_date, s.checked)), + ); let delivery_result = pending_strat_db_delivery .as_ref() @@ -2227,7 +2230,8 @@ pub(super) fn run( &mut strat_db_initial, ); } - if sig != last_strat_sig { + let strategies_changed = sig != last_strat_sig; + if strategies_changed { last_strat_sig = sig; // The order table's Strat column resolves strat_id to kind through this same // registry in `build_order_row`. The registry is populated AFTER orders, while @@ -2263,6 +2267,34 @@ pub(super) fn run( break; } } + + // The core's folder tree, empty folders included. Read on its own cursor rather + // than inside the block above: a folder created or deleted with no strategy in it + // moves nothing the strategy signature covers, and that folder is exactly the one + // this list exists to carry. + let folders_version = strats.folders_last_modified(); + if folders_version != last_folders_version || strategies_changed { + last_folders_version = folders_version; + let folders = convert::folders_from_proto(strats); + // Digested rather than compared: the tree is republished on every strategy + // change, and holding a second copy of every path per core to answer "did it + // move" costs more than the fold does. + let digest = folders.paths.iter().fold( + u64::from(folders.supported) | (u64::from(folders.editable) << 1), + |acc, path| { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + std::hash::Hash::hash(path, &mut hasher); + acc.wrapping_mul(1099511628211) + .wrapping_add(std::hash::Hasher::finish(&hasher)) + }, + ); + if digest != last_folders_digest { + last_folders_digest = digest; + if tx.send(FeedMsg::Folders(folders)).is_err() { + break; + } + } + } // The database cursor is separate from the UI cursor: schema defaults can arrive // after an unchanged strategy set, and a full writer queue must leave the set due // for retry. Dumps are incomplete until defaults are available. diff --git a/crates/moon-core/src/feed/mod.rs b/crates/moon-core/src/feed/mod.rs index a3ffc553..83b1cb4a 100644 --- a/crates/moon-core/src/feed/mod.rs +++ b/crates/moon-core/src/feed/mod.rs @@ -4,12 +4,14 @@ pub(crate) mod assets; mod conn_verdict; mod core_label; +pub mod folder_tree; pub mod live; mod mode_advice; pub mod news; pub mod news_marks; mod order_edit; mod strategies; +pub mod strategy_order; pub mod synth; mod trade; pub mod types; @@ -366,7 +368,45 @@ pub enum CoreCmd { /// Move existing strategies or rename their folder. Each `moves` entry contains /// `(strategy_id, new_folder_path)`. The feed patches `path` for the listed strategies in the /// full set, advances `last_date`, and sends one `sync_local_strategies`. - MoveStrategies { moves: Vec<(u64, String)> }, + MoveStrategies { + moves: Vec<(u64, String)>, + /// The folder subtree this move renames or reparents, as `(old path, new path)`. + /// + /// An INTENT, not a tree. The core now keeps folders that hold no strategy, so rewriting + /// the rows' paths leaves the OLD path behind as an empty folder of its own, and only a + /// folder tree omitting it removes it — but which tree to omit it FROM is a question only + /// the feed can answer, because the newest one it knows includes edits the core has not + /// echoed yet. The protocol wants both halves in one snapshot, so the feed builds the tree + /// and sends it with these moves. + /// + /// `None` for a move that changes no folder's identity — dragging strategies between + /// folders, which must NOT delete the folder they came from. + rebase: Option<(String, String)>, + }, + /// Create one folder on the core, holding nothing. + /// + /// Carries the path alone, deliberately. The wire form is the complete desired tree — the core + /// deletes every empty folder the list omits — and a tree assembled by a window is assembled + /// from a snapshot that may already be stale, which turns a create into a silent delete of + /// whatever arrived meanwhile. The feed owns the list; this says only what to add to it. + AddFolder { path: String }, + /// Remove one folder and everything under it from the core's tree. + /// + /// For a folder that holds no strategy; rows are deleted separately and first. Same reasoning + /// as [`CoreCmd::AddFolder`]: the intent travels, the list is built where it is known. + RemoveFolder { path: String }, + /// Rearrange the core's strategy list. `order` is the complete desired id sequence. + /// + /// The sequence itself is the payload: moonproto synchronizes strategy order as the row order + /// of a Full snapshot, so the feed sorts the full set into `order` and sends one + /// `sync_local_strategies`. Nothing else about a strategy changes — no field is patched and no + /// `last_date` is advanced, because the order carries its own version on the wire (moonproto + /// `docs/strats.md`, "Strategy Order"). + /// + /// Ids the core does not have are ignored, and ids the core has but `order` omits keep their + /// relative places at the end, so a list that raced with a create or a delete still reorders + /// what it does name instead of dropping anything. + ReorderStrategies { order: Vec }, /// Transfer an asset between wallets of one core through drag and drop in the Assets tree. /// `from` and `to` are Spot, Futures, or Quarterly wallets; `qty` is in the base coin. TransferAsset { diff --git a/crates/moon-core/src/feed/strategy_order.rs b/crates/moon-core/src/feed/strategy_order.rs new file mode 100644 index 00000000..d5666a26 --- /dev/null +++ b/crates/moon-core/src/feed/strategy_order.rs @@ -0,0 +1,77 @@ +//! Applying a desired strategy sequence to a list that has moved on since it was chosen. +//! +//! A reorder names every strategy the operator could see when they pressed the button. By the time +//! it is applied — one frame later in the window, one round trip later in the core — the list can +//! hold strategies the sequence never named: one created, pasted or restored in between, or one +//! this side had simply not received yet. +//! +//! Where those unnamed rows land is not a detail. Sorting them to the END looks harmless and is +//! not: the core requires one folder's strategies to stay a contiguous group (moonproto +//! `docs/strats.md`, "Strategy Order"), so a new strategy flung to the tail leaves its folder split +//! in two, and the NEXT reorder sends that tail position back to the core as the deliberate +//! arrangement. One press then permanently moves a strategy nobody touched. +//! +//! So an unnamed row keeps the row it currently follows. That is the only placement that is a +//! no-op for it — it neither leaves its folder nor changes what it is adjacent to. + +/// Rearrange `items` into the desired sequence, keeping rows the sequence never named in place. +/// +/// Stable: rows sharing a position keep their current relative order. +/// +/// Args: +/// items: The full current list, in its current order. +/// rank_of: Position of one item in the desired sequence, or `None` when it names no position. +/// +/// Returns: +/// How many items ended up at a different index than they started at — zero when the list +/// already holds the desired arrangement, which callers use to send nothing at all. +pub fn resequence(items: &mut Vec, rank_of: impl Fn(&T) -> Option) -> usize { + /// Where one row sorts: its desired position, and whether it is a named row, one trailing a + /// named row, or one that precedes every named row. + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + struct Slot(usize, u8); + + let mut slots: Vec = Vec::with_capacity(items.len()); + // Rank of the last named row seen, so an unnamed row can attach itself to it. + let mut trailing: Option = None; + for item in items.iter() { + slots.push(match rank_of(item) { + Some(rank) => { + trailing = Some(rank); + Slot(rank, 1) + } + // Directly after the named row it currently follows — or, before any named row has + // been seen, ahead of the whole sequence, which is equally where it already sits. + None => match trailing { + Some(rank) => Slot(rank, 2), + None => Slot(0, 0), + }, + }); + } + + let mut order: Vec = (0..items.len()).collect(); + order.sort_by_key(|&at| (slots[at], at)); + let moved = order + .iter() + .enumerate() + .filter(|(now, was)| *now != **was) + .count(); + if moved > 0 { + // Moved, never cloned: `T` here is a whole `StrategySnapshot` with every field string it + // carries, and a reorder that deep-copied the account's entire strategy set would spend + // more on the copy than on the sync it exists to prepare. Taking each element out through + // an `Option` is what lets the permutation be applied by index without `T: Clone`. + let mut taken: Vec> = std::mem::take(items).into_iter().map(Some).collect(); + *items = order + .into_iter() + // `order` is a sorted `0..len`, so every slot is visited exactly once. Stated rather + // than skipped: were that ever untrue, dropping the slot would quietly SHORTEN the + // strategy list this becomes on the wire. + .map(|at| taken[at].take().expect("resequence visits each index once")) + .collect(); + } + moved +} + +#[cfg(test)] +mod tests; diff --git a/crates/moon-core/src/feed/strategy_order/tests.rs b/crates/moon-core/src/feed/strategy_order/tests.rs new file mode 100644 index 00000000..fe051b23 --- /dev/null +++ b/crates/moon-core/src/feed/strategy_order/tests.rs @@ -0,0 +1,56 @@ +//! Unit tests for applying a desired sequence to a list that has moved on. + +use super::resequence; + +/// Runs one resequence over ids, ranking the ones the desired sequence names. +fn apply(items: &[u64], desired: &[u64]) -> (Vec, usize) { + let mut items = items.to_vec(); + let moved = resequence(&mut items, |id| desired.iter().position(|want| want == id)); + (items, moved) +} + +/// The ordinary case: every row is named, and the list comes out in exactly that sequence. +#[test] +fn a_fully_named_list_takes_the_desired_sequence() { + assert_eq!(apply(&[1, 2, 3], &[3, 1, 2]), (vec![3, 1, 2], 3)); +} + +/// Nothing moved means nothing to send, and that zero is what a caller uses to stay off the wire. +#[test] +fn an_order_the_list_already_holds_moves_nothing() { + assert_eq!(apply(&[1, 2, 3], &[1, 2, 3]), (vec![1, 2, 3], 0)); +} + +/// The point of the module. A strategy created after the sequence was chosen keeps the row it +/// follows instead of being flung to the end, where it would leave its folder split in two and be +/// sent back to the core as a deliberate arrangement by the next press. +#[test] +fn an_unnamed_row_keeps_the_row_it_follows() { + assert_eq!(apply(&[1, 9, 2, 3], &[3, 1, 2]), (vec![3, 1, 9, 2], 4)); +} + +/// Several unnamed rows behind the same named one keep their own order behind it. +#[test] +fn unnamed_rows_behind_one_row_keep_their_relative_order() { + assert_eq!(apply(&[1, 8, 9, 2], &[2, 1]), (vec![2, 1, 8, 9], 4)); +} + +/// A row that precedes every named one has nothing to follow, so it stays at the front — which is +/// also where it already is. +#[test] +fn a_row_before_every_named_one_stays_at_the_front() { + assert_eq!(apply(&[7, 1, 2], &[2, 1]), (vec![7, 2, 1], 2)); +} + +/// Ids the sequence names but the list does not hold are simply absent; they must not create gaps, +/// duplicate anything, or drop a row. +#[test] +fn ids_the_list_no_longer_holds_are_ignored() { + assert_eq!(apply(&[1, 3], &[3, 2, 1]).0, vec![3, 1]); +} + +/// An empty desired sequence names nothing at all, so every row keeps its place. +#[test] +fn an_empty_sequence_leaves_the_list_alone() { + assert_eq!(apply(&[1, 2, 3], &[]), (vec![1, 2, 3], 0)); +} diff --git a/crates/moon-core/src/feed/types.rs b/crates/moon-core/src/feed/types.rs index 3bd2b1a7..9b8cc67e 100644 --- a/crates/moon-core/src/feed/types.rs +++ b/crates/moon-core/src/feed/types.rs @@ -1,10 +1,12 @@ //! Domain types sent from the backend to the UI. They are independent of moonproto so the UI and //! rendering layer do not need to know about the transport. +mod core_folders; mod core_problem; mod core_settings; mod core_status; +pub use core_folders::CoreFolders; pub use core_problem::{CoreProblem, CoreProblemCategory, CoreProblems}; pub use core_settings::{ AutoBuySettings, AutoStartSettings, BtcBlinkSettings, CORE_HOTKEY_ACTION_COUNT, CoreConfig, @@ -1290,6 +1292,14 @@ pub enum FeedMsg { /// counter rather than on arrival — the core republishes the same list on reconnect and on /// every newly confirmed row alike. Problems(CoreProblems), + /// The core's folder tree, empty folders included, whenever it or the folders the strategies + /// imply have changed. + /// + /// A FULL replace like `Problems`, for the same reason: the protocol delivers the whole tree + /// and a folder that vanished from it has no event of its own. `folders_rev` moves only when + /// the projection actually differs, so a core republishing an identical tree on reconnect + /// wakes nothing. + Folders(CoreFolders), /// Core startup progress and channel measurements, POLLED from the moonproto client rather /// than pushed by an event — MoonProto publishes it as a passive snapshot at its own bounded /// rate. Sent only while the core is starting, plus once when it settles, so an already-started diff --git a/crates/moon-core/src/feed/types/core_folders.rs b/crates/moon-core/src/feed/types/core_folders.rs new file mode 100644 index 00000000..520b2dbd --- /dev/null +++ b/crates/moon-core/src/feed/types/core_folders.rs @@ -0,0 +1,54 @@ +//! The core's own folder tree — including the folders that hold no strategy at all. +//! +//! Until moonproto `55f78d0` a folder existed only as a prefix of some strategy's path, so an empty +//! one could not be represented on the wire and the terminal kept its own local list instead. The +//! core now maintains a VERSIONED folder tree and reports every folder in it, empty ones included +//! (`docs/strats.md`, "Folders, Including Empty Folders"). That makes "new folder" a real edit the +//! core can hold, rather than a mark that lives until the window closes. +//! +//! Whether a given core can do that is not a question this terminal gets to ask directly. The +//! answer is [`CoreFolders::supported`], and it means exactly one thing: a versioned tree has +//! arrived. A core too old to send one leaves it false forever, and every folder path here is then +//! whatever the strategies themselves imply. + +/// Folders one core currently holds, as it reports them. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct CoreFolders { + /// Whether this core has published a versioned folder tree. + /// + /// False covers two situations the terminal cannot tell apart and does not need to: a core + /// whose build predates folder synchronization, and one whose first tree has not arrived yet. + /// In both, an empty folder cannot be sent, so the caller keeps its local fallback and does not + /// promise the operator that a new folder will survive. + /// + /// Deliberately NOT latched across connections: a replacement feed can point at a different + /// MoonBot, and a core downgraded below the extension has to be able to go back to "not known". + pub supported: bool, + /// Whether this terminal may EDIT that tree. + /// + /// Stricter than [`Self::supported`], and the difference is a real account rather than a + /// hypothetical one. A folder edit is submitted as the complete desired tree, and moonproto + /// validates every path in it — splitting on each `/` and refusing a segment with surrounding + /// whitespace. MoonBot, meanwhile, allows a `/` INSIDE a folder name, and for a strategy in one + /// of those moonproto's own state adds the split halves as parent folders. So a core holding a + /// folder called `"EMA / ORGANIC"` reports a tree containing `"EMA "`, which nothing can send + /// back: every folder edit on that core would be refused whole. + /// + /// Rather than discover that per command, the terminal asks once. False here means the window + /// keeps its local marks and promises the operator nothing, exactly as for a core too old to + /// synchronize folders at all — while [`Self::supported`] still lets it DRAW what the core + /// reports. + pub editable: bool, + /// Every confirmed folder the core reports, parents included, sorted. + /// + /// SORTED here rather than kept in whatever order the core hands them over: the protocol calls + /// that order unspecified, moonproto iterates a map to produce it, and a set that reshuffles + /// itself would republish as a change on every rehash. + /// + /// A DISPLAY projection: paths are cleaned of control characters and clamped, so this is not + /// the list an edit is built from — the feed builds those from the core's own untouched paths. + /// Nothing here splits them either. Which slashes separate folders is a question this terminal + /// answers in ONE place, the Strategies window's own path module, and a second opinion formed + /// at the boundary would invent folders that exist nowhere. + pub paths: Vec, +} diff --git a/crates/moon-core/src/session/commands.rs b/crates/moon-core/src/session/commands.rs index c458d304..19cfbebc 100644 --- a/crates/moon-core/src/session/commands.rs +++ b/crates/moon-core/src/session/commands.rs @@ -260,12 +260,92 @@ impl SessionManager { } /// Change the folder of existing core strategies to rename a folder or move strategies. - /// Each `moves` entry is `(strategy_id, new_folder_path)`. Send one batch per core. - pub fn move_strategies(&self, core: CoreId, moves: Vec<(u64, String)>) -> Result<()> { - if moves.is_empty() { + /// + /// Args: + /// core: Core owning the strategies. + /// moves: `(strategy id, new folder path)` per strategy. Send one batch per core. + /// rebase: The folder subtree this move renames or reparents, as `(old path, new path)`. + /// A rename or a folder drag must pass one, or the emptied old path stays behind on the + /// core as a folder of its own — see [`CoreCmd::MoveStrategies`]. + /// + /// Returns: + /// Success once the intent entered that core's command queue. A call with neither moves nor + /// a rebase asks for nothing and is dropped here. + pub fn move_strategies( + &self, + core: CoreId, + moves: Vec<(u64, String)>, + rebase: Option<(String, String)>, + ) -> Result<()> { + // A folder holding no strategy has no moves to carry it, and its rename is still a real + // edit — so emptiness alone is not the test. + if moves.is_empty() && rebase.is_none() { + return Ok(()); + } + self.send_core_cmd( + core, + CoreCmd::MoveStrategies { moves, rebase }, + "move strategies", + ) + } + + /// Create one folder on a core, holding nothing. + /// + /// Args: + /// core: Core to create it on. + /// path: Canonical folder path. + /// + /// Returns: + /// Success once the intent entered that core's command queue. Whether the core can hold an + /// empty folder at all is decided on the feed thread, which logs what it declines; callers + /// read `CoreFolders::editable` to decide what to PROMISE the operator, not to gate this. + pub fn add_core_folder(&self, core: CoreId, path: String) -> Result<()> { + if path.is_empty() { return Ok(()); } - self.send_core_cmd(core, CoreCmd::MoveStrategies { moves }, "move strategies") + self.send_core_cmd(core, CoreCmd::AddFolder { path }, "add folder") + } + + /// Remove one folder and its whole subtree from a core's tree. + /// + /// Args: + /// core: Core to remove it from. + /// path: Canonical folder path. + /// + /// Returns: + /// Success once the intent entered that core's command queue. Strategies inside are NOT + /// deleted by this — the core keeps a folder that still holds one — so a caller removing a + /// populated folder deletes its rows first. + pub fn remove_core_folder(&self, core: CoreId, path: String) -> Result<()> { + if path.is_empty() { + return Ok(()); + } + self.send_core_cmd(core, CoreCmd::RemoveFolder { path }, "remove folder") + } + + /// Rearrange a core's strategy list into `order`, the complete desired id sequence. + /// + /// The core confirms by echoing a full strategy snapshot in that sequence; nothing here waits + /// for it. See [`CoreCmd::ReorderStrategies`] for what the feed does with a list that raced + /// with a create or a delete. + /// + /// Args: + /// core: Core whose list is being rearranged. + /// order: Every strategy id of that core, in the desired order. + /// + /// Returns: + /// Success once the intent entered that core's command queue. + /// + /// Deliberately without a "too short to matter" early return: one that answered `Ok(())` + /// without queueing anything would hand the caller a success for a command that does not + /// exist, and the caller draws an unconfirmed arrangement on the strength of it. Whether an + /// order says anything is the feed's decision, taken against the list it actually holds. + pub fn reorder_strategies(&self, core: CoreId, order: Vec) -> Result<()> { + self.send_core_cmd( + core, + CoreCmd::ReorderStrategies { order }, + "reorder strategies", + ) } /// Transfer an asset between wallets of one core through drag and drop in the Assets window. diff --git a/crates/moon-core/src/session/lifecycle.rs b/crates/moon-core/src/session/lifecycle.rs index a2265ff4..a4a8f547 100644 --- a/crates/moon-core/src/session/lifecycle.rs +++ b/crates/moon-core/src/session/lifecycle.rs @@ -377,6 +377,16 @@ impl SessionManager { stats.ui_state |= core.problems_rev != before; } } + FeedMsg::Folders(folders) => { + // Gated for the same reason as `Problems`: the tree is republished whenever + // the strategies move, which on a busy account is constantly, and only an + // actual difference is worth a repaint. + if let Some(core) = self.store.core_mut(sess.id) { + let before = core.folders_rev; + core.apply(FeedMsg::Folders(folders)); + stats.ui_state |= core.folders_rev != before; + } + } other => { if let Some(core) = self.store.core_mut(sess.id) { core.apply(other); diff --git a/crates/moon-core/src/session/store.rs b/crates/moon-core/src/session/store.rs index 76ec8fe5..8671a780 100644 --- a/crates/moon-core/src/session/store.rs +++ b/crates/moon-core/src/session/store.rs @@ -263,6 +263,11 @@ pub struct CoreData { /// connection, so the cost of clearing is the seconds until it arrives, stated honestly as /// "not known" rather than as a clean bill. pub problems: crate::feed::CoreProblems, + /// The core's folder tree, empty folders included, observed through `folders_rev`. + /// + /// Its `supported` flag is what tells a caller whether an empty folder can be sent to this core + /// at all; see [`crate::feed::CoreFolders`]. + pub folders: crate::feed::CoreFolders, /// Latest startup progress and channel measurements polled from the moonproto client. /// The Core Status panel observes it through `startup_rev`. It FREEZES once the core settles, /// so after a successful startup `elapsed_ms` is how long that core took to come up, not a @@ -368,6 +373,8 @@ pub struct CoreData { /// list on every reconnect and again for each newly confirmed row, so an ungated counter would /// repaint the panel for a list that has not changed at all. pub problems_rev: u64, + /// Advances when the reported folder tree actually differs. + pub folders_rev: u64, /// Advances when the polled startup snapshot reports different PROGRESS, per /// `CoreStartupStatus::progress_eq`. Deliberately separate from `sys_rev`: that counter is /// documented as covering `KernelHealth` metrics and the decoded endpoint, its field is CLEARED @@ -463,7 +470,9 @@ impl CoreData { chart_alerts_rev: 0, sys_rev: 0, problems: crate::feed::CoreProblems::default(), + folders: crate::feed::CoreFolders::default(), problems_rev: 0, + folders_rev: 0, startup_rev: 0, news_rev: 0, time_offset: crate::feed::CoreTimeOffsetStatus::default(), @@ -608,6 +617,13 @@ impl CoreData { self.problems = crate::feed::CoreProblems::default(); self.problems_rev = self.problems_rev.wrapping_add(1); } + // The folder tree belongs to the replaced MoonBot too, and clearing `supported` with it is + // again the point rather than a side effect: a replacement feed may point at a core that + // cannot hold an empty folder, and a retained flag would keep promising that it can. + if self.folders != crate::feed::CoreFolders::default() { + self.folders = crate::feed::CoreFolders::default(); + self.folders_rev = self.folders_rev.wrapping_add(1); + } // A replacement feed may point at a different MoonBot on a different clock, so last // connection's estimate carries no evidence about this one. // @@ -1005,6 +1021,15 @@ impl CoreData { self.problems_rev = self.problems_rev.wrapping_add(1); } } + FeedMsg::Folders(folders) => { + // Compared before adopting, like the diagnostics above: the tree is republished + // whenever the strategies move, and an unconditional revision would rebuild the + // window's whole strategy tree for a folder list that did not change. + if self.folders != folders { + self.folders = folders; + self.folders_rev = self.folders_rev.wrapping_add(1); + } + } FeedMsg::ConnFault(fault) => { // A plain overwrite: the newest attempt is the one being explained, and the feed // emits this exactly once per terminal failure. No revision counter — see the diff --git a/crates/moon-core/src/session/store/tests.rs b/crates/moon-core/src/session/store/tests.rs index 92adffb1..70ae54f1 100644 --- a/crates/moon-core/src/session/store/tests.rs +++ b/crates/moon-core/src/session/store/tests.rs @@ -1045,3 +1045,51 @@ fn a_new_problem_list_replaces_the_old_one_and_gates_the_revision() { assert_eq!(core.problems.items[0].kind, 2); assert_eq!(core.problems_rev, after_first + 1); } + +/// The folder tree follows the same rule as the diagnostics beside it, and for the same reason: a +/// replacement feed may point at a MoonBot that cannot hold an empty folder at all. Carrying +/// `supported` forward would leave the window promising a persistence the new core does not offer. +#[test] +fn a_replacement_connection_returns_folders_to_nothing_known() { + let mut core = CoreData::new(); + assert!(!core.folders.supported, "nothing known before any tree"); + + core.apply(FeedMsg::Folders(crate::feed::CoreFolders { + supported: true, + editable: true, + paths: vec!["Research".to_string()], + })); + assert!(core.folders.supported); + assert_eq!(core.folders_rev, 1); + + core.begin_connection_attempt(); + + assert!(!core.folders.supported); + assert!(core.folders.paths.is_empty()); + assert_eq!(core.folders_rev, 2, "clearing is a change consumers see"); +} + +/// The tree is republished whenever the strategies move, which on a busy account is constantly. +/// Only a real difference may advance the revision — `session::lifecycle` wakes the window on that +/// counter, and the whole strategy tree is rebuilt behind it. +#[test] +fn an_unchanged_folder_tree_does_not_advance_its_revision() { + let mut core = CoreData::new(); + let tree = crate::feed::CoreFolders { + supported: true, + editable: true, + paths: vec!["Research".to_string(), "Research/Deep".to_string()], + }; + + core.apply(FeedMsg::Folders(tree.clone())); + let after_first = core.folders_rev; + core.apply(FeedMsg::Folders(tree)); + assert_eq!(after_first, core.folders_rev, "the same tree said twice"); + + core.apply(FeedMsg::Folders(crate::feed::CoreFolders { + supported: true, + editable: true, + paths: vec!["Research".to_string()], + })); + assert_ne!(after_first, core.folders_rev, "a folder that went away"); +} diff --git a/crates/moon-ui-gpui/src/diagnostics/crash.rs b/crates/moon-ui-gpui/src/diagnostics/crash.rs index 018787fd..7a302877 100644 --- a/crates/moon-ui-gpui/src/diagnostics/crash.rs +++ b/crates/moon-ui-gpui/src/diagnostics/crash.rs @@ -81,16 +81,20 @@ unsafe extern "system" fn native_exception_filter( body.push_str(""); } + // WHAT faulted, written before the backtrace is attempted. Symbolizing goes through + // `dbghelp`, which is not thread-safe and can fault while handling a fault — and a second + // access violation inside this handler takes the process down with nothing recorded at all. + // Written first, the crash's own code and address survive that. + // + // Writes the file directly, without the global logger: the faulting thread may already hold its + // lock. Same sink as the Rust panic hook, which is why it lives in `applog`. + moon_core::applog::panic_log(&format!("NATIVE CRASH: {body}")); + // The filter runs on the thread that faulted, and `force_capture` walks its current stack while // the handler is executing. It does not unwind from the saved `ContextRecord`; available PDBs // symbolize the captured frames in the same way as the panic hook. let bt = std::backtrace::Backtrace::force_capture(); - - // Writes the file directly, without the global logger: the faulting thread may already hold its - // lock. Same sink as the Rust panic hook, which is why it lives in `applog`. - moon_core::applog::panic_log(&format!( - "NATIVE CRASH: {body}\n--- backtrace ---\n{bt}\n--- end ---" - )); + moon_core::applog::panic_log(&format!("--- backtrace ---\n{bt}\n--- end ---")); EXCEPTION_CONTINUE_SEARCH } diff --git a/crates/moon-ui-gpui/src/main.rs b/crates/moon-ui-gpui/src/main.rs index 9e396784..18571044 100644 --- a/crates/moon-ui-gpui/src/main.rs +++ b/crates/moon-ui-gpui/src/main.rs @@ -640,6 +640,18 @@ struct CoreFilterRevision; /// Returns: /// Success after the selected process role exits. fn main() -> anyhow::Result<()> { + // FIRST, and at the base of the stack on purpose. `rust_i18n` builds its translation backend + // once, lazily, on the first `t!()` — and its generated initializer materialises all ~2600 keys + // in ONE stack frame, large enough that the `__chkstk` probe entering it faults outright when + // the first lookup happens deep inside window construction. That is not hypothetical: it is a + // 0xC00000FD stack overflow at startup, and because the stack was already exhausted the crash + // handler could not even capture a backtrace — the process died inside `dbghelp` instead, + // leaving an empty log and a crash that named the wrong module. + // + // Touched here, the frame is built where there is room, and every later `t!()` is a map lookup + // costing nothing. The key is arbitrary; only the initialization it forces matters. + let _ = rust_i18n::t!("common.loading"); + // Before the updater, before the configuration, before a window: the UI-atlas tools that work // on a file the crawl already wrote need none of it, and running them through a normal launch // would put a six-minute walk between a rule and its result. diff --git a/crates/moon-ui-gpui/src/startup.rs b/crates/moon-ui-gpui/src/startup.rs index 6f50ab2a..9190e40d 100644 --- a/crates/moon-ui-gpui/src/startup.rs +++ b/crates/moon-ui-gpui/src/startup.rs @@ -402,15 +402,20 @@ pub(crate) fn run(startup_update: Option) -> anyho .copied() .or_else(|| info.payload().downcast_ref::().map(|s| s.as_str())) .unwrap_or(""); - // Force a backtrace without RUST_BACKTRACE: clamp panics report a location inside core, - // while we need the CALLING frame in our code. - let bt = std::backtrace::Backtrace::force_capture(); + // WHAT panicked, written before anything else is attempted. Capturing a backtrace + // means symbolizing through `dbghelp`, which is not thread-safe and can fault on its + // own — and when it does, the process dies inside the handler with an access violation + // in `dbghelp.dll` and NOTHING is recorded: no location, no message, an empty log. This + // ordering means the worst such a failure can cost is the backtrace. + // // Both sinks redact on their own: `panic_log` owns the file, `TeeLogger` the log. // A panic message can quote foreign text carrying an endpoint. - moon_core::applog::panic_log(&format!( - "PANIC at {loc}: {payload}\n--- backtrace ---\n{bt}\n--- end ---" - )); + moon_core::applog::panic_log(&format!("PANIC at {loc}: {payload}")); log::error!("PANIC at {loc}: {payload}"); + // Forced rather than left to RUST_BACKTRACE: clamp panics report a location inside + // core, while we need the CALLING frame in our code. + let bt = std::backtrace::Backtrace::force_capture(); + moon_core::applog::panic_log(&format!("--- backtrace ---\n{bt}\n--- end ---")); default_hook(info); })); } diff --git a/crates/moon-ui-gpui/src/strategies/logic.rs b/crates/moon-ui-gpui/src/strategies/logic.rs index 114ab5ac..d082d942 100644 --- a/crates/moon-ui-gpui/src/strategies/logic.rs +++ b/crates/moon-ui-gpui/src/strategies/logic.rs @@ -570,30 +570,81 @@ pub(super) fn toggle(set: &mut HashSet, ke } /// Folder-tree node containing named child folders and strategies directly in this folder. +/// +/// Children are kept in INSERTION order, not sorted by name. The strategy list a core sends is an +/// ordered list the operator arranges in MoonBot, and moonproto synchronizes that order as the row +/// sequence of a Full snapshot (`docs/strats.md`, "Strategy Order"), with one folder's strategies +/// forming a contiguous group. So the order a folder first appears in that sequence IS its place in +/// the core's own tree, and the previous `BTreeMap` threw it away for a byte-wise alphabet that +/// matched neither MoonBot nor anything the operator arranged — `Zeta` above `alpha`, Cyrillic +/// names after every Latin one. +/// +/// Folders holding no strategy have no place in that sequence; [`ensure_folder`] appends them, so +/// its caller decides their order among themselves. #[derive(Default)] pub(super) struct FolderNode<'a> { - pub(super) children: std::collections::BTreeMap>, + /// Child folders, in the order this level first saw each of them. + children: Vec<(String, FolderNode<'a>)>, pub(super) strategies: Vec<&'a StrategyRow>, } +impl<'a> FolderNode<'a> { + /// Child folders paired with their names, in display order. + pub(super) fn children(&self) -> impl Iterator)> { + self.children + .iter() + .map(|(name, node)| (name.as_str(), node)) + } + + /// Borrow one child folder by name, appending it when this level has not seen it yet. + /// + /// Scanned from the END because the core groups one folder's strategies contiguously: for a + /// run of rows sharing a path the match is the last child every time, which keeps the walk + /// linear in practice instead of quadratic in the sibling count. + /// + /// Args: + /// name: One path segment, exactly as the data spells it. + /// + /// Returns: + /// The existing child of that name, or a fresh empty one appended after every sibling. + fn child_mut(&mut self, name: &str) -> &mut FolderNode<'a> { + match self.children.iter().rposition(|(seen, _)| seen == name) { + Some(at) => &mut self.children[at].1, + None => { + self.children + .push((name.to_string(), FolderNode::default())); + let appended = self.children.len() - 1; + &mut self.children[appended].1 + } + } + } +} + /// Build a nested tree from strategy paths, split through [`path_segments`]. +/// +/// The iterator must arrive in the core's own strategy order — that sequence is what decides where +/// each folder sits. See [`FolderNode`]. pub(super) fn build_node<'a>(it: impl Iterator) -> FolderNode<'a> { let mut root = FolderNode::default(); for r in it { let mut node = &mut root; for part in path_segments(&r.folder_path) { - node = node.children.entry(part.to_string()).or_default(); + node = node.child_mut(part); } node.strategies.push(r); } root } -/// Ensure that a path exists for empty UI folders that contain no strategies yet. +/// Ensure that a path exists for empty folders that contain no strategies yet. +/// +/// A missing level is APPENDED after everything the strategy walk already placed, so the caller's +/// own order over these paths is the order they appear in — feed them in a deterministic one or the +/// tree reshuffles between frames. pub(super) fn ensure_folder(root: &mut FolderNode, parts: &[String]) { let mut node = root; for part in parts { - node = node.children.entry(part.clone()).or_default(); + node = node.child_mut(part); } } @@ -607,12 +658,34 @@ pub(super) fn ensure_folder(root: &mut FolderNode, parts: &[String]) { /// introduce none of their own. #[derive(Default)] pub(super) struct FolderCounts { - by_path: HashMap, + by_path: HashMap, + /// The same prefixes lowercased, for the question the CORE asks: it keys folders + /// case-insensitively, so a tree spelling `Research` against strategies spelling `research` + /// names one folder, and a case-sensitive answer would draw the second one as empty beside it. + /// Bounded by the folder count rather than the row count — one entry per prefix, on first + /// visit. + folded: HashSet, root: (usize, usize), /// Set for a core whose folders are not built, so no per-folder entry is worth allocating. totals_only: bool, } +/// What one folder prefix accumulates: its caption's numbers, and its place in the core's order. +struct FolderStat { + /// Checked strategies at or below this folder, after the caption's own filter. + active: usize, + /// All strategies at or below it, after that same filter. + total: usize, + /// Index of the FIRST strategy of this folder in the core's own list, filtered by nothing. + /// + /// The tree orders folders by this rather than by where their first VISIBLE row sits, and the + /// difference is not cosmetic: with a filter on, a folder whose early rows are hidden would + /// otherwise slide below a folder that starts later, so typing in the search box would + /// rearrange the tree around the reader. Recorded on the first visit to each prefix, which is + /// the earliest by construction — the walk goes through the list in order. + first_seen: usize, +} + impl FolderCounts { /// Counts only the core total, for a collapsed core whose folder rows are never built. /// [`Self::for_path`] reports nothing in this mode and must not be consulted. @@ -632,16 +705,20 @@ impl FolderCounts { /// Args: /// row: Strategy contributing to its folder chain when it passes the count predicate. /// filter: Prepared kind/direction count predicate. + /// at: Index of this strategy in the core's own list, for [`FolderStat::first_seen`]. /// /// Returns: /// Nothing; matching counters are updated in place. - pub(super) fn add(&mut self, row: &StrategyRow, filter: &PreparedFilter) { - if !filter.counts(row) { - return; - } - let hit = usize::from(row.checked); + pub(super) fn add(&mut self, row: &StrategyRow, filter: &PreparedFilter, at: usize) { + // The walk itself is NOT gated: a folder's place in the tree comes from the core's list as + // it stands, not from the rows a filter happens to leave. Only the numbers are gated. + // `(checked, counted)` as numbers, once: the three sites below add exactly these two. + let (hit, one) = match filter.counts(row) { + true => (usize::from(row.checked), 1), + false => (0, 0), + }; self.root.0 += hit; - self.root.1 += 1; + self.root.1 += one; if self.totals_only { return; } @@ -653,10 +730,18 @@ impl FolderCounts { key.push_str(seg); // Look up before inserting so a repeat visit to a known folder does not clone the key. if let Some(e) = self.by_path.get_mut(&key) { - e.0 += hit; - e.1 += 1; + e.active += hit; + e.total += one; } else { - self.by_path.insert(key.clone(), (hit, 1)); + self.folded.insert(key.to_lowercase()); + self.by_path.insert( + key.clone(), + FolderStat { + active: hit, + total: one, + first_seen: at, + }, + ); } } } @@ -664,7 +749,27 @@ impl FolderCounts { /// Returns `(active, total)` at or below a folder, or `(0, 0)` for a folder holding no /// strategies — the case of a UI-only folder created before its first strategy. pub(super) fn for_path(&self, path: &str) -> (usize, usize) { - self.by_path.get(path).copied().unwrap_or((0, 0)) + self.by_path + .get(path) + .map_or((0, 0), |stat| (stat.active, stat.total)) + } + + /// Whether any strategy of this core lives at or below a folder path. + /// + /// The accumulator walks every row's whole prefix chain regardless of the filter, so this + /// answers about the CORE's folders rather than about the rows currently drawn — which is what + /// a caller deciding "is this folder empty" has to ask. Case-insensitively, because that is how + /// the core decides whether two spellings name one folder. + pub(super) fn knows(&self, path: &str) -> bool { + self.folded.contains(&path.to_lowercase()) + } + + /// Where a folder sits in the core's own order, or `None` for one holding no strategy at all. + /// + /// `None` is the empty folder — it appears in no strategy's path, so the core's list says + /// nothing about where it belongs, and the tree appends those instead. + pub(super) fn order_of(&self, path: &str) -> Option { + self.by_path.get(path).map(|stat| stat.first_seen) } /// Returns `(active, total)` over every counted strategy, which is the core's own caption. diff --git a/crates/moon-ui-gpui/src/strategies/logic/tests.rs b/crates/moon-ui-gpui/src/strategies/logic/tests.rs index 7e3f60fc..d4224b35 100644 --- a/crates/moon-ui-gpui/src/strategies/logic/tests.rs +++ b/crates/moon-ui-gpui/src/strategies/logic/tests.rs @@ -108,8 +108,8 @@ fn prefixes() -> Vec> { /// `add`, so pre-filtering here would hide a regression in that gate. fn accumulate(rows: &[StrategyRow], filter: &PreparedFilter) -> FolderCounts { let mut counts = FolderCounts::default(); - for r in rows { - counts.add(r, filter); + for (at, r) in rows.iter().enumerate() { + counts.add(r, filter, at); } counts } @@ -508,3 +508,67 @@ fn a_checkbox_a_colour_or_a_string_is_never_rejected() { assert!(!super::draft_rejected(&color, "FF00AA")); assert!(!super::draft_rejected(&edit_field("String"), "")); } + +/// Names every folder of a built node in the order the tree would draw them, depth first. +/// +/// Written as its own walk rather than by calling the renderer, so the assertion below compares the +/// structure with an expectation instead of with the production traversal. +fn folder_order(node: &super::FolderNode, prefix: &str) -> Vec { + let mut out = Vec::new(); + for (name, child) in node.children() { + let path = match prefix.is_empty() { + true => name.to_string(), + false => format!("{prefix}/{name}"), + }; + out.push(path.clone()); + out.extend(folder_order(child, &path)); + } + out +} + +/// The core's strategy list is an ORDER the operator arranged, and moonproto synchronizes it as the +/// row sequence of a Full snapshot. So a folder belongs where it first appears in that sequence — +/// not where a byte-wise alphabet would put it, which is what the previous `BTreeMap` did and which +/// sorted `Zeta` above `alpha` and every Cyrillic name after every Latin one. +#[test] +fn folders_follow_the_cores_own_strategy_order() { + let rows = [ + row(1, "Zeta", 0, false, true), + row(2, "alpha", 0, false, true), + row(3, "Zeta/inner", 0, false, true), + row(4, "омега", 0, false, true), + row(5, "alpha", 0, false, true), + ]; + let node = super::build_node(rows.iter()); + assert_eq!( + folder_order(&node, ""), + vec!["Zeta", "Zeta/inner", "alpha", "омега"] + ); +} + +/// A folder's strategies stay in the order the core sent them, which is the same claim one level +/// down: the rows inside one folder are a sequence, not a set. +#[test] +fn strategies_inside_a_folder_keep_the_cores_order() { + let rows = [ + row(7, "a", 0, false, true), + row(3, "a", 0, false, true), + row(5, "a", 0, false, true), + ]; + let node = super::build_node(rows.iter()); + let (_, folder) = node.children().next().expect("one folder"); + let ids: Vec = folder.strategies.iter().map(|r| r.id).collect(); + assert_eq!(ids, vec![7, 3, 5]); +} + +/// An empty folder appears in no strategy's path, so it has no place in the core's order and is +/// appended after the folders that do. Two rows are the point of the case: `ensure_folder` must +/// find the existing `a` rather than append a second one beside it. +#[test] +fn an_empty_folder_is_appended_without_duplicating_a_live_one() { + let rows = [row(1, "b", 0, false, true), row(2, "a", 0, false, true)]; + let mut node = super::build_node(rows.iter()); + super::ensure_folder(&mut node, &["a".to_string(), "ghost".to_string()]); + super::ensure_folder(&mut node, &["zz".to_string()]); + assert_eq!(folder_order(&node, ""), vec!["b", "a", "a/ghost", "zz"]); +} diff --git a/crates/moon-ui-gpui/src/strategies/mod.rs b/crates/moon-ui-gpui/src/strategies/mod.rs index 9cea9386..d29be88d 100644 --- a/crates/moon-ui-gpui/src/strategies/mod.rs +++ b/crates/moon-ui-gpui/src/strategies/mod.rs @@ -189,6 +189,11 @@ pub struct StrategiesView { selected_folder: Option<(CoreId, String)>, /// Empty UI folders before their first strategy is added, keyed by core and slash-separated path. ui_folders: HashSet<(CoreId, String)>, + /// Strategy orders sent to a core and not yet echoed back by it, keyed by core. + /// + /// The tree draws these instead of the core's own sequence until it answers — see + /// [`tree::reorder`] for why the answer cannot simply be waited for. + pending_order: HashMap, /// Active create, rename, or confirmation modal for a tree operation. op: Option, /// Create/rename modal input, recreated on each opening to use the current initial value. @@ -265,6 +270,15 @@ impl Render for StrategiesView { // rate, and rebuilding the whole adapter for it measured 1.0-1.3 ms per frame on a live // account. `tree::cache` owns the signature and the argument for why it is complete. let tree_timer = crate::diag::timer(); + // Before the signature, because the signature covers the overlay this may drop, and a frame + // that hashed an overlay it then stopped drawing would cache the wrong tree under it. + // + // Here as well as in the backend observer: an unconfirmed order's deadline has to elapse for + // a core that has gone quiet, and a quiet core is exactly the one that raises no notify. + if !self.pending_order.is_empty() { + let backend = self.backend.clone(); + self.reconcile_pending_order(backend.read(cx).session.store()); + } let sig = { let backend = self.backend.read(cx); let store = backend.session.store(); diff --git a/crates/moon-ui-gpui/src/strategies/state.rs b/crates/moon-ui-gpui/src/strategies/state.rs index f59941f4..917344e1 100644 --- a/crates/moon-ui-gpui/src/strategies/state.rs +++ b/crates/moon-ui-gpui/src/strategies/state.rs @@ -206,6 +206,12 @@ fn strategies_sig(b: &Backend, workspace_cores: Option<&[CoreId]>) -> u64 { a.wrapping_mul(31) .wrapping_add(c.strategies_rev) .wrapping_mul(31) + // The core's folder tree, which moves on its own: an empty folder created or + // deleted changes no strategy, and without this the window would not repaint for + // it — not even for the confirmation of a folder it created itself. It is also what + // makes `reconcile_ui_folders` run at the moment its new rule becomes true. + .wrapping_add(c.folders_rev) + .wrapping_mul(31) .wrapping_add(c.schema_rev) .wrapping_mul(31) .wrapping_add(c.strategy_edit_rev) @@ -339,6 +345,11 @@ impl StrategiesView { marker_moved = true; } } + // Asked on EVERY backend tick, not only on a strategy change: a reorder the core + // silently drops produces no strategy change at all, and its overlay would then outlive + // its window with nothing to notice. Costs a walk of a map that is empty except in the + // seconds after the operator pressed a move button. + let order_dropped = this.reconcile_pending_order(b.session.store()); if strategies_changed || goto { if strategies_changed { this.reconcile_ui_folders(b.session.store()); @@ -348,11 +359,12 @@ impl StrategiesView { this.clamp_selected_section(cx); this.persist_session(cx); cx.notify(); - } else if marker_moved { - // The arm above already repainted. This one covers the case it does not reach: + } else if marker_moved || order_dropped { + // The arm above already repainted. This one covers the cases it does not reach: // a core the preset HIDES connecting or leaving moves the marker's counts while // `strategies_sig` — which folds only over cores the scope already shows — does - // not budge. + // not budge; and an expired reorder overlay changes what the tree draws without + // any core having sent anything. cx.notify(); } }) @@ -546,6 +558,10 @@ impl StrategiesView { .as_ref() .map(|s| s.ui_folders.clone()) .unwrap_or_default(), + // Deliberately not restored from the session snapshot: an order sent before the window + // closed was either confirmed by the core — in which case the store already holds it — + // or lost, and a reopened window must not re-assert it. + pending_order: HashMap::new(), op: None, op_input: None, op_input_init: String::new(), diff --git a/crates/moon-ui-gpui/src/strategies/tree/cache.rs b/crates/moon-ui-gpui/src/strategies/tree/cache.rs index 6dee9a9d..549a1036 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/cache.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/cache.rs @@ -95,7 +95,8 @@ impl TreeCache { /// The two halves are the store and the window's own state: /// /// * per core, in the order the window lists them: its id, its display name, venue presence and -/// identity/caption fields, `strategies_rev` (the strategy snapshot), and the rendered +/// identity/caption fields, `strategies_rev` (the strategy snapshot), `folders_rev` (the +/// core's own folder tree, which carries the folders no strategy implies), and the rendered /// open-order digest. A core appearing, disappearing or being renamed moves the list itself. /// * per window field: venue grouping, the filter — search, kind, direction, EXCHANGE and /// active-only — the three expansion sets plus the Auto rail overlay, the UI-only @@ -136,6 +137,10 @@ pub(crate) fn data_sig( continue; }; cd.strategies_rev.hash(&mut h); + // The folder tree, which the build reads for the folders holding no strategy. Its own + // counter rather than `strategies_rev`: an empty folder created or deleted moves nothing + // about the strategies, and that folder is exactly what this input contributes. + cd.folders_rev.hash(&mut h); open_orders_digest(cd).hash(&mut h); } let store_sig = h.finish(); @@ -158,6 +163,16 @@ pub(crate) fn data_sig( unordered(view.expanded_folders.iter()).hash(&mut h); unordered(view.expanded_deleted.iter()).hash(&mut h); unordered(view.ui_folders.iter()).hash(&mut h); + // The unconfirmed orders themselves, each folded over its own SEQUENCE. A cheaper key — which + // cores carry one, or how long each is — would collide on the case this exists for: a second + // move pressed before the core answered replaces one overlay with another of the same length on + // the same core, and the tree would then keep drawing the arrangement from the press before. + unordered( + view.pending_order + .iter() + .map(|(core, pending)| (*core, pending.ids())), + ) + .hash(&mut h); unordered(view.sel.iter()).hash(&mut h); let staged = unordered(view.staged.iter()); staged.hash(&mut h); diff --git a/crates/moon-ui-gpui/src/strategies/tree/dialogs.rs b/crates/moon-ui-gpui/src/strategies/tree/dialogs.rs index 2dc967b0..ad41283a 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/dialogs.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/dialogs.rs @@ -604,7 +604,7 @@ impl StrategiesView { core, ) { - self.add_ui_folder(core, &target, name.trim()); + self.create_folder(core, &target, name.trim(), cx); self.persist_session(cx); } } @@ -868,6 +868,10 @@ impl StrategiesView { ) { return Ok(()); } + let mut new_path = old_path.to_vec(); + if let Some(leaf) = new_path.last_mut() { + *leaf = new_name.to_string(); + } let moves = { let store = self.backend.read(cx).session.store(); let Some(cd) = store.core(core) else { @@ -875,7 +879,14 @@ impl StrategiesView { }; ops::rename_folder(&cd.strategies, old_path, new_name) }; - self.backend.read(cx).session.move_strategies(core, moves)?; + // The subtree that moved travels WITH the rows, because rewriting their paths leaves the + // old folder behind on a core that keeps folders of its own. A folder holding no strategy + // has no rows at all, and its rename is this same edit with an empty move list. + self.backend.read(cx).session.move_strategies( + core, + moves, + Some((ops::join_path(old_path), ops::join_path(&new_path))), + )?; // Rename an empty UI-only folder locally only after the move command succeeds. self.rename_ui_folder(core, old_path, new_name); self.persist_session(cx); @@ -970,10 +981,24 @@ impl StrategiesView { ) { return Ok(()); } - self.backend - .read(cx) - .session - .delete_folder(core, ops::join_path(path))?; + // Two shapes, and which one applies is decided by what the folder HOLDS, not only by what + // the core can do. Omission from the desired tree removes a folder and nothing else — the + // core keeps any folder a strategy still occupies, and moonproto re-adds it — so it reaches + // exactly the folder the legacy command cannot: an empty one on a core that keeps a tree. + // A folder with strategies in it still goes the legacy way, which deletes the rows with it. + let by_omission = { + let store = self.backend.read(cx).session.store(); + store + .core(core) + .is_some_and(|cd| cd.folders.editable && !ops::has_row_under(&cd.strategies, path)) + }; + let backend = self.backend.read(cx); + match by_omission { + true => backend + .session + .remove_core_folder(core, ops::join_path(path))?, + false => backend.session.delete_folder(core, ops::join_path(path))?, + } self.remove_ui_folder(core, path); self.persist_session(cx); Ok(()) diff --git a/crates/moon-ui-gpui/src/strategies/tree/dnd.rs b/crates/moon-ui-gpui/src/strategies/tree/dnd.rs index 549c9db4..e93c815d 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/dnd.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/dnd.rs @@ -160,11 +160,14 @@ impl StrategiesView { .unwrap_or_default(); ops::move_to(&rows, &target) }; - if let Err(error) = self - .backend - .read(cx) - .session - .move_strategies(target_core, moves) + // No folder tree: dragging strategies OUT of a folder must not delete that folder. The + // core keeps an emptied folder now, which is the behaviour to preserve — a tree omitting + // it would take it away as a side effect of moving rows. + if let Err(error) = + self.backend + .read(cx) + .session + .move_strategies(target_core, moves, None) { log::warn!("move strategies failed: {error}"); return; @@ -220,22 +223,28 @@ impl StrategiesView { } let path = drag.path.clone(); if drag.core == target_core { + let mut moved_to = target.clone(); + moved_to.extend(path.last().cloned()); let moves = { let store = self.backend.read(cx).session.store(); - store - .core(target_core) - .map(|c| ops::move_folder(&c.strategies, &path, &target)) - .unwrap_or_default() + let Some(cd) = store.core(target_core) else { + return; + }; + ops::move_folder(&cd.strategies, &path, &target) }; - if moves.is_empty() { - return; // Reject self/descendant targets and empty folders. + // Rejects a drop onto the folder itself or into its own subtree, where source and + // destination are the same place. An EMPTY folder is no longer rejected here: it has no + // rows to move, and its relocation travels as the subtree intent alone. + if moves.is_empty() && (moved_to == path || target.starts_with(&path)) { + return; } - if let Err(error) = self - .backend - .read(cx) - .session - .move_strategies(target_core, moves) - { + // Same pairing as a rename: without the subtree the folder's old path survives on the + // core as an empty folder, now that empty folders are something it can hold. + if let Err(error) = self.backend.read(cx).session.move_strategies( + target_core, + moves, + Some((ops::join_path(&path), ops::join_path(&moved_to))), + ) { log::warn!("move strategy folder failed: {error}"); return; } diff --git a/crates/moon-ui-gpui/src/strategies/tree/menu.rs b/crates/moon-ui-gpui/src/strategies/tree/menu.rs index 3a4afb12..68bc332e 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/menu.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/menu.rs @@ -145,6 +145,42 @@ impl StrategiesView { ); } MenuTarget::Strategy(id) => { + // Acts on the SELECTION, which already contains this row: opening the menu on an + // unselected strategy focuses it first (`strategy_row`). So right-clicking one row + // moves that row, and right-clicking inside a multi-selection moves the block. + let (can_up, can_down) = { + let backend = self.backend.read(cx); + let store = backend.session.store(); + self.move_availability(store, backend.session.core_venues()) + }; + for (step, enabled) in + [(ops::MoveStep::Up, can_up), (ops::MoveStep::Down, can_down)] + { + let (key, label, chord) = match step { + ops::MoveStep::Up => ( + "move-up", + t!("strat.menu_move_up"), + t!("strat.move_up_chord"), + ), + ops::MoveStep::Down => ( + "move-down", + t!("strat.menu_move_down"), + t!("strat.move_down_chord"), + ), + }; + items.push( + MoonMenuItem::with_key(key, label.to_string()) + .right_label(chord.to_string()) + .disabled(!enabled) + .on_click({ + let view = view.clone(); + move |_, window, app| { + window.close_context_menu(app); + view.update(app, |this, cx| this.move_selection(step, cx)); + } + }), + ); + } items.push( MoonMenuItem::with_key("copy-strategy", t!("strat.menu_copy").to_string()) .on_click({ diff --git a/crates/moon-ui-gpui/src/strategies/tree/mod.rs b/crates/moon-ui-gpui/src/strategies/tree/mod.rs index eafaed61..bd141d63 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/mod.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/mod.rs @@ -12,6 +12,7 @@ pub(crate) mod menu; pub(crate) mod moon; pub(crate) mod ops; pub(in crate::strategies) mod pane_cache; +pub(crate) mod reorder; pub(crate) mod ui; #[cfg(test)] @@ -575,6 +576,10 @@ impl StrategiesView { let measured_label_width = pane.footer_label_width; // Five native leading icons remain in both densities. The full state additionally reserves // their label gaps, six group gaps, outer padding, and the hairline divider. + // + // The two move buttons are counted separately below because they never take a label: they + // cost their own width and one group gap each in BOTH densities, and leaving them out of + // this sum would let the labelled state be chosen for a row that no longer fits it. let action_icon_width = (design::font_value(cx, design::ACTION_LABEL_BASE) + 1.0).clamp(10.0, 14.0); // Action size ships with pad_x = 0. Labeled footer buttons opt into the same 7-unit @@ -583,7 +588,9 @@ impl StrategiesView { let fixed_width = 5.0 * action_icon_width + design::ui_value(cx, 5.0 * 6.0 + 6.0 * design::CHROME_GAP + 16.0) + 1.0 - + labeled_pad; + + labeled_pad + + 2.0 * design::glyph_btn_w(cx) + + 2.0 * design::ui_value(cx, design::CHROME_GAP); let show_labels = ui::footer_labels_fit(self.panels.tree_w, fixed_width, measured_label_width); @@ -657,7 +664,7 @@ impl StrategiesView { // The footer's leading icons carry no text of their own; its action labels are prose. // `staged_slot` pins its mixed caption/count node back to mono above. .font_family(design::ui_font()) - .child(self.selection_toolbar(store, show_labels, !cores.is_empty(), cx)) + .child(self.selection_toolbar(store, show_labels, !cores.is_empty(), pane.moves, cx)) .child(design::chrome_divider(cx, MoonPalette::active(cx))) .child(staged_slot) .child(right) diff --git a/crates/moon-ui-gpui/src/strategies/tree/moon.rs b/crates/moon-ui-gpui/src/strategies/tree/moon.rs index 21034fdd..8656b9b7 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/moon.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/moon.rs @@ -23,6 +23,7 @@ use super::super::logic::{ }; use super::super::{Key, StrategiesView, moon_alpha}; use super::checks; +use super::ops; use super::ui::{ContextMenu, DragChip, FolderDrag, MenuTarget, StratDrag}; use crate::design; use moon_core::feed::StrategyRow; @@ -195,6 +196,8 @@ pub(crate) enum NodeData { checked: bool, }, Folder { + /// What the folder holds, which decides whether its caret and checkbox are drawn at all. + fill: FolderFill, core: CoreId, path: Vec, label: String, @@ -404,8 +407,8 @@ fn build_core_root( }; let mut matched: Vec<&StrategyRow> = Vec::new(); let mut any_matched = false; - for row in &cd.strategies { - counts.add(row, filter); + for (at, row) in cd.strategies.iter().enumerate() { + counts.add(row, filter, at); if filter.matches(row) { any_matched = true; if core_open { @@ -413,9 +416,30 @@ fn build_core_root( } } } - if !any_matched { + // A core with no matching strategy is still worth a row when it holds folders that hold + // none: on an account whose folders were prepared before its strategies, that is everything + // there is to show. Asked of the folders that would actually be DRAWN — a core whose every + // folder is occupied by strategies the filter removed has nothing to show and stays hidden. + let empty_folders = empty_folder_paths(view, cd, core, searching); + if !any_matched && empty_folders.is_empty() { return None; } + // Only for a core whose rows are actually built: `matched` is filled solely when the core is + // open, so resequencing it for a collapsed one is a whole-list walk nothing reads. + if let Some(pending) = core_open.then(|| view.pending_order.get(&core)).flatten() { + // A reorder this core has not echoed yet. Drawn instead of its own sequence because the + // library keeps the confirmed order until the core answers, and the operator would + // otherwise watch their own press do nothing for a whole round trip — see `tree::reorder`. + // + // Applied to the WHOLE list and filtered afterwards, not to the rows that survived the + // filter. The rule places a strategy the sent sequence never named directly after the row + // it follows, and "the row it follows" is a different row once a filter has removed its + // neighbours — so ordering the filtered list would draw an arrangement the planner, which + // reads the whole one, does not agree with. + let mut all: Vec<&StrategyRow> = cd.strategies.iter().collect(); + moon_core::feed::strategy_order::resequence(&mut all, |row| pending.rank(row.id)); + matched = all.into_iter().filter(|row| filter.matches(row)).collect(); + } let (active, total) = counts.root(); let open_orders_total = cd.orders.iter().filter(|order| !order.job_is_done).count(); @@ -431,6 +455,7 @@ fn build_core_root( searching, &counts, &matched, + &empty_folders, &mut children, data, flat, @@ -465,6 +490,89 @@ fn build_core_root( ) } +/// Folders of one core that hold no strategy, in the order the tree appends them. +/// +/// Two sources, and which one a core uses is the core's own answer: one that keeps a folder tree +/// reports its empty folders itself, and the local marks are what a core that cannot keep them — +/// or has not confirmed one yet — leaves the window to draw. +/// +/// Answered once per build and used twice: it decides both what to append and whether a core with +/// no matching strategy is worth a row at all. +/// +/// Occupancy is derived from the strategies HERE rather than read from [`FolderCounts`], and that +/// is not duplication: a collapsed core's counts are totals-only and hold no folder at all, so +/// asking them would call every folder of every collapsed core empty. The walk costs nothing on +/// the ordinary core, which reports no folders and holds no marks and returns below immediately. +/// +/// Args: +/// view: Window holding the local marks. +/// cd: The core's live data, including the tree it reports. +/// core: Core being built. +/// searching: Whether a text query is narrowing the tree. +/// +/// Returns: +/// Segment paths of the folders to draw as empty; always empty while searching, since an empty +/// folder matches no query and cannot contain a match. +fn empty_folder_paths( + view: &StrategiesView, + cd: &moon_core::session::store::CoreData, + core: CoreId, + searching: bool, +) -> Vec> { + if searching { + return Vec::new(); + } + let marks = view.ui_folder_paths(core); + let reported: Vec> = match cd.folders.supported { + false => Vec::new(), + true => cd + .folders + .paths + .iter() + // Paths MoonProto itself would refuse are skipped, and that is not a formality: for a + // strategy in MoonBot's `"EMA / ORGANIC"` — one folder there — moonproto's own state + // adds the split halves as parent folders, so the tree reports `"EMA "`. Drawn, that is + // a folder which exists on no core and which no edit could ever name. + .filter(|path| moon_core::feed::folder_tree::sendable([path.as_str()].into_iter())) + .map(|path| ops::split_path(path)) + .collect(), + }; + if marks.is_empty() && reported.is_empty() { + return Vec::new(); + } + + // Every folder the strategies occupy, folded as the core folds them when deciding whether two + // spellings are one folder. + let mut occupied: std::collections::HashSet = std::collections::HashSet::new(); + for row in &cd.strategies { + let mut key = String::new(); + for segment in ops::path_segments(&row.folder_path) { + if !key.is_empty() { + key.push('/'); + } + key.push_str(segment); + occupied.insert(key.to_lowercase()); + } + } + + let mut empty: Vec> = marks + .into_iter() + .chain(reported) + .filter(|parts| !parts.is_empty() && !occupied.contains(&parts.join("/").to_lowercase())) + .collect(); + // One order over both sources: the core lists its folders in an order the protocol calls + // meaningless, and the local marks come out of a set, so without this two frames drawing + // identical data would place the same folders differently. Folded first so that a mark and a + // reported path differing only in case land together — and are then deduped as the one folder + // they are. + empty.sort_by_cached_key(|parts| { + let joined = parts.join("/"); + (joined.to_lowercase(), joined) + }); + empty.dedup_by(|a, b| a.join("/").to_lowercase() == b.join("/").to_lowercase()); + empty +} + /// Builds the folder and strategy rows of one open core, followed by its Deleted folder. #[allow(clippy::too_many_arguments)] fn build_core_subtree( @@ -475,6 +583,7 @@ fn build_core_subtree( searching: bool, counts: &FolderCounts, matched: &[&StrategyRow], + empty_folders: &[Vec], children: &mut Vec, data: &mut HashMap, flat: &mut Vec, @@ -491,17 +600,33 @@ fn build_core_subtree( open: core_paths(&view.expanded_folders, core), }; - // Build the folder tree from visible strategies plus empty UI-only folders. + // Build the folder tree from visible strategies plus empty UI-only folders. `matched` holds + // the core's own strategy order, which is what places the folders that hold strategies. let mut root = build_node(matched.iter().copied()); - for parts in view.ui_folder_paths(core) { - ensure_folder(&mut root, &parts); + for parts in empty_folders { + ensure_folder(&mut root, parts); } + // Folders the core keeps in its own tree, folded for the case-insensitive compare it uses. An + // empty folder outside that set exists only in this window, which is what its row says. Asked + // of `supported` and NOT of `editable`: a core whose tree cannot be edited still KEEPS the + // folders it reports, and calling one of those local would tell the operator it disappears on + // restart when it does not. + let confirmed_folders: std::collections::HashSet = match cd.folders.supported { + false => std::collections::HashSet::new(), + true => cd + .folders + .paths + .iter() + .map(|path| ops::join_path(&ops::split_path(path)).to_lowercase()) + .collect(), + }; let mut prefix: Vec = Vec::new(); convert_node( &root, core, counts, + &confirmed_folders, &order_counts, &selected_ids, &folders, @@ -627,6 +752,7 @@ fn convert_node( node: &super::super::logic::FolderNode, core: CoreId, counts: &FolderCounts, + confirmed_folders: &std::collections::HashSet, order_counts: &HashMap, selected_ids: &Rc<[u64]>, folders: &FolderSets<'_>, @@ -640,11 +766,33 @@ fn convert_node( flat: &mut Vec, expanded: &mut Vec, ) { - for (name, child) in &node.children { - prefix.push(name.clone()); - // One joined path keeps the node id, expansion probe, selection comparison, and count - // lookup on the same folder identity without repeated allocation. - let path = prefix.join("/"); + // Ordered by where each folder's first strategy sits in the core's OWN list — not by where its + // first visible one sits, which is what the child order alone would say and which would let a + // search box rearrange the tree. An empty folder has no such place and keeps the order it was + // appended in, after every folder that does. One allocation per child per level, inside a build + // the frame cache already skips on an unchanged signature. + let parent = prefix.join("/"); + let mut children: Vec<(usize, String, &str, &super::super::logic::FolderNode)> = node + .children() + .map(|(name, child)| { + // One joined path per child for the whole level. It decides the order here and is then + // handed to the loop, which needs the same string for this node's id, its expansion + // probe, its selection comparison and its count lookup. + let path = match parent.is_empty() { + true => name.to_string(), + false => format!("{parent}/{name}"), + }; + ( + counts.order_of(&path).unwrap_or(usize::MAX), + path, + name, + child, + ) + }) + .collect(); + children.sort_by_key(|(at, _, _, _)| *at); + for (_, path, name, child) in children { + prefix.push(name.to_string()); let fid = id_folder(core, &path); let fopen = searching || folders.open.contains(path.as_str()); // Read before `path` is moved into the selection comparison below. @@ -654,6 +802,16 @@ fn convert_node( core, ); let (active, total) = counts.for_path(&path); + // Asked of the COUNTS, not of `total`, which the kind and direction filters narrow: a + // folder whose strategies are all filtered away is not an empty folder, and drawing it as + // one would take its caret away while its contents are one filter click from returning. + let fill = match counts.knows(&path) { + true => FolderFill::Populated, + false => match confirmed_folders.contains(&path.to_lowercase()) { + true => FolderFill::EmptyOnCore, + false => FolderFill::EmptyLocal, + }, + }; let mut fchildren = Vec::new(); if fopen { expanded.push(fid.clone()); @@ -661,6 +819,7 @@ fn convert_node( child, core, counts, + confirmed_folders, order_counts, selected_ids, folders, @@ -680,15 +839,16 @@ fn convert_node( NodeData::Folder { core, path: prefix.clone(), - label: name.clone(), + label: name.to_string(), active, total, selected: view.selected_folder.as_ref() == Some(&(core, path)), checked: fchecked, + fill, }, ); out.push( - MoonTreeItem::new(fid, name.clone()) + MoonTreeItem::new(fid, name.to_string()) .folder(true) .children(fchildren), ); @@ -935,11 +1095,15 @@ fn render_row( p.blue, 600.0, ToggleTarget::Core(core), + // A core root is a heading, not a folder: it keeps its caret and its bulk box even + // with nothing under it, because what it covers is the whole core. + FolderFill::Populated, step, app, ) } NodeData::Folder { + fill, core, path, label, @@ -959,10 +1123,22 @@ fn render_row( indent, label.clone(), // A folder carries no order count of its own; the core root above it owns that. - RowCounts::subtree(*active, *total, 0), - p.text_soft, + // An empty one shows no numbers at all rather than `0/0`: there is nothing to + // count, and the slot's tooltip says what the row is instead. + match fill.has_contents() { + true => RowCounts::subtree(*active, *total, 0), + false => RowCounts::empty_folder(fill.empty_tip()), + }, + // An empty folder reads as quieter than one with strategies in it, which is the + // whole of what its emptiness looks like — nothing is hidden and no glyph is + // invented for it. + match fill.has_contents() { + true => p.text_soft, + false => p.text_muted, + }, 400.0, ToggleTarget::Folder(core, path), + *fill, step, app, ) @@ -1008,6 +1184,8 @@ fn render_row( p.text_muted, 400.0, ToggleTarget::Deleted(core), + // Deleted is only ever drawn when it holds rows. + FolderFill::Populated, step, app, ) @@ -1133,6 +1311,24 @@ impl RowCounts { } } + /// Counters for a folder holding nothing: no numbers, and a tooltip that says why. + /// + /// Both slots stay reserved by the row itself, so an empty folder's caption keeps the column + /// its siblings' captions sit on. + /// + /// Args: + /// tip: What this row is, from [`FolderFill::empty_tip`]. + /// + /// Returns: + /// Empty counters carrying that tooltip. + fn empty_folder(tip: Option) -> Self { + Self { + primary: String::new(), + orders: String::new(), + tip: SharedString::from(tip.unwrap_or_default()), + } + } + /// Counters for a core's Deleted heading, which carries one number and no orders. fn deleted(count: usize) -> Self { Self { @@ -1170,6 +1366,39 @@ fn counts_slot(text: String, width: f32, color: u32, step: f32, app: &App) -> im ) } +/// What a folder row has inside it, which decides how much of a row it draws. +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum FolderFill { + /// Holds at least one strategy, filtered away or not. + Populated, + /// Holds nothing, and the CORE says so — it keeps this folder in its own tree. + EmptyOnCore, + /// Holds nothing and exists only in this window: either the core cannot keep empty folders, or + /// it has not confirmed this one yet. + EmptyLocal, +} + +impl FolderFill { + /// Whether this row has anything to expand or to check. + /// + /// Both controls are omitted when it does not, because both would otherwise be live controls + /// that cannot do anything: a caret that opens onto nothing, and a box whose "check every + /// strategy below" covers no strategy. Their space is still reserved, so the caption of an + /// empty folder stays on the same column as its siblings'. + fn has_contents(self) -> bool { + matches!(self, Self::Populated) + } + + /// The tooltip an empty folder's counter carries, or `None` for a populated one. + fn empty_tip(self) -> Option { + match self { + Self::Populated => None, + Self::EmptyOnCore => Some(rust_i18n::t!("strat.folder_empty_tip").to_string()), + Self::EmptyLocal => Some(rust_i18n::t!("strat.folder_empty_local_tip").to_string()), + } + } +} + enum ToggleTarget { Core(CoreId), Folder(CoreId, Vec), @@ -1191,7 +1420,6 @@ impl ToggleTarget { } } -#[allow(clippy::too_many_arguments)] /// Render a clickable core, folder, or Deleted heading in the strategy tree. /// /// Actual folders also receive the context menu assembled inside this function. The disclosure @@ -1214,6 +1442,7 @@ impl ToggleTarget { /// /// Returns: /// The complete interactive tree row. +#[allow(clippy::too_many_arguments)] fn core_folder_row( view: &Entity, row_id: SharedString, @@ -1226,6 +1455,7 @@ fn core_folder_row( color: u32, weight: f32, target: ToggleTarget, + fill: FolderFill, step: f32, app: &App, ) -> AnyElement { @@ -1235,6 +1465,7 @@ fn core_folder_row( // The bulk checkbox stages this row's own subtree, which the core root spells as the empty // path. Deleted holds no live strategy, so it addresses no folder and carries no checkbox. // Resolved once: the row renders on every repaint, and each resolve deep-clones the path. + let expandable = fill.has_contents(); let folder_key = target.folder_key(); let check_target = folder_key.clone().map(|(core, path)| (core, path, checked)); let menu = match &target { @@ -1273,12 +1504,19 @@ fn core_folder_row( // The unscaled base rides the pane's local text step so the caret stays proportional to // the row it marks; `MoonDisclosure` still applies the UI scale on top of it internally, // so the value passed here must stay unscaled. - .child( - MoonDisclosure::glyph(expanded) + .child(match fill.has_contents() { + true => MoonDisclosure::glyph(expanded) .size(design::DISCLOSURE_GLYPH_MARKER + step) - .box_size(design::DISCLOSURE_BOX + step), - ) - .child(match check_target { + .box_size(design::DISCLOSURE_BOX + step) + .into_any_element(), + // Reserved, not omitted: the caption of an empty folder belongs on the same column as + // every sibling's, and a caret that opens onto nothing is a control that lies. + false => div() + .flex_none() + .w(design::ui_px(app, design::DISCLOSURE_BOX + step)) + .into_any_element(), + }) + .child(match check_target.filter(|_| fill.has_contents()) { Some((core, path, checked)) => { checks::bulk_check(view, &check_row_id, core, path, checked) } @@ -1338,7 +1576,13 @@ fn core_folder_row( this.selected_folder = Some((*c, String::new())); } ToggleTarget::Folder(c, path) => { - toggle(&mut this.expanded_folders, (*c, path.join("/"))); + // Nothing to open, so nothing is toggled: the caret is not drawn for an + // empty folder, and flipping hidden expansion state would still churn the + // hashed set the whole tree is cached on. Selecting it below is what a + // click on it is for. + if expandable { + toggle(&mut this.expanded_folders, (*c, path.join("/"))); + } // Match Moonbot by selecting the clicked folder for highlighting and Ctrl+C. this.selected_folder = Some((*c, path.join("/"))); } diff --git a/crates/moon-ui-gpui/src/strategies/tree/moon/tests.rs b/crates/moon-ui-gpui/src/strategies/tree/moon/tests.rs index d3815fc5..3cb23924 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/moon/tests.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/moon/tests.rs @@ -71,6 +71,7 @@ fn core_node(core: CoreId) -> NodeData { /// Folder row with the given path segments; must remain a live drop destination. fn folder_node(core: CoreId, path: &[&str]) -> NodeData { NodeData::Folder { + fill: super::FolderFill::Populated, core, path: path.iter().map(|p| (*p).to_string()).collect(), label: "folder".into(), diff --git a/crates/moon-ui-gpui/src/strategies/tree/ops.rs b/crates/moon-ui-gpui/src/strategies/tree/ops.rs index 68046674..5fae8325 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/ops.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/ops.rs @@ -511,5 +511,104 @@ pub fn move_to(rows: &[&StrategyRow], target: &[String]) -> Vec<(u64, String)> { rows.iter().map(|r| (r.id, path.clone())).collect() } +// --- Reordering inside a folder ------------------------------------------- + +/// Which way one reorder step moves the selection. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum MoveStep { + /// One place towards the start of the folder. + Up, + /// One place towards its end. + Down, +} + +/// Move every selected strategy one place inside its own folder, and return the core's new order. +/// +/// A core's strategy list is an ORDER the operator arranged in MoonBot, and moonproto synchronizes +/// it as the row sequence of a Full snapshot (`docs/strats.md`, "Strategy Order"). So a reorder is +/// not a local view preference: the whole list goes back to the core in its new sequence. +/// +/// Two rules keep it behaving like every other list reorder: +/// +/// * A row moves only inside its own folder. The protocol asks that one folder's strategies stay +/// a contiguous group, and a row that could walk out the top of its folder would silently +/// change what folder it is in — that is what dragging is for. +/// * Only rows the tree currently DRAWS take part. With a filter on, "up" therefore means above +/// the row visibly above it, while a hidden strategy keeps the slot it holds in the core's own +/// list. Moving against invisible neighbours instead would spend a press on nothing. +/// +/// A block of adjacent selected rows moves together and stops at its folder's edge, one row at a +/// time, which is the usual behaviour of such a control. +/// +/// Args: +/// rows: The core's complete strategy list, in the order the tree currently shows it. +/// selected: Strategy ids the operator is moving. +/// visible: Whether one row is drawn under the active filter. +/// step: Direction to move the selection. +/// +/// Returns: +/// The core's complete new id sequence, or `None` when nothing in the selection can move — +/// an empty selection, or a block already sitting against the edge of its folder. +pub fn reorder_step( + rows: &[&StrategyRow], + selected: &HashSet, + visible: impl Fn(&StrategyRow) -> bool, + step: MoveStep, +) -> Option> { + // Per folder, the INDEXES of the rows that folder draws. Indexes rather than ids because the + // permutation is written back into exactly these slots, which leaves every hidden row and every + // other folder's row untouched wherever it sits. + let mut slots_by_folder: Vec> = Vec::new(); + let mut folder_at: std::collections::HashMap = std::collections::HashMap::new(); + for (at, row) in rows.iter().enumerate() { + if !visible(row) { + continue; + } + // The tree's own folder identity, so a row groups with the folder it is drawn under even + // when the wire spelled the path with a backslash or a doubled separator. + let folder = join_path(&split_path(&row.folder_path)); + let group = match folder_at.get(&folder) { + Some(&group) => group, + None => { + folder_at.insert(folder, slots_by_folder.len()); + slots_by_folder.push(Vec::new()); + slots_by_folder.len() - 1 + } + }; + slots_by_folder[group].push(at); + } + + let mut order: Vec = rows.iter().map(|row| row.id).collect(); + let mut moved = false; + for slots in &slots_by_folder { + let mut ids: Vec = slots.iter().map(|&at| order[at]).collect(); + match step { + // Front to back going up, back to front going down: each selected row is swapped with + // the neighbour beyond it only when that neighbour is NOT itself selected, so a block + // shifts by one and cannot pass through its own members. + MoveStep::Up => { + for at in 1..ids.len() { + if selected.contains(&ids[at]) && !selected.contains(&ids[at - 1]) { + ids.swap(at - 1, at); + moved = true; + } + } + } + MoveStep::Down => { + for at in (0..ids.len().saturating_sub(1)).rev() { + if selected.contains(&ids[at]) && !selected.contains(&ids[at + 1]) { + ids.swap(at, at + 1); + moved = true; + } + } + } + } + for (&at, id) in slots.iter().zip(ids) { + order[at] = id; + } + } + moved.then_some(order) +} + #[cfg(test)] mod tests; diff --git a/crates/moon-ui-gpui/src/strategies/tree/ops/tests.rs b/crates/moon-ui-gpui/src/strategies/tree/ops/tests.rs index 84f0a858..cb1cf5ff 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/ops/tests.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/ops/tests.rs @@ -359,3 +359,90 @@ fn move_to_flattens_to_target() { assert!(edits.contains(&(1, "dest".to_string()))); assert!(edits.contains(&(2, "dest".to_string()))); } + +// --- reorder_step --------------------------------------------------------- + +/// Runs one step over a core list, with every row visible unless `hidden` names it. +fn step(rows: &[StrategyRow], selected: &[u64], hidden: &[u64], dir: MoveStep) -> Option> { + let refs: Vec<&StrategyRow> = rows.iter().collect(); + let picked: HashSet = selected.iter().copied().collect(); + reorder_step(&refs, &picked, |row| !hidden.contains(&row.id), dir) +} + +/// The whole core list comes back, not just the moved pair: the sequence IS the payload the core +/// is sent, so a partial list would tell it to rearrange everything else too. +#[test] +fn one_step_swaps_with_the_neighbour_and_returns_the_whole_list() { + let rows = vec![ + row(1, "a", "f", false), + row(2, "b", "f", false), + row(3, "c", "f", false), + ]; + assert_eq!(step(&rows, &[3], &[], MoveStep::Up), Some(vec![1, 3, 2])); + assert_eq!(step(&rows, &[1], &[], MoveStep::Down), Some(vec![2, 1, 3])); +} + +/// A row at the edge of its folder has nowhere to go, and `None` is what disables the button rather +/// than sending the core an order identical to the one it holds. +#[test] +fn a_row_against_the_edge_of_its_folder_reports_nothing_to_do() { + let rows = vec![row(1, "a", "f", false), row(2, "b", "f", false)]; + assert_eq!(step(&rows, &[1], &[], MoveStep::Up), None); + assert_eq!(step(&rows, &[2], &[], MoveStep::Down), None); + assert_eq!(step(&rows, &[], &[], MoveStep::Up), None); +} + +/// Folders are independent lists. A row must not walk out of the top of its own folder into the one +/// drawn above it — that would change what folder it is in, silently, through a button whose whole +/// promise is that it only moves things around. +#[test] +fn a_row_never_leaves_its_own_folder() { + let rows = vec![ + row(1, "a", "one", false), + row(2, "b", "two", false), + row(3, "c", "two", false), + ]; + assert_eq!(step(&rows, &[2], &[], MoveStep::Up), None); + // ... and moving inside the second folder leaves the first one's row exactly where it sits. + assert_eq!(step(&rows, &[3], &[], MoveStep::Up), Some(vec![1, 3, 2])); +} + +/// The same folder spelled two ways is ONE folder in the tree, so it has to be one list here too. +#[test] +fn folder_identity_follows_the_trees_own_path_split() { + let rows = vec![ + row(1, "a", "deep/inner", false), + row(2, "b", "deep\\inner", false), + ]; + assert_eq!(step(&rows, &[2], &[], MoveStep::Up), Some(vec![2, 1])); +} + +/// A selected block moves as a block and stops at the edge together, instead of collapsing onto +/// itself when its members swap through each other. +#[test] +fn a_block_moves_together_and_stops_at_the_edge() { + let rows = vec![ + row(1, "a", "f", false), + row(2, "b", "f", false), + row(3, "c", "f", false), + row(4, "d", "f", false), + ]; + assert_eq!( + step(&rows, &[3, 4], &[], MoveStep::Up), + Some(vec![1, 3, 4, 2]) + ); + assert_eq!(step(&rows, &[1, 2], &[], MoveStep::Up), None); +} + +/// With a filter on, "up" means above the row visibly above it. A hidden strategy keeps the slot it +/// holds in the core's list — the press moves the selection past it, not into its place. +#[test] +fn a_hidden_row_keeps_its_slot_while_the_visible_ones_move_around_it() { + let rows = vec![ + row(1, "a", "f", false), + row(2, "hidden", "f", false), + row(3, "c", "f", false), + ]; + // Slots 0 and 2 are the drawn ones; they exchange, and slot 1 still holds the hidden row. + assert_eq!(step(&rows, &[3], &[2], MoveStep::Up), Some(vec![3, 2, 1])); +} diff --git a/crates/moon-ui-gpui/src/strategies/tree/pane_cache.rs b/crates/moon-ui-gpui/src/strategies/tree/pane_cache.rs index 64b1b0ca..d2f61bf1 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/pane_cache.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/pane_cache.rs @@ -63,6 +63,8 @@ pub(in crate::strategies) struct LeftPaneFrame { pub(super) footer_label_width: f32, /// Staged checkbox count the footer both measures against and renders. pub(super) staged: usize, + /// Whether the footer's two move buttons have anything to do, as `(up, down)`. + pub(super) moves: (bool, bool), } /// Everything the Start/Stop plan reads, as one comparable key. @@ -95,6 +97,24 @@ pub(in crate::strategies) struct PaneCache { exchanges: Option<(u64, SharedString, ExchangeList)>, plan: Option<(PlanKey, Arc)>, labels: Option<(LabelKey, f32)>, + moves: Option<(MoveKey, (bool, bool))>, +} + +/// Everything the move buttons' enablement reads, as one comparable key. +/// +/// Both halves of the tree signature, because the answer depends on the core's strategy list AND on +/// the window: the selection, the filter that decides which rows are drawn, and any unconfirmed +/// order overlaying them. All three live in the view half already. +#[derive(Clone, Copy, PartialEq, Eq)] +struct MoveKey { + /// Cores and their strategy snapshots. + store: u64, + /// Selection, filter, expansion, and the unconfirmed-order overlay. + view: u64, + /// The workspace scope itself, which neither half of the tree signature carries: a preset can + /// change which cores the plan admits while naming exactly the cores already listed, moving + /// nothing the other two hash. + workspace: u64, } impl StrategiesView { @@ -121,7 +141,42 @@ impl StrategiesView { plan: self.pane_plan(sig, cores, cx), footer_label_width: self.pane_footer_label_width(staged, cx), staged, + moves: self.pane_moves(sig, cx), + } + } + + /// Enablement of the two move buttons, recomputed only when its inputs moved. + /// + /// Retained for the same reason as everything else here: answering it walks every strategy of + /// every core the selection touches and allocates a canonical folder path per row, and this row + /// is rebuilt on every hover repaint. Asked once per button, per frame, it was the largest new + /// per-frame cost in the pane. + /// + /// Args: + /// sig: Tree signature already computed for this frame. + /// cx: Application context used to read the store. + /// + /// Returns: + /// Whether a move up, and a move down, would rearrange anything. + fn pane_moves(&mut self, sig: TreeSig, cx: &App) -> (bool, bool) { + let key = MoveKey { + store: sig.store, + view: sig.view, + workspace: workspace_digest(self.workspace_cores.as_deref()), + }; + if let Some((cached, moves)) = &self.pane_cache.moves + && *cached == key + { + return *moves; } + crate::diag::bump(&crate::diag::STRAT_PANE_BUILD); + let built = { + let backend = self.backend.read(cx); + let store = backend.session.store(); + self.move_availability(store, backend.session.core_venues()) + }; + self.pane_cache.moves = Some((key, built)); + built } /// Strategy kinds across the visible cores, rebuilt only when the store moved. @@ -352,3 +407,22 @@ fn footer_label_width(cx: &App, staged: usize) -> f32 { } width } + +/// Fold the effective workspace scope into one comparable number. +/// +/// Ordered, because the scope IS an ordered list of ids and two different scopes must not collide; +/// `None` — unscoped Classic — is distinct from an empty scope, which admits no core at all. +/// +/// Args: +/// cores: Concrete scoped core ids, or `None` when the window is unscoped. +/// +/// Returns: +/// A digest that changes whenever the scope does. +fn workspace_digest(cores: Option<&[CoreId]>) -> u64 { + match cores { + None => 0, + Some(cores) => cores + .iter() + .fold(1u64, |acc, core| acc.wrapping_mul(31).wrapping_add(*core)), + } +} diff --git a/crates/moon-ui-gpui/src/strategies/tree/pane_cache/tests.rs b/crates/moon-ui-gpui/src/strategies/tree/pane_cache/tests.rs index ec0677ce..1aa5f359 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/pane_cache/tests.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/pane_cache/tests.rs @@ -135,3 +135,131 @@ fn the_selection_toolbar_takes_core_visibility_from_its_caller() { ); assert!(can_paste.contains("has_visible_cores")); } + +/// The same demand as the plan key, for the move buttons' enablement: every piece of WINDOW state +/// the planner touches must be covered by a named component of `MoveKey`. +/// +/// Two functions are scanned, not one. `movable_selection` reads most of it, but it reaches the +/// unconfirmed-order overlay through `displayed_rows`; whitelisting that name as if it were a field +/// would put everything behind it — today `pending_order`, tomorrow whatever is added — outside the +/// scan this test is named for. +/// +/// And covering the view HALF is only half an argument, since that half is built elsewhere. So the +/// second block below checks the other end: that `data_sig` still folds each of these fields into +/// it. Without that pair, `MoveKey` could name a component that no longer carries what it claims. +/// +/// One input the scan cannot see either way: `workspace_cores`, which the planner reads through +/// `selected_keys` and which NEITHER half of the tree signature hashes — a preset naming exactly +/// the connected cores moves no signature at all. It gets its own component, asserted at the end. +#[test] +fn the_move_key_covers_every_field_the_enablement_reads() { + /// Window state the planner reads directly, all of it inside the signature's view half. + const VIEW_FIELDS: [&str; 4] = [ + "filter", + "expanded_cores", + "rail_expanded_core", + // Read by `displayed_rows`, which is scanned alongside the planner for exactly this reason. + "pending_order", + ]; + + let reorder = include_str!("../reorder.rs"); + let body = |marker: &str| { + reorder + .split(marker) + .nth(1) + .and_then(|tail| tail.split("\n }").next()) + .expect("the scanned function must exist") + .to_string() + }; + let scanned = format!( + "{}{}", + body("fn movable_selection<"), + body("fn displayed_rows<") + ); + let packed: String = scanned.chars().filter(|c| !c.is_whitespace()).collect(); + let cache: String = include_str!("../pane_cache.rs") + .chars() + .filter(|c| !c.is_whitespace()) + .collect(); + let signature = include_str!("../cache.rs"); + + let mut missing = Vec::new(); + let mut checked = 0; + for chunk in packed.split("self.").skip(1) { + let field: String = chunk + .chars() + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + // `displayed_rows` is the scanned callee itself, not an input of its own. + if field.is_empty() || field == "displayed_rows" { + continue; + } + checked += 1; + if !VIEW_FIELDS.contains(&field.as_str()) { + missing.push(field); + } + } + assert!( + checked >= 5, + "the scan found only {checked} field reads — a read moved into a helper and left this \ + test guarding less than it claims" + ); + missing.sort(); + missing.dedup(); + assert!( + missing.is_empty(), + "these inputs decide the move buttons but are absent from MoveKey, so their enablement \ + would go stale:\n{}", + missing.join("\n") + ); + + // The key names the view half... + assert!( + cache.contains("view:sig.view"), + "the view half must be a component of MoveKey, since every field above lives in it" + ); + // ... and the view half still carries each of those fields. + for field in VIEW_FIELDS { + assert!( + signature.contains(&format!("view.{field}")), + "data_sig no longer folds `{field}` into the view half, so MoveKey's claim to cover it \ + through `sig.view` is empty" + ); + } + + // The one input neither scan nor signature can account for. + assert!( + packed.contains("selected_keys(self)"), + "the selection must come from the canonical resolver, which is what applies the workspace \ + scope the component below stands for" + ); + assert!( + cache.contains("workspace:workspace_digest(self.workspace_cores"), + "the workspace scope is hashed by neither half of the tree signature, so MoveKey must \ + carry it directly" + ); +} + +/// The enablement walks every strategy of every selected core and allocates a canonical folder path +/// per row. It belongs behind the pane cache, and every render path must take the answer from its +/// caller — the same rule `the_plan_key_covers_every_field_the_plan_reads` enforces for the +/// Start/Stop plan. +/// +/// The context menu is exempt and named here so the exemption is deliberate: it is built on a +/// right-click, not on a frame. +#[test] +fn the_move_buttons_take_their_enablement_from_the_pane_cache() { + for (name, source) in [ + ("tree/ui.rs", include_str!("../ui.rs")), + ("tree/mod.rs", include_str!("../mod.rs")), + ("tree/moon.rs", include_str!("../moon.rs")), + ("tree/cache.rs", include_str!("../cache.rs")), + ("strategies/mod.rs", include_str!("../../mod.rs")), + ] { + assert!( + !source.contains("move_availability("), + "{name} must render the cached answer, not re-derive it per frame" + ); + } + assert!(include_str!("../pane_cache.rs").contains("self.move_availability(")); +} diff --git a/crates/moon-ui-gpui/src/strategies/tree/reorder.rs b/crates/moon-ui-gpui/src/strategies/tree/reorder.rs new file mode 100644 index 00000000..3f1774ad --- /dev/null +++ b/crates/moon-ui-gpui/src/strategies/tree/reorder.rs @@ -0,0 +1,345 @@ +//! Moving strategies up and down inside their folder, and holding the result on screen until the +//! core confirms it. +//! +//! A core's strategy list is an ORDER, not a set: the operator arranges it in MoonBot and moonproto +//! synchronizes that arrangement as the row sequence of a Full snapshot. The permutation itself is +//! [`ops::reorder_step`]; this module is the part that has a window — which rows a press acts on, +//! the command that carries the result to the core, and the overlay below. +//! +//! ## Why the overlay +//! +//! `sync_local_strategies` does NOT reorder moonproto's retained list. The library keeps the +//! core-confirmed order and rewrites it only from the core's own Full echo (`apply_server_order`), +//! so between the press and that echo `CoreData::strategies` still holds the OLD sequence. Drawn +//! straight, the tree would sit still for the whole round trip and — far worse — a second press +//! would be computed from the arrangement the first one already replaced, so holding the button +//! would produce one move instead of five. [`PendingOrder`] is that gap, and nothing more: it +//! overlays the sequence already sent, and it is dropped the moment the core answers. +//! +//! ## What it does not do +//! +//! Nothing tells the window that a reorder was REFUSED. The command's outcome lives on the feed +//! thread — a core whose state is not ready, a send that fails — and reaches only the log. Such an +//! overlay is retired by [`CONFIRMATION_WINDOW`] instead of by an answer, so the arrangement on +//! screen reverts silently rather than saying why. Closing that would take a result path from the +//! feed back to the view, which no strategy command has today. + +use std::collections::{HashMap, HashSet}; +use std::time::{Duration, Instant}; + +use moon_core::feed::StrategyRow; +use moon_core::session::{CoreId, CoreStore}; +use moon_core::venue::CoreVenue; + +use super::super::StrategiesView; +use super::super::filter::PreparedFilter; +use super::super::logic::selected_keys; +use super::ops::{self, MoveStep}; + +/// How long an unconfirmed order keeps overriding what the core reports. +/// +/// OUR bound, not a protocol promise. moonproto's `Edit*` lifecycle — including its 45-second +/// confirmation timeout — tracks strategy ROWS and explicitly not reorder-only actions, so a +/// reorder has no deadline of its own on the wire. The number is borrowed from that window for +/// consistency; what matters is only that one exists, because the alternative is an arrangement +/// that no core holds staying on screen for the rest of the session. +const CONFIRMATION_WINDOW: Duration = Duration::from_secs(45); + +/// One core's strategy order that has been sent and not yet echoed back. +pub(in crate::strategies) struct PendingOrder { + /// The sequence that was sent, which the tree cache hashes and a confirmation is compared to. + ids: Vec, + /// Position of each of those ids, so the tree can place a row without scanning the sequence. + ranks: HashMap, + /// When it went to the core, for [`CONFIRMATION_WINDOW`]. + sent: Instant, +} + +impl PendingOrder { + /// Record a freshly sent sequence. + fn new(ids: Vec) -> Self { + let ranks = ids + .iter() + .enumerate() + .map(|(rank, id)| (*id, rank)) + .collect(); + Self { + ids, + ranks, + sent: Instant::now(), + } + } + + /// The sent sequence, for the tree cache's signature — which must separate two orders of the + /// same ids, the case a second press before the core answers produces. + pub(in crate::strategies) fn ids(&self) -> &[u64] { + &self.ids + } + + /// Position of one strategy in the sent sequence, or `None` for an id it never named. + pub(in crate::strategies) fn rank(&self, id: u64) -> Option { + self.ranks.get(&id).copied() + } + + /// Whether `live` — the core's own current sequence — already agrees with what was sent. + /// + /// Read as: the ids the two have in COMMON appear in the live list in ascending sent position. + /// Only the shared ones, because a strategy created, deleted or restored in the meantime is not + /// a disagreement about order — without that, one unrelated create would pin the overlay open + /// for the whole confirmation window. + /// + /// Args: + /// live: The core's current strategy ids, in the order it reports them. + /// + /// Returns: + /// Whether the two sequences order their shared ids the same way. + fn confirmed_by(&self, live: impl Iterator) -> bool { + let mut previous: Option = None; + for id in live { + let Some(rank) = self.rank(id) else { continue }; + if previous.is_some_and(|prior| rank < prior) { + return false; + } + previous = Some(rank); + } + true + } +} + +/// What one core contributes to a move: its rows as the tree draws them, and the ids selected in it. +struct MovableCore<'a> { + /// Core these rows belong to. + core: CoreId, + /// Its strategies in display order — the core's own, or an unconfirmed order overlaying it. + rows: Vec<&'a StrategyRow>, + /// Strategy ids of the current selection that live in this core. + selected: HashSet, +} + +/// Everything one press acts on, resolved once for both directions. +struct MoveScope<'a> { + /// Row predicate deciding which strategies are drawn, shared by every core below. + filter: PreparedFilter, + /// The cores the tree actually shows, in the order the selection first named them. + cores: Vec>, +} + +impl StrategiesView { + /// The sequence one core's rows are currently DRAWN in: its own, unless an unconfirmed reorder + /// is overlaying it. + /// + /// This is what a reorder must be computed from, or a second press repeats the first. + /// + /// Args: + /// store: Live per-core strategy snapshots. + /// core: Core to read. + /// + /// Returns: + /// Borrowed rows in display order; empty when the core is not in the store. + fn displayed_rows<'a>(&self, store: &'a CoreStore, core: CoreId) -> Vec<&'a StrategyRow> { + let Some(data) = store.core(core) else { + return Vec::new(); + }; + let mut rows: Vec<&StrategyRow> = data.strategies.iter().collect(); + if let Some(pending) = self.pending_order.get(&core) { + // The same rule the feed and the tree apply, so all three agree on where a strategy the + // sent sequence never named belongs. + moon_core::feed::strategy_order::resequence(&mut rows, |row| pending.rank(row.id)); + } + rows + } + + /// Group the selection by core, keeping only the cores the tree actually shows. + /// + /// A core is skipped when the exchange filter has taken it off the screen or when its row is + /// collapsed: in both cases the operator cannot see the rows, so a press that rearranged them + /// would be a change nobody watched being made. + /// + /// One pass, and shared by both directions, because the two move buttons ask this same question + /// on every pane-cache miss — which is every strategy revision on any core. + /// + /// Args: + /// store: Live per-core strategy snapshots. + /// venues: Session venue identities, for the exchange filter. + /// + /// Returns: + /// The prepared row filter, and per core its drawn row order with the ids selected in it. + fn movable_selection<'a>( + &self, + store: &'a CoreStore, + venues: &HashMap, + ) -> MoveScope<'a> { + let filter = self.filter.prepare(); + // Search forces every core and folder open, exactly as the tree build reads it. + let searching = filter.searching(); + let mut cores: Vec> = Vec::new(); + for (core, id) in selected_keys(self) { + if let Some(entry) = cores.iter_mut().find(|entry| entry.core == core) { + entry.selected.insert(id); + continue; + } + if !self.filter.core_matches(venues.get(&core)) { + continue; + } + let open = searching + || super::super::state::core_is_open( + &self.expanded_cores, + self.rail_expanded_core, + core, + ); + if !open { + continue; + } + cores.push(MovableCore { + core, + rows: self.displayed_rows(store, core), + selected: HashSet::from([id]), + }); + } + MoveScope { filter, cores } + } + + /// Plan one reorder step for every core the selection reaches. + /// + /// Args: + /// store: Live per-core strategy snapshots. + /// venues: Session venue identities, for the exchange filter. + /// step: Direction the operator asked for. + /// + /// Returns: + /// `(core, complete new id sequence)` for each core that can actually move. + fn reorder_plan( + &self, + store: &CoreStore, + venues: &HashMap, + step: MoveStep, + ) -> Vec<(CoreId, Vec)> { + let scope = self.movable_selection(store, venues); + scope + .cores + .into_iter() + .filter_map(|entry| { + ops::reorder_step( + &entry.rows, + &entry.selected, + |row| scope.filter.matches(row), + step, + ) + .map(|order| (entry.core, order)) + }) + .collect() + } + + /// Whether each move button has anything to do, as `(up, down)`. + /// + /// Derived from the same rows the click acts on, so a button is enabled exactly when pressing + /// it would change something — a selection at the top of its folder disables Up and nothing + /// else. Both directions come out of ONE grouping pass; asking twice walked every strategy of + /// every selected core a second time for an answer built from identical inputs. + /// + /// Called through the pane cache rather than per frame: see [`super::pane_cache`]. + /// + /// Args: + /// store: Live per-core strategy snapshots. + /// venues: Session venue identities, for the exchange filter. + /// + /// Returns: + /// Whether a move up, and a move down, would rearrange at least one core. + pub(in crate::strategies) fn move_availability( + &self, + store: &CoreStore, + venues: &HashMap, + ) -> (bool, bool) { + let scope = self.movable_selection(store, venues); + let mut up = false; + let mut down = false; + for entry in &scope.cores { + let visible = |row: &StrategyRow| scope.filter.matches(row); + let ask = |step| ops::reorder_step(&entry.rows, &entry.selected, visible, step); + up = up || ask(MoveStep::Up).is_some(); + down = down || ask(MoveStep::Down).is_some(); + } + (up, down) + } + + /// Move the selection one place inside its folder and send the new order to each core. + /// + /// Args: + /// step: Direction the operator asked for. + /// cx: View context used to reach the session and repaint. + /// + /// Returns: + /// Nothing; a selection that cannot move, and a core that refuses the command, both leave + /// the view untouched. + pub(in crate::strategies) fn move_selection( + &mut self, + step: MoveStep, + cx: &mut gpui::Context, + ) { + let plan = { + let backend = self.backend.read(cx); + self.reorder_plan(backend.session.store(), backend.session.core_venues(), step) + }; + let mut sent = false; + for (core, order) in plan { + // Read per core rather than hoisted: `self.pending_order` is written inside this loop, + // and the borrow checker is right that the two cannot overlap. + let result = self + .backend + .read(cx) + .session + .reorder_strategies(core, order.clone()); + match result { + Ok(()) => { + self.pending_order.insert(core, PendingOrder::new(order)); + sent = true; + } + // Left OUT of `pending_order` on purpose: an overlay for a command that never + // reached the queue would show an arrangement no core will ever confirm. A + // multi-core selection applies core by core, so the ones that were queued stay + // queued — there is no arrangement spanning two cores to roll back to. + Err(error) => log::warn!("reorder strategies failed: {error}"), + } + } + if sent { + cx.notify(); + } + } + + /// Drop overlays the core has answered, or waited long enough for. + /// + /// Called from two places, and it needs both: from the backend observer, where the core's echo + /// arrives, and from render, because the deadline has to elapse even for a core that has gone + /// quiet — which is the very failure the deadline exists for, and the one case that produces no + /// backend notify at all. + /// + /// Args: + /// store: Live per-core strategy snapshots. + /// + /// Returns: + /// Whether anything was dropped, so the caller can repaint on the frame the tree stops + /// showing the overlay. + pub(in crate::strategies) fn reconcile_pending_order(&mut self, store: &CoreStore) -> bool { + if self.pending_order.is_empty() { + return false; + } + let before = self.pending_order.len(); + let now = Instant::now(); + self.pending_order.retain(|core, pending| { + // A core REMOVED FROM CONFIGURATION, which is the only thing that takes its data out of + // the store — a disconnect leaves the snapshot in place, and that overlay is retired by + // the deadline below or by the full list the core resends when it comes back. + let Some(data) = store.core(*core) else { + return false; + }; + if pending.confirmed_by(data.strategies.iter().map(|row| row.id)) { + return false; + } + now.duration_since(pending.sent) < CONFIRMATION_WINDOW + }); + before != self.pending_order.len() + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/moon-ui-gpui/src/strategies/tree/reorder/tests.rs b/crates/moon-ui-gpui/src/strategies/tree/reorder/tests.rs new file mode 100644 index 00000000..2db30539 --- /dev/null +++ b/crates/moon-ui-gpui/src/strategies/tree/reorder/tests.rs @@ -0,0 +1,50 @@ +//! Unit tests for the unconfirmed-order overlay. +//! +//! Imports are explicit rather than `use super::*`: this module's ancestors re-export `gpui::*`, +//! whose `test` shadows the attribute and makes `#[test]` recurse (see `presentation/tests.rs`). + +use super::PendingOrder; + +/// The core has applied exactly what was sent, so the overlay has done its job and must go — a +/// confirmed overlay that stayed would keep outranking the core on every later change. +#[test] +fn an_echo_of_the_sent_sequence_confirms_it() { + let pending = PendingOrder::new(vec![3, 1, 2]); + assert!(pending.confirmed_by([3, 1, 2].into_iter())); + assert!(!pending.confirmed_by([1, 2, 3].into_iter())); +} + +/// A strategy created, deleted or restored between the press and the echo is not a disagreement +/// about order. Comparing the raw lists instead would leave the overlay open for its full window +/// after any unrelated create, and the tree would keep drawing a stale arrangement all that time. +#[test] +fn a_created_or_deleted_strategy_does_not_block_confirmation() { + let pending = PendingOrder::new(vec![3, 1, 2]); + // 9 was created after the press; the shared ids still read 3, 1, 2. + assert!(pending.confirmed_by([3, 1, 9, 2].into_iter())); + // 1 was deleted; what remains is still in the order that was asked for. + assert!(pending.confirmed_by([3, 2].into_iter())); + // ... but a genuine disagreement among the survivors is still one. + assert!(!pending.confirmed_by([2, 3].into_iter())); +} + +/// The sequence is what the tree cache hashes, and its one job there is to separate two orders of +/// the SAME ids — the case a second press before the core answers produces. +#[test] +fn the_sequence_is_exposed_in_order_for_the_cache() { + assert_eq!(PendingOrder::new(vec![1, 3, 2]).ids(), &[1, 3, 2]); + assert_ne!( + PendingOrder::new(vec![1, 2, 3]).ids(), + PendingOrder::new(vec![1, 3, 2]).ids() + ); +} + +/// Rank is what places a row, and an id the sequence never named has none — such a row keeps the +/// row it follows instead of taking a position the operator never chose. +#[test] +fn an_unnamed_id_has_no_rank() { + let pending = PendingOrder::new(vec![7, 4]); + assert_eq!(pending.rank(7), Some(0)); + assert_eq!(pending.rank(4), Some(1)); + assert_eq!(pending.rank(5), None); +} diff --git a/crates/moon-ui-gpui/src/strategies/tree/tests.rs b/crates/moon-ui-gpui/src/strategies/tree/tests.rs index 825ed9c6..5ed412d1 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/tests.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/tests.rs @@ -95,6 +95,7 @@ impl Render for StratDragHarness { checked: false, }); let folder_dest = drop_dest(&NodeData::Folder { + fill: super::moon::FolderFill::Populated, core: 7, path: vec!["desk".into(), "live".into()], label: "folder".into(), diff --git a/crates/moon-ui-gpui/src/strategies/tree/ui.rs b/crates/moon-ui-gpui/src/strategies/tree/ui.rs index 7288f161..ebd8d5a1 100644 --- a/crates/moon-ui-gpui/src/strategies/tree/ui.rs +++ b/crates/moon-ui-gpui/src/strategies/tree/ui.rs @@ -31,6 +31,24 @@ pub(super) fn footer_labels_fit( available_width >= fixed_width + measured_label_width } +/// Return whether a keystroke is the reorder chord, Ctrl+Shift with an up or down arrow. +/// +/// Compared against the whole modifier set rather than testing the two that must be down, and that +/// is load bearing for one of them: `alt-up`/`alt-down` ship as the bindings for "shift the sell +/// order's price" (`moon_core::config::hotkeys`), and one chord that rearranges a list in one +/// window and moves a live order's price in another is a trap regardless of which window has focus +/// today. +/// +/// Args: +/// modifiers: Modifier state of the keystroke. +/// key: Key name from the keystroke. +/// +/// Returns: +/// `true` for exactly Ctrl+Shift plus an arrow. +fn reorder_chord(modifiers: &Modifiers, key: &str) -> bool { + *modifiers == Modifiers::control_shift() && matches!(key, "up" | "down") +} + /// Where a paste — or a Create — should land, given the tree's two kinds of selection. /// /// The folder outranks the strategy, and that is structural rather than a preference: making a @@ -331,10 +349,46 @@ impl StrategiesView { // ── UI-only folders, empty until populated ──────────────────────────────── - pub(super) fn add_ui_folder(&mut self, core: CoreId, parent: &str, name: &str) { + /// Create an empty folder: asked of the core, and marked locally either way. + /// + /// The local mark is not a fallback but a latency answer — a core that accepts the folder still + /// has to echo its tree back, and an operator who just typed a name should not watch nothing + /// happen for a round trip. `reconcile_ui_folders` drops the mark once the core reports the + /// folder as its own; on a core that keeps no folder tree the mark is all there ever is. + /// + /// Only the PATH goes to the feed. The wire form is the complete desired tree — the core + /// deletes every folder the list omits — and a tree assembled here would be assembled from a + /// snapshot that may already be stale, turning a create into a silent delete of whatever + /// arrived meanwhile. The feed owns the list; see `CoreCmd::AddFolder`. + /// + /// Args: + /// core: Core to create the folder on. + /// parent: Folder path to create it under; empty for the core root. + /// name: Leaf name the operator typed, already trimmed. + /// cx: View context used to send the command. + /// + /// Returns: + /// Nothing; a core that cannot hold the folder keeps the local mark and nothing else. + pub(super) fn create_folder( + &mut self, + core: CoreId, + parent: &str, + name: &str, + cx: &mut Context, + ) { let mut parts = ops::split_path(parent); parts.push(name.to_string()); let key = ops::join_path(&parts); + + if let Err(error) = self + .backend + .read(cx) + .session + .add_core_folder(core, key.clone()) + { + log::warn!("create folder failed: {error}"); + } + self.ui_folders.insert((core, key)); // Expand the core and parent chain, excluding the new folder itself, so it is immediately // visible. @@ -370,33 +424,53 @@ impl StrategiesView { } } - /// Returns empty UI-only folder paths for a core so they can be merged into the tree. + /// Returns empty UI-only folder paths for a core, in the order the tree appends them. + /// + /// SORTED here rather than by the caller: these come out of a `HashSet`, whose iteration order + /// is not stable, and the tree appends them in the order it receives them — so unsorted, two + /// frames rendering identical data would put the same folders in different places. Ordered + /// case-insensitively first and by the spelling itself second, because on the folded key alone + /// two siblings differing only in case compare equal and the tie goes back to the set. pub(super) fn ui_folder_paths(&self, core: CoreId) -> Vec> { - self.ui_folders + let mut paths: Vec> = self + .ui_folders .iter() .filter(|(c, _)| *c == core) .map(|(_, p)| ops::split_path(p)) - .collect() + .collect(); + paths.sort_by_cached_key(|parts| { + let joined = parts.join("/"); + (joined.to_lowercase(), joined) + }); + paths } - /// Drop UI-only ownership once live core data represents a folder through a strategy row. + /// Drop UI-only ownership once the core itself represents the folder. /// - /// Empty folders exist only in this view until first use. Retaining their local marker after - /// a strategy arrives would make the folder reappear as a ghost if another surface later - /// deletes that strategy and asks the core to remove the now-empty folder. + /// Two ways that happens: a strategy arrives in it, or — on a core that synchronizes folders — + /// the core reports the folder in its own tree. Retaining the local marker past either would + /// make the folder reappear as a ghost after another surface deleted it, which is precisely + /// what the mark cannot be allowed to do once the core owns the answer. /// /// Args: - /// store: Current per-core live strategy snapshots. + /// store: Current per-core live strategy and folder snapshots. pub(in crate::strategies) fn reconcile_ui_folders(&mut self, store: &CoreStore) { self.ui_folders.retain(|(core, path)| { - keep_ui_folder( - path, - store.core(*core).map(|data| data.strategies.as_slice()), - ) + let Some(data) = store.core(*core) else { + // The core is gone from the store entirely; nothing can contradict the mark. + return true; + }; + let confirmed = data.folders.supported + && data + .folders + .paths + .iter() + .any(|seen| seen.to_lowercase() == path.to_lowercase()); + !confirmed && keep_ui_folder(path, Some(data.strategies.as_slice())) }); } - // ── Keyboard: Ctrl+C, Ctrl+V, and Delete ────────────────────────────────── + // ── Keyboard: Ctrl+C, Ctrl+V, Ctrl+Shift+Up/Down, and Delete ────────────── /// Copy the last clicked visible folder/core root, otherwise the visible strategy selection. /// @@ -444,6 +518,18 @@ impl StrategiesView { self.default_target(b.session.store(), &cores) }; self.paste_into(core, target, cx); + } else if reorder_chord(m, key) { + // A HELD arrow is refused. OS auto-repeat fires this handler tens of times a second, + // and each pass queues a whole-list reorder that the feed turns into a full snapshot to + // the core; the repo's own keyboard path (`hotkeys::pre_dispatch`) refuses repeats for + // the same reason. One press, one move. + if !ev.is_held { + let step = match key == "up" { + true => ops::MoveStep::Up, + false => ops::MoveStep::Down, + }; + self.move_selection(step, cx); + } } else if key == "delete" { self.request_delete_selection(window, cx); } @@ -466,6 +552,7 @@ impl StrategiesView { store: &CoreStore, show_labels: bool, has_visible_cores: bool, + moves: (bool, bool), cx: &Context, ) -> AnyElement { let (has_sel, all_off) = self.selection_summary(store); @@ -478,6 +565,9 @@ impl StrategiesView { let delete_label = t!("strat.action_delete").to_string(); let icon_width = design::glyph_btn_w(cx); + let move_up = self.move_button(ops::MoveStep::Up, moves.0, icon_width, cx); + let move_down = self.move_button(ops::MoveStep::Down, moves.1, icon_width, cx); + let mut copy = MoonButton::new("sel-copy") .outline() .size(MoonButtonSize::Action) @@ -525,12 +615,60 @@ impl StrategiesView { .flex_none() .items_center() .gap(design::ui_px(cx, group_gap)) + .child(move_up.render()) + .child(move_down.render()) .child(copy.render()) .child(paste.render()) .child(delete.render()) .into_any_element() } + /// Build one of the footer's two move buttons. + /// + /// Icon-only in BOTH densities, unlike its neighbours: the group already carries three labelled + /// buttons at the labelled density, and two more would push it past the footer's width at the + /// pane sizes this window is normally used at. An arrow needs the word less than "Copy" does, + /// and the tooltip names both the action and the chord. + /// + /// Args: + /// step: Direction this button moves the selection. + /// enabled: Whether the cached plan says it would rearrange anything. + /// icon_width: Shared icon-density button width. + /// cx: View context used to build the click listener. + /// + /// Returns: + /// The configured button, ready to render. + fn move_button( + &self, + step: ops::MoveStep, + enabled: bool, + icon_width: f32, + cx: &Context, + ) -> MoonButton { + let (id, icon, label, chord) = match step { + ops::MoveStep::Up => ( + "sel-move-up", + "icons/arrow-up.svg", + t!("strat.action_move_up"), + t!("strat.move_up_chord"), + ), + ops::MoveStep::Down => ( + "sel-move-down", + "icons/arrow-down.svg", + t!("strat.action_move_down"), + t!("strat.move_down_chord"), + ), + }; + MoonButton::new(id) + .outline() + .size(MoonButtonSize::Action) + .width(icon_width) + .leading_icon(MoonButtonIconSlot::new(icon)) + .tooltip(format!("{label} · {chord}")) + .disabled(!enabled) + .on_click(cx.listener(move |this, _, _, cx| this.move_selection(step, cx))) + } + /// Builds the Create dropdown for a strategy or folder in the tree header. pub(super) fn create_dropdown( &self, diff --git a/locales/strategies.yml b/locales/strategies.yml index e18cdd92..6558547c 100644 --- a/locales/strategies.yml +++ b/locales/strategies.yml @@ -369,6 +369,14 @@ strat.menu_delete_strategy: ru: "Удалить…" en: "Delete…" es: "Eliminar…" +strat.menu_move_up: + ru: "Переместить выше" + en: "Move up" + es: "Mover arriba" +strat.menu_move_down: + ru: "Переместить ниже" + en: "Move down" + es: "Mover abajo" strat.action_copy: ru: "копировать" en: "copy" @@ -382,6 +390,33 @@ strat.action_delete: en: "delete" es: "eliminar" +strat.folder_empty_tip: + ru: "Пустая папка" + en: "Empty folder" + es: "Carpeta vacía" +strat.folder_empty_local_tip: + ru: "Пустая папка, известная только этому окну: ядро её не хранит и после перезапуска её не будет" + en: "Empty folder, known only to this window: the core does not keep it, and a restart loses it" + es: "Carpeta vacía, conocida solo por esta ventana: el núcleo no la guarda y se pierde al reiniciar" +strat.action_move_up: + ru: "выше в папке" + en: "move up in folder" + es: "subir en la carpeta" +strat.action_move_down: + ru: "ниже в папке" + en: "move down in folder" + es: "bajar en la carpeta" +# The chord itself, in one place: the footer tooltips and the context-menu hints both read it, and +# a keystroke spelled two ways is a keystroke the reader stops trusting. +strat.move_up_chord: + ru: "Ctrl+Shift+↑" + en: "Ctrl+Shift+↑" + es: "Ctrl+Shift+↑" +strat.move_down_chord: + ru: "Ctrl+Shift+↓" + en: "Ctrl+Shift+↓" + es: "Ctrl+Shift+↓" + # --- Versions pane (strategy version history and statistics) --- strat.versions: ru: "Версии"