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

@@ -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 |

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;

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()

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<Self, BarcodeError> {
+        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]

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<usize>, count: usize) -> Option<usize> {
+    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);
+    }
+}

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() {

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;

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)),
                             )
                     }),
             )

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<PathBuf, &'static str> {
@@ -32,7 +32,7 @@ pub(crate) fn picture(paths: &[PathBuf]) -> Result<PathBuf, &'static str> {
         .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"));

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;
 

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};

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;