diff --git a/crates/oa-desktop/src/app/source_rename.rs b/crates/oa-desktop/src/app/source_rename.rs index 50b742d3..8c0da85b 100644 --- a/crates/oa-desktop/src/app/source_rename.rs +++ b/crates/oa-desktop/src/app/source_rename.rs @@ -111,6 +111,7 @@ struct Form { hunk: usize, line: usize, segment: usize, + preview_destination: Id, details: bool, detail_segment: usize, detail_editor: text_editor::Content, @@ -311,6 +312,7 @@ impl App { } pub(super) fn source_rename_event(&mut self, event: Event) -> Task { + let mut reset_preview_scroll = false; match event { Event::Begin(_) => return self.begin_source_rename(), Event::Close(_) => self.close_source_rename(), @@ -359,7 +361,11 @@ impl App { } Event::Search(_, value) => { let form = self.source_rename.form.as_mut().unwrap(); + if form.search == value { + return Task::none(); + } form.search = value; + reset_preview_scroll = form.approval.is_some(); if let Some(approval) = &form.approval && let Some(index) = approval .preview @@ -379,6 +385,7 @@ impl App { | Event::Segment(_, _, next) => { let form = self.source_rename.form.as_mut().unwrap(); let ready = form.approval.as_ref().unwrap(); + let previous_page = (form.file, form.hunk, form.line, form.segment); match event { Event::File(..) => { let matching: Vec<_> = ready @@ -414,6 +421,8 @@ impl App { } _ => form.segment = step(form.segment, segment_total(form), next), } + reset_preview_scroll = + previous_page != (form.file, form.hunk, form.line, form.segment); } Event::Details(_, value) => { let form = self.source_rename.form.as_mut().unwrap(); @@ -441,6 +450,9 @@ impl App { } } } + if reset_preview_scroll && let Some(form) = &mut self.source_rename.form { + return reset_preview_position(form); + } Task::none() } @@ -492,6 +504,7 @@ impl App { hunk: 0, line: 0, segment: 0, + preview_destination: Id::new_v4(), details: false, detail_segment: 0, detail_editor: text_editor::Content::new(), @@ -591,6 +604,7 @@ impl App { } let form = self.source_rename.form.as_mut().unwrap(); form.pending = None; + let mut reset_preview_scroll = false; match completion.payload { Payload::Target(Ok(target)) => { form.replacement = target.alias.clone(); @@ -619,6 +633,7 @@ impl App { form.line = 0; form.segment = 0; form.search.clear(); + reset_preview_scroll = true; } } Payload::Target(Err(error)) | Payload::Preview(Err(error)) => form.notice = error, @@ -626,6 +641,9 @@ impl App { if form.details { refresh_details(form); } + if reset_preview_scroll { + return reset_preview_position(form); + } Task::none() } @@ -920,6 +938,31 @@ fn input_id(invocation: Id) -> iced::widget::Id { iced::widget::Id::from(format!("source-rename-{invocation}")) } +// Each navigation gets a fresh destination even when returning to the same page +// or changing a filter that leaves the page tuple unchanged (the ABA case). +fn reset_preview_position(form: &mut Form) -> Task { + form.preview_destination = Id::new_v4(); + if !form.details + && let Some(ready) = &form.approval + { + iced::widget::operation::snap_to( + preview_scroll_id(form, ready.id), + iced::widget::scrollable::RelativeOffset::START, + ) + } else { + Task::none() + } +} + +// The operation targets this visit, not a later page, visit or approval. +// A new widget id alone does not reset Iced's retained Scrollable state. +fn preview_scroll_id(form: &Form, approval: Id) -> iced::widget::Id { + iced::widget::Id::from(format!( + "source-rename-diff-{}-{approval}-{}-{}-{}-{}-{}", + form.invocation, form.preview_destination, form.file, form.hunk, form.line, form.segment + )) +} + /// Complete indexed preview with bounded rendering: eight admitted source rows /// and one UTF-8 segment per side at a time. No candidate content is discarded. pub(crate) fn dialog(app: &App) -> Element<'_, Message> { @@ -1064,7 +1107,7 @@ where pager(format!("Row segments {}/{} · both sides", form.segment + 1, segments.max(1)), (form.segment > 0).then(|| message(Event::Segment(invocation, preview_id, false))), (form.segment + 1 < segments).then(|| message(Event::Segment(invocation, preview_id, true)))), - scrollable(displayed).id("source-rename-diff").height(Fill).width(Fill).spacing(8), + scrollable(displayed).id(preview_scroll_id(form, preview_id)).height(Fill).width(Fill).spacing(8), ].spacing(6).height(Fill).into() } else { scrollable(column![ diff --git a/crates/oa-desktop/src/app/source_rename/preview_glyph_tests.rs b/crates/oa-desktop/src/app/source_rename/preview_glyph_tests.rs index 06297dfe..90b38641 100644 --- a/crates/oa-desktop/src/app/source_rename/preview_glyph_tests.rs +++ b/crates/oa-desktop/src/app/source_rename/preview_glyph_tests.rs @@ -152,6 +152,7 @@ impl shaped::Renderer for GlyphRecorder { #[derive(Default)] struct ScrollGeometry { + id: Option, bounds: Option, content: Option, offset: Vector, @@ -168,7 +169,7 @@ impl widget::Operation for ScrollGeometry { offset: Vector, _: &mut dyn widget::operation::Scrollable, ) { - if id == Some(&widget::Id::new("source-rename-diff")) { + if id == self.id.as_ref() { self.bounds = Some(bounds); self.content = Some(content); self.offset = offset; @@ -262,7 +263,10 @@ fn assert_page_glyphs_visible(app: &App, host: Size) { let node = view.as_widget_mut() .layout(&mut tree, &renderer, &layout::Limits::new(Size::ZERO, host)); - let mut geometry = ScrollGeometry::default(); + let mut geometry = ScrollGeometry { + id: Some(preview_scroll_id(form, ready.id)), + ..ScrollGeometry::default() + }; view.as_widget_mut() .operate(&mut tree, Layout::new(&node), &renderer, &mut geometry); let bounds = geometry.bounds.unwrap(); @@ -279,7 +283,7 @@ fn assert_page_glyphs_visible(app: &App, host: Size) { Layout::new(&node), &renderer, &mut widget::operation::scrollable::scroll_to( - widget::Id::new("source-rename-diff"), + preview_scroll_id(form, ready.id), widget::operation::scrollable::AbsoluteOffset { x: None, y: Some(offset), diff --git a/crates/oa-desktop/src/app/source_rename/preview_scroll_tests.rs b/crates/oa-desktop/src/app/source_rename/preview_scroll_tests.rs new file mode 100644 index 00000000..4641cf7f --- /dev/null +++ b/crates/oa-desktop/src/app/source_rename/preview_scroll_tests.rs @@ -0,0 +1,310 @@ +//! Execute the controller's scroll operations on a retained, scrolled dialog. +use super::*; +use crate::views::source_tests::FontLayout; +use iced::advanced::{Layout, layout, widget}; +use iced::futures::StreamExt; +use iced::{Rectangle, Size, Vector}; + +const HOST: Size = Size::new(720., 660.); + +fn paged_fixture() -> App { + let mut source = bundle(); + let name = "Long engineering name 雨 ".repeat(18); + source.files.get_mut("defs.arcadia").unwrap().replace_range( + .., + &format!( + "oa_activity pump \"{name}\" id \"00000000-0000-0000-0000-000000000002\" {{}}\r\n\ + oa_activity backup \"Backup\" id \"00000000-0000-0000-0000-000000000004\" {{}}\r\n" + ), + ); + let mut uses = String::new(); + for index in 0..16 { + if index == 12 { + uses.push_str(&"// unchanged gap\r\n".repeat(10)); + } + uses.push_str(&format!( + "interaction use_{index} \"{name}\" id \"00000000-0000-0000-0001-{index:012}\" {{ source @pump; target @backup; }}\r\n" + )); + } + source.files.insert("uses.arcadia".into(), uses); + let mut app = fixture(); + app.window_size = HOST; + app.document = Document::from_bundle(source, LIMITS).unwrap(); + app.source_workspace = Workspace::new(app.document.bundle(), true); + app.source_workspace.active = "uses.arcadia".into(); + app.show_source_file(false); + let start = app.source_workspace.text().find("source @pump").unwrap() + "source @".len(); + app.source_editor + .move_to(cursor(app.source_workspace.text(), start, start + 4)); + begin(&mut app); + input(&mut app, "pump_long"); + let _ = preview(&mut app); + app +} + +fn scroll_id(app: &App) -> widget::Id { + let form = app.source_rename.form.as_ref().unwrap(); + preview_scroll_id(form, form.approval.as_ref().unwrap().id) +} + +struct Geometry { + id: widget::Id, + offset: Option, +} +impl widget::Operation for Geometry { + fn traverse(&mut self, visit: &mut dyn FnMut(&mut dyn widget::Operation)) { + visit(self); + } + fn scrollable( + &mut self, + id: Option<&widget::Id>, + _: Rectangle, + _: Rectangle, + offset: Vector, + _: &mut dyn widget::operation::Scrollable, + ) { + if id == Some(&self.id) { + self.offset = Some(offset); + } + } +} + +struct Mounted { + editor: text_editor::Content, + tree: widget::Tree, +} +impl Mounted { + fn new(app: &App) -> Self { + let editor = text_editor::Content::::new(); + let tree = widget::Tree::new(dialog_content(app, &editor).as_widget()); + Self { editor, tree } + } + fn operate(&mut self, app: &App, operation: &mut dyn widget::Operation) { + let mut view = dialog_content(app, &self.editor); + // Preserve the actual scroll state across the new page's view diff. + self.tree.diff(view.as_widget()); + let node = view.as_widget_mut().layout( + &mut self.tree, + &FontLayout, + &layout::Limits::new(Size::ZERO, HOST), + ); + view.as_widget_mut() + .operate(&mut self.tree, Layout::new(&node), &FontLayout, operation); + } + fn offset(&mut self, app: &App) -> f32 { + let mut geometry = Geometry { + id: scroll_id(app), + offset: None, + }; + self.operate(app, &mut geometry); + geometry.offset.expect("mounted preview scrollable").y + } + fn scroll_down(&mut self, app: &App) -> f32 { + self.operate( + app, + &mut widget::operation::scrollable::scroll_to( + scroll_id(app), + widget::operation::scrollable::AbsoluteOffset { + x: None, + y: Some(10_000.), + }, + ), + ); + let offset = self.offset(app); + assert!(offset > 10., "fixture must really be scrolled"); + offset + } + async fn execute(&mut self, app: &App, task: Task) -> usize { + let mut count = 0; + if let Some(mut actions) = iced_runtime::task::into_stream(task) { + while let Some(action) = actions.next().await { + let iced_runtime::Action::Widget(mut operation) = action else { + panic!("preview paging must only schedule a widget operation"); + }; + self.operate(app, operation.as_mut()); + count += 1; + } + } + count + } +} + +#[tokio::test] +async fn explicit_navigation_resets_retained_scroll_but_id_change_alone_does_not() { + let mut app = paged_fixture(); + let before = evidence(&app); + let form = app.source_rename.form.as_ref().unwrap(); + let invocation = form.invocation; + let approval = form.approval.as_ref().unwrap().id; + let mut mounted = Mounted::new(&app); + mounted.scroll_down(&app); + let old_id = scroll_id(&app); + let task = app.update(Message::SourceRename(Event::File( + invocation, approval, true, + ))); + assert_ne!(old_id, scroll_id(&app)); + // Negative control: even after diff/layout with the new id and new file, + // Iced retains the previous offset until the returned operation executes. + assert!(mounted.offset(&app) > 10.); + assert_eq!(mounted.execute(&app, task).await, 1); + assert_eq!(mounted.offset(&app), 0.); + + for navigation in [ + Event::Line(invocation, approval, true), + Event::Segment(invocation, approval, true), + Event::Hunk(invocation, approval, true), + Event::File(invocation, approval, false), + Event::Search(invocation, "uses".into()), + Event::Search(invocation, "no-path-match".into()), + Event::Search(invocation, String::new()), + ] { + mounted.scroll_down(&app); + let task = app.update(Message::SourceRename(navigation)); + assert_eq!(mounted.execute(&app, task).await, 1); + assert_eq!(mounted.offset(&app), 0.); + assert_eq!(evidence(&app), before); + } + // Passive redraw and unchanged filter input preserve deliberate scrolling. + let offset = mounted.scroll_down(&app); + assert_eq!(mounted.offset(&app), offset); + let task = app.update(Message::SourceRename(Event::Search( + invocation, + String::new(), + ))); + assert_eq!(mounted.execute(&app, task).await, 0); + assert_eq!(mounted.offset(&app), offset); + let task = app.update(Message::SourceRename(Event::Input( + invocation, + "pump_long".into(), + ))); + assert_eq!(mounted.execute(&app, task).await, 0); + assert_eq!(mounted.offset(&app), offset); + let task = app.update(Message::SourceRename(Event::File( + invocation, approval, false, + ))); + assert_eq!( + mounted.execute(&app, task).await, + 0, + "clamped first-page request is a no-op" + ); + assert_eq!(mounted.offset(&app), offset); + assert_eq!(evidence(&app), before); +} + +#[tokio::test] +async fn delayed_page_and_approval_operations_cannot_reset_a_new_destination() { + let mut app = paged_fixture(); + let before = evidence(&app); + let form = app.source_rename.form.as_ref().unwrap(); + let invocation = form.invocation; + let old_approval = form.approval.as_ref().unwrap().id; + let mut mounted = Mounted::new(&app); + let old_page_task = app.update(Message::SourceRename(Event::File( + invocation, + old_approval, + true, + ))); + let next_page_task = app.update(Message::SourceRename(Event::Line( + invocation, + old_approval, + true, + ))); + assert_eq!(mounted.execute(&app, next_page_task).await, 1); + let offset = mounted.scroll_down(&app); + assert_eq!(mounted.execute(&app, old_page_task).await, 1); + assert_eq!(mounted.offset(&app), offset, "old page target is absent"); + + let old_approval_task = app.update(Message::SourceRename(Event::Search( + invocation, + "defs".into(), + ))); + input(&mut app, "pump_new"); + let _ = preview(&mut app); // Real new approval in the same invocation/page. + assert_ne!( + app.source_rename + .form + .as_ref() + .unwrap() + .approval + .as_ref() + .unwrap() + .id, + old_approval + ); + let offset = mounted.scroll_down(&app); + assert_eq!(mounted.execute(&app, old_approval_task).await, 1); + assert_eq!( + mounted.offset(&app), + offset, + "old approval target is absent" + ); + let task = app.update(Message::SourceRename(Event::File( + invocation, + old_approval, + true, + ))); + assert_eq!( + mounted.execute(&app, task).await, + 0, + "stale envelope is rejected before scheduling" + ); + assert_eq!(mounted.offset(&app), offset); + assert_eq!(evidence(&app), before); +} + +#[tokio::test] +async fn repeated_page_visits_filters_and_fast_preview_completion_have_distinct_destinations() { + let mut app = paged_fixture(); + let before = evidence(&app); + let form = app.source_rename.form.as_ref().unwrap(); + let invocation = form.invocation; + let approval = form.approval.as_ref().unwrap().id; + let mut mounted = Mounted::new(&app); + + let first_visit = app.update(Message::SourceRename(Event::File( + invocation, approval, true, + ))); + let next_page = app.update(Message::SourceRename(Event::Line( + invocation, approval, true, + ))); + assert_eq!(mounted.execute(&app, next_page).await, 1); + let return_visit = app.update(Message::SourceRename(Event::Line( + invocation, approval, false, + ))); + assert_eq!(mounted.execute(&app, return_visit).await, 1); + let offset = mounted.scroll_down(&app); + assert_eq!(mounted.execute(&app, first_visit).await, 1); + assert_eq!( + mounted.offset(&app), + offset, + "old visit to the identical page is stale" + ); + + let old_filter = app.update(Message::SourceRename(Event::Search( + invocation, + "uses".into(), + ))); + let new_filter = app.update(Message::SourceRename(Event::Search( + invocation, + "us".into(), + ))); + assert_eq!(mounted.execute(&app, new_filter).await, 1); + let offset = mounted.scroll_down(&app); + assert_eq!(mounted.execute(&app, old_filter).await, 1); + assert_eq!( + mounted.offset(&app), + offset, + "old filter on the same page is stale" + ); + + input(&mut app, "pump_new"); + let job = start_preview(&mut app); + let completion = preview_completion(&app, job); + // No intermediate frame mounts the temporary no-approval view. The old + // Scrollable survives even though the new approval has a different id. + let ready = app.update(Message::SourceRenameReady(completion)); + assert!(mounted.offset(&app) > 10.); + assert_eq!(mounted.execute(&app, ready).await, 1); + assert_eq!(mounted.offset(&app), 0.); + assert_eq!(evidence(&app), before); +} diff --git a/crates/oa-desktop/src/app/source_rename/tests.rs b/crates/oa-desktop/src/app/source_rename/tests.rs index 2c8b4c54..e0dfe43b 100644 --- a/crates/oa-desktop/src/app/source_rename/tests.rs +++ b/crates/oa-desktop/src/app/source_rename/tests.rs @@ -6,6 +6,10 @@ use std::collections::BTreeMap; #[path = "preview_glyph_tests.rs"] mod preview_glyph_tests; +#[cfg(debug_assertions)] +#[path = "preview_scroll_tests.rs"] +mod preview_scroll_tests; + fn bundle() -> SourceBundle { SourceBundle { entry: "project.arcadia".into(), files: BTreeMap::from([ ("project.arcadia".into(), concat!("arcadia 7;\r\nproject model \"救援\" id \"00000000-0000-0000-0000-000000000001\" {\r\n", @@ -976,6 +980,7 @@ fn actual_dialog_real_paragraph_geometry_keeps_input_preview_and_footer_reachabl use iced::advanced::{Layout, layout, widget}; #[derive(Default)] struct Bounds { + diff_id: Option, input: Option, preview: Option, footer: Option, @@ -1004,7 +1009,7 @@ fn actual_dialog_real_paragraph_geometry_keeps_input_preview_and_footer_reachabl _: iced::Vector, _: &mut dyn widget::operation::Scrollable, ) { - if id == Some(&widget::Id::new("source-rename-diff")) { + if id == self.diff_id.as_ref() { self.diff = Some(bounds); } } @@ -1042,7 +1047,11 @@ fn actual_dialog_real_paragraph_geometry_keeps_input_preview_and_footer_reachabl &FontLayout, &layout::Limits::new(iced::Size::ZERO, host), ); - let mut bounds = Bounds::default(); + let form = app.source_rename.form.as_ref().unwrap(); + let mut bounds = Bounds { + diff_id: Some(preview_scroll_id(form, form.approval.as_ref().unwrap().id)), + ..Bounds::default() + }; view.as_widget_mut() .operate(&mut tree, Layout::new(&node), &FontLayout, &mut bounds); let input = bounds.input.unwrap();