From f8daaf282bd5d5c80c03b7712b5f8a554df4fe2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 25 Sep 2026 20:58:19 -0400 Subject: [PATCH] T12 review fixes: upload shape and formats, accent dot, test gating, value wrap, gallery clamp, Code 128 set switches, float sums, dense QR --- TASKS.md | 8 +++--- src/data_display/avatar.rs | 5 ++++ src/data_display/badge.rs | 5 +++- src/data_display/codes.rs | 53 +++++++++++++++++++++++++++++++++--- src/data_display/gallery.rs | 28 ++++++++++++++++++- src/data_display/measures.rs | 32 ++++++++++++++++------ src/data_display/mod.rs | 2 +- src/data_display/records.rs | 2 +- src/forms/avatar.rs | 22 +++++++++------ src/lists/mod.rs | 2 +- src/lists/tree/mod.rs | 2 +- src/tables/mod.rs | 2 +- 12 files changed, 131 insertions(+), 32 deletions(-) diff --git a/TASKS.md b/TASKS.md index 0880090..8a34707 100644 --- a/TASKS.md +++ b/TASKS.md @@ -186,10 +186,10 @@ Per-component lines live in `tasks/`. Tags there: | T11d | 1 | FAIL | the ripple drew inverted columns in a cut corner, motes crossed rounded corners, a narrow marquee jumped, confetti burst on a falling count, and the Lottie note misread velato | | T11d | 2 | FAIL | Marquee's doc still said its content is drawn twice | | T11d | 3 | PASS | | -| T12a | 1 | — | pending: codex answered 401 on every request; committed unreviewed, reviewed from its commit once codex logs in | -| T12b | 1 | — | pending: codex still logged out | -| T12c | 1 | — | pending: codex still logged out | -| T12d | 1 | — | pending: codex still logged out | +| T12a | 1 | FAIL | the avatar upload took HEIC, which gpui cannot decode, an accent badge's dot matched its fill, and a square avatar's upload veil stayed round | +| T12b | 1 | FAIL | the gpui test module compiled without test-support, and a long description value overflowed a narrow column | +| T12c | 1 | FAIL | the gallery kept an open index past a shrunk list, and the lightbox asserted on it | +| T12d | 1 | FAIL | Code 128 took a set-switch character, a usage bar's float sum tripped its assert, and a dense QR floored its modules to nothing | | T13a | 1 | — | pending: codex still logged out | | T13b | 1 | — | pending: codex still logged out | | T13c | 1 | — | pending: codex still logged out | diff --git a/src/data_display/avatar.rs b/src/data_display/avatar.rs index 91960c8..4c71cfe 100644 --- a/src/data_display/avatar.rs +++ b/src/data_display/avatar.rs @@ -92,6 +92,11 @@ impl Avatar { self } + /// Whether it is a rounded square rather than a circle. + pub(crate) fn is_square(&self) -> bool { + self.square + } + /// A rounded square, as teams and apps take. pub fn square(mut self) -> Self { self.square = true; diff --git a/src/data_display/badge.rs b/src/data_display/badge.rs index c1dcd22..8312587 100644 --- a/src/data_display/badge.rs +++ b/src/data_display/badge.rs @@ -102,7 +102,10 @@ impl RenderOnce for Badge { fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement { let theme = cx.theme(); let (fill, text) = self.tone.colors(&theme.colors); - let dot = self.tone.dot(&theme.colors); + let dot = match self.tone { + Tone::Accent => theme.colors.on_accent, + tone => tone.dot(&theme.colors), + }; div() .flex() .flex_none() diff --git a/src/data_display/codes.rs b/src/data_display/codes.rs index 5f9171b..ecacf38 100644 --- a/src/data_display/codes.rs +++ b/src/data_display/codes.rs @@ -1,3 +1,5 @@ +use std::sync::atomic::{AtomicBool, Ordering}; + use barcoders::sym::{code128::Code128, ean13::EAN13}; use gpui::{ App, Bounds, Div, IntoElement, ParentElement, Pixels, Refineable, RenderOnce, SharedString, @@ -9,9 +11,24 @@ pub use qrcode::types::QrError; use crate::theme::{ActiveTheme, Radius, TextSize}; +/// Whether a QR code has already said its tile is too small to draw it sharply. +static SMALL: AtomicBool = AtomicBool::new(false); + /// The quiet margin around a QR code, in modules, as the standard asks. const QUIET: usize = 4; +/// A module's side: whole pixels for crisp edges, or the exact share when whole pixels would floor to nothing. +fn module(side: Pixels, span: f32) -> Pixels { + let fit = side / span; + if f32::from(fit) >= 1.0 { + return fit.floor(); + } + if !SMALL.swap(true, Ordering::Relaxed) { + log::warn!("qr code: {span} modules share {fit:?} each; give the tile more room"); + } + fit +} + /// The dark runs in a row of modules, as start and length. fn runs(row: &[bool]) -> Vec<(usize, usize)> { let mut runs = Vec::new(); @@ -82,9 +99,16 @@ impl RenderOnce for QrCode { let theme = cx.theme(); let (paper, ink) = (theme.colors.paper, theme.colors.ink); let rows = self.rows; + let span = (rows.len() + QUIET * 2) as f32; + let need = theme.barcode_module() * span; + let side = if need.0 > theme.qr_code().0 { + need + } else { + theme.qr_code() + }; let mut tile = div() .flex_none() - .size(theme.qr_code()) + .size(side) .rounded(theme.radius(Radius::Sm)) .bg(paper); tile.style().refine(self.base.style()); @@ -92,8 +116,7 @@ impl RenderOnce for QrCode { canvas( |_, _, _| {}, move |bounds, _, window, _| { - let span = (rows.len() + QUIET * 2) as f32; - let module = (bounds.size.width.min(bounds.size.height) / span).floor(); + let module = module(bounds.size.width.min(bounds.size.height), span); let inset = (rows.len() as f32 * module) / 2.0; let origin = bounds.center() - point(inset, inset); paint_runs(&rows, origin, module, module, ink, window); @@ -139,6 +162,9 @@ impl Barcode { /// Code 128 in its set B: letters, digits and printable ASCII marks. pub fn code128(text: &str) -> Result { + if !text.chars().all(|letter| (' '..='~').contains(&letter)) { + return Err(BarcodeError::Character); + } let modules = Code128::new(format!("\u{0181}{text}"))?.encode(); Ok(Self::new(modules, text.to_string())) } @@ -207,7 +233,9 @@ impl RenderOnce for Barcode { #[cfg(test)] mod tests { - use super::{Barcode, BarcodeError, check_digit, runs}; + use gpui::px; + + use super::{Barcode, BarcodeError, check_digit, module, runs}; #[test] fn dark_modules_merge_into_runs() { @@ -232,6 +260,23 @@ mod tests { Barcode::code128("caf\u{e9}"), Err(BarcodeError::Character) )); + assert!( + matches!(Barcode::code128("\u{c0}123"), Err(BarcodeError::Character)), + "barcoders reads \u{c0} as a switch to set A" + ); + } + + #[test] + fn a_dense_qr_keeps_modules_it_can_draw() { + assert_eq!( + module(px(254.0), 25.0), + px(10.0), + "whole pixels when they fit" + ); + assert!( + module(px(100.0), 185.0) > px(0.0), + "a share under a pixel stays drawn" + ); } #[test] diff --git a/src/data_display/gallery.rs b/src/data_display/gallery.rs index e8d2512..e032bd7 100644 --- a/src/data_display/gallery.rs +++ b/src/data_display/gallery.rs @@ -9,6 +9,11 @@ use crate::{ primitives::{FocusRing, Image, tab_stop}, }; +/// The open picture if it is still there, the last when it went past the end, none when all are gone. +fn clamp(open: Option, count: usize) -> Option { + open.and_then(|at| count.checked_sub(1).map(|last| at.min(last))) +} + /// Pictures in square tiles. A press, or Enter on a focused tile, opens the lightbox at that picture. #[derive(IntoElement)] pub struct Gallery { @@ -87,7 +92,15 @@ impl RenderOnce for Gallery { ), ); } - let lightbox = (*open.read(cx)).map(|at| { + let at = clamp(*open.read(cx), self.slides.len()); + if at != *open.read(cx) { + log::info!( + "gallery {:?}: pictures went away; open is now {at:?}", + self.id + ); + open.update(cx, |open, _| *open = at); + } + let lightbox = at.map(|at| { let (close, step) = (show(None), open.clone()); Lightbox::new( (self.id.clone(), "lightbox"), @@ -110,3 +123,16 @@ impl RenderOnce for Gallery { .children(lightbox) } } + +#[cfg(test)] +mod tests { + use super::clamp; + + #[test] + fn an_open_picture_clamps_when_pictures_go_away() { + assert_eq!(clamp(Some(1), 3), Some(1)); + assert_eq!(clamp(Some(2), 1), Some(0), "past the end opens the last"); + assert_eq!(clamp(Some(0), 0), None, "no pictures closes it"); + assert_eq!(clamp(None, 3), None); + } +} diff --git a/src/data_display/measures.rs b/src/data_display/measures.rs index 7473d05..34073b7 100644 --- a/src/data_display/measures.rs +++ b/src/data_display/measures.rs @@ -309,16 +309,24 @@ impl UsageBar { } } +impl UsageBar { + /// What the parts use and whether they fill the total, within float error. + fn used(&self) -> (f64, bool) { + let used: f64 = self.parts.iter().map(|(_, amount)| amount).sum(); + assert!( + used <= self.total * (1.0 + 1e-9), + "the parts ({used}) pass the total ({})", + self.total + ); + (used, used >= self.total * (1.0 - 1e-9)) + } +} + impl RenderOnce for UsageBar { fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement { let theme = cx.theme(); let colors = &theme.colors; - let used: f64 = self.parts.iter().map(|(_, amount)| amount).sum(); - assert!( - used <= self.total, - "the parts ({used}) pass the total ({})", - self.total - ); + let (used, full) = self.used(); assert!( self.parts.len() <= colors.chart.len(), "a usage bar has colors for {} parts", @@ -339,9 +347,9 @@ impl RenderOnce for UsageBar { grown(amount / total) .bg(colors.chart[ix]) .when(ix == 0, |part| part.rounded_l_full()) - .when(ix == last && used == total, |part| part.rounded_r_full()) + .when(ix == last && full, |part| part.rounded_r_full()) })) - .when(used < total, |bar| { + .when(!full, |bar| { bar.child( grown((total - used) / total) .bg(colors.border) @@ -382,7 +390,13 @@ impl RenderOnce for UsageBar { #[cfg(test)] mod tests { - use super::Level; + use super::{Level, UsageBar}; + + #[test] + fn parts_that_sum_to_the_total_fill_it_despite_float_error() { + assert!(UsageBar::new(0.3).part("a", 0.1).part("b", 0.2).used().1); + assert!(!UsageBar::new(1.0).part("a", 0.5).used().1); + } #[test] fn a_share_turns_amber_then_red() { diff --git a/src/data_display/mod.rs b/src/data_display/mod.rs index 676b6e1..edb1543 100644 --- a/src/data_display/mod.rs +++ b/src/data_display/mod.rs @@ -12,7 +12,7 @@ mod records; mod spark; mod stars; mod stat; -#[cfg(test)] +#[cfg(all(test, feature = "test-support"))] mod tests; mod timeline; mod watermark; diff --git a/src/data_display/records.rs b/src/data_display/records.rs index 41f0b7b..e39ad43 100644 --- a/src/data_display/records.rs +++ b/src/data_display/records.rs @@ -75,7 +75,7 @@ impl RenderOnce for DescriptionList { .flex_1() .min_w_0() .text_color(colors.fg) - .child(value), + .child(div().min_w_0().child(value)), ) }), ) diff --git a/src/forms/avatar.rs b/src/forms/avatar.rs index f5112f0..3df6bba 100644 --- a/src/forms/avatar.rs +++ b/src/forms/avatar.rs @@ -9,11 +9,11 @@ use super::{files::hovering, path::choose}; use crate::{ data_display::Avatar, primitives::{FocusRing, Icon, IconName, tab_stop}, - theme::{ActiveTheme, IconSize}, + theme::{ActiveTheme, IconSize, Radius}, }; /// File kinds an avatar takes. -const PICTURES: [&str; 6] = ["png", "jpg", "jpeg", "gif", "webp", "heic"]; +const PICTURES: [&str; 5] = ["png", "jpg", "jpeg", "gif", "webp"]; /// The one picture a drop or a pick brings, or why it is refused. pub(crate) fn picture(paths: &[PathBuf]) -> Result { @@ -32,7 +32,7 @@ pub(crate) fn picture(paths: &[PathBuf]) -> Result { .and_then(|kind| kind.to_str()) .map(str::to_lowercase); if !kind.is_some_and(|kind| PICTURES.contains(&kind.as_str())) { - return Err("pictures only: png, jpg, gif, webp or heic"); + return Err("pictures only: png, jpg, gif or webp"); } Ok(path.clone()) } @@ -76,14 +76,21 @@ impl RenderOnce for AvatarUpload { }) }; let theme = cx.theme(); + let corner = |element: gpui::Div, square: bool| { + if square { + element.rounded(theme.radius(Radius::Md)) + } else { + element.rounded_full() + } + }; + let square = self.avatar.is_square(); let group = SharedString::from(format!("{:?}", self.id)); let (pick, drop_in, what_drop) = (take.clone(), take, what.clone()); - div() + corner(div(), square) .id(self.id) .group(group.clone()) .relative() .flex_none() - .rounded_full() .border_1() .border_color(gpui::transparent_black()) .cursor_pointer() @@ -91,13 +98,12 @@ impl RenderOnce for AvatarUpload { .focus_ring(cx) .child(self.avatar) .child( - div() + corner(div(), square) .absolute() .inset_0() .flex() .items_center() .justify_center() - .rounded_full() .bg(theme.colors.media_backdrop.opacity(0.5)) .opacity(0.0) .group_hover(group, |style| style.opacity(1.0)) @@ -154,7 +160,7 @@ mod tests { assert_eq!(picture(std::slice::from_ref(&face)), Ok(face.clone())); assert_eq!( picture(&[notes]), - Err("pictures only: png, jpg, gif, webp or heic") + Err("pictures only: png, jpg, gif or webp") ); assert_eq!(picture(&[face.clone(), face]), Err("one picture at a time")); assert_eq!(picture(&[dir]), Err("files only, not folders")); diff --git a/src/lists/mod.rs b/src/lists/mod.rs index 20ea0bb..8d8a304 100644 --- a/src/lists/mod.rs +++ b/src/lists/mod.rs @@ -7,7 +7,7 @@ mod outline; mod select; mod sortable; mod swipe; -#[cfg(test)] +#[cfg(all(test, feature = "test-support"))] mod tests; mod tree; diff --git a/src/lists/tree/mod.rs b/src/lists/tree/mod.rs index 2e903a7..6519a3a 100644 --- a/src/lists/tree/mod.rs +++ b/src/lists/tree/mod.rs @@ -1,7 +1,7 @@ mod model; mod row; mod select; -#[cfg(test)] +#[cfg(all(test, feature = "test-support"))] mod tests; use std::{collections::HashSet, rc::Rc}; diff --git a/src/tables/mod.rs b/src/tables/mod.rs index e625521..4130fca 100644 --- a/src/tables/mod.rs +++ b/src/tables/mod.rs @@ -12,7 +12,7 @@ mod rules; mod sheets; mod simple; mod table; -#[cfg(test)] +#[cfg(all(test, feature = "test-support"))] mod tests; mod toolbar; mod tree_table;