diff --git a/crates/ui-egui/src/panels/comments_panel.rs b/crates/ui-egui/src/panels/comments_panel.rs index c549ed8e3e..d54daf5717 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -241,6 +241,10 @@ pub(crate) fn show( } let mut page = usize::MAX; let mut group: Option = None; + // A card (or page header) out of view that was measured at this width only takes up its + // height: laying out hundreds of them every frame put the panel over the frame budget + // (#5488). Shorter lists are laid out whole, so every card stays in the accessibility tree. + let virtual_list = roots.len() > LAY_OUT_WHOLE; for a in roots { if sort != SortBy::Page { let g = group_of(a); @@ -252,48 +256,118 @@ pub(crate) fn show( } } else if a.page != page { page = a.page; - let n = page_counts.get(&page).copied().unwrap_or_default(); - let open = !view.comments.collapsed_pages.contains(&page); - ui.add_space(6.0); - let clicked = ui - .horizontal(|ui| { - let chevron = if open { "chevron-down" } else { "chevron-right" }; - let toggle = icons::button(ui, chevron, 18.0, false, if open { tl!("Collapse") } else { tl!("Expand") }).clicked(); - let label = ui.add( - egui::Label::new( - egui::RichText::new(crate::i18n::fmt(tl!("Page {label}"), &[("label", &info.pages[page].label)])) - .font(theme::semibold(12.5)) - .color(t.text_muted), - ) - .sense(Sense::click()), - ); - ui.with_layout(Layout::right_to_left(Align::Center), |ui| { - ui.label(egui::RichText::new(n.to_string()).color(t.text_faint).small()) - }); - toggle || label.clicked() - }) - .inner; - if clicked { - if open { - view.comments.collapsed_pages.push(page); - } else { - view.comments.collapsed_pages.retain(|&c| c != page); + // Per document: two documents shown at the same width don't share heights. + let header = ui.scope_id().with(("comment-page-header", view.id, page)); + if !(virtual_list && skip_if_hidden(ui, header)) { + let top = ui.cursor().min.y; + page_header(ui, t, info, page, page_counts.get(&page).copied().unwrap_or_default(), &mut view.comments.collapsed_pages); + if virtual_list { + remember_height(ui, header, top); } } - ui.add_space(2.0); } if sort == SortBy::Page && view.comments.collapsed_pages.contains(&a.page) { continue; } + let key = (a.page, a.index); let thread = replies_of(a); + let selected = view.comments.selected == Some(key); + // A height is kept for what the card showed when it was measured: its document, what the + // thread says, whether it was the selected card (which draws its menu and editor) and + // whether an editor was open on it. An + // edit, undo or redo that changes the card measures it afresh; other cards keep theirs. + // A card with an editor open on it or on a reply is always drawn too (CodeRabbit on + // #5583): a kept height would show it at its size before the editor opened. + let editing = view.comments.editing.as_ref().is_some_and(|(p, i, _)| (*p, *i) == key || thread.iter().any(|r| (r.page, r.index) == (*p, *i))); + let id = ui.scope_id().with(("comment-card", view.id, key, card_signature(a, thread), selected, editing)); + // The selected card is always drawn, so it can scroll itself into view and keep its menu + // and editor. + if virtual_list && !selected && !editing && skip_if_hidden(ui, id) { + continue; + } let rotation = info.pages.get(a.page).map_or(0, |p| p.rotation); + let top = ui.cursor().min.y; if let Some(e) = card(ui, view, CardContext { theme: t, annotation: a, rotation, thread, doc, prefs, allowed, nav }) { edit = Some(e); } + if virtual_list { + remember_height(ui, id, top); + } } edit } +/// The "Page N" group header with its fold chevron and comment count `n`; a click folds or +/// unfolds the page. +fn page_header(ui: &mut egui::Ui, t: &Tokens, info: &DocInfo, page: usize, n: usize, collapsed: &mut Vec) { + let open = !collapsed.contains(&page); + ui.add_space(6.0); + let clicked = ui + .horizontal(|ui| { + let chevron = if open { "chevron-down" } else { "chevron-right" }; + let toggle = icons::button(ui, chevron, 18.0, false, if open { tl!("Collapse") } else { tl!("Expand") }).clicked(); + let label = ui.add( + egui::Label::new( + egui::RichText::new(crate::i18n::fmt(tl!("Page {label}"), &[("label", info.pages.get(page).map_or("?", |p| p.label.as_str()))])) + .font(theme::semibold(12.5)) + .color(t.text_muted), + ) + .sense(Sense::click()), + ); + ui.with_layout(Layout::right_to_left(Align::Center), |ui| ui.label(egui::RichText::new(n.to_string()).color(t.text_faint).small())); + toggle || label.clicked() + }) + .inner; + ui.add_space(2.0); + if clicked { + if open { + collapsed.push(page); + } else { + collapsed.retain(|&c| c != page); + } + } +} + +/// What a card's height depends on in the document: the comment and its replies as shown (their +/// kind, author, text, date, status, colour and lock). Two cards with the same signature lay out +/// alike at one width. +fn card_signature(a: &Annotation, thread: &[&Annotation]) -> u64 { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + for x in std::iter::once(a).chain(thread.iter().copied()) { + (&x.subtype, &x.author, &x.contents, &x.modified, &x.state, x.locked).hash(&mut h); + x.color.map(|c| c.map(f32::to_bits)).hash(&mut h); + } + h.finish() +} + +/// Comment lists up to this long are laid out whole; longer ones lay out only what is in view. +const LAY_OUT_WHOLE: usize = 100; + +/// For a part of the list (a card or a page header) that was measured at this width and would +/// fall out of view: takes up its height without laying it out, and returns `true`. +fn skip_if_hidden(ui: &mut egui::Ui, id: egui::Id) -> bool { + let width = ui.available_width(); + let Some(height) = ui.data(|d| d.get_temp::<(f32, f32)>(id)).filter(|(w, _)| (w - width).abs() < 0.5).map(|(_, h)| h) else { + return false; + }; + if ui.is_rect_visible(egui::Rect::from_min_size(ui.cursor().min, vec2(width, height))) { + return false; + } + ui.allocate_space(vec2(width, height)); + true +} + +/// Records the height part `id` took since the cursor was at `top`, for [`skip_if_hidden`]: less +/// the spacing `allocate_space` adds back after it. +fn remember_height(ui: &egui::Ui, id: egui::Id, top: f32) { + let height = ui.cursor().min.y - top - ui.spacing().item_spacing.y; + if height.is_finite() && height > 0.0 { + let width = ui.available_width(); + ui.data_mut(|d| d.insert_temp(id, (width, height))); + } +} + /// Draw one comment thread card, including author, body or inline editor, replies, status and per-comment /// actions; returns an edit to apply. struct CardContext<'a> { diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 664f61733e..057663a76c 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -3022,3 +3022,73 @@ fn an_upright_comment_on_a_turned_page_is_drawn_where_it_goes_before_release() { h.drop_at(from + delta); h.run_steps(3); } + +/// `n` pages with one comment each, "Card 1" to "Card n". +fn one_comment_per_page(n: usize) -> Vec { + let mut objs = vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string()]; + let kids: Vec = (0..n).map(|i| format!("{} 0 R", 3 + 2 * i)).collect(); + objs.push(format!("<< /Type /Pages /Kids [{}] /Count {n} /MediaBox [0 0 300 200] >>", kids.join(" "))); + for i in 0..n { + objs.push(format!("<< /Type /Page /Parent 2 0 R /Annots [{} 0 R] >>", 4 + 2 * i)); + objs.push(format!("<< /Type /Annot /Subtype /Square /Rect [20 20 80 80] /C [1 0 0] /T (Ada) /Contents (Card {}) >>", i + 1)); + } + let mut out = b"%PDF-1.7\n".to_vec(); + for (i, o) in objs.iter().enumerate() { + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + out.extend_from_slice(b"trailer << /Root 1 0 R >>\n%%EOF\n"); + out +} + +/// #5488: the panel laid out every card on every frame, which put a few hundred comments over the +/// frame budget. Cards out of view now only take up their height, and the selected one is still +/// drawn wherever it is, so it can scroll into view. +#[test] +fn the_comments_panel_lays_out_only_the_cards_in_view() { + let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap()); + h.run_steps(4); + let drawn = h.query_all_by_label_contains("Card ").count(); + assert!((1..40).contains(&drawn), "{drawn} of 120 cards laid out"); + h.state_mut().views[0].comments.selected = Some((100, 0)); + h.state_mut().views[0].comments.reveal = true; + h.run_steps(4); + assert!(h.query_by_label("Card 101").is_some(), "the selected card is drawn and scrolled to"); + assert!(h.query_by_label("Card 1").is_none(), "the top of the list scrolled out of view"); +} + +/// CodeRabbit on #5583: a card's measured height belongs to what it showed. Card 1, far above the +/// cards in view, grows by two replies; the cards in view move down by its growth, not stay where +/// card 1's old height would put them. +#[test] +fn a_card_out_of_view_that_grows_moves_the_cards_below_it() { + let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap()); + h.run_steps(4); + h.state_mut().views[0].comments.selected = Some((60, 0)); + h.state_mut().views[0].comments.reveal = true; + h.run_steps(4); + assert!(h.query_by_label("Card 1").is_none(), "card 1 is out of view"); + let before = h.get_by_label("Card 61").rect().top(); + for text in ["A first reply", "A second reply"] { + h.state_mut().apply_edit(pdfcraft_engine::Edit::ReplyToAnnotation { page: 0, index: 0, text: text.into(), author: "Bo".into() }); + } + h.run_steps(4); + let after = h.get_by_label("Card 61").rect().top(); + assert!(after > before + 40.0, "the cards below a grown card move down: {before} → {after}"); +} + +/// CodeRabbit on #5583: a card with an editor open on it is drawn wherever it is, so the cards +/// below it make room for the editor rather than keep the card's height from before it opened. +#[test] +fn a_card_with_an_editor_open_out_of_view_is_drawn_at_its_size() { + let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap()); + h.run_steps(4); + h.state_mut().views[0].comments.selected = Some((60, 0)); + h.state_mut().views[0].comments.reveal = true; + h.run_steps(4); + assert!(h.query_by_label("Card 1").is_none(), "card 1 is out of view"); + let before = h.get_by_label("Card 61").rect().top(); + h.state_mut().views[0].comments.editing = Some((0, 0, "Card 1".into())); + h.run_steps(4); + let after = h.get_by_label("Card 61").rect().top(); + assert!(after > before + 20.0, "the cards below make room for card 1's editor: {before} → {after}"); +}