Merge pr/1467

f32::clamp panics when the range is reversed or NaN, which a value
field's DragValue range tolerates; the step keeps the value unclamped
then.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01UHgFEHEsefoQoBParH47JA

apps/photocraft/src/control_server.rs

@@ -94,7 +94,7 @@ fn serve(stream: TcpStream, token: &str, tx: Sender<ControlRequest>, ctx: egui::
                 }
                 r
             }
-            Err(e) => json!({"ok": false, "error": format!("bad JSON: {e}")}),
+            Err(e) => json!({"id": null, "ok": false, "error": format!("bad JSON: {e}")}),
         };
         if write_reply(&mut out, &reply).is_err() {
             break;
@@ -154,4 +154,46 @@ mod tests {
         handler.join().unwrap();
         server.join().unwrap();
     }
+
+    #[test]
+    fn bad_json_reply_carries_a_null_id_and_the_connection_keeps_serving() {
+        const TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef";
+        let listener = TcpListener::bind("127.0.0.1:0").unwrap();
+        let addr = listener.local_addr().unwrap();
+        let (tx, rx) = channel::<ControlRequest>();
+        let server = std::thread::spawn(move || {
+            let (stream, _) = listener.accept().unwrap();
+            serve(stream, TOKEN, tx, egui::Context::default());
+        });
+        let handler = std::thread::spawn(move || {
+            let req = rx.recv_timeout(Duration::from_secs(5)).unwrap();
+            assert_eq!(req.method, "test.after");
+            req.reply.send(json!({"ok": true, "result": "still serving"})).unwrap();
+        });
+        let mut stream = TcpStream::connect(addr).unwrap();
+        stream.set_read_timeout(Some(Duration::from_secs(5))).unwrap();
+        let mut reader = BufReader::new(stream.try_clone().unwrap());
+        writeln!(stream, "{}", json!({"id": "auth", "method": "auth", "params": {"token": TOKEN}})).unwrap();
+        let mut line = String::new();
+        reader.read_line(&mut line).unwrap();
+        assert_eq!(serde_json::from_str::<Value>(&line).unwrap()["ok"], true);
+        writeln!(stream, "{{not json}}").unwrap();
+        line.clear();
+        reader.read_line(&mut line).unwrap();
+        let bad: Value = serde_json::from_str(&line).unwrap();
+        let bad = bad.as_object().unwrap();
+        assert_eq!(bad.get("id"), Some(&Value::Null), "bad JSON reply must carry `id: null`: {line}");
+        assert_eq!(bad.get("ok"), Some(&Value::Bool(false)));
+        assert!(bad.get("error").and_then(Value::as_str).unwrap().starts_with("bad JSON: "));
+        writeln!(stream, "{}", json!({"id": 7, "method": "test.after"})).unwrap();
+        line.clear();
+        reader.read_line(&mut line).unwrap();
+        let after: Value = serde_json::from_str(&line).unwrap();
+        assert_eq!(after["id"], 7);
+        assert_eq!(after["result"], "still serving");
+        drop(reader);
+        drop(stream);
+        handler.join().unwrap();
+        server.join().unwrap();
+    }
 }

crates/engine/src/type_cmds.rs

@@ -523,6 +523,7 @@ pub fn specs() -> Vec<CommandSpec> {
                     Some(Value::String(value)) if value == "vertical" => Orientation::Vertical,
                     _ => return Err(bad("type.create", "orientation must be horizontal or vertical")),
                 };
+                check_kerning(p).map_err(|m| bad("type.create", m))?;
                 check_size_tracking(p).map_err(|m| bad("type.create", m))?;
                 let text = norm_text(p.get("text").and_then(Value::as_str).unwrap_or(""));
                 // Type › Save Default Type Styles sets the starting styles; the colour is always
@@ -1114,6 +1115,17 @@ mod tests {
         }
         assert!(s.execute("type.setStyle", json!({"layer": id, "kerning": "tight"})).is_err());
         assert_eq!(text_layer(&s, id).runs, before.runs);
