Merge pull request #261 from AlissonFA/fix/cell-vertical-alignment

M2: fix cell vertical alignment positioning and add keytips (#241)

crates/engine/src/tests.rs

@@ -1982,6 +1982,40 @@ fn notes_comments_and_links_survive_a_json_save() {
     assert_eq!(after.cells, before.cells);
 }
 
+#[test]
+fn vertical_alignment_commands_and_roundtrip() {
+    let mut s = s();
+    s.execute("cell.set", json!({"cell": "A1", "input": "top"})).unwrap();
+    s.execute("cell.set", json!({"cell": "A2", "input": "middle"})).unwrap();
+    s.execute("cell.set", json!({"cell": "A3", "input": "bottom"})).unwrap();
+
+    s.execute("selection.set", json!({"range": "A1"})).unwrap();
+    s.execute("home.alignTop", json!({})).unwrap();
+
+    s.execute("selection.set", json!({"range": "A2"})).unwrap();
+    s.execute("home.alignMiddle", json!({})).unwrap();
+
+    s.execute("selection.set", json!({"range": "A3"})).unwrap();
+    s.execute("home.alignBottom", json!({})).unwrap();
+
+    let style_of = |s: &Session, cell: &str| {
+        let d = s.doc().unwrap();
+        d.wb.styles.get(d.wb.active().unwrap().style_id(CellRef::parse(cell).unwrap())).clone()
+    };
+
+    assert_eq!(style_of(&s, "A1").align.v, gridcraft_model::VAlign::Top);
+    assert_eq!(style_of(&s, "A2").align.v, gridcraft_model::VAlign::Center);
+    assert_eq!(style_of(&s, "A3").align.v, gridcraft_model::VAlign::Bottom);
+
+    let d = s.doc().unwrap();
+    let bytes = gridcraft_xlsx::write_xlsx(&d.wb).unwrap();
+    let (reopened, _) = gridcraft_xlsx::read_xlsx(&bytes).unwrap();
+    let reopen_style = |cell: &str| reopened.styles.get(reopened.active().unwrap().style_id(CellRef::parse(cell).unwrap())).clone();
+    assert_eq!(reopen_style("A1").align.v, gridcraft_model::VAlign::Top);
+    assert_eq!(reopen_style("A2").align.v, gridcraft_model::VAlign::Center);
+    assert_eq!(reopen_style("A3").align.v, gridcraft_model::VAlign::Bottom);
+}
+
 /// Icon-set thresholds as read from a file (one per icon, the first the lower bound) decide the
 /// icon; a rule without thresholds uses Excel's defaults (percent 33 and 67) (#235).
 #[test]

crates/ui-egui/src/grid.rs

@@ -554,6 +554,28 @@ fn vertical_scrollbar(app: &mut SheetApp, ui: &mut egui::Ui, sh: &Sheet, geo: &G
     }
 }
 
+pub(crate) fn wrapped_cell_text_y(valign: VAlign, rect: Rect, gh: f32, z: f32) -> f32 {
+    let pad_v = 1.5 * z;
+    let y_top = rect.top() + pad_v;
+    let y_bottom = (rect.bottom() - gh - pad_v).max(y_top);
+    match valign {
+        VAlign::Top => y_top,
+        VAlign::Center | VAlign::Justify | VAlign::Distributed => (y_top + y_bottom) / 2.0,
+        VAlign::Bottom => y_bottom,
+    }
+}
+
+pub(crate) fn cell_text_y(valign: VAlign, rect: Rect, gh: f32, z: f32) -> f32 {
+    let top_inset = 1.0 * z;
+    let y_top = rect.top() - top_inset;
+    let y_bottom = (rect.bottom() - gh - 0.5 * z).max(y_top);
+    match valign {
+        VAlign::Top => y_top,
+        VAlign::Center | VAlign::Justify | VAlign::Distributed => (y_top + y_bottom) / 2.0,
+        VAlign::Bottom => y_bottom,
+    }
+}
+
 /// Mirror what the grid painted into `layer` (the shape indices in `range`) for a right-to-left
 /// sheet. Geometry is reflected; text and meshes (glyphs, pictures) keep their reading direction
 /// and only move so that their box lands mirrored. Floating objects (`objects`: shape index
@@ -768,11 +790,7 @@ fn paint_quadrant(p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet,
                 HAlign::Right => rect.right() - pad,
                 _ => text_left + pad + indent,
             };
-            let y = match st.align.v {
-                VAlign::Top => rect.top() + 1.0,
-                VAlign::Center => rect.center().y - galley.size().y / 2.0,
-                _ => rect.bottom() - galley.size().y - 1.0,
-            };
+            let y = wrapped_cell_text_y(st.align.v, *rect, galley.size().y, z);
             p.with_clip_rect(rect.intersect(p.clip_rect())).galley(pos2(x, y), galley, color);
             continue;
         }
@@ -838,11 +856,7 @@ fn paint_quadrant(p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet,
             _ => text_left + pad + indent,
         };
         let gh = galley.size().y;
