From d3b26f2a659b85c16ea01f949723a49f091a1487 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 21:00:37 +0000 Subject: [PATCH 1/4] Appearance: Sync with System keeps following macOS appearance changes after startup (#642) apply_visuals pinned egui's theme preference to Light or Dark on every apply. egui syncs that to the native window (ViewportCommand::SetTheme), winit then gives the NSWindow an explicit Aqua/DarkAqua appearance, and a window with its own appearance no longer receives the system's ThemeChanged events, so Sync with System froze on the first frame's appearance. Our visuals now go into both of egui's styles (light and dark), and the preference is System while the appearance mode is Sync with System, so the window keeps following the OS; fixed Light / Dark modes still pin it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f --- crates/ui-egui/src/lib.rs | 30 ++++++++++++++++++++++++++++++ crates/ui-egui/src/theme.rs | 21 ++++++++++++++++----- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 92eba0fb..00af44aa 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -427,6 +427,7 @@ impl EffectcraftApp { self.ui.theme = theme::ThemeKind::from_name(p.appearance.resolved_theme(system)).unwrap_or_default(); self.tokens = Tokens::from_prefs(p, self.ui.theme); theme::apply_visuals(ctx, &self.tokens); + ctx.set_theme(theme::theme_preference(&p.appearance.appearance_mode, self.ui.theme.is_light())); let tips = p.general.show_tool_tips; ctx.all_styles_mut(|s| s.interaction.tooltip_delay = if tips { 0.5 } else { f32::INFINITY }); // Settings ▸ Appearance ▸ UI Scale (#284). egui's own Ctrl+= / Ctrl+- / Ctrl+0 zoom stays @@ -1891,6 +1892,35 @@ mod appearance_tests { assert_eq!(app.ui.theme, ThemeKind::Dark); } + /// Sync with System leaves egui's theme preference at System, so egui never gives the native + /// window an appearance of its own (on macOS that stops system appearance changes reaching + /// the app, #642); egui's light and dark styles both carry the resolved theme. Fixed modes + /// pin the window to their family. + #[test] + fn auto_mode_leaves_the_native_window_following_the_system() { + let ctx = egui::Context::default(); + let mut app = app(); + app.hooks.system_theme = Some(Box::new(|_| Some(egui::Theme::Light))); + app.set_pref("appearance.appearanceMode", json!("auto")).unwrap(); + app.apply_prefs(&ctx); + assert_eq!(app.ui.theme, ThemeKind::Light); + assert_eq!(ctx.options(|o| o.theme_preference), egui::ThemePreference::System); + for theme in [egui::Theme::Light, egui::Theme::Dark] { + assert_eq!(ctx.style_of(theme).visuals.panel_fill, app.tokens.panel_bg, "{theme:?}"); + } + // egui asks the window to follow the system, never to take Light or Dark. + let mut out = ctx.run_ui(egui::RawInput::default(), |_| {}); + out.textures_delta.clear(); + let cmds = out.viewport_output.values().flat_map(|v| v.commands.iter()).collect::>(); + assert!(!cmds.iter().any(|c| matches!(c, egui::ViewportCommand::SetTheme(t) if t != &egui::SystemTheme::SystemDefault)), "{cmds:?}"); + for (mode, pref) in [("light", egui::ThemePreference::Light), ("dark", egui::ThemePreference::Dark)] { + app.set_pref("appearance.appearanceMode", json!(mode)).unwrap(); + app.apply_prefs(&ctx); + assert_eq!(ctx.options(|o| o.theme_preference), pref, "{mode}"); + assert_eq!(ctx.global_style().visuals.panel_fill, app.tokens.panel_bg, "{mode}"); + } + } + /// Picking a theme (`view.theme.*`, `ui.set {theme}`) fixes the mode to its family. #[test] fn picking_a_theme_fixes_its_mode() { diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 928dca9b..febd1991 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -405,11 +405,20 @@ fn is_chinese(language: &str) -> bool { matches!(language, "zh-hans" | "zh-hant") } +/// egui's theme preference for an appearance mode showing a `light` (or dark) theme. Sync with +/// System leaves it at System: a concrete preference makes egui set the native window's +/// appearance, and on macOS a window with its own appearance no longer hears when the system's +/// changes (#642). A fixed Light or Dark mode pins the window to match. +pub fn theme_preference(appearance_mode: &str, light: bool) -> egui::ThemePreference { + match appearance_mode { + "auto" => egui::ThemePreference::System, + _ if light => egui::ThemePreference::Light, + _ => egui::ThemePreference::Dark, + } +} + pub fn apply_visuals(ctx: &egui::Context, t: &Tokens) { let light = t.kind.is_light(); - // egui keeps one style per light/dark theme and by default picks it by the system appearance; - // pin it to ours so a fixed Dark or Light mode isn't swapped for egui's defaults. - ctx.set_theme(if light { egui::ThemePreference::Light } else { egui::ThemePreference::Dark }); let mut v = if light { Visuals::light() } else { Visuals::dark() }; v.panel_fill = t.panel_bg; v.window_fill = t.panel_bg; @@ -444,8 +453,10 @@ pub fn apply_visuals(ctx: &egui::Context, t: &Tokens) { v.widgets.active.fg_stroke = Stroke::new(1.0, t.tab_text_active); v.widgets.open.bg_fill = t.hover; v.widgets.open.weak_bg_fill = t.hover; - ctx.set_visuals(v); - ctx.global_style_mut(|s| { + // egui keeps one style per light/dark theme and picks it by its theme preference (see + // [`theme_preference`]); both get ours, so whichever it picks, egui's defaults never show. + ctx.all_styles_mut(|s| { + s.visuals = v.clone(); s.spacing.item_spacing = egui::vec2(6.0, 4.0); s.spacing.button_padding = egui::vec2(8.0, 3.0); s.spacing.interact_size.y = 22.0; From 1bcfe6912d84547eabe3a6ed2df7472f9292f813 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 21:11:53 +0000 Subject: [PATCH 2/4] Timeline: colour properties get an eyedropper after their swatch, as in After Effects (#633) A shape's Fill and Stroke Color (and every other colour property) in the Timeline only had a swatch that opens the colour picker; After Effects puts an eyedropper next to it to sample a colour from the Composition panel. The Timeline now shows that eyedropper (`timeline.prop..eyedropper`), reusing Effect Controls' viewer pick: click it, then click in the Composition panel, and the property is set to the sampled colour. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f --- crates/ui-egui/src/panels/effect_controls.rs | 5 ++- crates/ui-egui/src/panels/timeline.rs | 11 +++++ crates/ui-egui/tests/ui_timeline_shapes.rs | 46 ++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/panels/effect_controls.rs b/crates/ui-egui/src/panels/effect_controls.rs index 6612187e..fb388845 100644 --- a/crates/ui-egui/src/panels/effect_controls.rs +++ b/crates/ui-egui/src/panels/effect_controls.rs @@ -186,8 +186,9 @@ fn key_navigator( } } -/// Toggle a viewer pick (crosshair / eyedropper) for a parameter. -fn toggle_pick(app: &mut EffectcraftApp, kind: &str, layer: &Layer, prop: &Property) { +/// Toggle a viewer pick (crosshair / eyedropper) for a parameter (also the Timeline's colour +/// eyedroppers). +pub(crate) fn toggle_pick(app: &mut EffectcraftApp, kind: &str, layer: &Layer, prop: &Property) { let pick = FxPick { kind: kind.into(), layer: layer.id.0, prop: prop.uid, name: prop.name.clone() }; if app.ui.fx_pick.as_ref() == Some(&pick) { app.ui.fx_pick = None; diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs index 086e46dc..b28c0d23 100644 --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -3591,6 +3591,17 @@ fn value_editor( if resp.clicked() { widgets::open_popup(ui, pop); } + // The eyedropper after the swatch samples the Composition panel (shape Fill and + // Stroke colours, text and effect colours), as in After Effects' Timeline (#633). + let er = Rect::from_center_size(pos2(r.max.x + 12.0, at.y), vec2(16.0, 16.0)); + let active = app.ui.fx_pick.as_ref().is_some_and(|k| k.prop == uid && k.kind == "color"); + if super::fx_widgets::eyedropper_button(ui, er, active, egui::Id::new(("tl-eye", uid)), &t) + .on_hover_text(crate::i18n::tr("Eyedropper: click, then click in the Composition panel")) + .clicked() + { + super::effect_controls::toggle_pick(app, "color", layer, prop); + } + app.auto.add(&format!("timeline.prop.{uid}.eyedropper"), er, &prop.name); let mut rgb = [c[0] as f32, c[1] as f32, c[2] as f32]; if crate::widgets::color_popup(ui, pop, r.left_bottom(), &mut rgb) { set(actions, json!([rgb[0], rgb[1], rgb[2], 1.0])); diff --git a/crates/ui-egui/tests/ui_timeline_shapes.rs b/crates/ui-egui/tests/ui_timeline_shapes.rs index 811b55c7..dd9232d7 100644 --- a/crates/ui-egui/tests/ui_timeline_shapes.rs +++ b/crates/ui-egui/tests/ui_timeline_shapes.rs @@ -197,3 +197,49 @@ fn toolbar_stroke_edits_the_selected_shape_layer() { assert_eq!(width, Some(4.0), "a stroke was added with the toolbar's width"); assert_eq!(h.state().session.state.shape_tool.stroke_width, 4.0); } + +/// Issue #633: a colour property in the Timeline (a shape's Fill Color) has an eyedropper after +/// its swatch, as in After Effects: click it, then click in the Composition panel to sample. +#[test] +fn timeline_colour_eyedropper_samples_the_comp() { + let mut s = session(); + s.execute("layer.newSolid", json!({"name": "Back", "color": "#20c040"})).unwrap(); + let l = s.execute("layer.newShape", json!({"kind": "rect", "fill": [1.0, 0.0, 0.0], "size": [100.0, 60.0]})).unwrap()["layer"].as_u64().unwrap(); + let layer = s.active_comp().unwrap().layer(LayerId(l)).unwrap().clone(); + let mut fill = None; + layer.props.walk("", &mut |_, p| { + if fill.is_none() && matches!(p.value, effectcraft_engine::keyframe::Value::Color(_)) { + fill = Some(p.uid); + } + }); + let fill = fill.expect("a fill colour"); + fn groups(g: &effectcraft_engine::project::PropGroup, out: &mut Vec) { + for c in g.groups() { + out.push(c.uid); + groups(c, out); + } + } + let mut open = vec![]; + groups(&layer.props, &mut open); + let mut h = harness(s); + h.state_mut().ui.timeline.open_groups.extend(open); + open_layer(&mut h, l); + h.run_steps(2); + click(&mut h, &format!("timeline.prop.{fill}.eyedropper")); + assert_eq!(h.state().ui.fx_pick.as_ref().map(|p| (p.prop, p.kind.as_str())), Some((fill, "color")), "the eyedropper waits for a viewer click"); + let pos = effectcraft_ui_egui::panels::viewer::comp_to_screen(&h.ctx, [6.0, 6.0]).unwrap(); + click_at(&mut h, pos); + assert!(h.state().ui.fx_pick.is_none(), "the pick is used up"); + let layer = h.state().session.active_comp().unwrap().layer(LayerId(l)).unwrap().clone(); + let mut got = None; + layer.props.walk("", &mut |_, p| { + if p.uid == fill { + got = Some(p.value.clone()); + } + }); + let Some(effectcraft_engine::keyframe::Value::Color(c)) = got else { panic!("{got:?}") }; + let want = [0x20 as f64 / 255.0, 0xc0 as f64 / 255.0, 0x40 as f64 / 255.0]; + for k in 0..3 { + assert!((c[k] - want[k]).abs() < 0.02, "{c:?} vs {want:?}"); + } +} From e2a47495eb8ad4c5601ceba3280a1b8c3b2b267f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 21:17:18 +0000 Subject: [PATCH 3/4] GPU: an effect kernel the shader compiler rejects no longer turns the GPU compositor off (#613) GpuContext::new built every compute pipeline up front and failed as a whole when one didn't build. On DirectX 12, FXC (wgpu's default D3D shader compiler) aborts on the `pointwise` effect kernel, so the whole app fell back to CPU compositing. The compositor's own kernels are still required, but the effect kernels (`pointwise`, the blur/glow/shadow passes and every fx_* kernel) are now optional: one that fails to build is logged and listed (GpuContext::unavailable_kernels), and a GPU effect chain that dispatches a missing kernel reports it (Enc::take_missing) so those effects render on the CPU while everything else stays on the GPU. This does not make FXC accept `pointwise` (that needs a Windows/FXC box to bisect); it limits the damage to the effects using it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f --- crates/gpu/src/context.rs | 95 +++++++++++++++++++++++++++------------ crates/gpu/src/effects.rs | 9 ++++ crates/gpu/src/tests.rs | 32 +++++++++++++ 3 files changed, 108 insertions(+), 28 deletions(-) diff --git a/crates/gpu/src/context.rs b/crates/gpu/src/context.rs index e994dd1b..c5065eae 100644 --- a/crates/gpu/src/context.rs +++ b/crates/gpu/src/context.rs @@ -18,14 +18,6 @@ const ENTRIES: &[&str] = &[ "quantize", "convert", "half", - "box_h", - "box_v", - "directional", - "glow_bright", - "glow_combine", - "shadow_make", - "shadow_combine", - "pointwise", "adjust_mix", "bokeh_boost", "bokeh_prefix", @@ -33,6 +25,11 @@ const ENTRIES: &[&str] = &[ "fill", ]; +/// Effect entry points in `kernels.wgsl`. Like the `fx_*` kernels, one that fails to build +/// (a driver's shader compiler rejecting it, #613) leaves its effects to the CPU instead of +/// turning the whole compositor off. +const EFFECT_ENTRIES: &[&str] = &["box_h", "box_v", "directional", "glow_bright", "glow_combine", "shadow_make", "shadow_combine", "pointwise"]; + /// Entry points that also bind group 1 (four read-only storage buffers; see /// [`Enc::dispatch_ext`]). const EXT_ENTRIES: &[&str] = &["classic3d"]; @@ -244,6 +241,8 @@ pub struct GpuContext { bgl: wgpu::BindGroupLayout, bgl_ext: wgpu::BindGroupLayout, pipelines: HashMap<&'static str, wgpu::ComputePipeline>, + /// Effect kernels that failed to build, and why (their effects render on the CPU). + unavailable: Vec<(&'static str, String)>, display_bgl: wgpu::BindGroupLayout, display: wgpu::ComputePipeline, /// The display texture's next mip level from the one above (`display.wgsl` `mip`). @@ -450,7 +449,29 @@ impl GpuContext { immediate_size: 0, }) })?; - let pipelines = ENTRIES + let build = |e: &'static str, layout: &wgpu::PipelineLayout| { + init_resource(&device, e, || { + device.create_compute_pipeline(&wgpu::ComputePipelineDescriptor { + label: Some(e), + layout: Some(layout), + module: &module, + entry_point: Some(e), + compilation_options: Default::default(), + cache: None, + }) + }) + }; + // The compositor's own kernels are required; an effect kernel that doesn't build is + // left out, and its effects render on the CPU (see `Enc::take_missing`). + let mut pipelines = ENTRIES + .iter() + .chain(crate::adv3d::SKY_KERNELS) + .map(|e| (e, &layout)) + .chain(EXT_ENTRIES.iter().map(|e| (e, &layout_ext))) + .map(|(e, layout)| Ok((*e, build(e, layout)?))) + .collect::, String>>()?; + let mut unavailable = vec![]; + for e in EFFECT_ENTRIES .iter() .chain(crate::fx_color::KERNELS) .chain(crate::fx_distort::KERNELS) @@ -472,23 +493,17 @@ impl GpuContext { .chain(crate::fx_time::KERNELS) .chain(crate::fx_pixel2::KERNELS) .chain(crate::fx_gen2::KERNELS) - .chain(crate::adv3d::SKY_KERNELS) - .map(|e| (e, &layout)) - .chain(EXT_ENTRIES.iter().map(|e| (e, &layout_ext))) - .map(|(e, layout)| { - let p = init_resource(&device, e, || { - device.create_compute_pipeline(&wgpu::ComputePipelineDescriptor { - label: Some(e), - layout: Some(layout), - module: &module, - entry_point: Some(e), - compilation_options: Default::default(), - cache: None, - }) - })?; - Ok((*e, p)) - }) - .collect::, String>>()?; + { + match build(e, &layout) { + Ok(p) => { + pipelines.insert(*e, p); + } + Err(why) => { + log::warn!("gpu {name}: kernel {e} unavailable, its effects render on the CPU: {why}"); + unavailable.push((*e, why)); + } + } + } let dmodule = init_resource(&device, "display shader module", || { device.create_shader_module(wgpu::ShaderModuleDescriptor { label: Some("effectcraft display"), @@ -547,6 +562,7 @@ impl GpuContext { bgl, bgl_ext, pipelines, + unavailable, display_bgl, display, display_mip, @@ -721,6 +737,18 @@ impl GpuContext { /// Readbacks (GPU → CPU) are possible: waited for natively, or deferred (a browser /// worker, [`crate::deferred`]). Impossible on the browser's main thread. + /// Effect kernels the device couldn't build, with why: their effects render on the CPU. + pub fn unavailable_kernels(&self) -> &[(&'static str, String)] { + &self.unavailable + } + + /// Leave kernel `entry` out, as if it had failed to build (tests of the CPU fallback). + #[cfg(test)] + pub(crate) fn drop_kernel(&mut self, entry: &'static str) { + self.pipelines.remove(entry); + self.unavailable.push((entry, "dropped by a test".into())); + } + pub fn can_readback(&self) -> bool { self.readbacks.check().is_ok() && (self.can_wait() || self.deferred.is_some()) } @@ -859,6 +887,8 @@ pub(crate) struct Enc<'g> { /// The open command encoder and its id in the texture pool. enc: Option<(wgpu::CommandEncoder, u64)>, pending: usize, + /// A dispatch named a kernel the device doesn't have (see [`Enc::take_missing`]). + missing: bool, } impl Drop for Enc<'_> { @@ -874,7 +904,13 @@ impl Drop for Enc<'_> { impl<'g> Enc<'g> { pub fn new(g: &'g GpuContext) -> Enc<'g> { - Enc { g, enc: None, pending: 0 } + Enc { g, enc: None, pending: 0, missing: false } + } + + /// Whether a dispatch since the last call named a kernel the device doesn't have (one + /// that failed to build): its output is wrong, and the effect must render on the CPU. + pub(crate) fn take_missing(&mut self) -> bool { + std::mem::take(&mut self.missing) } pub(crate) fn encoder(&mut self) -> &mut wgpu::CommandEncoder { @@ -954,7 +990,10 @@ impl<'g> Enc<'g> { ) { let g = self.g; let Some(pipe) = g.pipelines.get(entry) else { - log::error!("gpu: no kernel {entry}"); + if !g.unavailable.iter().any(|(e, _)| *e == entry) { + log::error!("gpu: no kernel {entry}"); + } + self.missing = true; return; }; let ub = g.device.create_buffer_init(&wgpu::util::BufferInitDescriptor { label: None, contents: &p.bytes(), usage: wgpu::BufferUsages::UNIFORM }); diff --git a/crates/gpu/src/effects.rs b/crates/gpu/src/effects.rs index 00cc4fd9..426931d7 100644 --- a/crates/gpu/src/effects.rs +++ b/crates/gpu/src/effects.rs @@ -66,8 +66,12 @@ pub(crate) fn run_chain(e: &mut Enc, chain: &[FxStep], buf: &Buf, levels: Option } let misses = deferred.as_ref().map(|d| d.misses()); let mut b = GBuf { img: e.g.upload_image(&buf.img)?, offset: buf.offset, scale: buf.scale }; + e.take_missing(); for step in chain { b = apply(e, step.spec.id, &step.ctx, b)?; + if e.take_missing() { + return None; + } if let Some(l) = levels { b.img = ops::quantize(e, &b.img, l); } @@ -119,8 +123,13 @@ impl effectcraft_render::FxTarget for GpuFx<'_, '_> { fn gpu(&mut self, steps: &[FxStep]) -> bool { let Some(b) = &self.b else { return false }; let mut cur = GBuf { img: b.img.clone(), offset: b.offset, scale: b.scale }; + self.e.take_missing(); for step in steps { let Some(next) = apply(self.e, step.spec.id, &step.ctx, cur) else { return false }; + // A kernel the device couldn't build: the effects render on the CPU (#613). + if self.e.take_missing() { + return false; + } cur = next; if let Some(l) = self.levels { cur.img = ops::quantize(self.e, &cur.img, l); diff --git a/crates/gpu/src/tests.rs b/crates/gpu/src/tests.rs index 39be29a9..3629d8ea 100644 --- a/crates/gpu/src/tests.rs +++ b/crates/gpu/src/tests.rs @@ -745,3 +745,35 @@ fn within_memory_returns_the_work_when_there_is_room() { let img = g.within_memory(|| g.ctx.image(64, 32)).expect("room for a 64×32 texture"); assert_eq!((img.width, img.height), (64, 32)); } + +/// #613: an effect kernel the device's shader compiler rejects (FXC on DirectX 12 aborting on +/// `pointwise`) no longer turns the whole GPU compositor off: the context is built without it, +/// lists it, and the effects that need it render on the CPU, matching the CPU render. +#[test] +fn effects_whose_kernels_failed_render_on_the_cpu() { + if gpu().is_none() { + return; + } + let Ok(mut ctx) = pollster::block_on(crate::context::GpuContext::request()) else { return }; + ctx.drop_kernel("pointwise"); + ctx.drop_kernel("box_h"); + let names: Vec<&str> = ctx.unavailable_kernels().iter().map(|(e, _)| *e).collect(); + assert_eq!(names, ["pointwise", "box_h"]); + let g = Gpu::from_context(ctx); + for depth in [BitDepth::Bpc8, BitDepth::Bpc32] { + let mut s = Scene::new(depth); + let mut l = s.footage(80, 50); + s.effect(&mut l, "ec.color.exposure", &[("master/exposure", n(0.7))]); + s.effect(&mut l, "ec.color.tint", &[("black", c(0.1, 0.0, 0.3)), ("amount", n(80.0))]); + s.effect(&mut l, "ec.blur.gaussian", &[("blurriness", n(5.0))]); + s.effect(&mut l, "ec.channel.invert", &[("blend", n(50.0))]); + s.push(l); + let cpu = Renderer::new(&s.p, &Pattern, RenderOpts { backend: Backend::Cpu, ..opts() }).comp_frame_cpu(s.cid, Tick::ZERO); + let mut r = Renderer::new(&s.p, &Pattern, RenderOpts { backend: Backend::Gpu, ..opts() }); + r.accel = Some(&g); + let img = g.render(&r, s.cid, Tick::ZERO).expect("the GPU renders the frame"); + let mut d = diff(&cpu, &img, tolerance(depth)); + d.quantized = depth != BitDepth::Bpc32; + check(&format!("missing kernels {depth:?}"), Some(d), 0.0); + } +} From ef96419bec78d541b8699def1fae219178fd7e86 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 21:29:46 +0000 Subject: [PATCH 4/4] Appearance: the Light theme repaints Effect Controls headers, Timeline wells and the expression field (#644) Effect Controls painted effect headers with fixed dark fills (#2a2a2a, #2f3a52 when active) under the theme's text colour, so on Light the effect names were dark on dark (1.1:1). The Timeline kept the selected layer's time-graph row, the A/V and switch wells, the time navigator, the work area bar and the expression field on fixed dark fills too. Those surfaces are now theme tokens (fx_header, fx_header_active, tl_graph_selected, switch_well, tl_nav_bar, tl_work_area_bar, expr_bg, expr_text) with the dark values unchanged and light ones for Light. The expression editor's syntax colours come from the theme as well: on the light field a darker set keeps every category, enabled/disabled expressions, matched brackets and line numbers at 4.5:1 or better (contrast test, with a negative control showing the dark set fails there). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f --- crates/ui-egui/src/panels/effect_controls.rs | 2 +- crates/ui-egui/src/panels/expr_editor.rs | 133 ++++++++++++++++--- crates/ui-egui/src/panels/timeline.rs | 29 ++-- crates/ui-egui/src/theme.rs | 36 +++++ 4 files changed, 161 insertions(+), 39 deletions(-) diff --git a/crates/ui-egui/src/panels/effect_controls.rs b/crates/ui-egui/src/panels/effect_controls.rs index fb388845..8df4447b 100644 --- a/crates/ui-egui/src/panels/effect_controls.rs +++ b/crates/ui-egui/src/panels/effect_controls.rs @@ -1102,7 +1102,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let open = !app.ui.fx_closed.contains(&g.uid); let selected = app.session.state.selected_props.iter().any(|(l, u)| *l == layer.id && *u == g.uid); let active = effect_active(g, layer.id, &app.session.state.selected_props); - bp.rect_filled(r, 0.0, if active { Color32::from_rgb(0x2f, 0x3a, 0x52) } else { Color32::from_rgb(0x2a, 0x2a, 0x2a) }); + bp.rect_filled(r, 0.0, if active { t.fx_header_active } else { t.fx_header }); let fxr = Rect::from_center_size(pos2(r.min.x + 14.0, r.center().y), vec2(16.0, 16.0)); if widgets::icon_toggle(ui, fxr, Icon::Fx, g.enabled, &t, egui::Id::new(("ec-fx", g.uid)), Sense::click()).clicked() { actions.push(("effect.toggle".into(), json!({"layer": layer.id.0, "effect": g.uid}))); diff --git a/crates/ui-egui/src/panels/expr_editor.rs b/crates/ui-egui/src/panels/expr_editor.rs index 1efd6876..ffb61738 100644 --- a/crates/ui-egui/src/panels/expr_editor.rs +++ b/crates/ui-egui/src/panels/expr_editor.rs @@ -9,13 +9,42 @@ use egui::{Color32, FontId, Rect, Stroke, pos2, vec2}; use crate::theme::Tokens; -/// Syntax colours. -const KEYWORD: Color32 = Color32::from_rgb(0xc6, 0x8a, 0xe6); -const NUMBER: Color32 = Color32::from_rgb(0xe0, 0xb4, 0x6c); -const STRING: Color32 = Color32::from_rgb(0x9c, 0xd0, 0x7a); -const COMMENT: Color32 = Color32::from_rgb(0x80, 0x88, 0x90); -const API: Color32 = Color32::from_rgb(0x6c, 0xb8, 0xf0); -const BRACKET_BG: Color32 = Color32::from_rgb(0x4a, 0x55, 0x6a); +/// Syntax colours, and the matched brackets' background. +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct Syntax { + pub keyword: Color32, + pub number: Color32, + pub string: Color32, + pub comment: Color32, + pub api: Color32, + pub bracket_bg: Color32, +} + +impl Syntax { + /// The colours for the theme: on the Light theme's light expression field, darker ones that + /// stay readable (#644). + pub fn for_tokens(t: &Tokens) -> Syntax { + if t.kind.is_light() { + Syntax { + keyword: Color32::from_rgb(0x7a, 0x2f, 0xa8), + number: Color32::from_rgb(0x8a, 0x4b, 0x00), + string: Color32::from_rgb(0x2c, 0x6a, 0x10), + comment: Color32::from_rgb(0x52, 0x5a, 0x62), + api: Color32::from_rgb(0x0a, 0x57, 0x94), + bracket_bg: Color32::from_rgb(0xd0, 0xdc, 0xf0), + } + } else { + Syntax { + keyword: Color32::from_rgb(0xc6, 0x8a, 0xe6), + number: Color32::from_rgb(0xe0, 0xb4, 0x6c), + string: Color32::from_rgb(0x9c, 0xd0, 0x7a), + comment: Color32::from_rgb(0x80, 0x88, 0x90), + api: Color32::from_rgb(0x6c, 0xb8, 0xf0), + bracket_bg: Color32::from_rgb(0x4a, 0x55, 0x6a), + } + } + } +} const KEYWORDS: &[&str] = &[ "var", @@ -184,7 +213,7 @@ pub fn completions(text: &str, cursor: usize) -> (String, Vec<&'static str>) { } /// Colour runs for JavaScript: (byte start, byte end, colour). -pub fn highlight(text: &str, base: Color32) -> Vec<(usize, usize, Color32)> { +pub fn highlight(text: &str, base: Color32, sx: &Syntax) -> Vec<(usize, usize, Color32)> { let b = text.as_bytes(); let mut out = vec![]; let mut i = 0; @@ -195,35 +224,35 @@ pub fn highlight(text: &str, base: Color32) -> Vec<(usize, usize, Color32)> { while i < b.len() && b[i] != b'\n' { i += 1; } - COMMENT + sx.comment } else if c == b'/' && b.get(i + 1) == Some(&b'*') { i += 2; while i < b.len() && !(b[i] == b'*' && b.get(i + 1) == Some(&b'/')) { i += 1; } i = (i + 2).min(b.len()); - COMMENT + sx.comment } else if c == b'"' || c == b'\'' || c == b'`' { i += 1; while i < b.len() && b[i] != c { i += if b[i] == b'\\' { 2 } else { 1 }; } i = (i + 1).min(b.len()); - STRING + sx.string } else if c.is_ascii_digit() { while i < b.len() && (b[i].is_ascii_alphanumeric() || b[i] == b'.') { i += 1; } - NUMBER + sx.number } else if c.is_ascii_alphabetic() || c == b'_' || c == b'$' { while i < b.len() && (b[i].is_ascii_alphanumeric() || b[i] == b'_' || b[i] == b'$') { i += 1; } let w = &text[start..i]; if KEYWORDS.contains(&w) { - KEYWORD + sx.keyword } else if COMPLETIONS.contains(&w) { - API + sx.api } else { base } @@ -246,9 +275,9 @@ fn byte_of(text: &str, ci: usize) -> usize { } /// The layout job of the editor text: syntax colours, the matched brackets' background, wrap. -fn layout(text: &str, font: FontId, base: Color32, p: &Scripting, brackets: Option<(usize, usize)>, wrap: f32) -> LayoutJob { +fn layout(text: &str, font: FontId, base: Color32, sx: &Syntax, p: &Scripting, brackets: Option<(usize, usize)>, wrap: f32) -> LayoutJob { let mut job = LayoutJob::default(); - let runs = if p.syntax_highlighting { highlight(text, base) } else { vec![(0, text.len(), base)] }; + let runs = if p.syntax_highlighting { highlight(text, base, sx) } else { vec![(0, text.len(), base)] }; let marks: Vec = brackets.filter(|_| p.bracket_matching).map(|(a, b)| vec![byte_of(text, a), byte_of(text, b)]).unwrap_or_default(); for (s, e, col) in runs { // Split runs at the marked brackets. @@ -263,7 +292,7 @@ fn layout(text: &str, font: FontId, base: Color32, p: &Scripting, brackets: Opti cuts.sort_unstable(); cuts.dedup(); for w in cuts.windows(2) { - let bg = if marks.contains(&w[0]) { BRACKET_BG } else { Color32::TRANSPARENT }; + let bg = if marks.contains(&w[0]) { sx.bracket_bg } else { Color32::TRANSPARENT }; job.append(&text[w[0]..w[1]], 0.0, TextFormat { font_id: font.clone(), color: col, background: bg, ..Default::default() }); } } @@ -281,8 +310,9 @@ pub fn editor(ui: &mut egui::Ui, id: egui::Id, buf: &mut String, rect: Rect, p: let cursor = egui::TextEdit::load_state(ui.ctx(), id).and_then(|s| s.cursor.char_range()).map(|r| r.primary.index.0); let brackets = cursor.and_then(|c| matching_bracket(buf, c)); let pc = p.clone(); + let sx = Syntax::for_tokens(t); let mut layouter = move |ui: &egui::Ui, text: &dyn egui::TextBuffer, wrap: f32| { - let job = layout(text.as_str(), font.clone(), color, &pc, brackets, wrap); + let job = layout(text.as_str(), font.clone(), color, &sx, &pc, brackets, wrap); ui.fonts_mut(|f| f.layout_job(job)) }; let mut child = ui.new_child(egui::UiBuilder::new().max_rect(er)); @@ -392,3 +422,70 @@ pub fn error_banner(app: &mut crate::EffectcraftApp, ui: &egui::Ui, area: Rect) } } } + +#[cfg(test)] +mod contrast_tests { + use super::*; + use crate::theme::ThemeKind; + + /// WCAG contrast ratio of two opaque sRGB colours. + fn contrast(a: Color32, b: Color32) -> f32 { + let lum = |c: Color32| { + let f = |v: u8| { + let v = v as f32 / 255.0; + if v <= 0.04045 { v / 12.92 } else { ((v + 0.055) / 1.055).powf(2.4) } + }; + 0.2126 * f(c.r()) + 0.7152 * f(c.g()) + 0.0722 * f(c.b()) + }; + let (x, y) = (lum(a), lum(b)); + (x.max(y) + 0.05) / (x.min(y) + 0.05) + } + + /// #644: on every theme, effect names stay readable on Effect Controls' headers, and the + /// expression field's text (every syntax colour, enabled and disabled expressions, matched + /// brackets, line numbers) on its background; the switch icons show on their wells. + #[test] + fn custom_painted_surfaces_keep_text_readable_on_every_theme() { + for kind in [ThemeKind::Dark, ThemeKind::Darker, ThemeKind::Light] { + let t = Tokens::for_kind(kind); + let sx = Syntax::for_tokens(&t); + for (what, bg) in [("effect header", t.fx_header), ("active effect header", t.fx_header_active)] { + assert!(contrast(t.text, bg) >= 4.5, "{kind:?}: effect name on {what}: {:.2}", contrast(t.text, bg)); + } + assert!(contrast(t.icon, t.switch_well) >= 3.0, "{kind:?}: switch icons on their wells"); + if !kind.is_light() { + continue; + } + let fg = [ + ("keyword", sx.keyword), + ("number", sx.number), + ("string", sx.string), + ("comment", sx.comment), + ("api", sx.api), + ("enabled expression", t.expr_text), + ("disabled expression", t.text_dim), + ("line numbers", t.text_faint), + ]; + for (what, c) in fg { + assert!(contrast(c, t.expr_bg) >= 4.5, "{kind:?}: {what} on the expression field: {:.2}", contrast(c, t.expr_bg)); + assert!(contrast(c, sx.bracket_bg) >= 4.0, "{kind:?}: {what} on a matched bracket: {:.2}", contrast(c, sx.bracket_bg)); + } + // The dark theme's syntax colours would not do on the light field. + let dark = Syntax::for_tokens(&Tokens::for_kind(ThemeKind::Dark)); + assert!([dark.keyword, dark.number, dark.string, dark.api].iter().all(|c| contrast(*c, t.expr_bg) < 4.5)); + } + } + + /// Syntax highlighting uses the theme's colours. + #[test] + fn highlight_uses_the_theme_palette() { + let t = Tokens::for_kind(ThemeKind::Light); + let sx = Syntax::for_tokens(&t); + let text = "var amount = 12; // note"; + let runs = highlight(text, t.expr_text, &sx); + let col = |s: &str| runs.iter().find(|(a, e, _)| text.get(*a..*e) == Some(s)).map(|r| r.2); + assert_eq!(col("var"), Some(sx.keyword)); + assert_eq!(col("12"), Some(sx.number)); + assert_eq!(col("// note"), Some(sx.comment)); + } +} diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs index b28c0d23..a5305df9 100644 --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -58,11 +58,6 @@ fn classic_geometry_row(comp: &Comp, l: &Layer) -> bool { comp.renderer == effectcraft_engine::project::Renderer::Classic3D && l.is_3d() && matches!(l.source, LayerSource::Text | LayerSource::Shape) } -/// Time navigator's visible-span bar and the work area bar. -const NAV_BAR: Color32 = Color32::from_rgb(0x55, 0x55, 0x55); -const WORK_AREA_BAR: Color32 = Color32::from_rgb(0x5c, 0x5c, 0x5c); -/// Fill of the timeline's switch / A/V boxes (dark wells, as in After Effects). -const SWITCH_BOX: Color32 = Color32::from_rgb(0x19, 0x19, 0x19); /// Background of the selected layer's name cell. const SELECTED_NAME_BG: Color32 = Color32::from_rgb(0xa6, 0xa6, 0xa6); @@ -1335,7 +1330,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let vis1 = (tm.t(graph_x1) / comp.duration.seconds()) as f32; let nav_vis = Rect::from_min_max(pos2(nav.min.x + nav.width() * vis0.clamp(0.0, 1.0), nav.min.y), pos2(nav.min.x + nav.width() * vis1.clamp(0.0, 1.0), nav.max.y)); - p.rect_filled(nav_vis, 0.0, NAV_BAR); + p.rect_filled(nav_vis, 0.0, t.tl_nav_bar); for x in [nav_vis.min.x, nav_vis.max.x] { p.rect_filled(Rect::from_min_max(pos2(x - 3.0, nav.min.y - 1.0), pos2(x + 3.0, nav.max.y + 1.0)), 3.0, t.accent); } @@ -1375,7 +1370,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let wa_row = Rect::from_min_max(pos2(graph_x0, top + header_h), pos2(rect.max.x, top + header_h + colhdr_h)); let (wa0, wa1) = (comp.work_area.0.seconds(), comp.work_area.1.seconds()); let wa = Rect::from_min_max(pos2(tm.x(wa0), wa_row.min.y + 3.0), pos2(tm.x(wa1), wa_row.max.y - 6.0)); - p.with_clip_rect(wa_row).rect_filled(wa, 0.0, WORK_AREA_BAR); + p.with_clip_rect(wa_row).rect_filled(wa, 0.0, t.tl_work_area_bar); let shift = ui.input(|i| i.modifiers.shift); let bar = wa.intersect(wa_row); let bresp = ui.interact(bar, egui::Id::new("wa-bar"), Sense::click_and_drag()); @@ -1736,7 +1731,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { // (outline and time graph alike), not as alternating stripes. let _ = ri; lp.rect_filled(left, 0.0, if is_sel { t.row_selected } else { t.row_alt }); - gp.rect_filled(Rect::from_min_max(pos2(graph_x0, r.min.y), r.max), 0.0, if is_sel { Color32::from_rgb(0x2a, 0x2a, 0x2a) } else { t.tl_bg }); + gp.rect_filled(Rect::from_min_max(pos2(graph_x0, r.min.y), r.max), 0.0, if is_sel { t.tl_graph_selected } else { t.tl_bg }); lp.line_segment([pos2(rect.min.x, r.max.y - 0.5), pos2(graph_x0, r.max.y - 0.5)], Stroke::new(1.0, t.app_bg)); gp.line_segment([pos2(graph_x0, r.max.y - 0.5), pos2(r.max.x, r.max.y - 0.5)], Stroke::new(1.0, t.app_bg)); // A/V features. @@ -1752,7 +1747,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { if !applicable || !vis.av { continue; } - lp.rect_filled(br.shrink(0.5), 1.0, SWITCH_BOX); + lp.rect_filled(br.shrink(0.5), 1.0, t.switch_well); layer_switch(ui, &t, r, br, icon, on, egui::Id::new(("av", layer.id.0, i)), layer.id.0, name, &mut actions); app.auto.add(&format!("timeline.layer.{}.{name}", layer.id.0), br, name); } @@ -1878,7 +1873,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { continue; } let br = Rect::from_center_size(pos2(cw.switches + SW * i as f32 + SW / 2.0, cy), vec2(16.0, 16.0)); - lp.rect_filled(br.shrink(1.0), 1.0, SWITCH_BOX); + lp.rect_filled(br.shrink(1.0), 1.0, t.switch_well); layer_switch(ui, &t, r, br, icon, on, egui::Id::new(("sw", layer.id.0, i)), layer.id.0, name, &mut actions); app.auto.add(&format!("timeline.layer.{}.switch.{name}", layer.id.0), br, name); } @@ -2233,7 +2228,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let en_r = Rect::from_center_size(pos2(cw.switches + 10.0, cy), vec2(16.0, 16.0)); let en = ui.interact(en_r, egui::Id::new(("expr-en", uid)), Sense::click()).on_hover_text(crate::i18n::tr("Enable Expression")); lp.rect_stroke(en_r.shrink(1.0), 2.0, Stroke::new(1.0, t.separator), StrokeKind::Inside); - let expr_col = Color32::from_rgb(0xe8, 0x7c, 0x5c); + let expr_col = t.expr_text; lp.text( en_r.center(), Align2::CENTER_CENTER, @@ -2318,7 +2313,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } // The editor in the time-graph area. ui.set_clip_rect(right_clip); - gp.rect_filled(Rect::from_min_max(pos2(graph_x0, r.min.y), r.max), 0.0, Color32::from_rgb(0x1a, 0x1a, 0x1a)); + gp.rect_filled(Rect::from_min_max(pos2(graph_x0, r.min.y), r.max), 0.0, t.expr_bg); let er = Rect::from_min_max(pos2(graph_x0 + 8.0, r.min.y + 3.0), pos2(rect.max.x - 14.0, r.max.y - 3.0)); let mut buf: String = ctx.data(|d| d.get_temp(buf_id)).unwrap_or_else(|| ex.text.clone()); if let Some(after) = ctx.data_mut(|d| d.remove_temp::(expr_refocus_id(*uid))) { @@ -2437,13 +2432,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let is_tr_rot = layer.transform().and_then(|t| t.get("rotation")).is_some_and(|r| r.uid == prop.uid); let base = if is_tr_rot && prop.name == "Rotation" && layer.is_3d() { "Z Rotation".to_string() } else { prop.name.clone() }; let pname = if prop.has_expression() { format!("{base} =") } else { base }; - lp.text( - pos2(name_x, cy), - Align2::LEFT_CENTER, - &pname, - Tokens::ui(12.0), - if prop.has_expression() { Color32::from_rgb(0xe8, 0x7c, 0x5c) } else { t.text }, - ); + lp.text(pos2(name_x, cy), Align2::LEFT_CENTER, &pname, Tokens::ui(12.0), if prop.has_expression() { t.expr_text } else { t.text }); let name_rect = Rect::from_min_max(pos2(name_x, r.min.y), pos2(cw.switches - 2.0, r.max.y)); let name_resp = ui.interact(name_rect, egui::Id::new(("pname", uid)), Sense::click_and_drag()); app.auto.add(&format!("timeline.prop.{uid}.name"), name_rect, &prop.name); @@ -2488,7 +2477,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { Align2::LEFT_CENTER, format!("= {}", e.text.lines().next().unwrap_or("")), Tokens::mono(11.0), - Color32::from_rgb(0xe8, 0x7c, 0x5c), + t.expr_text, ); } // Keyframes. diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index febd1991..953bf66d 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -81,6 +81,20 @@ pub struct Tokens { pub timecode: Color32, pub danger: Color32, pub warning: Color32, + /// Effect Controls effect headers, and the header of the effect being worked on (it or one + /// of its properties selected). + pub fx_header: Color32, + pub fx_header_active: Color32, + /// Timeline: a selected layer's row in the time graph, the boxes behind the A/V and layer + /// switches, the time navigator and the work area bar. + pub tl_graph_selected: Color32, + pub switch_well: Color32, + pub tl_nav_bar: Color32, + pub tl_work_area_bar: Color32, + /// The Timeline's expression field and the colour of enabled expressions (their text and + /// the names of properties that have one). + pub expr_bg: Color32, + pub expr_text: Color32, /// Label colours (`Label::ALL` order), from Settings ▸ Labels. pub labels: [Color32; 17], pub radius: f32, @@ -153,6 +167,14 @@ impl Tokens { timecode: Color32::from_rgb(0x3d, 0x8f, 0xf5), danger: Color32::from_rgb(0xe0, 0x4a, 0x3c), warning: Color32::from_rgb(0xe8, 0x9a, 0x2c), + fx_header: Color32::from_rgb(0x2a, 0x2a, 0x2a), + fx_header_active: Color32::from_rgb(0x2f, 0x3a, 0x52), + tl_graph_selected: Color32::from_rgb(0x2a, 0x2a, 0x2a), + switch_well: Color32::from_rgb(0x19, 0x19, 0x19), + tl_nav_bar: Color32::from_rgb(0x55, 0x55, 0x55), + tl_work_area_bar: Color32::from_rgb(0x5c, 0x5c, 0x5c), + expr_bg: Color32::from_rgb(0x1a, 0x1a, 0x1a), + expr_text: Color32::from_rgb(0xe8, 0x7c, 0x5c), labels: default_labels(), radius: 6.0, radius_sm: 3.0, @@ -202,6 +224,16 @@ impl Tokens { pasteboard: Color32::from_rgb(0xa8, 0xa8, 0xa8), // Keyframes have no outline: dark enough to stand out on the light time graph. keyframe: Color32::from_rgb(0x6c, 0x6c, 0x6c), + // Custom-painted surfaces in the Light palette, so dark text stays readable on + // them (#644). + fx_header: Color32::from_rgb(0xd8, 0xd8, 0xd8), + fx_header_active: Color32::from_rgb(0xb0, 0xcf, 0xec), + tl_graph_selected: Color32::from_rgb(0xc2, 0xc2, 0xc2), + switch_well: Color32::from_rgb(0xf2, 0xf2, 0xf2), + tl_nav_bar: Color32::from_rgb(0x9a, 0x9a, 0x9a), + tl_work_area_bar: Color32::from_rgb(0x94, 0x94, 0x94), + expr_bg: Color32::from_rgb(0xf2, 0xf2, 0xf2), + expr_text: Color32::from_rgb(0xa3, 0x34, 0x17), ..dark }, } @@ -268,6 +300,10 @@ impl Tokens { &mut self.tl_ruler_bg, &mut self.pasteboard, &mut self.work_area, + &mut self.fx_header, + &mut self.tl_graph_selected, + &mut self.switch_well, + &mut self.expr_bg, ] { adj(c); }