+        // type.create rejects the same values and creates no layer (#994).
+        let layers = s.active().unwrap().doc.layer_count();
+        for k in [json!("tight"), json!(1e9), json!(-5000), json!(true), json!([1])] {
+            let p = json!({"x": 0, "y": 10, "text": "AV", "kerning": k});
+            assert!(s.execute("type.create", p.clone()).is_err(), "{p}");
+        }
+        assert_eq!(s.active().unwrap().doc.layer_count(), layers);
+        let id = s.execute("type.create", json!({"x": 0, "y": 10, "text": "AV", "kerning": "optical"})).unwrap()["layer"].as_u64().unwrap();
+        assert!(kerning_of(&s, id).iter().all(|k| *k == (Kerning::Optical, 0.0)));
+        let id = s.execute("type.create", json!({"x": 0, "y": 10, "text": "AV", "kerning": -50})).unwrap()["layer"].as_u64().unwrap();
+        assert!(kerning_of(&s, id).iter().all(|k| *k == (Kerning::Off, -50.0)));
     }
 
     /// Cost of one Alt+←/→ press (`kernPair`: two measuring layouts + the edit's re-render) on

crates/ui-egui/src/camera_raw_ui.rs

@@ -835,9 +835,48 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) {
         app.ui.status = e;
         app.ui.status_error = true;
     }
-    if app.camera_raw.is_none() {
+    let Some(d) = app.camera_raw.as_ref() else { return };
+    // Where the platform has no extra windows (the web build), draw over the main window.
+    if ctx.embed_viewports() {
+        draw(app, ctx, false);
         return;
     }
+    let window_title = d.window_title();
+    // Camera Raw is modal, as in Photoshop: the main window is dimmed and takes no input while it
+    // is open (in a separate window, or drawn over it where there are no extra windows).
+    let main = ctx.content_rect();
+    egui::Area::new(egui::Id::new("camera-raw-modal")).order(egui::Order::Foreground).fixed_pos(main.min).show(ctx, |ui| {
+        let (r, _) = ui.allocate_exact_size(main.size(), Sense::click_and_drag());
+        ui.painter().rect_filled(r, 0.0, Color32::from_black_alpha(110));
+    });
+    // A window of its own (title bar, moved and resized like any window), like Adobe Camera Raw.
+    let builder = egui::ViewportBuilder::default().with_title(window_title).with_inner_size([1360.0, 900.0]).with_min_inner_size([900.0, 600.0]);
+    let mut close = false;
+    ctx.show_viewport_immediate(egui::ViewportId::from_hash_of("camera-raw-window"), builder, |ui, class| {
+        if ui.ctx().input(|i| i.viewport().close_requested()) {
+            close = true;
+        }
+        draw(app, ui.ctx(), class != egui::ViewportClass::EmbeddedWindow);
+    });
+    if close && app.camera_raw.is_some() {
+        cancel(app);
+    }
+}
+
+impl CameraRawDialog {
+    /// "Camera Raw (name)" for the open-time dialog, "Camera Raw Filter (layer)" otherwise.
+    fn window_title(&self) -> String {
+        if self.target == Target::OpenRaw {
+            crate::i18n::fmt(tl!("Camera Raw ({name})"), &[("name", &self.layer_name)])
+        } else {
+            crate::i18n::fmt(tl!("Camera Raw Filter ({layer})"), &[("layer", &self.layer_name)])
+        }
+    }
+}
+
+/// The dialog's contents over the whole of `ctx`'s window. `own_window`: the OS window already
+/// shows the title, so none is painted.
+fn draw(app: &mut PhotocraftApp, ctx: &egui::Context, own_window: bool) {
     let t = Tokens::get(ctx);
     let screen = ctx.content_rect();
     let mut action: Option<&str> = None;
@@ -851,16 +890,13 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) {
         let (full, _) = ui.allocate_exact_size(screen.size(), Sense::click());
         let painter = ui.painter().clone();
         painter.rect_filled(full, 0.0, t.chrome);
-        let title = ERect::from_min_size(full.min, vec2(full.width(), 30.0));
-        painter.rect_filled(title, 0.0, t.dock);
-        painter.line_segment([title.left_bottom(), title.right_bottom()], Stroke::new(1.0, t.separator));
-        let window_title = if d.target == Target::OpenRaw {
-            crate::i18n::fmt(tl!("Camera Raw ({name})"), &[("name", &d.layer_name)])
-        } else {
-            crate::i18n::fmt(tl!("Camera Raw Filter ({layer})"), &[("layer", &d.layer_name)])
-        };
+        let title = ERect::from_min_size(full.min, vec2(full.width(), if own_window { 0.0 } else { 30.0 }));
+        if !own_window {
+            painter.rect_filled(title, 0.0, t.dock);
+            painter.line_segment([title.left_bottom(), title.right_bottom()], Stroke::new(1.0, t.separator));
+            painter.text(title.center(), Align2::CENTER_CENTER, d.window_title(), FontId::proportional(13.0), t.text);
+        }
         let ok_label = if d.target == Target::OpenRaw { tl!("Open") } else { tl!("OK") };
-        painter.text(title.center(), Align2::CENTER_CENTER, window_title, FontId::proportional(13.0), t.text);
         let footer_h = 48.0;
         let body = ERect::from_min_max(pos2(full.left(), title.bottom()), pos2(full.right(), full.bottom() - footer_h));
         let scope = &mut app.ui.camera_raw_scope;

crates/ui-egui/src/theme.rs

@@ -359,12 +359,29 @@ pub fn install_fonts_with(ctx: &egui::Context, cjk: crate::cjk_fonts::Sources) {
         size_ui_font(ctx, name, Arc::make_mut(data));
     }
     ctx.set_fonts(fonts);
-    ctx.add_plugin(UiFontSizePlugin { applied: size });
+    // egui keeps the first plugin of a type, so a re-install (language change) resets the
+    // existing one. The new definitions land next pass: the plugin must not rescale before then.
+    if ctx
+        .with_plugin::<UiFontSizePlugin, _>(|plugin| {
+            plugin.applied = size;
+            plugin.defer_after_install = true;
+        })
+        .is_none()
+    {
+        ctx.add_plugin(UiFontSizePlugin { applied: size, defer_after_install: true });
+    }
     // Japanese / Chinese / Korean fallback fonts (craft-fonts' Japanese ones if built in, then
     // the system's) are registered on demand (cjk_fonts.rs).
     crate::cjk_fonts::install_with(ctx, cjk);
 }
 
