unom/punktfunk
6
0
Fork 1
Code Issues 2 Pull Requests Actions Packages Projects Releases 40 Wiki Activity

fix(client/windows): dialogs stop panicking the shell, and the chrome stops shouting

Browse Source 
Deleting a profile reliably killed the app with E_BOUNDS ("Daten außerhalb des gültigen
Bereichs"): a ContentDialog is a PHANTOM child in the reactor backend — tracked logically,
never attached to the panel — and the reconciler unmounts a child before removing it, so
by the time `remove_child` runs the dialog's handle is gone, the backend no longer
recognises it as phantom, and it RemoveAt()s a visual child that never existed. (The third
upstream windows-reactor bug this client documents.) Both confirmation dialogs — delete
profile, forget host — are now ALWAYS MOUNTED with `is_open` doing the arming, and both
live in STABLE overlay slots (settings: [nav, sheet slot, dialog]; hosts: [page, add-modal
slot, dialog], each closed slot a same-kind background-less Border) so no pass ever
removes or repositions a dialog. UIA-verified: create profile -> delete -> confirm, the
shell survives.

The rest is the review feedback:
* A group with no fields renders NOTHING — Decoding and Library showed a heading over an
  empty card in profile scope (device facts, never per profile).
* The override marker leaves the caption: a small "Overridden" chip and the Reset button
  sit beside the control, bottom-aligned; the description below stays a description,
  identical in both states.
* Host tiles stop wearing three badges: Paired is the resting state and earns no chip
  (a chip is for a decision — Trusted/PIN/Open), and the bound profile becomes a small
  dot in its accent colour plus the name in caption text.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
enricobuehler
2026-07-29 18:33:10 +02:00
co-authored by Claude Fable 5
parent 7bbee0eeb6
commit ba7667ee2b
2 changed files with 227 additions and 177 deletions
Download Patch File Download Diff File Expand all files Collapse all files
clients/windows/src/app/hosts.rs
+101 -62
View File
@@ -168,20 +168,26 @@ pub(crate) struct Hover {
} }
/// The status row at the bottom of a tile: the host's OS mark (when advertised), presence /// The status row at the bottom of a tile: the host's OS mark (when advertised), presence
/// dot + Online/Offline, plus the trust chip. /// dot + Online/Offline, plus a trust chip only where it says something (see
fn status_row(os: &str, online: Option<bool>, badge: &str, kind: Pill) -> Element { /// [`status_row_with`]).
status_row_with(os, online, badge, kind, None) fn status_row(os: &str, online: Option<bool>, badge: Option<(&str, Pill)>) -> Element {
status_row_with(os, online, badge, None)
} }
/// [`status_row`] plus the profile chip: what a plain click on THIS tile will use — its own /// [`status_row`] plus the profile: what a plain click on THIS tile will use — its own
/// profile on a pinned tile, the host's binding on the primary one. A binding whose profile /// profile on a pinned tile, the host's binding on the primary one. A binding whose profile
/// was deleted shows nothing and resolves as the defaults, which is what will happen on /// was deleted shows nothing and resolves as the defaults, which is what will happen on
/// connect (design §6). /// connect (design §6).
///
/// The row is METADATA, not a badge shelf — three chips side by side read as noise. Paired
/// is the normal resting state of a saved host, so it earns NO chip at all; a chip appears
/// only where it carries a decision ("Trusted" = TOFU without pairing, "PIN"/"Open" on a
/// discovered host). The profile is a small dot in the profile's own colour plus its name
/// in plain caption text — recognisable at a glance without competing with the host name.
fn status_row_with( fn status_row_with(
os: &str, os: &str,
online: Option<bool>, online: Option<bool>,
badge: &str, badge: Option<(&str, Pill)>,
kind: Pill,
profile: Option<(&str, Option<String>)>, profile: Option<(&str, Option<String>)>,
) -> Element { ) -> Element {
let mut items: Vec<Element> = Vec::new(); let mut items: Vec<Element> = Vec::new();
@@ -212,29 +218,45 @@ fn status_row_with(
.into(), .into(),
); );
} }
items.push( if let Some((badge, kind)) = badge {
pill(badge, kind) items.push(
.vertical_alignment(VerticalAlignment::Center) pill(badge, kind)
.into(), .vertical_alignment(VerticalAlignment::Center)
); .into(),
);
}
if let Some((name, accent)) = profile { if let Some((name, accent)) = profile {
// A profile's own colour where it has one, the neutral chip where it doesn't — the // The profile's own colour where it has one, a neutral disc where it doesn't — the
// palette stays opt-in, and an unparsable value falls back rather than being trusted. // palette stays opt-in, and an unparsable value falls back rather than being trusted.
let chip = match accent.as_deref().and_then(super::settings::hex_color) { let colour = accent
Some(colour) => border( .as_deref()
text_block(name) .and_then(super::settings::hex_color)
.font_size(11.0) .unwrap_or(Color {
.semibold() a: 120,
.foreground(colour), r: 128,
) g: 128,
.background(Color { a: 46, ..colour }) b: 128,
.border_brush(ThemeRef::CardStroke) });
.border_thickness(uniform(1.0)) items.push(
.corner_radius(10.0) border(vstack(Vec::<Element>::new()).width(8.0).height(8.0))
.padding(edges(9.0, 2.0, 9.0, 2.0)), .background(colour)
None => pill(name, Pill::Neutral), .corner_radius(4.0)
}; .margin(edges(
items.push(chip.vertical_alignment(VerticalAlignment::Center).into()); if items.is_empty() { 0.0 } else { 4.0 },
0.0,
0.0,
0.0,
))
.vertical_alignment(VerticalAlignment::Center)
.into(),
);
items.push(
text_block(name)
.font_size(11.0)
.foreground(ThemeRef::SecondaryText)
.vertical_alignment(VerticalAlignment::Center)
.into(),
);
} }
hstack(items) hstack(items)
.spacing(6.0) .spacing(6.0)
@@ -809,9 +831,9 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element {
status_row_with( status_row_with(
&k.os, &k.os,
Some(online), Some(online),
if k.paired { "Paired" } else { "Trusted" }, // Paired is the resting state — no chip; TOFU-only trust is worth one.
if k.paired { Pill::Good } else { Pill::Info }, (!k.paired).then_some(("Trusted", Pill::Info)),
// The chip carries the profile's own colour where it has one — // The dot carries the profile's own colour where it has one —
// that is what makes two bound hosts tell apart at a glance. // that is what makes two bound hosts tell apart at a glance.
k.profile_id k.profile_id
.as_ref() .as_ref()
@@ -853,8 +875,7 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element {
status_row_with( status_row_with(
&k.os, &k.os,
Some(online), Some(online),
if k.paired { "Paired" } else { "Trusted" }, (!k.paired).then_some(("Trusted", Pill::Info)),
if k.paired { Pill::Good } else { Pill::Info },
Some((name.as_str(), accent.clone())), Some((name.as_str(), accent.clone())),
), ),
None, None,
@@ -916,7 +937,7 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element {
&hover, &hover,
&h.name, &h.name,
&format!("{}:{}", h.addr, h.port), &format!("{}:{}", h.addr, h.port),
status_row(&h.os, None, badge, kind), status_row(&h.os, None, Some((badge, kind))),
None, None,
Some(Box::new(move || initiate(&ctx2, target.clone(), &ss, &st))), Some(Box::new(move || initiate(&ctx2, target.clone(), &ss, &st))),
)); ));
@@ -924,35 +945,45 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element {
body.push(tile_grid(tiles, cols, TILE_GAP)); body.push(tile_grid(tiles, cols, TILE_GAP));
} }
// Forget confirmation (modal; shown while `forget` holds a pending host). Confirmed first, // Forget confirmation, armed while `forget` holds a pending host. ALWAYS MOUNTED with
// since it's destructive and re-establishing trust needs a fresh pairing. // `is_open` doing the arming, and in a STABLE trailing layer rather than in `body`
if let Some((fp, name)) = forget { // (whose child list shifts with discovery): unmounting — or positionally re-pairing —
// a ContentDialog trips the reactor backend's phantom-child bookkeeping (the handle
// dies before `remove_child` runs, the dialog stops being recognised as phantom, and a
// visual child that never existed gets RemoveAt()'d — E_BOUNDS panic; see the
// delete-profile dialog in settings.rs). Confirmed first, since it's destructive and
// re-establishing trust needs a fresh pairing.
let forget_confirm: Element = {
let sf = set_forget.clone(); let sf = set_forget.clone();
body.push( let pending = forget.clone();
ContentDialog::new("Remove saved host?") let content = pending
.content(format!( .as_ref()
.map(|(_, name)| {
format!(
"Forget \u{201C}{name}\u{201D}? You'll need to pair (or trust) it again to \ "Forget \u{201C}{name}\u{201D}? You'll need to pair (or trust) it again to \
reconnect." reconnect."
)) )
.primary_button_text("Remove") })
.close_button_text("Cancel") .unwrap_or_default();
.is_open(true) ContentDialog::new("Remove saved host?")
.on_closed(move |r: ContentDialogResult| { .content(content)
if r == ContentDialogResult::Primary { .primary_button_text("Remove")
.close_button_text("Cancel")
.is_open(pending.is_some())
.on_closed(move |r: ContentDialogResult| {
if r == ContentDialogResult::Primary {
if let Some((fp, _)) = &pending {
let mut known = KnownHosts::load(); let mut known = KnownHosts::load();
known.remove_by_fp(&fp); known.remove_by_fp(fp);
let _ = known.save(); let _ = known.save();
} }
sf.call(None); // re-renders the page; the row is gone on the next load }
}) sf.call(None); // re-renders the page; the row is gone on the next load
.into(), })
); .into()
} };
let page = page_wide(body); let page = page_wide(body);
if !show_add {
return page;
}
// "Add host" modal: a scrim + centered card. It's an in-tree overlay, not a WinUI // "Add host" modal: a scrim + centered card. It's an in-tree overlay, not a WinUI
// ContentDialog, because ContentDialog is text-only in windows-reactor (no room for a text // ContentDialog, because ContentDialog is text-only in windows-reactor (no room for a text
@@ -1044,12 +1075,20 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element {
24.0, 24.0,
)); ));
// The scrim fades in with the same tween. // The scrim fades in with the same tween. Its layer slot is STABLE (a same-kind,
let scrim = border(modal).background(Color { // background-less Border when closed — invisible and not hit-testable) so the layer
a: (140.0 * props.add_anim) as u8, // list never changes shape around the always-mounted dialog after it.
r: 0, let add_slot: Element = if show_add {
g: 0, border(modal)
b: 0, .background(Color {
}); a: (140.0 * props.add_anim) as u8,
grid(vec![page, scrim.into()]).into() r: 0,
g: 0,
b: 0,
})
.into()
} else {
border(vstack(Vec::<Element>::new())).into()
};
grid(vec![page, add_slot, forget_confirm]).into()
} }
clients/windows/src/app/settings.rs
+126 -115
View File
@@ -518,58 +518,47 @@ fn described_overridable(
control: impl Into<Element>, control: impl Into<Element>,
caption: &str, caption: &str,
) -> Element { ) -> Element {
if scope.is_empty() { if scope.is_empty() || !overridden {
return described(control, caption); return described(control, caption);
} }
// Nothing that appears on an edit may move what was edited (the GTK client's rule, // The override marker lives on the CONTROL's line, not in the caption: a small tinted
// learned there the hard way): both states of a profile-scope row reserve the same // "Overridden" chip and the Reset button sit to the control's right, bottom-aligned to
// caption-line height, so the marker + Reset materialise IN PLACE instead of shoving // its box. The caption below stays plain and identical in both states — appearing to
// every row below them down by a button's height. // the SIDE means nothing moves and the description stays a description (the mixed
const CAPTION_MIN_H: f64 = 34.0; // marker-plus-caption line read as neither).
if !overridden {
return vstack((
control.into(),
hstack(vec![text_block(caption)
.font_size(12.0)
.foreground(ThemeRef::SecondaryText)
.wrap()
.max_width(420.0)
.horizontal_alignment(HorizontalAlignment::Left)
.vertical_alignment(VerticalAlignment::Center)
.into()])
.min_height(CAPTION_MIN_H),
))
.spacing(5.0)
.into();
}
let (rev, set_rev) = (rev.0, rev.1.clone()); let (rev, set_rev) = (rev.0, rev.1.clone());
let scope = scope.to_string(); let scope = scope.to_string();
vstack(( vstack((
control.into(),
hstack(( hstack((
text_block(format!("\u{25cf} Overridden here \u{00b7} {caption}")) control.into(),
.font_size(12.0) pill("Overridden", Pill::Info)
.foreground(ThemeRef::AccentText) .vertical_alignment(VerticalAlignment::Bottom)
.wrap() .margin(edges(0.0, 0.0, 0.0, 5.0)),
.max_width(360.0) button("Reset")
.horizontal_alignment(HorizontalAlignment::Left) .icon(Symbol::Undo)
.vertical_alignment(VerticalAlignment::Center), .tooltip("Back to inheriting from Default settings")
button("Reset").icon(Symbol::Undo).on_click(move || { .vertical_alignment(VerticalAlignment::Bottom)
let mut catalog = ProfilesFile::load(); .on_click(move || {
if let Some(p) = catalog.profiles.iter_mut().find(|p| p.id == scope) { let mut catalog = ProfilesFile::load();
p.overrides.clear(field); if let Some(p) = catalog.profiles.iter_mut().find(|p| p.id == scope) {
if let Err(e) = catalog.save() { p.overrides.clear(field);
tracing::warn!(error = %format!("{e:#}"), "clearing an override"); if let Err(e) = catalog.save() {
tracing::warn!(error = %format!("{e:#}"), "clearing an override");
}
} }
} // The catalog changed behind the controls, and nothing the page reads
// The catalog changed behind the controls, and nothing the page reads as // as state did — bump the revision so the row re-renders showing the
// state did — bump the revision so the row re-renders showing the inherited // inherited value again.
// value again. set_rev.call(rev + 1);
set_rev.call(rev + 1); }),
}),
)) ))
.spacing(8.0) .spacing(8.0),
.min_height(CAPTION_MIN_H), text_block(caption)
.font_size(12.0)
.foreground(ThemeRef::SecondaryText)
.wrap()
.max_width(420.0)
.horizontal_alignment(HorizontalAlignment::Left),
)) ))
.spacing(5.0) .spacing(5.0)
.into() .into()
@@ -607,7 +596,13 @@ fn group_heading(label: &str) -> Element {
/// One settings group: an optional sub-section label, a card of fields, and an optional /// One settings group: an optional sub-section label, a card of fields, and an optional
/// form-level note under it (Apple's Section header/footer). Groups stack down the page. /// form-level note under it (Apple's Section header/footer). Groups stack down the page.
/// A group with NO fields renders NOTHING — several groups pass an empty list in profile
/// scope (Decoding, Library: device facts, never per profile), and a heading over an empty
/// card read as a bug.
fn group(header: Option<&str>, fields: Vec<Element>, footer: Option<&str>) -> Vec<Element> { fn group(header: Option<&str>, fields: Vec<Element>, footer: Option<&str>) -> Vec<Element> {
if fields.is_empty() {
return Vec::new();
}
let mut out = Vec::with_capacity(3); let mut out = Vec::with_capacity(3);
if let Some(h) = header { if let Some(h) = header {
out.push(group_heading(h)); out.push(group_heading(h));
@@ -1484,68 +1479,83 @@ pub(crate) fn settings_page(
.opacity(progress) .opacity(progress)
.margin(edges(0.0, (1.0 - progress) * 22.0, 0.0, 0.0)); .margin(edges(0.0, (1.0 - progress) * 22.0, 0.0, 0.0));
let content: Element = scrolled.into(); let content: Element = scrolled.into();
// The delete confirmation, when armed. Declarative, like every other dialog in this shell: // The delete confirmation. Declarative like every dialog in this shell — but ALWAYS
// it is an element in the tree with `is_open`, not a call. // MOUNTED, with `is_open` doing the arming: a ContentDialog is a "phantom" child in the
let confirm: Option<Element> = delete_pending.as_ref().and_then(|id| { // reactor backend (tracked logically, never attached to the panel), and unmounting one
let p = ProfilesFile::load().find_by_id(id).cloned()?; // destroys its handle before `remove_child` runs, so the backend stops recognising it
// The warning counts what actually breaks: hosts that fall back to the defaults, and // as phantom and RemoveAt()s a visual child that does not exist — E_BOUNDS, main-thread
// pinned cards that disappear (design §6). // panic ("Daten außerhalb des gültigen Bereichs"), reliably on every delete. A mounted
let known = KnownHosts::load(); // dialog is never removed, so the bug has nothing to bite. (Upstream report material —
let bound = known // the third windows-reactor bug this client documents.)
.hosts let confirm: Element = {
.iter() let pending = delete_pending
.filter(|h| h.profile_id.as_deref() == Some(p.id.as_str())) .as_ref()
.count(); .and_then(|id| ProfilesFile::load().find_by_id(id).cloned());
let pinned = known // The warning counts what actually breaks: hosts that fall back to the defaults,
.hosts // and pinned cards that disappear (design §6).
.iter() let body = pending
.filter(|h| h.pinned_profiles.iter().any(|x| x == &p.id)) .as_ref()
.count(); .map(|p| {
let mut body = format!("\u{201c}{}\u{201d} will be removed.", p.name); let known = KnownHosts::load();
if bound > 0 { let bound = known
body.push_str(&format!( .hosts
" {bound} host{} will fall back to Default settings.", .iter()
if bound == 1 { "" } else { "s" } .filter(|h| h.profile_id.as_deref() == Some(p.id.as_str()))
)); .count();
} let pinned = known
if pinned > 0 { .hosts
body.push_str(&format!( .iter()
" {pinned} pinned card{} will disappear.", .filter(|h| h.pinned_profiles.iter().any(|x| x == &p.id))
if pinned == 1 { "" } else { "s" } .count();
)); let mut body = format!("\u{201c}{}\u{201d} will be removed.", p.name);
} if bound > 0 {
body.push_str(&format!(
" {bound} host{} will fall back to Default settings.",
if bound == 1 { "" } else { "s" }
));
}
if pinned > 0 {
body.push_str(&format!(
" {pinned} pinned card{} will disappear.",
if pinned == 1 { "" } else { "s" }
));
}
body
})
.unwrap_or_default();
let (id, set_scope, set_delete, set_edit) = ( let (id, set_scope, set_delete, set_edit) = (
p.id.clone(), pending.as_ref().map(|p| p.id.clone()),
set_scope.clone(), set_scope.clone(),
set_delete.clone(), set_delete.clone(),
set_edit.clone(), set_edit.clone(),
); );
Some( ContentDialog::new("Delete profile?")
ContentDialog::new("Delete profile?") .content(body)
.content(body) .primary_button_text("Delete")
.primary_button_text("Delete") .close_button_text("Cancel")
.close_button_text("Cancel") .is_open(pending.is_some())
.is_open(true) .on_closed(move |r: ContentDialogResult| {
.on_closed(move |r: ContentDialogResult| { set_delete.call(None);
set_delete.call(None); if r != ContentDialogResult::Primary {
if r != ContentDialogResult::Primary { return;
return; }
} let Some(id) = id.clone() else {
let mut catalog = ProfilesFile::load(); return;
catalog.profiles.retain(|p| p.id != id); };
// Bindings and pins are left dangling on purpose: they resolve as "no let mut catalog = ProfilesFile::load();
// profile" everywhere, and rewriting every host record here would be a catalog.profiles.retain(|p| p.id != id);
// second, racier source of truth. // Bindings and pins are left dangling on purpose: they resolve as "no
if catalog.save().is_ok() { // profile" everywhere, and rewriting every host record here would be a
set_scope.call(String::new()); // second, racier source of truth.
// The profile the Edit modal was showing is gone — without this, the if catalog.save().is_ok() {
// still-armed flag would pop the modal open on the NEXT profile pick. set_scope.call(String::new());
set_edit.call(false); // The profile the sheet was showing is gone — without this, the
} // still-armed flag would pop the sheet open on the NEXT profile pick.
}) set_edit.call(false);
.into(), }
) })
}); .into()
};
let nav = NavigationView::new(items, content) let nav = NavigationView::new(items, content)
.pane_title("Settings") .pane_title("Settings")
.selected_tag(section) .selected_tag(section)
@@ -1560,14 +1570,16 @@ pub(crate) fn settings_page(
move || ss.call(Screen::Hosts) move || ss.call(Screen::Hosts)
}); });
// Overlay layers fill the NAV's cell (grids stretch children; a vstack would hand the // Overlay layers fill the NAV's cell (grids stretch children; a vstack would hand the
// NavigationView its desired height — clipped short, floating tall): the nav, the // NavigationView its desired height — clipped short, floating tall). The layer list is
// profile-sheet scrim + card over it, and the delete confirmation (a ContentDialog, its // STABLE — always [nav, sheet slot, dialog] — so no pass ever removes a grid child:
// own WinUI layer, so its slot is irrelevant — it just needs to be in the tree). // removals are where the reconciler's phantom-dialog bookkeeping breaks (see `confirm`
let mut layers: Vec<Element> = vec![nav.into()]; // above), and a closed sheet leaves a same-kind, background-less Border in its slot
// The profile sheet — "Edit profile…" in the bar. The bar owns the scope choice, so // (invisible, and per style.rs a null background is not hit-testable, so it swallows
// the sheet carries only the profile being edited. // no clicks).
if edit_open && profile_mode { let sheet_slot: Element = if edit_open && profile_mode {
layers.push(edit_profile_modal( // The profile sheet — "Edit profile…" in the bar. The bar owns the scope choice,
// so the sheet carries only the profile being edited.
edit_profile_modal(
active.as_ref(), active.as_ref(),
None, None,
set_scope, set_scope,
@@ -1575,16 +1587,15 @@ pub(crate) fn settings_page(
set_edit, set_edit,
rev, rev,
set_rev, set_rev,
)); )
} } else {
if let Some(dialog) = confirm { border(vstack(Vec::<Element>::new())).into()
layers.push(dialog); };
}
// The bar rides an Auto row above the nav's Star row, so the nav (and the sheet's scrim // The bar rides an Auto row above the nav's Star row, so the nav (and the sheet's scrim
// over it) still fills the rest of the window. // over it) still fills the rest of the window.
grid(vec![ grid(vec![
scope_bar.grid_row(0), scope_bar.grid_row(0),
Element::from(grid(layers)).grid_row(1), Element::from(grid(vec![nav.into(), sheet_slot, confirm])).grid_row(1),
]) ])
.rows([GridLength::Auto, GridLength::STAR]) .rows([GridLength::Auto, GridLength::STAR])
.into() .into()