From 82e11c4db9b4da912859356274679096d81fc172 Mon Sep 17 00:00:00 2001 From: kirillDevPro <113171057+kirillDevPro@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:51:36 +0200 Subject: [PATCH] fix(connections): quieter core rows and a Save that knows if there is anything to save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every Connections row carries a hex code beside its colour swatch, a bare "V0" and "8/8" with no explanation, an always-empty "Чарты" column, underlined headers that read as links, and a Save button that looks the same whether or not anything changed. Give V0 and 8/8 their row tooltips (the header ones already existed), fix the "Чарты" column at the call site (MoonUI drops the widget-side placeholder when an external state is passed, so the bundle-name input rendered empty), draw the headers like every other table, and make Save compare the draft against the saved state: Primary with a caption when dirty, Default with "изменений нет" when clean. The save path, key fields, delete and reconnect handlers are untouched. Hiding the hex needs a MoonColorPicker prop and is left as a MoonUI request. Claude-Session: https://claude.ai/code/session_01RaQ2tnAyr4pyiAiwGktPBv --- .../src/settings/connections/mod.rs | 39 +++- .../src/settings/connections/table.rs | 125 +++++++++--- .../src/settings/connections/tests.rs | 114 ++++++++++- crates/moon-ui-gpui/src/settings/mod.rs | 193 +++++++++++++++++- crates/moon-ui-gpui/src/settings/render.rs | 59 +++++- crates/moon-ui-gpui/src/settings/security.rs | 23 +++ .../tests/theme_contract/theme.rs | 162 +++++++++++++++ locales/settings.yml | 10 + 8 files changed, 684 insertions(+), 41 deletions(-) diff --git a/crates/moon-ui-gpui/src/settings/connections/mod.rs b/crates/moon-ui-gpui/src/settings/connections/mod.rs index 231ef62d..ea2550eb 100644 --- a/crates/moon-ui-gpui/src/settings/connections/mod.rs +++ b/crates/moon-ui-gpui/src/settings/connections/mod.rs @@ -23,6 +23,7 @@ use std::sync::atomic::{AtomicU64, Ordering}; use gpui::*; use moon_ui::{MoonColorPickerState, MoonInputEvent, MoonInputState}; +use rust_i18n::t; use super::SettingsView; use crate::Backend; @@ -60,7 +61,7 @@ pub(super) struct ConnRow { color: Entity, } -/// The ten per-row element-id strings the row factory used to rebuild with `format!` on every +/// The thirteen per-row element-id strings the row factory used to rebuild with `format!` on every /// frame. Built once in [`build_conn`] and read from thereafter, so `server_row` allocates none of /// them. /// @@ -73,7 +74,15 @@ pub(super) struct ConnRowIds { pub(super) group: SharedString, pub(super) bundle: SharedString, pub(super) feed: SharedString, + /// Id of the interactive wrapper that carries the data cell's tooltip. + /// + /// Separate from [`Self::feed`]: `MoonDropdown` has no tooltip prop, so the tooltip lives on + /// a `div` around it, and gpui needs its own id for an interactive element. + pub(super) feed_tip: SharedString, pub(super) proto: SharedString, + /// Id of the interactive wrapper that carries the proto cell's tooltip. See + /// [`Self::feed_tip`]. + pub(super) proto_tip: SharedString, pub(super) preset: SharedString, pub(super) act: SharedString, pub(super) win: SharedString, @@ -117,7 +126,9 @@ impl ConnRowIds { group: SharedString::from(format!("group-{ident}")), bundle: SharedString::from(format!("bundle-{ident}")), feed: SharedString::from(format!("feed-{ident}")), + feed_tip: SharedString::from(format!("feed-tip-{ident}")), proto: SharedString::from(format!("proto-{ident}")), + proto_tip: SharedString::from(format!("proto-tip-{ident}")), preset: SharedString::from(format!("preset-{ident}")), act: SharedString::from(format!("act-{ident}")), win: SharedString::from(format!("win-{ident}")), @@ -151,6 +162,8 @@ pub(super) fn sync_groups_from_servers( /// i: Draft index of the server field. /// row_key: Per-session identity of the owning row. /// init: Initial field value. +/// placeholder: Hint shown while the field is empty, or `None` for a field whose empty +/// state needs no explanation. /// get: Accessor for the draft field. /// set: Mutator for the draft field. /// sync_groups: Whether a change must synchronize draft group rows. @@ -163,11 +176,22 @@ fn conn_input( i: usize, row_key: u64, init: String, + placeholder: Option, get: fn(&ServerConfig) -> String, set: fn(&mut ServerConfig, String), sync_groups: bool, ) -> Entity { - let st = cx.new(|cx| MoonInputState::new(window, cx).default_value(init)); + // The placeholder belongs on the STATE, never on the `MoonInput` builder: MoonUI applies + // `MoonInput::placeholder` only inside its `self.state.unwrap_or_else(..)` branch, so a + // widget handed an external `.state(..)` -- which every field here is -- drops it silently. + // That is why the "Charts" column rendered as an empty box with no hint at all. + let st = cx.new(|cx| { + let st = MoonInputState::new(window, cx).default_value(init); + match placeholder { + Some(ph) => st.placeholder(ph), + None => st, + } + }); cx.subscribe(&st, move |this, emitter, ev: &MoonInputEvent, cx| { if matches!(ev, MoonInputEvent::Change) { let val = emitter.read(cx).value().to_string(); @@ -251,6 +275,9 @@ pub(super) fn build_conn( i, row_key, s.name.clone(), + // No placeholder: a nameless core is not a state worth explaining, and the + // user's own text is the only thing this field ever holds. + None, |s| s.name.clone(), |s, v| s.name = v, false, @@ -264,6 +291,7 @@ pub(super) fn build_conn( i, row_key, s.key.expose().to_string(), + Some(t!("conn.key_ph").to_string()), |s| s.key.expose().to_string(), |s, v| { // Typing or Ctrl+V into the field fills a row's transport mode the @@ -290,6 +318,9 @@ pub(super) fn build_conn( i, row_key, s.group.clone(), + // No placeholder: `build_conn` is only ever handed saved or pending rows + // whose group defaults to "default", so the field is never empty in practice. + None, |s| s.group.clone(), |s, v| s.group = v, true, @@ -300,6 +331,10 @@ pub(super) fn build_conn( i, row_key, s.chart_bundle.clone(), + // An empty bundle field is the DEFAULT, not an omission: the core then + // follows the global chart setting. The hint is what says so, and without it + // the column reads as a blank box nobody can interpret. + Some(t!("conn.bundle_ph").to_string()), |s| s.chart_bundle.clone(), |s, v| s.chart_bundle = v, false, diff --git a/crates/moon-ui-gpui/src/settings/connections/table.rs b/crates/moon-ui-gpui/src/settings/connections/table.rs index 0df29f97..add564ba 100644 --- a/crates/moon-ui-gpui/src/settings/connections/table.rs +++ b/crates/moon-ui-gpui/src/settings/connections/table.rs @@ -519,6 +519,35 @@ impl SettingsView { } } +/// Wrap one control in an interactive div carrying a wrapping tooltip. +/// +/// Used where the control itself has no tooltip prop -- `MoonDropdown` has none -- and where a +/// cryptic label (`V0`, `8/8`) would otherwise be decodable only by finding its column heading. +/// gpui needs an id on an interactive element, so the caller supplies one from [`ConnRowIds`]. +/// +/// Args: +/// id: Element id of the wrapper, distinct from the control's own. +/// tip: Already-localized tooltip text. +/// max_w: Wrap width; the transport explanation is long enough to need more than the default. +/// control: The control to wrap. +/// +/// Returns: +/// The control under a hover tooltip, occupying the same cell. +fn with_tip( + id: SharedString, + tip: SharedString, + max_w: f32, + control: impl IntoElement, +) -> impl IntoElement { + div() + .id(id) + .tooltip(move |_window, cx| { + cx.new(|_| MoonTooltipView::new(tip.clone()).max_width(max_w)) + .into() + }) + .child(control) +} + /// Build the `Data n/8` dropdown ported from egui's `feed_button`. /// /// The trigger reports enabled feed flags; its eight checkbox items update the draft. @@ -537,7 +566,7 @@ impl SettingsView { /// cx: Application context. /// /// Returns: -/// The feed-flag dropdown for one core row. +/// The feed-flag dropdown wrapped in its cell tooltip. fn feed_popover( view: &SettingsView, weak: &WeakEntity, @@ -594,7 +623,9 @@ fn feed_popover( // that MoonUI stores for the life of the element, and a strong handle there would close // SettingsView -> element -> closure -> SettingsView and keep the window alive forever. let view_weak = weak.clone(); - MoonDropdown::new(ids.feed.clone()) + // `8/8` is a count with no visible denominator meaning: the column tooltip already explains + // what the eight categories are and what the amber tint means, so reuse it on the cell. + let dropdown = MoonDropdown::new(ids.feed.clone()) .label(format!("{on}/8")) .trigger_caret(true) .trigger_variant(if tinted { @@ -617,7 +648,13 @@ fn feed_popover( this.feed_open = now_open.then_some(row_key); cx.notify(); }); - }) + }); + with_tip( + ids.feed_tip.clone(), + t!("conn.tip.flags").to_string().into(), + 320.0, + dropdown, + ) } /// Build the MoonProto transport selector for one server row. @@ -635,11 +672,12 @@ fn feed_popover( /// view: Settings state read for the row's current draft value. /// weak: Weak owner the select handler closes over. /// i: Draft index of the server being edited. +/// row_key: Owning row's identity, the value `proto_open` is compared against. /// ids: Precomputed element ids for the row. /// cx: Application context. /// /// Returns: -/// A compact dropdown bound to draft `servers[i].transport`. +/// The transport dropdown, wrapped in its row-level explanatory tooltip. fn proto_dropdown( view: &SettingsView, weak: &WeakEntity, @@ -704,7 +742,9 @@ fn proto_dropdown( }; let view_weak = weak.clone(); - MoonDropdown::new(ids.proto.clone()) + // `V0` alone says nothing: the existing column tooltip is the only thing that explains the + // MoonProto trio, and a reader looking at a row should not have to find the heading first. + let dropdown = MoonDropdown::new(ids.proto.clone()) .label(cur.map_or(SharedString::from("-"), |v| SharedString::from(v.label()))) .trigger_caret(true) .trigger_variant(MoonButtonVariant::Neutral) @@ -720,7 +760,13 @@ fn proto_dropdown( this.proto_open = now_open.then_some(row_key); cx.notify(); }); - }) + }); + with_tip( + ids.proto_tip.clone(), + t!("conn.tip.proto").to_string().into(), + 320.0, + dropdown, + ) } /// Build the workspace-preset selector for one server row. @@ -946,8 +992,6 @@ pub(super) fn server_row( MoonInput::new(ids.key.clone()) .state(&row.key) .small() - // Indicate that this field expects a core key. - .placeholder(t!("conn.key_ph").to_string()) .mask_toggle() // Allow the key to be cleared quickly before replacement. .cleanable(true), @@ -967,13 +1011,12 @@ pub(super) fn server_row( .state(&row.group) .small() .into_any_element(), - // An empty bundle field is the DEFAULT, not an omission, so the placeholder names what - // the field would hold rather than nudging: it is a bundle NAME (`ChartBucket::Bundle` - // in `moon-core/src/config/servers.rs`), which an empty white cell said nothing about. + // Both placeholders here live on the STATE instead, in `super::conn_input`: MoonUI honours + // `MoonInput::placeholder` only for a widget that creates its own state, so setting it + // beside `.state(..)` drops it silently -- which is why this column drew as a blank box. MoonInput::new(ids.bundle.clone()) .state(&row.bundle) .small() - .placeholder(t!("conn.bundle_ph").to_string()) .into_any_element(), feed_popover(view, weak, i, row_key, ids, cx).into_any_element(), MoonColorPicker::new(&row.color).into_any_element(), @@ -1055,10 +1098,20 @@ impl SettingsView { /// Build one column heading, with its tooltip, ported from egui's `head_tip`. /// - /// Underlining and brighter text signal hover help. A column with no `label` -- colour, - /// delete, reconnect, status -- yields the bare cell, which still has to be emitted: the - /// header's growing columns only receive the same free space the rows give them when the - /// trailing widths are reserved too. + /// The heading reads like every other table header in the app: muted, unadorned, its tooltip + /// found by hovering rather than advertised. It used to be underlined in `text_soft` and set + /// in full-strength `text`, which made a sort-and-tooltip heading look like a hyperlink -- + /// the one place in this codebase that did (`grep -n "underline()"`). The reference is + /// MoonUI's own `MoonDataTable` header (`moon/data_table/header.rs`), which paints + /// `style.header_text` -- `p.text_muted` -- and attaches its tooltip with no visual + /// affordance at all; Report and Orders are drawn by it. + /// + /// Losing the affordance costs nothing here, because the two cryptic cells that actually + /// needed decoding -- `V0` and `8/8` -- now carry their own tooltips on the row itself. + /// + /// A column with no `label` -- colour, delete, reconnect, status -- yields the bare cell, + /// which still has to be emitted: the header's growing columns only receive the same free + /// space the rows give them when the trailing widths are reserved too. /// /// Args: /// col: Which column to head. @@ -1080,6 +1133,9 @@ impl SettingsView { return base.into_any_element(); }; let tip: SharedString = t!(spec.tip.unwrap_or(label_key)).to_string().into(); + // The id and the tooltip go on `base` ITSELF, never on a wrapper: `base` carries the + // column's `flex_basis`, cap and grow policy, so anything wrapped around it would become + // the flex item instead and the heading would drift off its own column. base.id(spec.id) .child( div() @@ -1089,9 +1145,7 @@ impl SettingsView { // widen it, or the heading would push the grid it is describing. .truncate() .text_size(design::t_body(cx)) - .text_color(rgb(p.text)) - .underline() - .text_decoration_color(rgb(p.text_soft)) + .text_color(rgb(p.text_muted)) .child(t!(label_key).to_string()), ) .tooltip(move |_window, cx| { @@ -1101,26 +1155,35 @@ impl SettingsView { .into_any_element() } - /// Build an arbitrary-width help label with underlining and a wrapping tooltip. + /// Build an arbitrary-width help label with a wrapping tooltip. /// /// Used for section or group headings that need an explanation on hover rather than a column. + /// Bold `text` at full strength, matching `settings/common.rs:section`, and NOT underlined: + /// a heading is not a link. Same reasoning as [`Self::col_head`] above. + /// + /// Args: + /// id: Stable identity for the tooltip wrapper. + /// label: Visible heading text. + /// tip: Already-localized explanatory tooltip text. + /// p: Active palette supplying the heading colour. + /// + /// Returns: + /// The full-strength heading label wrapped in its explanatory tooltip. pub(super) fn hint_label( id: &'static str, label: impl Into, tip: SharedString, p: MoonPalette, ) -> impl IntoElement { - div() - .id(id) - .font_bold() - .text_color(rgb(p.text)) - .underline() - .text_decoration_color(rgb(p.text_soft)) - .child(label.into()) - .tooltip(move |_window, cx| { - cx.new(|_| MoonTooltipView::new(tip.clone()).max_width(360.0)) - .into() - }) + with_tip( + SharedString::from(id), + tip, + 360.0, + div() + .font_bold() + .text_color(rgb(p.text)) + .child(label.into()), + ) } /// Render the core table header over the columns it names. diff --git a/crates/moon-ui-gpui/src/settings/connections/tests.rs b/crates/moon-ui-gpui/src/settings/connections/tests.rs index 087b8be1..ab6309c3 100644 --- a/crates/moon-ui-gpui/src/settings/connections/tests.rs +++ b/crates/moon-ui-gpui/src/settings/connections/tests.rs @@ -6,9 +6,10 @@ use super::tab::{ ServerRowMeta, apply_group_transport, pending_server_indices, visible_group_rows, }; use crate::core_order::CoreOrder; +use crate::settings::draft_dirty; use moon_core::config::{ - AppConfig, FeedFlags, GroupConfig, GroupExitSettings, GroupTradeSettings, Secret, ServerConfig, - TakeProfitMode, TransportVersion, + AppConfig, CoreGroup, FeedFlags, GroupConfig, GroupExitSettings, GroupTradeSettings, Secret, + ServerConfig, TakeProfitMode, TransportVersion, }; use moon_core::venue::CoreVenue; @@ -298,3 +299,112 @@ fn only_current_server_group_names_become_visible_branches() { "pending, shared, and separate current names must keep their stored metadata" ); } + +/// `settings/mod.rs:draft_dirty` must include the masked core key in the draft signature. +/// +/// Breakage: skipping `Secret` because it is masked lets a newly pasted core key read clean, so +/// closing Settings silently loses an authentication change with no recovery path. +#[test] +fn draft_dirty_detects_a_core_key_change() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups = vec![GroupConfig::new("desk")]; + let mut draft = saved.clone(); + + draft.servers[0].key = Secret::new("k2"); + + assert!(draft_dirty(&saved, &draft)); +} + +/// `settings/mod.rs:draft_dirty` must normalize missing saved group rows before comparing. +/// +/// Breakage: removing `ensure_server_group_configs` from the saved baseline makes a seeded-open +/// Connections window read dirty forever, teaching users that its Save indicator means nothing. +#[test] +fn draft_dirty_treats_seeded_preview_groups_as_clean() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups.clear(); + let mut draft = saved.clone(); + sync_groups_from_servers(&draft.servers, &mut draft.groups); + + assert!(!draft_dirty(&saved, &draft)); +} + +/// `settings/mod.rs:draft_dirty` must include the per-core MoonProto transport mode. +/// +/// Breakage: reusing `settings_sig`, which does not hash transport, makes a protocol-mode change +/// read clean and disappear when the user closes Settings. +#[test] +fn draft_dirty_detects_a_transport_change() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups = vec![GroupConfig::new("desk")]; + let mut draft = saved.clone(); + + draft.servers[0].transport = Some(TransportVersion::V2); + + assert!(draft_dirty(&saved, &draft)); +} + +/// `settings/mod.rs:draft_dirty` must include each server's chart-bundle override. +/// +/// Breakage: using `AppConfig::structural_sig`, which deliberately blanks this field, makes a +/// chart-bundle edit read clean and lose the user's per-core chart selection on close. +#[test] +fn draft_dirty_detects_a_chart_bundle_change() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups = vec![GroupConfig::new("desk")]; + let mut draft = saved.clone(); + + draft.servers[0].chart_bundle = "x".into(); + + assert!(draft_dirty(&saved, &draft)); +} + +/// `settings/mod.rs:draft_dirty` must canonicalize an orphaned intermediate group on both sides. +/// +/// Breakage: normalizing only the saved side leaves `des` behind after `desk -> des -> desk`, so +/// a reverted group rename reads dirty forever and Save no longer tells users whether work remains. +#[test] +fn draft_dirty_is_clean_after_a_group_rename_is_reverted() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups = vec![GroupConfig::new("desk")]; + let mut draft = saved.clone(); + + assert!(!draft_dirty(&saved, &draft)); + draft.servers[0].group = "des".into(); + sync_groups_from_servers(&draft.servers, &mut draft.groups); + assert!(draft_dirty(&saved, &draft)); + draft.servers[0].group = "desk".into(); + sync_groups_from_servers(&draft.servers, &mut draft.groups); + assert!(!draft_dirty(&saved, &draft)); +} + +/// `settings/mod.rs:draft_dirty` must serialize Settings aggregates as well as servers. +/// +/// Breakage: dropping one aggregate from the streamed signature makes Hotkeys, per-group trading, +/// or a saved core set look clean, so an entire Settings tab can be discarded on close. +#[test] +fn draft_dirty_detects_hotkey_group_trade_and_core_group_changes() { + let mut saved = AppConfig::load(None, false).expect("test-binary config must load"); + saved.servers = vec![server("desk")]; + saved.groups = vec![GroupConfig::new("desk")]; + + let mut hotkey_draft = saved.clone(); + hotkey_draft.hotkeys.cancel_buy = "ctrl-shift-k".into(); + assert!(draft_dirty(&saved, &hotkey_draft)); + + let mut group_draft = saved.clone(); + group_draft.groups[0].trade.exit.stop_loss_pct = -3.0; + assert!(draft_dirty(&saved, &group_draft)); + + let mut core_group_draft = saved.clone(); + core_group_draft.core_groups.push(CoreGroup { + name: "desk".into(), + cores: vec![1], + }); + assert!(draft_dirty(&saved, &core_group_draft)); +} diff --git a/crates/moon-ui-gpui/src/settings/mod.rs b/crates/moon-ui-gpui/src/settings/mod.rs index eb6e4be9..86b6ff98 100644 --- a/crates/moon-ui-gpui/src/settings/mod.rs +++ b/crates/moon-ui-gpui/src/settings/mod.rs @@ -38,7 +38,7 @@ use rust_i18n::t; use crate::Backend; use crate::media::icons::IconSet; -use moon_core::config::{AppConfig, CoreSortMode, Language}; +use moon_core::config::{AppConfig, CoreSortMode, GroupConfig, Language}; use moon_core::db::valuation::ValuationMode; use moon_core::market::MarketDataMode; use moon_core::session::CoreId; @@ -229,6 +229,20 @@ pub struct SettingsView { conn_edit_pending: Option, /// Signature of data consumed by Settings: draft/configuration fields plus session statuses. last_sig: u64, + /// Whether the CONFIG draft differed from the saved config as of the last backend + /// notification. + /// + /// Kept beside [`Self::last_sig`] because it is the SECOND repaint trigger: `settings_sig` + /// deliberately ignores `transport` and `chart_bundle`, so a keystroke in the Charts field or + /// a pick in the Proto dropdown moves this flag while leaving that signature untouched, and + /// the footer would never learn about it. + /// + /// It carries the CONFIG term ONLY, and the observer below is its ONLY writer. The footer + /// combines it with the pending-password term locally and writes nothing back: a field + /// written under one definition and compared under another masks exactly the transitions it + /// exists to catch. Reading it in `render` rather than recomputing also keeps `draft_sig` + /// off the render path, where 56 servers would be serialized twice per frame. + draft_dirty: bool, /// Last valid Main auto-close timeout retained for this Settings session. /// /// Disabling the checkbox writes zero to the draft; re-enabling restores this value instead @@ -444,10 +458,19 @@ impl SettingsView { .detach(); let initial_sig = settings_sig(backend.read(cx)); + let initial_dirty = backend_dirty(backend.read(cx)); cx.observe(&backend, |this, backend, cx| { - let sig = settings_sig(backend.read(cx)); - if sig != this.last_sig { + let b = backend.read(cx); + let sig = settings_sig(b); + // The dirty flag is a SECOND repaint trigger, not a consequence of the first: + // `settings_sig` skips `transport` and `chart_bundle`, so an edit to either moves + // only this one and the footer would otherwise keep painting the stale caption. + // It flips at most once per clean-to-dirty transition, so this costs one repaint, + // not one per keystroke. + let dirty = backend_dirty(b); + if sig != this.last_sig || dirty != this.draft_dirty { this.last_sig = sig; + this.draft_dirty = dirty; cx.notify(); } }) @@ -497,6 +520,7 @@ impl SettingsView { conn_entries: Rc::new(Vec::new()), conn_edit_pending: None, last_sig: initial_sig, + draft_dirty: initial_dirty, idle_last_secs: std::cell::Cell::new(0), import: None, security: security_ed, @@ -595,6 +619,169 @@ fn settings_sig(b: &Backend) -> u64 { h.finish() } +/// Feed a serde stream straight into a hasher, so no serialized copy is ever allocated. +/// +/// `ServerConfig::key` is a `Secret`, which is `#[serde(transparent)]` over the PLAINTEXT core +/// key (`moon_core::config::secrets`). `serde_json::to_string`/`to_vec` would therefore build a +/// `String` holding every one of the user's Moonbot keys -- unzeroized, once per check. Streaming +/// into the hasher allocates nothing and leaves no plaintext copy behind. +struct HashSink<'a>(&'a mut DefaultHasher); + +impl std::io::Write for HashSink<'_> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + Hasher::write(self.0, buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +/// Fold one serializable config aggregate into the running signature. +/// +/// Serde is what makes this EXHAUSTIVE per struct: a field added to `ServerConfig` or +/// `GroupConfig` joins the signature without anyone remembering to list it here. +/// +/// INVARIANT for whoever adds the next field: every type hashed here must have an INFALLIBLE +/// `Serialize`. Today all of them do, so the error branch below is unreachable. It is not +/// merely a fallback if that ever changes: `HashSink` forwards each buffer to the hasher before +/// any error is known, so a mid-stream failure leaves the bytes written so far mixed in, and two +/// configs differing only AFTER the common failure point would hash equal. A fallible field +/// needs a fixed sentinel or a buffered path, not this branch. The one shape that introduces +/// one is a map with non-string or non-finite-float keys. +/// +/// KNOWN LIMIT, deliberately not chased: `serde_json` writes a non-finite `f32`/`f64` as `null` +/// rather than failing, so `NaN` and `+Infinity` in the same nested float hash equal. Only a +/// hand-edited TOML can produce one, the effect is confined to this caption, and Save is never +/// gated on the flag -- the scalar floats this function hashes directly go through `to_bits()` +/// and are unaffected. +/// +/// Args: +/// h: Running signature sink that receives the serialized bytes. +/// value: Config aggregate to serialize into the signature. +/// +/// Returns: +/// Nothing; appends the aggregate's JSON representation to `h`. +fn hash_json(h: &mut DefaultHasher, value: &T) { + if let Err(err) = serde_json::to_writer(HashSink(h), value) { + err.to_string().hash(h); + } +} + +/// Reduce a config's group list to exactly what a Save would persist. +/// +/// `AppConfig::save_impl` runs `ensure_server_group_configs` (add a row for every server group +/// that lacks one) and then `prune_orphan_groups` (drop every row no server references). The net +/// persisted set is therefore one row per DISTINCT server group name, and this restates that in +/// one rule -- restated rather than called because `prune_orphan_groups` is private to +/// `moon_core::config`. +/// +/// Doing it on BOTH sides of the comparison is what makes the dirty check answer "would saving +/// change anything?" instead of "do these two structs differ?". Two live failures depend on it: +/// the window seeds its draft through `sync_groups_from_servers`, so an un-normalized compare +/// reads dirty the moment Settings opens; and a group rename typed and then typed back leaves the +/// intermediate row behind in the draft (`connections/tests.rs` proves it survives until save), +/// so a reverted edit would report unsaved changes forever. +/// +/// Args: +/// cfg: Saved configuration or live draft to normalize. +/// +/// Returns: +/// The group rows a Save would persist for the configuration's current server groups. +fn canonical_groups(cfg: &AppConfig) -> Vec { + let mut names: Vec<&str> = cfg.servers.iter().map(|s| s.group.as_str()).collect(); + names.sort_unstable(); + names.dedup(); + names + .into_iter() + .map(|name| { + cfg.groups + .iter() + .find(|g| g.name == name) + .cloned() + .unwrap_or_else(|| GroupConfig::new(name)) + }) + .collect() +} + +/// Signature of everything a Settings Save would write, for the draft-vs-saved comparison. +/// +/// Distinct from [`settings_sig`] beside it, which is a REPAINT gate and is deliberately lossy -- +/// it hashes only `key.is_empty()` and skips `transport`, `chart_bundle` and +/// `workspace_membership` entirely, so reusing it here would call a changed core key or a changed +/// transport mode "no changes". Distinct from `AppConfig::structural_sig`, which neutralizes +/// exactly the presentation fields the Connections tab edits. +/// +/// Three `AppConfig` fields are deliberately EXCLUDED, named here so the `theme_contract` +/// exhaustiveness test can see them and so a reader knows the omission was a decision: +/// `next_uid` moves only inside `save`, which then writes the same candidate into both the draft +/// and the saved config; `settings_unreadable` and `chart_core_remap_needed` are runtime flags no +/// tab edits. Every other field is covered. +/// +/// Args: +/// cfg: Saved config or live draft; both sides go through this identically. +/// +/// Returns: +/// A signature equal for two configs that would persist the same bytes. +fn draft_sig(cfg: &AppConfig) -> u64 { + let mut h = DefaultHasher::new(); + + hash_json(&mut h, &cfg.servers); + hash_json(&mut h, &canonical_groups(cfg)); + hash_json(&mut h, &cfg.core_groups); + hash_json(&mut h, &cfg.hotkeys); + hash_json(&mut h, &cfg.theme); + hash_json(&mut h, &cfg.orders); + hash_json(&mut h, &cfg.badges); + + // `MarketDataMode` and `ValuationMode` are not `Serialize`, so they take the same + // stable-code path `settings_sig` uses; the rest are plain scalars. + cfg.language.code().hash(&mut h); + cfg.market_mode.code().hash(&mut h); + cfg.core_sort.hash(&mut h); + cfg.report_valuation_mode.hash(&mut h); + cfg.ui_theme_mode.hash(&mut h); + cfg.charts_split_by_core.hash(&mut h); + cfg.charts_stack_scroll.hash(&mut h); + cfg.charts_stack_compress.hash(&mut h); + cfg.chart_stack_height.hash(&mut h); + cfg.separate_control_zones.hash(&mut h); + cfg.main_idle_close_secs.hash(&mut h); + cfg.log_to_file.hash(&mut h); + cfg.log_retention_days.hash(&mut h); + cfg.chart_memory_percent.hash(&mut h); + cfg.ui_font_delta.to_bits().hash(&mut h); + cfg.ui_scale.to_bits().hash(&mut h); + + h.finish() +} + +/// Whether saving the draft would change anything on disk. +/// +/// Args: +/// saved: The config as it currently sits on disk. +/// draft: The Settings window's live draft. +/// +/// Returns: +/// `true` when a Save would persist something different. +pub(super) fn draft_dirty(saved: &AppConfig, draft: &AppConfig) -> bool { + draft_sig(saved) != draft_sig(draft) +} + +/// Whether the backend currently holds a draft that differs from the saved config. +/// +/// Args: +/// b: Settings backend whose saved configuration and optional draft are compared. +/// +/// Returns: +/// `true` when an open draft would change the saved configuration; `false` without a draft. +fn backend_dirty(b: &Backend) -> bool { + b.preview + .as_ref() + .is_some_and(|draft| draft_dirty(&b.config, draft)) +} + /// Open Settings on its default tab, in a separate OS window backed by a live-preview draft. /// /// Args: diff --git a/crates/moon-ui-gpui/src/settings/render.rs b/crates/moon-ui-gpui/src/settings/render.rs index 00831f84..e8d5716b 100644 --- a/crates/moon-ui-gpui/src/settings/render.rs +++ b/crates/moon-ui-gpui/src/settings/render.rs @@ -116,10 +116,33 @@ impl Render for SettingsView { let body = div().flex_1().min_h(px(0.0)).w_full().child(body_inner); // ── Footer: Save and status ───────────────────────────────────────── + // Whether Save has anything to do. TWO sources, because Save itself has two: the config + // draft, and the password draft that `security.rs` deliberately keeps outside `AppConfig` + // -- `save` applies the latter BEFORE the config write, so a password-only edit makes this + // very button write key slots to disk. `Some(Err(..))` counts as dirty too: the user typed + // a pair, Save will act on it and report the error, so "no changes" would be false there. + // + // The two are combined HERE and nowhere else, and `self.draft_dirty` is READ, never + // written. That field is the observer's own repaint memo and holds the config term ALONE + // (`settings/mod.rs`); folding the security term back into it made one field carry two + // different definitions, which broke both ways at once. A pending password left it stuck + // at `true`, so a later `transport` or `chart_bundle` edit -- the very fields + // `settings_sig` skips, and therefore the only ones this flag exists to catch -- found + // the observer already believing `true` and issued no repaint at all. In the other + // direction the observer wrote `false` back on the next backend notification and render + // wrote `true` again, repainting the whole window on every unrelated backend tick for as + // long as the password stayed pending. Reading the memo also drops two full `draft_sig` + // passes (56 servers serialized twice) from every render, including a plain tab switch. + let dirty = self.draft_dirty || self.security.has_pending_request(cx); + // Resolve status keys against the current locale here so a language change cannot leave // text from the previous locale behind. let status_el = match &self.status { - Some((msg, err)) => { + // A SUCCESS status is suppressed once the draft is dirty again: "Сохранено" beside a + // caption reading "есть несохранённые изменения" is a contradiction, and the stale + // half is the one that has to go. An ERROR status always stays -- it reports + // something that happened and was never superseded by a later edit. + Some((msg, err)) if *err || !dirty => { let text = match msg { super::StatusMsg::Key(k) => t!(*k).to_string(), super::StatusMsg::Text(s) => s.clone(), @@ -128,7 +151,7 @@ impl Render for SettingsView { .text_color(rgba_from(if *err { p.red } else { p.green }, 1.0)) .child(text) } - None => div(), + _ => div(), }; let footer = h_flex() .w_full() @@ -141,13 +164,43 @@ impl Render for SettingsView { .border_color(rgba_from(p.border, 1.0)) .child( MoonButton::new("save") - .primary() + // Primary while there is something to save, plain otherwise. NEVER disabled: + // an always-enabled Save costs a redundant write at worst, while a disabled + // one strands a user whose change the indicator failed to notice. + .variant(if dirty { + MoonButtonVariant::Blue + } else { + MoonButtonVariant::Neutral + }) .small() .width(110.0) .label(t!("settings.save").to_string()) .on_click(cx.listener(|this, _, window, cx| this.save(window, cx))) .render(), ) + .child( + // Shrinkable and truncating, because the status beside it is not: `StatusMsg::Text` + // carries arbitrary save and storage error text, and at a narrow Settings width + // this caption's intrinsic minimum would otherwise push that error out of the + // footer. The caption is the half that can afford to clip -- it says one of two + // known things, while the error says something only it knows. `min_w_0` sits on + // the truncating element itself, never on a wrapper around it. + // Shrinking is the flex default here -- `table.rs::cell` has to opt OUT of it with + // `flex_shrink_0` -- so `min_w_0` is the whole mechanism: it is what lets the item + // go below its content width, which is what `truncate` then acts on. + div() + .min_w_0() + .truncate() + .text_color(rgba_from(p.text_muted, 1.0)) + .child( + t!(if dirty { + "settings.dirty" + } else { + "settings.clean" + }) + .to_string(), + ), + ) .child(status_el) .child(div().flex_1()) // Put MoonBot import in the footer's right-hand area, which the General tab does not diff --git a/crates/moon-ui-gpui/src/settings/security.rs b/crates/moon-ui-gpui/src/settings/security.rs index d53fb339..8747bd02 100644 --- a/crates/moon-ui-gpui/src/settings/security.rs +++ b/crates/moon-ui-gpui/src/settings/security.rs @@ -135,6 +135,29 @@ impl SecurityEd { })) } + /// Whether this draft is asking Save to do anything at all. + /// + /// Exposed for the Save button's dirty indicator, which must count a pending password change: + /// the password draft lives OUTSIDE `AppConfig` by the design stated at the top of this file, + /// and `save` applies it BEFORE the config write, so a password-only edit makes the very same + /// button write key slots to disk. An `AppConfig`-only comparison would render it clean. + /// + /// A boolean rather than the request itself, deliberately: [`Self::pending`] returns the + /// private `vault::VaultChange`, and the footer has no business reaching it -- it needs to + /// know THAT there is work, never what the work is. + /// + /// An INVALID request (mismatched pair) counts as pending: the user typed something, Save + /// will act on it and report the error, so "no changes" would be false there too. + /// + /// Args: + /// cx: Application context used to read the password input states. + /// + /// Returns: + /// `true` when Save must process a password change or report an invalid request. + pub(super) fn has_pending_request(&self, cx: &App) -> bool { + self.pending(cx).is_some() + } + /// Empty every password field after the draft has been accepted. fn clear_fields(&self, window: &mut Window, cx: &mut App) { self.launch.clear(window, cx); diff --git a/crates/moon-ui-gpui/tests/theme_contract/theme.rs b/crates/moon-ui-gpui/tests/theme_contract/theme.rs index 6a4c977f..6d386922 100644 --- a/crates/moon-ui-gpui/tests/theme_contract/theme.rs +++ b/crates/moon-ui-gpui/tests/theme_contract/theme.rs @@ -578,3 +578,165 @@ fn chrome_toggles_resolve_their_tone_through_one_helper() { ); } } + +/// `settings/mod.rs:draft_sig` must stream serde into its hasher rather than +/// materializing plaintext `Secret` values. Breakage: replacing the writer with `to_string`, +/// `to_vec`, or debug formatting keeps every core key in an unzeroized allocation on repaint. +#[test] +fn connections_dirty_signature_never_materializes_secret_config_text() { + let dirty = read_src("settings/mod.rs"); + let hash_json = code_only(braced_body(&dirty, "fn hash_json<")); + + assert!( + hash_json.contains("serde_json::to_writer("), + "settings/mod.rs:hash_json must stream serialized settings into its hash sink" + ); + for allocating in ["serde_json::to_string(", "serde_json::to_vec("] { + assert!( + !hash_json.contains(allocating), + "settings/mod.rs:hash_json must not materialize secret-bearing config through `{allocating}`" + ); + } +} + +/// `settings/mod.rs:draft_sig` must account for every `AppConfig::blank` field or name it in its +/// explicit exclusion list. Breakage: adding a Settings-editable config field without updating the +/// signature makes its edits read clean forever, so close discards them without a compile error. +#[test] +fn settings_dirty_signature_covers_or_explicitly_excludes_every_config_field() { + let config = read_core_src("config/mod.rs"); + let blank = code_only(braced_body(&config, "pub(in crate::config) fn blank(")); + let settings = read_src("settings/mod.rs"); + let signature = code_only(braced_body(&settings, "fn draft_sig(")); + let excluded = ["next_uid", "settings_unreadable", "chart_core_remap_needed"]; + let fields = blank + .lines() + .filter_map(|line| line.trim().split_once(':').map(|(field, _)| field.trim())) + .filter(|field| { + !field.is_empty() + && field + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') + }) + .collect::>(); + + assert!( + !fields.is_empty(), + "moon-core AppConfig::blank must spell out config fields for this coverage contract" + ); + assert!( + settings.contains("deliberately EXCLUDED"), + "settings/mod.rs:draft_sig must keep an explicit EXCLUDED list for fields it intentionally omits" + ); + for field in excluded { + assert!( + settings.contains(field), + "settings/mod.rs:draft_sig must document its exclusion of AppConfig::{field}" + ); + } + for field in fields { + let covered = match field { + "groups" => signature.contains("canonical_groups("), + _ => excluded.contains(&field) || signature.contains(field), + }; + assert!( + covered, + "settings/mod.rs:draft_sig must hash or explicitly exclude AppConfig::{field}" + ); + } +} + +/// `connections/mod.rs:build_conn` must put key and bundle placeholders on their input state, +/// while `table.rs:server_row` keeps state-bound widgets free of widget placeholders. Breakage: +/// moving either placeholder back to `MoonInput` drops it silently, leaving an empty field blank. +#[test] +fn connections_placeholders_live_on_state_not_state_bound_widgets() { + let connections = read_src("settings/connections/mod.rs"); + let build_conn = code_only(braced_body(&connections, "fn build_conn(")); + let table = read_src("settings/connections/table.rs"); + let server_row = code_only(braced_body(&table, "fn server_row(")); + + assert!( + build_conn.contains("conn.key_ph") && build_conn.contains("conn.bundle_ph"), + "connections/mod.rs:build_conn must seed both state-owned placeholders" + ); + assert!( + !server_row.contains(".placeholder("), + "connections/table.rs:server_row must not put placeholders on state-bound MoonInput widgets" + ); +} + +/// `connections/table.rs` must retain tooltip keys beside the compact protocol and feed controls. +/// Breakage: removing either key leaves `V0` or `8/8` undecodable without searching for a header. +#[test] +fn connections_compact_cells_keep_their_explanatory_tooltips() { + let table = read_src("settings/connections/table.rs"); + let proto = code_only(braced_body(&table, "fn proto_dropdown(")); + let feed = code_only(braced_body(&table, "fn feed_popover(")); + + assert!( + proto.contains("conn.tip.proto"), + "connections/table.rs:proto_dropdown must retain the protocol tooltip" + ); + assert!( + feed.contains("conn.tip.flags"), + "connections/table.rs:feed_popover must retain the feed-flags tooltip" + ); +} + +/// `connections/table.rs:col_head` and `hint_label` must use muted, non-underlined headings. +/// Breakage: restoring underlines makes static headers look like hyperlinks and obscures hierarchy. +#[test] +fn connections_headers_are_muted_labels_not_links() { + let table = read_src("settings/connections/table.rs"); + let col_head = code_only(braced_body(&table, "fn col_head(")); + let hint_label = code_only(braced_body(&table, "fn hint_label(")); + + assert!( + !col_head.contains(".underline()") && col_head.contains("p.text_muted"), + "connections/table.rs:col_head must be muted and un-underlined" + ); + assert!( + !hint_label.contains(".underline()"), + "connections/table.rs:hint_label must not look like a link" + ); +} + +/// `settings/render.rs` must show dirty and clean captions and leave Save enabled in either state. +/// Breakage: disabling Save when clean rejects the explicit no-fence contract, while removing the +/// neutral variant or caption hides whether the current draft has changes. +#[test] +fn settings_save_button_exposes_dirty_state_without_a_disabled_fence() { + let render = read_src("settings/render.rs"); + let save = code_only(chain_between( + &render, + "MoonButton::new(\"save\")", + ".render()", + "settings save button", + )); + + assert!( + render.contains("settings.dirty") + && render.contains("settings.clean") + && render.contains("MoonButtonVariant::Neutral"), + "settings/render.rs must distinguish dirty and clean captions with a neutral clean variant" + ); + assert!( + !save.contains(".disabled("), + "settings/render.rs:MoonButton::new(\"save\") must remain enabled when clean" + ); +} + +/// `settings/render.rs:SettingsView::render` must include the password-only security draft in +/// Save's dirty predicate. Breakage: comparing only `AppConfig` paints a password change Neutral, +/// so the HOT-path indicator says no changes while Save writes new key slots to `servers.enc`. +#[test] +fn settings_save_dirty_predicate_includes_the_pending_security_draft() { + let render = read_src("settings/render.rs"); + let body = code_only(braced_body(&render, "fn render(")); + + assert!( + body.contains("self.security.has_pending_request(cx)"), + "settings/render.rs:SettingsView::render must include the pending security request in Save's dirty predicate" + ); +} diff --git a/locales/settings.yml b/locales/settings.yml index 40e06d27..f544d175 100644 --- a/locales/settings.yml +++ b/locales/settings.yml @@ -11,6 +11,16 @@ settings.saved: ru: "Сохранено" en: "Saved" es: "Guardado" +# Caption beside the Save button. It is an INDICATOR, never a fence: the button stays enabled in +# both states and closing the window still discards the draft without a confirmation. +settings.dirty: + ru: "есть несохранённые изменения" + en: "unsaved changes" + es: "hay cambios sin guardar" +settings.clean: + ru: "изменений нет" + en: "no changes" + es: "sin cambios" settings.window_title: ru: "MoonTerminal — Настройки" en: "MoonTerminal — Settings"