| author | |
| committer | |
| log | 0c15feff0434402e533f3d088940143f3bb4cfa5 |
| tree | 319fdcee459e2ba63910f517aeee897f2f265f97 |
| parent | b6dd7686aeeb8a0ed33c0ec5f614550446b6ec68 |
| signature | Signed by SSH key SHA256:52mNGHRsVFBDED9IAX5pe+LRWUefqTbxEReunq21QvU (~clover) |
Moving a page copied it into the other section, then deleted it from its own with an
ordinary Delete. An edit accepted in between, from another window or a sync, was deleted
with it while the copy held the older page. Mobile also queued the copy without waiting for
it, so a copy the other section refused still deleted the page.
Replica::apply_unchanged applies an edit only while a page is still as it was read, checked
on the section thread in order with every other edit, so nothing can land between the check
and the Delete. Desktop Send and Return and mobile move_page retire the source page through
it once the copy is durable. A page changed since stays beside its copy; one whose section
merely synced meanwhile moves as before.
Fixes #138
Assisted-by: claude-opus-5.58 files changed, 378 insertions(+), 25 deletions(-)
crates/mobile/src/library.rs+12-14| ... | @@ -1409,13 +1409,11 @@ impl Library { | ... | @@ -1409,13 +1409,11 @@ impl Library { |
| 1409 | { | 1409 | { |
| 1410 | ops.push(Op::Section(SectionOp::Delete(vec![discard]))); | 1410 | ops.push(Op::Section(SectionOp::Delete(vec![discard]))); |
| 1411 | } | 1411 | } |
| 1412 | Ok(section.apply( | 1412 | let edit = Edit { |
| 1413 | author, | 1413 | at: filetime(), |
| 1414 | Edit { | 1414 | ops, |
| 1415 | at: filetime(), | 1415 | }; |
| 1416 | ops, | 1416 | Ok(section.replica().apply(author, edit)?) |
| 1417 | }, | ||
| 1418 | )?) | ||
| 1419 | })?; | 1417 | })?; |
| 1420 | let added = self.with_section(from, |section| { | 1418 | let added = self.with_section(from, |section| { |
| 1421 | let mut ops = vec![Op::Section(SectionOp::Delete(vec![space]))]; | 1419 | let mut ops = vec![Op::Section(SectionOp::Delete(vec![space]))]; |
| ... | @@ -1428,13 +1426,13 @@ impl Library { | ... | @@ -1428,13 +1426,13 @@ impl Library { |
| 1428 | } | 1426 | } |
| 1429 | _ => None, | 1427 | _ => None, |
| 1430 | }; | 1428 | }; |
| 1431 | section.apply( | 1429 | let edit = Edit { |
| 1432 | author, | 1430 | at: filetime(), |
| 1433 | Edit { | 1431 | ops, |
| 1434 | at: filetime(), | 1432 | }; |
| 1435 | ops, | 1433 | section |
| 1436 | }, | 1434 | .replica() |
| 1437 | )?; | 1435 | .apply_unchanged(author, edit, space, page)?; |
| 1438 | Ok(added) | 1436 | Ok(added) |
| 1439 | })?; | 1437 | })?; |
| 1440 | let id = |space: Option<ExGuid>| space.map(|space| space.to_string()); | 1438 | let id = |space: Option<ExGuid>| space.map(|space| space.to_string()); |
crates/mobile/src/tests.rs+127-1| ... | @@ -1043,13 +1043,34 @@ fn a_share_notebook_syncs_only_its_readable_sections() { | ... | @@ -1043,13 +1043,34 @@ fn a_share_notebook_syncs_only_its_readable_sections() { |
| 1043 | /// Paths the test coordinator was asked for, with whether to write. | 1043 | /// Paths the test coordinator was asked for, with whether to write. |
| 1044 | static COORDINATED: Mutex<Vec<(String, bool)>> = Mutex::new(Vec::new()); | 1044 | static COORDINATED: Mutex<Vec<(String, bool)>> = Mutex::new(Vec::new()); |
| 1045 | 1045 | ||
| 1046 | /// A file whose next coordinated access the test coordinator holds, as another app may: who | ||
| 1047 | /// hears that it is held, and what lets it go. | ||
| 1048 | type Held = ( | ||
| 1049 | String, | ||
| 1050 | std::sync::mpsc::Sender<()>, | ||
| 1051 | std::sync::mpsc::Receiver<()>, | ||
| 1052 | ); | ||
| 1053 | static HELD: Mutex<Option<Held>> = Mutex::new(None); | ||
| 1054 | |||
| 1046 | extern "C" fn coordinator( | 1055 | extern "C" fn coordinator( |
| 1047 | path: *const c_char, | 1056 | path: *const c_char, |
| 1048 | write: bool, | 1057 | write: bool, |
| 1049 | body: extern "C" fn(*mut c_void), | 1058 | body: extern "C" fn(*mut c_void), |
| 1050 | context: *mut c_void, | 1059 | context: *mut c_void, |
| 1051 | ) { | 1060 | ) { |
| 1052 | COORDINATED.lock().unwrap().push((string(path), write)); | 1061 | let path = string(path); |
| 1062 | COORDINATED.lock().unwrap().push((path.clone(), write)); | ||
| 1063 | let held = { | ||
| 1064 | let mut held = HELD.lock().unwrap(); | ||
| 1065 | match &*held { | ||
| 1066 | Some((file, ..)) if *file == path => held.take(), | ||
| 1067 | _ => None, | ||
| 1068 | } | ||
| 1069 | }; | ||
| 1070 | if let Some((_, entered, released)) = held { | ||
| 1071 | let _ = entered.send(()); | ||
| 1072 | let _ = released.recv(); | ||
| 1073 | } | ||
| 1053 | body(context); | 1074 | body(context); |
| 1054 | } | 1075 | } |
| 1055 | 1076 | ||
| ... | @@ -1758,6 +1779,111 @@ fn a_page_moves_to_another_section_and_back_where_it_was() { | ... | @@ -1758,6 +1779,111 @@ fn a_page_moves_to_another_section_and_back_where_it_was() { |
| 1758 | assert_eq!(stored.page(back).unwrap().title, title); | 1779 | assert_eq!(stored.page(back).unwrap().title, title); |
| 1759 | } | 1780 | } |
| 1760 | 1781 | ||
| 1782 | /// A page edited while it moves stays, with the edit, beside the copy that arrived; a page added | ||
| 1783 | /// meanwhile lets it go. The files hold both once the notebook closes. | ||
| 1784 | #[test] | ||
| 1785 | fn a_page_edited_while_it_moves_stays_with_its_edit() { | ||
| 1786 | library::sb_set_coordinator(coordinator); | ||
| 1787 | for edited in [true, false] { | ||
| 1788 | let (_directory, root, library, features) = notebook_open("Features.one"); | ||
| 1789 | assert!(eventually(|| { | ||
| 1790 | let status = library.sync_status(); | ||
| 1791 | !status.is_empty() | ||
| 1792 | && status | ||
| 1793 | .iter() | ||
| 1794 | .all(|sync| sync.state == SyncState::UpToDate as u8) | ||
| 1795 | })); | ||
| 1796 | let space = space(&features, "Task controls"); | ||
| 1797 | // Empty.one is held for the move, which opens it, until the edit below is made. | ||
| 1798 | let (entered, holding) = std::sync::mpsc::channel(); | ||
| 1799 | let (release, released) = std::sync::mpsc::channel(); | ||
| 1800 | let empty = root.canonicalize().unwrap().join("Empty.one"); | ||
| 1801 | *HELD.lock().unwrap() = Some((empty.to_string_lossy().into_owned(), entered, released)); | ||
| 1802 | let moving = Arc::clone(&library); | ||
| 1803 | let date = ["Thursday, October 1, 2026", "9:00 AM"]; | ||
| 1804 | let move_page = std::thread::spawn(move || { | ||
| 1805 | moving | ||
| 1806 | .move_page( | ||
| 1807 | "Features.one", | ||
| 1808 | space, | ||
| 1809 | "Empty.one", | ||
| 1810 | None, | ||
| 1811 | 1, | ||
| 1812 | None, | ||
| 1813 | "Clover Test", | ||
| 1814 | date, | ||
| 1815 | ) | ||
| 1816 | .map(|_| ()) | ||
| 1817 | .map_err(|error| error.to_string()) | ||
| 1818 | }); | ||
| 1819 | holding.recv_timeout(Duration::from_secs(10)).unwrap(); | ||
| 1820 | if edited { | ||
| 1821 | let mut canvas = canvas(&features, "Task controls"); | ||
| 1822 | focus(&mut canvas, "Disabled standalone task"); | ||
| 1823 | canvas.insert("Edited ".into()).unwrap(); | ||
| 1824 | features | ||
| 1825 | .shared | ||
| 1826 | .apply(canvas.edit().unwrap().unwrap()) | ||
| 1827 | .unwrap(); | ||
| 1828 | } else { | ||
| 1829 | features.new_page(None, date[0], date[1]).unwrap(); | ||
| 1830 | } | ||
| 1831 | features.shared.section.written().unwrap(); | ||
| 1832 | drop(release); | ||
| 1833 | let moved = move_page.join().unwrap(); | ||
| 1834 | assert_eq!(moved.is_ok(), !edited, "{moved:?}"); | ||
| 1835 | drop(features); | ||
| 1836 | library.close(Duration::from_secs(20)).unwrap(); | ||
| 1837 | let arena = onestore::Arena::default(); | ||
| 1838 | let stored = onestore::read_file(root.join("Features.one")).unwrap(); | ||
| 1839 | let stayed = onestore::Section::open(&arena, stored).unwrap().page(space); | ||
| 1840 | assert_eq!(stayed.is_ok(), edited); | ||
| 1841 | if edited { | ||
| 1842 | assert!(canvas::search::page_text(&stayed.unwrap()).contains("Edited ")); | ||
| 1843 | } | ||
| 1844 | let arena = onestore::Arena::default(); | ||
| 1845 | let stored = onestore::read_file(root.join("Empty.one")).unwrap(); | ||
| 1846 | let mut empty = onestore::Section::open(&arena, stored).unwrap(); | ||
| 1847 | let copies: Vec<String> = (empty.pages().unwrap().into_iter()) | ||
| 1848 | .filter(|(_, title, _)| title == "Task controls") | ||
| 1849 | .map(|(copy, ..)| canvas::search::page_text(&empty.page(copy).unwrap())) | ||
| 1850 | .collect(); | ||
| 1851 | assert_eq!(copies.len(), 1); | ||
| 1852 | assert!(!copies[0].contains("Edited ")); | ||
| 1853 | } | ||
| 1854 | } | ||
| 1855 | |||
| 1856 | /// A page the section it moves to refuses stays where it was, and the move fails. | ||
| 1857 | #[test] | ||
| 1858 | fn a_page_the_other_section_refuses_stays() { | ||
| 1859 | let (_directory, _root, library, features) = notebook_open("Features.one"); | ||
| 1860 | let before = levels(&features); | ||
| 1861 | let (space, _) = before[1]; | ||
| 1862 | let page = features.shared.section.page(space).unwrap(); | ||
| 1863 | let empty = open(&library, "Empty.one"); | ||
| 1864 | let listed = levels(&empty); | ||
| 1865 | // A section's first page has level 1: Empty.one, holding none, refuses this one. | ||
| 1866 | assert!( | ||
| 1867 | library | ||
| 1868 | .move_page( | ||
| 1869 | "Features.one", | ||
| 1870 | space, | ||
| 1871 | "Empty.one", | ||
| 1872 | None, | ||
| 1873 | 2, | ||
| 1874 | None, | ||
| 1875 | "Clover Test", | ||
| 1876 | ["Thursday, October 1, 2026", "9:00 AM"], | ||
| 1877 | ) | ||
| 1878 | .is_err() | ||
| 1879 | ); | ||
| 1880 | empty.shared.section.written().unwrap(); | ||
| 1881 | features.shared.section.written().unwrap(); | ||
| 1882 | assert_eq!(levels(&empty), listed); | ||
| 1883 | assert_eq!(levels(&features), before); | ||
| 1884 | assert_eq!(features.shared.section.page(space).unwrap(), page); | ||
| 1885 | } | ||
| 1886 | |||
| 1761 | #[test] | 1887 | #[test] |
| 1762 | fn a_sections_last_page_moving_leaves_a_page_that_undo_takes_out() { | 1888 | fn a_sections_last_page_moving_leaves_a_page_that_undo_takes_out() { |
| 1763 | let (_directory, _root, library, features) = notebook_open("Features.one"); | 1889 | let (_directory, _root, library, features) = notebook_open("Features.one"); |
crates/notebook/README.md+5| ... | @@ -38,6 +38,11 @@ assert_eq!(reopened.pending()?.last().map(|edit| edit.id), Some(id)); | ... | @@ -38,6 +38,11 @@ assert_eq!(reopened.pending()?.last().map(|edit| edit.id), Some(id)); |
| 38 | # } | 38 | # } |
| 39 | ``` | 39 | ``` |
| 40 | 40 | ||
| 41 | `apply_unchanged(author, edit, space, page)` applies the edit only while page `space` | ||
| 42 | still holds `page`, checked on the section thread in order with every other edit, and | ||
| 43 | refuses it as `Rejected` otherwise: a move across sections copies a page there, then retires | ||
| 44 | it here this way, so an edit made since the copy keeps the page. | ||
| 45 | |||
| 41 | Page reads never wait for the sync thread: a rebase or reread of the queue runs on a | 46 | Page reads never wait for the sync thread: a rebase or reread of the queue runs on a |
| 42 | new thread while the current one keeps answering `page` and `pages` from the section as | 47 | new thread while the current one keeps answering `page` and `pages` from the section as |
| 43 | it was, then hands its requests over; edits sent meanwhile apply to the rebuilt section. | 48 | it was, then hands its requests over; edits sent meanwhile apply to the rebuilt section. |
crates/notebook/src/lib.rs+34| ... | @@ -308,6 +308,39 @@ impl Replica { | ... | @@ -308,6 +308,39 @@ impl Replica { |
| 308 | edit, | 308 | edit, |
| 309 | reply, | 309 | reply, |
| 310 | waiting: None, | 310 | waiting: None, |
| 311 | unchanged: None, | ||
| 312 | }) | ||
| 313 | }) | ||
| 314 | .await | ||
| 315 | } | ||
| 316 | |||
| 317 | /// `apply`, provided page `space` still holds `page`; refused as `Rejected` otherwise. | ||
| 318 | pub fn apply_unchanged( | ||
| 319 | &self, | ||
| 320 | author: &str, | ||
| 321 | edit: Edit, | ||
| 322 | space: ExGuid, | ||
| 323 | page: Page, | ||
| 324 | ) -> Result<u64> { | ||
| 325 | task::blocking(self.apply_unchanged_async(author, edit, space, page))? | ||
| 326 | } | ||
| 327 | |||
| 328 | /// `apply_async`, provided page `space` still holds `page`, checked where edits apply in | ||
| 329 | /// order: a move retires the page it copied this way, keeping any edit made since. | ||
| 330 | pub async fn apply_unchanged_async( | ||
| 331 | &self, | ||
| 332 | author: &str, | ||
| 333 | edit: Edit, | ||
| 334 | space: ExGuid, | ||
| 335 | page: Page, | ||
| 336 | ) -> Result<u64> { | ||
| 337 | self.ask_async(|reply| { | ||
| 338 | working::Request::Apply(working::Accepted { | ||
| 339 | author: author.to_owned(), | ||
| 340 | edit, | ||
| 341 | reply, | ||
| 342 | waiting: None, | ||
| 343 | unchanged: Some((space, page)), | ||
| 311 | }) | 344 | }) |
| 312 | }) | 345 | }) |
| 313 | .await | 346 | .await |
| ... | @@ -327,6 +360,7 @@ impl Replica { | ... | @@ -327,6 +360,7 @@ impl Replica { |
| 327 | edit, | 360 | edit, |
| 328 | reply, | 361 | reply, |
| 329 | waiting: Some(waiting), | 362 | waiting: Some(waiting), |
| 363 | unchanged: None, | ||
| 330 | })) | 364 | })) |
| 331 | } | 365 | } |
| 332 | 366 |
crates/notebook/src/session.rs+2-1| ... | @@ -2565,7 +2565,8 @@ impl Section { | ... | @@ -2565,7 +2565,8 @@ impl Section { |
| 2565 | } | 2565 | } |
| 2566 | 2566 | ||
| 2567 | /// Removes pages or conflict pages permanently, queued like the user's own edits (a | 2567 | /// Removes pages or conflict pages permanently, queued like the user's own edits (a |
| 2568 | /// move across sections is `import_page` there, then this here). | 2568 | /// move across sections imports a page there, then retires it here with |
| 2569 | /// `Replica::apply_unchanged`). | ||
| 2569 | pub fn delete_pages(&self, pages: &[ExGuid]) -> Result<u64> { | 2570 | pub fn delete_pages(&self, pages: &[ExGuid]) -> Result<u64> { |
| 2570 | crate::task::blocking(self.delete_pages_async(pages))? | 2571 | crate::task::blocking(self.delete_pages_async(pages))? |
| 2571 | } | 2572 | } |
crates/notebook/src/working.rs+11| ... | @@ -481,6 +481,8 @@ pub(crate) struct Accepted { | ... | @@ -481,6 +481,8 @@ pub(crate) struct Accepted { |
| 481 | pub reply: Reply<u64>, | 481 | pub reply: Reply<u64>, |
| 482 | /// Set where nobody blocks on the reply: a failed write keeps the edit and reports here. | 482 | /// Set where nobody blocks on the reply: a failed write keeps the edit and reports here. |
| 483 | pub waiting: Option<Notice>, | 483 | pub waiting: Option<Notice>, |
| 484 | /// A page as a move copied it, which the edit applies only while it still holds. | ||
| 485 | pub unchanged: Option<(ExGuid, Page)>, | ||
| 484 | } | 486 | } |
| 485 | 487 | ||
| 486 | struct Working<'a> { | 488 | struct Working<'a> { |
| ... | @@ -700,6 +702,15 @@ impl<'a> Working<'a> { | ... | @@ -700,6 +702,15 @@ impl<'a> Working<'a> { |
| 700 | /// Applies an edit, keeping it to be written with the burst. A refused edit is answered | 702 | /// Applies an edit, keeping it to be written with the burst. A refused edit is answered |
| 701 | /// here; true when it failed part way and the section must be reread. | 703 | /// here; true when it failed part way and the section must be reread. |
| 702 | fn accept(&mut self, accepted: Accepted) -> bool { | 704 | fn accept(&mut self, accepted: Accepted) -> bool { |
| 705 | if let Some((space, page)) = &accepted.unchanged | ||
| 706 | && !self.section.page(*space).is_ok_and(|now| now == *page) | ||
| 707 | { | ||
| 708 | let changed = OpError::StructureChanged( | ||
| 709 | "The page changed while it moved. Both sections now have a copy.", | ||
| 710 | ); | ||
| 711 | (accepted.reply)(Err(changed.into())); | ||
| 712 | return false; | ||
| 713 | } | ||
| 703 | match self.section.apply(&accepted.author, &accepted.edit) { | 714 | match self.section.apply(&accepted.author, &accepted.edit) { |
| 704 | Ok(()) => { | 715 | Ok(()) => { |
| 705 | self.touched | 716 | self.touched |
crates/notebook/tests/sync_pages.rs+94| ... | @@ -743,3 +743,97 @@ fn page_moves_and_a_removed_page_merge_as_onenote_merges_them() { | ... | @@ -743,3 +743,97 @@ fn page_moves_and_a_removed_page_merge_as_onenote_merges_them() { |
| 743 | ); | 743 | ); |
| 744 | } | 744 | } |
| 745 | } | 745 | } |
| 746 | |||
| 747 | /// A page a move copied leaves only as it was copied: an edit made here since, published or | ||
| 748 | /// not, or another writer's that a rebase took, keeps it; edits to other pages, published or | ||
| 749 | /// rebased, do not. Either way the queue reopens as it was left and publishes it. | ||
| 750 | #[test] | ||
| 751 | fn a_moved_page_leaves_only_as_it_was_copied() { | ||
| 752 | // The first two pages holding text. | ||
| 753 | let texts = texts(SOURCE); | ||
| 754 | let mut written = order(SOURCE).into_iter().filter_map(|(space, _)| { | ||
| 755 | let (&(_, text), _) = texts.iter().find(|((sid, _), _)| *sid == space)?; | ||
| 756 | Some((space, text)) | ||
| 757 | }); | ||
| 758 | let (moving, moving_text) = written.next().unwrap(); | ||
| 759 | let (other, other_text) = written.next().unwrap(); | ||
| 760 | fn here(cache: &Replica, (space, text): (ExGuid, ExGuid)) { | ||
| 761 | let op = onestore::op::PageOp::Text { | ||
| 762 | text, | ||
| 763 | range: 0..0, | ||
| 764 | with: "Here ".into(), | ||
| 765 | }; | ||
| 766 | let at = 133_000_000_000_000_001; | ||
| 767 | let ops = vec![Op::Page { space, op }]; | ||
| 768 | cache | ||
| 769 | .apply("Typist", onestore::op::Edit { at, ops }) | ||
| 770 | .unwrap(); | ||
| 771 | } | ||
| 772 | fn there(server: &mut Server, (space, text): (ExGuid, ExGuid)) { | ||
| 773 | server.visible = typed(&server.visible, space, text, 0..0, "There "); | ||
| 774 | server.durable.clone_from(&server.visible); | ||
| 775 | } | ||
| 776 | type Between = fn(&Replica, &mut Server, [(ExGuid, ExGuid); 2]); | ||
| 777 | let schedules: [(&str, bool, Between); 5] = [ | ||
| 778 | ("unrelated", true, |cache, server, [_, other]| { | ||
| 779 | here(cache, other); | ||
| 780 | cache.sync_once(server).unwrap(); | ||
| 781 | there(server, other); | ||
| 782 | cache.sync_once(server).unwrap(); | ||
| 783 | }), | ||
| 784 | ("appended", false, |cache, _, [moving, _]| { | ||
| 785 | here(cache, moving) | ||
| 786 | }), | ||
| 787 | ("published", false, |cache, server, [moving, _]| { | ||
| 788 | here(cache, moving); | ||
| 789 | cache.sync_once(server).unwrap(); | ||
| 790 | }), | ||
| 791 | ("rebased", false, |cache, server, [moving, _]| { | ||
| 792 | there(server, moving); | ||
| 793 | cache.sync_once(server).unwrap(); | ||
| 794 | }), | ||
| 795 | ( | ||
| 796 | "rebased and appended", | ||
| 797 | false, | ||
| 798 | |cache, server, [moving, other]| { | ||
| 799 | there(server, other); | ||
| 800 | here(cache, moving); | ||
| 801 | cache.sync_once(server).unwrap(); | ||
| 802 | }, | ||
| 803 | ), | ||
| 804 | ]; | ||
| 805 | for (schedule, leaves, between) in schedules { | ||
| 806 | let directory = tempfile::tempdir().unwrap(); | ||
| 807 | let path = directory.path().join("moving.sqlite"); | ||
| 808 | let cache = Replica::create(&path, SOURCE).unwrap(); | ||
| 809 | let mut server = Server::new(SOURCE); | ||
| 810 | let copied = cache.page(moving).unwrap(); | ||
| 811 | between( | ||
| 812 | &cache, | ||
| 813 | &mut server, | ||
| 814 | [(moving, moving_text), (other, other_text)], | ||
| 815 | ); | ||
| 816 | let kept = cache.page(moving).unwrap(); | ||
| 817 | let delete = onestore::op::Edit { | ||
| 818 | at: 133_000_000_000_000_002, | ||
| 819 | ops: vec![Op::Section(SectionOp::Delete(vec![moving]))], | ||
| 820 | }; | ||
| 821 | let retired = cache.apply_unchanged("Mover", delete, moving, copied); | ||
| 822 | assert_eq!(retired.is_ok(), leaves, "{schedule}: {retired:?}"); | ||
| 823 | assert!( | ||
| 824 | leaves || matches!(retired, Err(notebook::Error::Rejected(_))), | ||
| 825 | "{schedule}" | ||
| 826 | ); | ||
| 827 | drop(cache); | ||
| 828 | let cache = Replica::open(&path).unwrap(); | ||
| 829 | let listed = |image: &[u8]| order(image).iter().any(|(space, _)| *space == moving); | ||
| 830 | assert_eq!( | ||
| 831 | cache.page(moving).ok(), | ||
| 832 | (!leaves).then_some(kept), | ||
| 833 | "{schedule}" | ||
| 834 | ); | ||
| 835 | cache.sync_once(&mut server).unwrap(); | ||
| 836 | assert_eq!(listed(&server.durable), !leaves, "{schedule}"); | ||
| 837 | assert!(cache.pending().unwrap().is_empty(), "{schedule}"); | ||
| 838 | } | ||
| 839 | } |
crates/snowbound/src/undo.rs+93-9| ... | @@ -567,7 +567,8 @@ impl Site { | ... | @@ -567,7 +567,8 @@ impl Site { |
| 567 | let (Some(from), Some(path)) = (at(page), self.section_path(target)) else { | 567 | let (Some(from), Some(path)) = (at(page), self.section_path(target)) else { |
| 568 | return Ok(None); | 568 | return Ok(None); |
| 569 | }; | 569 | }; |
| 570 | let import = notebook::session::moved(&self.replica.page(page)?, &self.author)?; | 570 | let sent = self.replica.page(page)?; |
| 571 | let import = notebook::session::moved(&sent, &self.author)?; | ||
| 571 | let arrived = imported(&import); | 572 | let arrived = imported(&import); |
| 572 | let before = listed[from + 1..].first().map(|(space, ..)| *space); | 573 | let before = listed[from + 1..].first().map(|(space, ..)| *space); |
| 573 | let level = listed[from].2; | 574 | let level = listed[from].2; |
| ... | @@ -585,7 +586,13 @@ impl Site { | ... | @@ -585,7 +586,13 @@ impl Site { |
| 585 | ) | 586 | ) |
| 586 | .await?; | 587 | .await?; |
| 587 | section.close_async().await?; | 588 | section.close_async().await?; |
| 588 | self.apply(ops).await?; | 589 | let edit = Edit { |
| 590 | at: crate::filetime(), | ||
| 591 | ops, | ||
| 592 | }; | ||
| 593 | self.replica | ||
| 594 | .apply_unchanged_async(&self.author, edit, page, sent) | ||
| 595 | .await?; | ||
| 589 | Applied { | 596 | Applied { |
| 590 | undo: Change::Return { | 597 | undo: Change::Return { |
| 591 | page: arrived, | 598 | page: arrived, |
| ... | @@ -629,15 +636,13 @@ impl Site { | ... | @@ -629,15 +636,13 @@ impl Site { |
| 629 | ]; | 636 | ]; |
| 630 | let gone = self.take_added(added, &listed, &mut ops); | 637 | let gone = self.take_added(added, &listed, &mut ops); |
| 631 | self.apply(ops).await?; | 638 | self.apply(ops).await?; |
| 639 | let delete = Edit { | ||
| 640 | at: crate::filetime(), | ||
| 641 | ops: vec![Op::Section(SectionOp::Delete(vec![page]))], | ||
| 642 | }; | ||
| 632 | section | 643 | section |
| 633 | .replica() | 644 | .replica() |
| 634 | .apply_async( | 645 | .apply_unchanged_async(&self.author, delete, page, sent) |
| 635 | &self.author, | ||
| 636 | Edit { | ||
| 637 | at: crate::filetime(), | ||
| 638 | ops: vec![Op::Section(SectionOp::Delete(vec![page]))], | ||
| 639 | }, | ||
| 640 | ) | ||
| 641 | .await?; | 646 | .await?; |
| 642 | section.close_async().await?; | 647 | section.close_async().await?; |
| 643 | Applied { | 648 | Applied { |
| ... | @@ -1682,6 +1687,85 @@ mod tests { | ... | @@ -1682,6 +1687,85 @@ mod tests { |
| 1682 | assert_eq!(fixture.titles(), titled(&["First", "Third"])); | 1687 | assert_eq!(fixture.titles(), titled(&["First", "Third"])); |
| 1683 | } | 1688 | } |
| 1684 | 1689 | ||
| 1690 | /// A page edited while it is on its way to another section stays, with the edit, beside | ||
| 1691 | /// the copy that arrived there; an edit to another page meanwhile lets it go. Both hold | ||
| 1692 | /// once each section is opened again. | ||
| 1693 | #[test] | ||
| 1694 | fn a_page_edited_while_it_is_sent_stays_with_its_edit() { | ||
| 1695 | struct Unreached; | ||
| 1696 | impl notebook::Remote for Unreached { | ||
| 1697 | fn read(&mut self) -> std::io::Result<Vec<u8>> { | ||
| 1698 | unreachable!() | ||
| 1699 | } | ||
| 1700 | fn stamp(&mut self) -> std::io::Result<onestore::Stamp> { | ||
| 1701 | unreachable!() | ||
| 1702 | } | ||
| 1703 | fn publish(&mut self, _: &onestore::Transaction) -> Result<(), onestore::CommitError> { | ||
| 1704 | unreachable!() | ||
| 1705 | } | ||
| 1706 | fn confirm(&mut self, _: &onestore::Stamp) -> Result<(), onestore::CommitError> { | ||
| 1707 | unreachable!() | ||
| 1708 | } | ||
| 1709 | } | ||
| 1710 | |||
| 1711 | for edited in ["Second", "Third"] { | ||
| 1712 | let mut fixture = | ||
| 1713 | Fixture::new(&format!("send-{edited}"), &["First", "Second", "Third"]); | ||
| 1714 | let target = fixture.library.section_identity("B.one").unwrap(); | ||
| 1715 | let page = fixture.space("Second"); | ||
| 1716 | // B's sync holds in connecting until released, so the move, closing B, waits | ||
| 1717 | // there once the copy has landed. | ||
| 1718 | let mut b = fixture.library.open("B.one", || {}).unwrap(); | ||
| 1719 | let arrived = Arc::clone(b.replica()); | ||
| 1720 | let (entered, holding) = std::sync::mpsc::channel(); | ||
| 1721 | let (release, released) = std::sync::mpsc::channel::<()>(); | ||
| 1722 | b.relocate_with(fixture.folder.join("Notebook/B.one"), move |_| { | ||
| 1723 | let _ = entered.send(()); | ||
| 1724 | let _ = released.recv(); | ||
| 1725 | Err::<Unreached, _>(std::io::ErrorKind::TimedOut.into()) | ||
| 1726 | }) | ||
| 1727 | .unwrap(); | ||
| 1728 | holding.recv().unwrap(); | ||
| 1729 | fixture.library.keep("B.one", b); | ||
| 1730 | let mut site = fixture.site(); | ||
| 1731 | let shown = fixture.pages()[0].0; | ||
| 1732 | let sending = std::thread::spawn(move || { | ||
| 1733 | pollster::block_on(site.change(Change::Send { page, target }, shown)) | ||
| 1734 | .map(|applied| applied.is_some()) | ||
| 1735 | .map_err(|error| error.to_string()) | ||
| 1736 | }); | ||
| 1737 | while !(arrived.pages().unwrap().iter()).any(|(_, title, _)| title == "Second") { | ||
| 1738 | std::thread::sleep(std::time::Duration::from_millis(5)); | ||
| 1739 | } | ||
| 1740 | fixture.retitle(fixture.space(edited), &format!("{edited} edited")); | ||
| 1741 | drop(release); | ||
| 1742 | drop(arrived); | ||
| 1743 | let sent = sending.join().unwrap(); | ||
| 1744 | |||
| 1745 | let stayed = match edited { | ||
| 1746 | "Second" => { | ||
| 1747 | assert!(sent.is_err(), "{sent:?}"); | ||
| 1748 | titled(&["First", "Second edited", "Third"]) | ||
| 1749 | } | ||
| 1750 | _ => { | ||
| 1751 | assert_eq!(sent, Ok(true)); | ||
| 1752 | titled(&["First", "Third edited"]) | ||
| 1753 | } | ||
| 1754 | }; | ||
| 1755 | fixture.section.take().unwrap().close().unwrap(); | ||
| 1756 | let titles = |path: &str| { | ||
| 1757 | let section = fixture.library.open(path, || {}).unwrap(); | ||
| 1758 | let titles: Vec<(String, u32)> = (section.pages().unwrap().into_iter()) | ||
| 1759 | .map(|(_, title, level)| (title, level)) | ||
| 1760 | .collect(); | ||
| 1761 | section.close().unwrap(); | ||
| 1762 | titles | ||
| 1763 | }; | ||
| 1764 | assert_eq!(titles("New Section 1.one"), stayed); | ||
| 1765 | assert_eq!(titles("B.one"), titled(&["", "Second"])); | ||
| 1766 | } | ||
| 1767 | } | ||
| 1768 | |||
| 1685 | /// A title given in the page list goes back while the page still has it. | 1769 | /// A title given in the page list goes back while the page still has it. |
| 1686 | #[test] | 1770 | #[test] |
| 1687 | fn a_title_goes_back_while_the_page_has_it() { | 1771 | fn a_title_goes_back_while_the_page_has_it() { |