+/// Has [`install_fonts`]'s stack reached the active fonts? Before that (the first pass, when
+/// the desktop app installs fonts and loads preferences in the same frame) the active fonts
+/// are egui's defaults, and rescaling a copy of those would overwrite the queued stack.
+fn ui_fonts_active(ctx: &egui::Context) -> bool {
+    ctx.fonts(|f| f.definitions().families.contains_key(&FontFamily::Name("medium".into())))
+}
+
 fn ui_fonts_id() -> egui::Id {
     egui::Id::new("photocraft-ui-fonts")
 }
@@ -403,6 +420,9 @@ pub(crate) fn set_ui_font_size(ctx: &egui::Context, size: UiFontSize) {
 /// Keep only the original multipliers, not a second copy of large system font files.
 struct UiFontSizePlugin {
     applied: UiFontSize,
+    /// `set_fonts` takes effect next pass: skip one hook so the rescale starts from that stack
+    /// and not from the one it replaces.
+    defer_after_install: bool,
 }
 
 impl egui::Plugin for UiFontSizePlugin {
@@ -412,9 +432,16 @@ impl egui::Plugin for UiFontSizePlugin {
 
     fn output_hook(&mut self, ctx: &egui::Context, _output: &mut egui::FullOutput) {
         let size = ui_font_size(ctx);
+        let deferred = std::mem::take(&mut self.defer_after_install);
         if size == self.applied {
             return;
         }
+        if deferred || !ui_fonts_active(ctx) {
+            // Cloning the active definitions now would discard the queued stack (the desktop
+            // app installs fonts and loads the saved size in its first frame).
+            ctx.request_repaint();
+            return;
+        }
         // Start from the current stack so lazily registered fallback faces are retained.
         let mut fonts = ctx.fonts(|f| f.definitions().clone());
         for (name, data) in &mut fonts.font_data {
@@ -563,6 +590,53 @@ mod tests {
         assert_eq!(ctx.zoom_factor(), 1.0);
     }
 
+    /// The desktop app installs fonts and loads the saved UI Font Size in its first frame, while
+    /// egui's default fonts are still active. Any size but Small then panicked on the next pass
+    /// ("FontFamily::Name("medium") is not bound to any fonts") because the plugin rescaled a
+    /// copy of the defaults and that replaced the queued Inter stack.
+    #[test]
+    fn saved_ui_font_size_survives_first_frame_install() {
+        let ctx = egui::Context::default();
+        let medium_bound = |ctx: &egui::Context| ctx.fonts(|f| f.families().contains(&FontFamily::Name("medium".into())));
+        ctx.run_ui(Default::default(), |ui| {
+            assert!(!medium_bound(ui.ctx()), "egui's defaults are active in the first frame");
+            install_fonts(ui.ctx());
+            apply(ui.ctx(), ThemeKind::Pro);
+            set_ui_font_size(ui.ctx(), UiFontSize::Medium);
+        })
+        .textures_delta
+        .clear();
+        let mut width = 0.0;
+        ctx.run_ui(Default::default(), |ui| {
+            assert!(medium_bound(ui.ctx()), "the installed stack reaches the second pass");
+            width = ui.painter().layout_no_wrap("PhotoCraft".into(), medium(13.0), Color32::WHITE).size().x;
+        })
+        .textures_delta
+        .clear();
+        assert!(width > 0.0);
+        // The saved size applies one pass later, from the installed stack.
+        ctx.run_ui(Default::default(), |_| {}).textures_delta.clear();
+        let mut scaled = 0.0;
+        ctx.run_ui(Default::default(), |ui| {
+            assert!(medium_bound(ui.ctx()));
+            scaled = ui.painter().layout_no_wrap("PhotoCraft".into(), medium(13.0), Color32::WHITE).size().x;
+        })
+        .textures_delta
+        .clear();
+        assert!((scaled - width * font_scale(UiFontSize::Medium)).abs() < 1.0, "{scaled} vs {width} × 14/12");
+        // Re-installing (a language change) keeps the size and the named families.
+        ctx.run_ui(Default::default(), |ui| install_fonts(ui.ctx())).textures_delta.clear();
+        for _ in 0..2 {
+            ctx.run_ui(Default::default(), |ui| {
+                assert!(medium_bound(ui.ctx()));
+                let w = ui.painter().layout_no_wrap("PhotoCraft".into(), medium(13.0), Color32::WHITE).size().x;
+                assert!((w - scaled).abs() < 1.0, "{w} vs {scaled}");
+            })
+            .textures_delta
+            .clear();
+        }
+    }
+
     #[test]
     fn theme_names_parse() {
         assert_eq!(ThemeKind::from_name("Classic"), Some(ThemeKind::Classic));

crates/ui-egui/src/widgets.rs

@@ -181,7 +181,9 @@ pub fn value_field(ui: &mut Ui, value: &mut f32, range: std::ops::RangeInclusive
         // Round to the step before adding it, so whole-number fields drop decimals (55.4 + 1 = 56).
         let v = (*value / grid).round() * grid + step;
         // Round to 4 decimal places, so repeated 0.1 steps don't leave float errors.
-        *value = ((v * 1e4).round() / 1e4).clamp(lo, hi);
+        let v = (v * 1e4).round() / 1e4;
+        // `f32::clamp` panics on a reversed or NaN range, which `DragValue::range` accepts.
+        *value = if lo <= hi { v.clamp(lo, hi) } else { v };
         ui.memory_mut(|m| m.request_focus(resp.id));
         resp.mark_changed();
     }
@@ -786,6 +788,14 @@ mod tests {
         assert_eq!(h.state().1, 6, "every step reports a change, so callers apply it");
     }
 
+    #[test]
+    fn arrow_keys_on_a_field_with_a_reversed_range_do_not_panic() {
+        let mut h = focused(5.0, 10.0..=0.0);
+        let _ = press(&mut h, Modifiers::NONE, Key::ArrowUp);
+        let mut h = focused(5.0, f32::NAN..=10.0);
+        let _ = press(&mut h, Modifiers::NONE, Key::ArrowDown);
+    }
+
     #[test]
     fn arrow_keys_round_decimals_step_fine_fields_and_clamp() {
         for (start, mods, key, want) in [

crates/ui-egui/src/workspace_ui.rs

@@ -275,6 +275,26 @@ fn custom_par(app: &mut PhotocraftApp, p: &Value) -> Result<Value, String> {
     Ok(json!({"pixelAspectRatio": c.ratio, "name": c.name}))
 }
 
+/// The Show Extras Options… checkboxes as (field, label). The fields are the serialized
+/// [`crate::view_cmds::Show`] names, the keys the dialog is seeded with and its command accepts.
+fn extras_options() -> [(&'static str, &'static str); 13] {
+    [
+        ("layer_edges", tl!("Layer Edges")),
+        ("selection_edges", tl!("Selection Edges")),
+        ("target_path", tl!("Target Path")),
+        ("notes", tl!("Notes")),
+        ("pixel_grid", tl!("Pixel Grid")),
+        ("slices", tl!("Slices")),
+        ("count", tl!("Count")),
+        ("smart_guides", tl!("Smart Guides")),
+        ("brush_preview", tl!("Brush Preview")),
+        ("mesh", tl!("Mesh")),
+        ("edit_pins", tl!("Edit Pins")),
+        ("canvas_guides", tl!("Canvas Guides")),
+        ("artboard_guides", tl!("Artboard Guides")),
+    ]
+}
+
 /// The open dialog's OK as (command id, params).
 pub fn dialog_command(kind: &str, f: &Map<String, Value>) -> Option<(&'static str, Value)> {
     Some(match kind {
@@ -396,21 +416,7 @@ fn dialog(app: &mut PhotocraftApp, ctx: &egui::Context) {
                     });
                 }
                 _ => {
-                    for (k, label) in [
-                        ("layerEdges", tl!("Layer Edges")),
-                        ("selectionEdges", tl!("Selection Edges")),
-                        ("targetPath", tl!("Target Path")),
-                        ("notes", tl!("Notes")),
-                        ("pixelGrid", tl!("Pixel Grid")),
-                        ("slices", tl!("Slices")),
-                        ("count", tl!("Count")),
-                        ("smartGuides", tl!("Smart Guides")),
-                        ("brushPreview", tl!("Brush Preview")),
-                        ("mesh", tl!("Mesh")),
-                        ("editPins", tl!("Edit Pins")),
-                        ("canvasGuides", tl!("Canvas Guides")),
-                        ("artboardGuides", tl!("Artboard Guides")),
-                    ] {
+                    for (k, label) in extras_options() {
                         check(ui, &mut f, k, label);
                     }
                 }
@@ -553,6 +559,31 @@ mod tests {
         crate::analysis_ui::tests::render(&mut app, &ctx, windows);
     }
 
+    #[test]
+    fn extras_options_checkboxes_show_and_change_every_flag() {
+        let (mut app, ctx) = app();
+        let before = serde_json::to_value(app.ui.view.show).unwrap();
+        crate::menus::invoke(&mut app, &ctx, "view.show.showExtrasOptions", json!({})).unwrap();
+        let (kind, mut f) = app.ui.shell.dialog.clone().unwrap();
+        // Every checkbox reads the seeded value of its flag (Selection Edges is on by default).
+        assert_eq!(extras_options().len(), f.len());
+        for (k, _) in extras_options() {
+            assert!(before[k].is_boolean(), "{k} is not a Show field");
+            assert_eq!(f.get(k).and_then(Value::as_bool), before[k].as_bool(), "{k}");
+        }
+        // Ticking every box (one- and multi-word flags in one submission) applies them all on OK.
+        for (k, _) in extras_options() {
+            let on = !f.get(k).and_then(Value::as_bool).unwrap();
+            f.insert(k.into(), json!(on));
+        }
+        let (id, p) = dialog_command(&kind, &f).unwrap();
+        crate::menus::invoke(&mut app, &ctx, id, p).unwrap();
+        let after = serde_json::to_value(app.ui.view.show).unwrap();
+        for (k, _) in extras_options() {
+            assert_eq!(after[k].as_bool(), before[k].as_bool().map(|b| !b), "{k}");
+        }
+    }
+
     #[test]
     fn thirty_two_bit_preview_dialog() {
         let (mut app, ctx) = app();

docs/control-protocol.md

@@ -307,6 +307,8 @@ a driver process crash is detected by the startup marker on the next launch.
 
 ### Camera Raw dialog
 
+On the desktop the dialog is a window of its own (title bar, moved and resized like any window), as Adobe Camera Raw is; the main window is dimmed and takes no input while it is open, and closing the window is Cancel. Where the platform has no extra windows (the web build) it is drawn over the main window.
+
 `ui.menu.invoke {"id":"filter.cameraRaw","params":{}}` opens Camera Raw on the active
 RGB/Grayscale layer. `params: {"smartFilter": {"layer": id, "index": i}}` opens it on an existing
 Camera Raw smart filter instead (as double-clicking the filter in the Layers panel does): the