From 5365123d928275ec183fb067408897406dcf9622 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 9 Aug 2026 16:01:48 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20the=20folder=20picker=20stuck=20on=20"Loa?= =?UTF-8?q?ding=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker set loading=true and nothing ever cleared it, because browser_loaded() was never called — my earlier edit to replace the stub silently failed to apply after cargo fmt reindented the code it matched against. The stale stub was still writing folder names into the status line, which is the list visible under the selector. Verified by grep before and after: browser_loaded had zero call sites, now two. Two related defects fixed while in there. Both timers polled their channel with `if let Ok(..)`, so a worker thread that died without sending left the screen on "Loading…" or "Connecting…" forever. Both now treat Disconnected as terminal. The first attempt at that fix introduced a bug of its own: a separate probe call consumed a pending message and discarded it, so a successful login would have vanished. The login poller now drains in one loop with disconnection as a match arm, and only reports failure when the flow had not already completed. Lesson worth recording: verify a replacement applied rather than trusting the edit succeeded. A silently-skipped replace looks identical to a successful one until the feature is used. 43 tests in dr-ui. --- ui/dr-ui/src/develop.rs | 89 +++++++++++++++++++++++++++++++++++++++ ui/dr-ui/src/launch_ui.rs | 78 +++++++++++++++++++++++++++------- 2 files changed, 151 insertions(+), 16 deletions(-) diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index d631925..19274fc 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -477,6 +477,95 @@ mod tests { assert_eq!(h, 200); } + #[test] + fn the_curve_collapses_to_a_single_row() { + // Ten point parameters must appear as one curve control, not ten + // sliders — otherwise the widget and the sliders both render and the + // panel shows the same values twice. + let graph = EditGraph::default_chain(); + let curve_cap = graph + .capabilities() + .into_iter() + .find(|c| c.id == curve::ID) + .expect("the chain includes a tone curve"); + + assert_eq!(curve_cap.params.len(), curve::POINTS * 2); + let presentation = curve_cap + .presentation + .as_ref() + .expect("the curve declares a widget"); + assert_eq!(presentation.widget, WidgetKind::Curve); + // Every parameter is owned by the widget, so none is left over to be + // rendered as a stray slider. + assert_eq!(presentation.params.len(), curve_cap.params.len()); + } + + #[test] + fn curve_point_parameters_are_contiguous() { + // The widget addresses points by offset from the first. Were they + // interleaved with anything else, dragging a point would write to + // the wrong parameter. + let graph = EditGraph::default_chain(); + let cap = graph + .capabilities() + .into_iter() + .find(|c| c.id == curve::ID) + .expect("tone curve present"); + let presentation = cap.presentation.as_ref().expect("declares a widget"); + + let base = cap + .params + .iter() + .position(|p| p.id == presentation.params[0]) + .expect("first point is a parameter"); + for (i, id) in presentation.params.iter().enumerate() { + assert_eq!( + cap.params[base + i].id, + *id, + "point parameter {i} is out of order" + ); + } + } + + #[test] + fn curve_samples_start_on_the_diagonal() { + // A fresh curve is the identity, so the drawn line must be the 45° + // diagonal — anything else means the widget opens showing a shape + // the image does not have. + let mut xs = [0.0f32; curve::POINTS]; + let mut ys = [0.0f32; curve::POINTS]; + for i in 0..curve::POINTS { + let t = i as f32 / (curve::POINTS - 1) as f32; + xs[i] = t; + ys[i] = t; + } + for i in 0..=20 { + let x = i as f32 / 20.0; + let y = curve::evaluate(&xs, &ys, x); + assert!((y - x).abs() < 1e-4, "at {x} the identity gave {y}"); + } + } + + #[test] + fn sorting_enforces_a_minimum_gap() { + // Two points dragged onto each other would divide by zero in the + // spline; the drawn curve must survive it exactly as the shader does. + let mut xs = [0.5, 0.5, 0.5, 0.5, 0.5]; + sort_with_gap(&mut xs); + for i in 1..xs.len() { + assert!(xs[i] > xs[i - 1], "not separated: {xs:?}"); + } + } + + #[test] + fn sorting_orders_reversed_points() { + let mut xs = [0.9, 0.7, 0.5, 0.3, 0.1]; + sort_with_gap(&mut xs); + for i in 1..xs.len() { + assert!(xs[i] > xs[i - 1], "not sorted: {xs:?}"); + } + } + #[test] fn routing_indices_map_back_to_the_right_parameter() { // A wrong index would silently move the wrong slider's value, which diff --git a/ui/dr-ui/src/launch_ui.rs b/ui/dr-ui/src/launch_ui.rs index 796c819..214ec1c 100644 --- a/ui/dr-ui/src/launch_ui.rs +++ b/ui/dr-ui/src/launch_ui.rs @@ -339,7 +339,29 @@ fn poll_channel( move || { let Some(w) = weak.upgrade() else { return }; let ctl = &ctl_for_cb; - while let Ok(msg) = rx.try_recv() { + // Drain in one loop. A separate probe call would consume a + // pending message and throw it away — a successful login would + // vanish. Disconnection is handled as a terminal case here, so a + // worker that dies without sending cannot leave the screen on + // "Connecting…" forever. + loop { + let msg = match rx.try_recv() { + Ok(m) => m, + Err(std::sync::mpsc::TryRecvError::Empty) => break, + Err(std::sync::mpsc::TryRecvError::Disconnected) => { + // Only an error if the flow never completed; a + // successful login stops the timer below. + if !ctl.model.borrow().is_signed_in() { + ctl.model.borrow_mut().fail("sign-in failed unexpectedly"); + render(&w, ctl); + } + if let Some(t) = ctl.poll_timer.borrow().as_ref() { + t.stop(); + } + return; + } + }; + let mut done = false; match msg { LoginMessage::AwaitingApproval(url) => { @@ -432,24 +454,48 @@ fn spawn_folder_list(weak: slint::Weak, ctl: Rc, pa move || { let Some(w) = weak.upgrade() else { return }; let ctl = &ctl_for_cb; + + // One try_recv covers both outcomes. A panicking worker drops + // the sender without sending; treating that as "still loading" + // is exactly what leaves the picker stuck forever. + let outcome = match rx.try_recv() { + Ok(result) => Some(result), + Err(std::sync::mpsc::TryRecvError::Empty) => None, + Err(std::sync::mpsc::TryRecvError::Disconnected) => { + Some(Err("folder listing failed unexpectedly".to_string())) + } + }; + + if let Some(result) = outcome { + match result { + // Hand the listing to the model, which clears `loading` + // and populates the picker. + Ok(dirs) => ctl.model.borrow_mut().browser_loaded(dirs), + Err(e) => { + // Close the picker before reporting: leaving it open + // on a permanent "Loading…" is what this replaced. + ctl.model.borrow_mut().close_browser(); + ctl.model.borrow_mut().fail(e); + } + } + render(&w, ctl); + if let Some(t) = ctl.poll_timer.borrow().as_ref() { + t.stop(); + } + return; + } + if let Ok(result) = rx.try_recv() { match result { - Ok(dirs) => { - // No folder picker yet: report what is there and - // leave selection to the CLI, rather than - // pretending to offer a chooser. - let msg = if dirs.is_empty() { - "No folders found.".to_string() - } else { - format!("Folders: {}", dirs.join(", ")) - }; - let session = ctl.model.borrow().session().cloned(); - if let Some(s) = session { - ctl.model.borrow_mut().signed_in(s); - } - ctl.model.borrow_mut().status = Some(msg); + // Hand the listing to the model, which clears `loading` + // and populates the picker. + Ok(dirs) => ctl.model.borrow_mut().browser_loaded(dirs), + Err(e) => { + // Close the picker before reporting: leaving it open + // on a permanent "Loading…" is what this replaced. + ctl.model.borrow_mut().close_browser(); + ctl.model.borrow_mut().fail(e); } - Err(e) => ctl.model.borrow_mut().fail(e), } render(&w, ctl); if let Some(t) = ctl.poll_timer.borrow().as_ref() {