M4: drag bookmark trees and insert after selection (#2120)

ROADMAP.md

@@ -157,6 +157,8 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
 | 8 | 1.0 polish: performance, right-to-left, missing languages, keyboard-only (M14) | 55–95 |
 
 ## Progress log
+
+- **2026-10-17 (PR maintenance):** Integrated upstream `924c4da4`, retaining current safety gates and feature-specific behavior. Estimates unchanged.
 - **2026-10-15 (M2, gap 12):** `cargo xtask text-oracle` scores reading order as well as word F1: the longest common subsequence of the two word sequences over the longer one. F1 is a bag of words and cannot see order at all -- measuring #2431, a change that moves text and nothing else, it called 26 of the 29 files whose output changed identical, where the order score separated 18 improvements from 6 regressions. Reports, does not gate; a disagreement with Poppler, not a verdict. Estimates unchanged.
 - **2026-10-15 (M2, #2305 follow-up):** WinAnsiEncoding 0xA0 and 0xAD and MacRomanEncoding 0xCA extract as the ordinary space and hyphen Annex D.2 names them, in the vendored hayro tables (patch 30) as well as `crates/fonts`, so search, copy and redaction see characters that were hidden behind U+00A0 and U+00AD. Across the pdf.js corpus the soft hyphens we extract fall 117 to 13 and the non-breaking spaces 30 to 26; the rest come from `/ToUnicode`. An explicit `/Differences` name keeps its glyph list value, as pdftotext does. Estimates unchanged.
 - **2026-10-13 (community, M3, #1569 review):** Pixel-only Wayland wheel, wheel-tilt and continuous input use standalone moved events, so Ctrl zoom does not remain latched after the modifier is released (#991); vendored winit patch (13), with a policy test. Estimates unchanged.
@@ -168,6 +170,8 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
 - **2026-10-15 (M14, gap 24):** `misc.localization` and `misc.installers` move from planned to partial. Localization: 22 catalogs ship, but Chinese shows tofu in releases, Telugu has no release font and Arabic is not mirrored. Installers: macOS and Windows releases are signed, Linux packages are not. The Acrobat menu diff itself remains open. Estimates unchanged.
 - **2026-10-13 (G14, Vietnamese localization):** Added Vietnamese (`vi`, Tiếng Việt) as an interface and date language with a 2,591-entry catalog (`plural_none`) covering 100% of the current localizable key union (plus `theme`/`Appearance`). Locale tags `vi` / `vi-VN` / `vi_VN.UTF-8` resolve; calendar months use numeric `Tháng 1`…`12` / `tháng 1`…`12`. Native-speaker community review recorded. The twelve-language localization mean moves from ≈59% to ≈68%; weighted readiness ≈49% → ≈50%. Status stays `partial`: engine/OS details, document contents, filenames, CLI/MCP and external help stay outside the catalog by design.
 
+- **2026-10-14 (M4, #2120 candidate):** Adds bookmark tree drag/drop and insertion after the document-local selection, reusing engine edits; synthetic pointer, cancellation, undo/redo and save/reopen regressions. Reordering is disabled for filtered or truncated sidebar trees; estimates unchanged.
+
 - **2026-10-12 (M6, XML field names):** AcroForm XML data export constrains generated element names to XML 1.0 NCNames, so Latin-1 names such as `Area²` and `Aµ` produce readable data files. Original PDF field names travel through the existing `xfdf:original` attribute; previously valid tags, including Greek and CJK, keep their spelling. Saved-PDF regressions cover invalid prefixes, combining letters and normalized-name collisions. Estimates unchanged.
 - **2026-10-12 (M6, CSV carriage returns):** Form-data CSV export quotes carriage returns in field names and values, so a trailing carriage return cannot turn into a record separator and disappear on reimport. Saved-PDF round-trip regressions cover CR, LF, CRLF, commas and quotes; estimates unchanged.
 - **2026-10-13 (community, M14, #1553 review):** Relative Fontconfig includes resolve beside each including config, including nested configuration selected through `FONTCONFIG_PATH`. Directory includes accept only digit-prefixed `*.conf` in lexical order. Isolated regressions reproduce both review findings; configuration size/count/depth limits, cycles, symlinked config and valid reset ordering are covered. Estimates unchanged.

crates/ui-egui/src/bookmark_ui.rs

@@ -0,0 +1,246 @@
+//! Document-local bookmark interaction state. All destinations retain PDF outline paths.
+use pdfcraft_engine::Edit;
+use pdfcraft_render::OutlineItem;
+
+#[derive(Default)]
+pub struct BookmarkState {
+    pub selected: Option<Vec<usize>>,
+    pub rename: Option<(Vec<usize>, String)>,
+    pub(crate) drag: Option<Drag>,
+    pub(crate) expanded: std::collections::HashMap<Vec<usize>, bool>,
+    generation: Option<u64>,
+}
+
+pub(crate) struct Drag {
+    pub source: Vec<usize>,
+    pub hover: Option<(Vec<usize>, f64)>,
+}
+
+#[derive(Clone, Copy, Debug)]
+pub(crate) enum Placement {
+    Before,
+    After,
+    Inside,
+}
+
+pub(crate) fn at<'a>(mut items: &'a [OutlineItem], path: &[usize]) -> Option<&'a OutlineItem> {
+    let (last, parents) = path.split_last()?;
+    for &i in parents {
+        items = &items.get(i)?.children;
+    }
+    items.get(*last)
+}
+
+fn siblings<'a>(items: &'a [OutlineItem], parent: &[usize]) -> Option<&'a [OutlineItem]> {
+    if parent.is_empty() { Some(items) } else { Some(&at(items, parent)?.children) }
+}
+
+/// Remove a path's source sibling, adjusting any destination parent that runs through a
+/// later sibling. The engine performs this adjustment itself; callers pass the ORIGINAL
+/// parent to MoveBookmark and use this only for the resulting selection path.
+fn after_removal(path: &[usize], source: &[usize]) -> Option<Vec<usize>> {
+    if path.starts_with(source) {
+        return None;
+    }
+    let (&index, parent) = source.split_last()?;
+    let mut path = path.to_vec();
+    if path.starts_with(parent)
+        && let Some(i) = path.get_mut(parent.len())
+        && *i > index
+    {
+        *i -= 1;
+    }
+    Some(path)
+}
+
+fn after_insertion(mut path: Vec<usize>, parent: &[usize], index: usize) -> Vec<usize> {
+    if path.starts_with(parent)
+        && let Some(i) = path.get_mut(parent.len())
+        && *i >= index
+    {
+        *i = i.saturating_add(1);
+    }
+    path
+}
+
+/// Translate a row's pre-removal gap to the engine's post-removal index. Reject self,
+/// descendants, invalid paths and the two gaps that would leave the source unchanged.
+pub(crate) fn drop_edit(items: &[OutlineItem], source: &[usize], target: &[usize], placement: Placement) -> Option<Edit> {
+    at(items, source)?;
+    let target_item = at(items, target)?;
+    if target.starts_with(source) {
+        return None;
+    }
+    let (&target_index, target_parent) = target.split_last()?;
+    let (parent, gap) = match placement {
+        Placement::Before => (target_parent.to_vec(), target_index),
+        Placement::After => (target_parent.to_vec(), target_index.checked_add(1)?),
+        Placement::Inside => (target.to_vec(), target_item.children.len()),
+    };
+    let (&source_index, source_parent) = source.split_last()?;
+    let index = if parent == source_parent && source_index < gap { gap.saturating_sub(1) } else { gap };
+    if parent == source_parent && index == source_index {
+        return None;
+    }
+    Some(Edit::MoveBookmark { from: source.to_vec(), to_parent: parent, index })
+}
+
+impl BookmarkState {
+    pub(crate) fn prepared(&self, edit: &Edit, items: &[OutlineItem]) -> Self {
+        let mut next = Self { selected: self.selected.clone(), expanded: self.expanded.clone(), ..Default::default() };
+        next.committed(edit, items, self.generation.unwrap_or_default());
+        next
+    }
+
+    pub(crate) fn accept_generation(&mut self, generation: u64) {
+        self.generation = Some(generation);
+    }
+    pub(crate) fn sync(&mut self, generation: u64, items: &[OutlineItem]) {
+        if self.generation.is_some_and(|old| old != generation) {
+            // An edit outside this UI, reload or undo may reuse a valid path for a different
+            // bookmark. Never infer identity from a title (titles may be duplicated).
+            self.selected = None;
+            self.rename = None;
+            self.drag = None;
+            self.expanded.clear();
+        }
+        self.generation = Some(generation);
+        if self.selected.as_ref().is_some_and(|p| at(items, p).is_none()) {
+            self.selected = None;
+        }
+        if self.rename.as_ref().is_some_and(|(p, _)| at(items, p).is_none()) {
+            self.rename = None;
+        }
+    }
+
+    pub(crate) fn insertion(&self, items: &[OutlineItem]) -> (Vec<usize>, usize) {
+        if let Some(path) = &self.selected
+            && at(items, path).is_some()
+            && let Some((&i, parent)) = path.split_last()
+        {
+            return (parent.to_vec(), i.saturating_add(1));
+        }
+        (vec![], items.len())
+    }
+
+    /// Prepare remapped state using the outline BEFORE the edit for lengths. The caller
+    /// installs this state and its new generation only if the engine edit succeeds.
+    pub(crate) fn committed(&mut self, edit: &Edit, items: &[OutlineItem], generation: u64) {
+        self.drag = None;
+        self.rename = None;
+        self.generation = Some(generation);
+        self.selected = self.selected.take().and_then(|path| remap(path, edit, items));
+        self.expanded = std::mem::take(&mut self.expanded).into_iter().filter_map(|(path, open)| Some((remap(path, edit, items)?, open))).collect();
+    }
+}
+
+fn remap(path: Vec<usize>, edit: &Edit, items: &[OutlineItem]) -> Option<Vec<usize>> {
+    match edit {
+        Edit::AddBookmark { parent, index, .. } => {
+            let index = (*index).min(siblings(items, parent)?.len());
+            Some(after_insertion(path, parent, index))
+        }
+        Edit::DeleteBookmark { path: source } => after_removal(&path, source),
+        Edit::MoveBookmark { from, to_parent, index } => {
+            let (_, source_parent) = from.split_last()?;
+            let n = siblings(items, to_parent)?.len().saturating_sub(usize::from(source_parent == to_parent));
+            let index = (*index).min(n);
+            let parent = after_removal(to_parent, from)?;
+            if let Some(tail) = path.strip_prefix(from.as_slice()) {
+                let mut moved = parent;
+                moved.push(index);
+                moved.extend_from_slice(tail);
+                Some(moved)
+            } else {
+                Some(after_insertion(after_removal(&path, from)?, &parent, index))
+            }
+        }
+        Edit::RenameBookmark { .. } | Edit::SetBookmarkPage { .. } => Some(path),
+        // Other operations can replace the outline (including page imports). Clearing is
+        // safer than pointing an insertion reference at an unrelated bookmark.
+        _ => None,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn item(title: &str, children: Vec<OutlineItem>) -> OutlineItem {
+        OutlineItem { title: title.into(), page: Some(0), view: pdfcraft_render::DestView::Top, children, open: true }
+    }
+
+    #[test]
+    fn every_root_gap_matches_an_independent_remove_insert_model() {
+        let items: Vec<_> = (0..8).map(|i| item(&i.to_string(), vec![])).collect();
+        for source in 0..items.len() {
+            for target in 0..items.len() {
+                for placement in [Placement::Before, Placement::After] {
+                    let mut expected: Vec<_> = (0..items.len()).collect();
+                    // Find the target by identity AFTER removing the source, rather than using
+                    // the production gap adjustment. A self target is always rejected.
+                    expected.remove(source);
+                    let edit = drop_edit(&items, &[source], &[target], placement);
+                    if source == target {
+                        assert!(edit.is_none());
+                        continue;
+                    }
+                    let position = expected.iter().position(|i| *i == target).unwrap() + usize::from(matches!(placement, Placement::After));
+                    expected.insert(position, source);
+                    if expected == (0..items.len()).collect::<Vec<_>>() {
+                        assert!(edit.is_none());
+                    } else {
+                        let edit = edit.unwrap();
+                        assert!(
+                            matches!(&edit, Edit::MoveBookmark { from, to_parent, index } if from == &[source] && to_parent.is_empty() && *index == position)
+                        );
+                        // Every selection, including later siblings, follows its identity.
+                        for selected in 0..items.len() {
+                            let mut state = BookmarkState { selected: Some(vec![selected]), ..Default::default() };
+                            state.committed(&edit, &items, 1);
+                            assert_eq!(state.selected, Some(vec![expected.iter().position(|i| *i == selected).unwrap()]));
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    #[test]
+    fn related_subtree_moves_remap_selected_descendants_and_unrelated_siblings() {
+        let items =
+            vec![item("A", vec![item("A0", vec![item("A00", vec![])]), item("A1", vec![])]), item("B", vec![item("B0", vec![])]), item("C", vec![])];
+        for (from, parent, index, selected, expected) in [
+            (vec![0], vec![1, 0], 0, vec![0, 0, 0], vec![0, 0, 0, 0, 0]),
+            (vec![0], vec![1, 0], 0, vec![1, 0], vec![0, 0]),
+            (vec![0, 0], vec![], 1, vec![0, 0, 0], vec![1, 0]),
+            (vec![0, 0], vec![], 1, vec![1, 0], vec![2, 0]),
+            (vec![1, 0], vec![0], 1, vec![0, 1], vec![0, 2]),
+            (vec![1, 0], vec![0], 1, vec![1], vec![1]),
+        ] {
+            let mut state = BookmarkState { selected: Some(selected), ..Default::default() };
+            state.committed(&Edit::MoveBookmark { from, to_parent: parent, index }, &items, 1);
+            assert_eq!(state.selected, Some(expected));
+        }
+        for (source, target) in [(vec![], vec![0]), (vec![9], vec![0]), (vec![0], vec![9]), (vec![0], vec![0, 0, 0])] {
+            assert!(drop_edit(&items, &source, &target, Placement::Inside).is_none());
+        }
+    }
+
+    #[test]
+    fn stale_selection_rename_and_drag_cannot_refer_to_a_reused_path() {
+        let items = vec![item("Duplicate", vec![]), item("Duplicate", vec![])];
+        let mut state = BookmarkState {
+            selected: Some(vec![0]),
+            rename: Some((vec![0], "Duplicate".into())),
+            drag: Some(Drag { source: vec![0], hover: None }),
+            generation: Some(4),
+            expanded: Default::default(),
+        };
+        state.sync(5, &items);
+        assert!(state.selected.is_none() && state.rename.is_none() && state.drag.is_none());
+        assert_eq!(state.insertion(&items), (vec![], 2));
+        state.selected = Some(vec![42]);
+        assert_eq!(state.insertion(&items), (vec![], 2));
+    }
+}

crates/ui-egui/src/canvas.rs

@@ -368,6 +368,7 @@ pub(crate) fn snapshot_tab_name(title: &str, file_name: &str, snapshot: &Snapsho
 }
 
 pub struct DocView {
+    pub bookmarks: crate::bookmark_ui::BookmarkState,
     pub id: DocId,
     pub zoom: f32,
     pub fit: Fit,
@@ -786,6 +787,7 @@ impl DocView {
             sign: Default::default(),
             org_drag: None,
             panel_drag: None,
+            bookmarks: Default::default(),
             marquee: None,
             marquee_done: None,
             fill_text: None,