-        let y = match st.align.v {
-            VAlign::Top => rect.top() + 1.0,
-            VAlign::Center | VAlign::Justify | VAlign::Distributed => rect.center().y - gh / 2.0,
-            VAlign::Bottom => rect.bottom() - gh - 0.5 * z,
-        };
+        let y = cell_text_y(st.align.v, *rect, gh, z);
         let cp = p.with_clip_rect(clip.intersect(p.clip_rect()));
         if rotated {
             let angle = -(st.align.rotation as f32).to_radians();
@@ -2568,6 +2582,53 @@ fn paint_pages(app: &mut SheetApp, p: &Painter, geo: &Geo, sh: &Sheet) {
     p.rect_stroke(pr, 0.0, Stroke::new(3.0, blue), StrokeKind::Outside);
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn cell_text_y_moves_monotonically_in_standard_and_tall_rows() {
+        let rect_std = Rect::from_min_max(pos2(0.0, 0.0), pos2(100.0, 20.0));
+        let gh = 18.5;
+        let z = 1.0;
+
+        let y_top_std = cell_text_y(VAlign::Top, rect_std, gh, z);
+        let y_center_std = cell_text_y(VAlign::Center, rect_std, gh, z);
+        let y_bottom_std = cell_text_y(VAlign::Bottom, rect_std, gh, z);
+
+        assert!(y_top_std < y_center_std, "Top ({y_top_std}) must be higher than Center ({y_center_std})");
+        assert!(y_center_std < y_bottom_std, "Center ({y_center_std}) must be higher than Bottom ({y_bottom_std})");
+
+        // Verify tall row (e.g. 40px)
+        let rect_tall = Rect::from_min_max(pos2(0.0, 0.0), pos2(100.0, 40.0));
+        let y_top_tall = cell_text_y(VAlign::Top, rect_tall, gh, z);
+        let y_center_tall = cell_text_y(VAlign::Center, rect_tall, gh, z);
+        let y_bottom_tall = cell_text_y(VAlign::Bottom, rect_tall, gh, z);
+
+        assert!(y_top_tall < y_center_tall, "Top ({y_top_tall}) must be higher than Center ({y_center_tall})");
+        assert!(y_center_tall < y_bottom_tall, "Center ({y_center_tall}) must be higher than Bottom ({y_bottom_tall})");
+    }
+
+    #[test]
+    fn wrapped_cell_text_y_positions_correctly_and_safeguards_overflow() {
+        let rect = Rect::from_min_max(pos2(0.0, 0.0), pos2(100.0, 40.0));
+        let gh = 30.0;
+        let z = 1.0;
+
+        let y_top = wrapped_cell_text_y(VAlign::Top, rect, gh, z);
+        let y_center = wrapped_cell_text_y(VAlign::Center, rect, gh, z);
+        let y_bottom = wrapped_cell_text_y(VAlign::Bottom, rect, gh, z);
+
+        assert!(y_top < y_center);
+        assert!(y_center < y_bottom);
+
+        // Overflow case: gh exceeds rect height
+        let gh_overflow = 60.0;
+        let y_bottom_ovf = wrapped_cell_text_y(VAlign::Bottom, rect, gh_overflow, z);
+        assert!(y_bottom_ovf >= rect.top());
+    }
+}
+
 #[cfg(test)]
 mod pane_tests {
     use super::*;

crates/ui-egui/src/keytips.rs

@@ -101,6 +101,9 @@ impl KeyTips {
                                 "HAR" => self.finish(Action::Command("home.alignRight"), &mut actions),
                                 "HAL" => self.finish(Action::Command("home.alignLeft"), &mut actions),
                                 "HAC" => self.finish(Action::Command("home.alignCenter"), &mut actions),
+                                "HAT" => self.finish(Action::Command("home.alignTop"), &mut actions),
+                                "HAM" => self.finish(Action::Command("home.alignMiddle"), &mut actions),
+                                "HAB" => self.finish(Action::Command("home.alignBottom"), &mut actions),
                                 "HVS" | "ES" => self.finish(Action::Dialog("pasteSpecial"), &mut actions),
                                 "HOI" | "OCA" => self.finish(Action::Command("home.autofitColumnWidth"), &mut actions),
                                 _ => {}

crates/ui-egui/src/ribbon.rs

@@ -527,7 +527,16 @@ fn home(app: &mut SheetApp, ui: &mut Ui) {
                 (Icon::AlignMiddle, VAlign::Center, "home.alignMiddle", "Middle Align"),
                 (Icon::AlignBottom, VAlign::Bottom, "home.alignBottom", "Bottom Align"),
             ] {
-                if toggle_button(ui, icon, st.align.v == v, tip).clicked() {
+                let response = toggle_button(ui, icon, st.align.v == v, tip);
+                if matches!(app.keytips.prefix(), Some("H" | "HA")) {
+                    let sequence = match v {
+                        VAlign::Top => "HAT",
+                        VAlign::Center => "HAM",
+                        _ => "HAB",
+                    };
+                    keytip(app, ui, sequence, response.rect.center_bottom());
+                }
+                if response.clicked() {
                     act(app, id, json!({}));
                 }
             }

crates/ui-egui/tests/keytips.rs

@@ -318,3 +318,12 @@ fn ordinary_typing_editor_shortcuts_and_altgr_remain_text_input() {
     assert_eq!(h.state().keytips.prefix(), None, "macOS Option must remain available for text composition");
     assert_eq!(h.state().editor.as_ref().unwrap().text, "é");
 }
+
+#[test]
+fn vertical_alignment_keytips_run_their_commands() {
+    for (letter, id) in [(Key::T, "home.alignTop"), (Key::M, "home.alignMiddle"), (Key::B, "home.alignBottom")] {
+        let mut h = harness(blank());
+        sequence(&mut h, &[Key::H, Key::A, letter], false);
+        assert_eq!(h.state().session.journal.iter().filter(|(j, _)| j == id).count(), 1, "HA{} runs {id}", letter.name());
+    }
+}