Merge pull request #447 from matrix-org/aringenbach/replace_legacy_re…

…move_list_item

Replace some legacy methods with dom methods

crates/wysiwyg/src/composer_model/delete_text.rs

@@ -52,7 +52,7 @@ where
             // TODO: should probably also get inside here if our selection
             // only contains a zero-wdith space.
             let range = self.state.dom.find_range(s, e);
-            self.backspace_single_cursor(range, e)
+            self.backspace_single_cursor(range)
         } else {
             self.do_backspace()
         }
@@ -298,11 +298,7 @@ where
         }
     }
 
-    fn backspace_single_cursor(
-        &mut self,
-        range: Range,
-        end_position: usize,
-    ) -> ComposerUpdate<S> {
+    fn backspace_single_cursor(&mut self, range: Range) -> ComposerUpdate<S> {
         // Find the first leaf node in this selection - note there
         // should only be one because s == e, so we don't have a
         // selection that spans multiple leaves.
@@ -316,7 +312,7 @@ where
                 .dom
                 .find_parent_list_item_or_self(&leaf.node_handle);
             if let Some(parent_handle) = parent_list_item_handle {
-                self.do_backspace_in_list(&parent_handle, end_position)
+                self.do_backspace_in_list(&parent_handle)
             } else {
                 self.do_backspace()
             }

crates/wysiwyg/src/composer_model/lists.rs

@@ -17,8 +17,6 @@ use std::collections::HashMap;
 use crate::dom::nodes::dom_node::DomNodeKind;
 use crate::dom::nodes::{ContainerNode, DomNode};
 use crate::dom::range::DomLocationPosition;
-use crate::dom::to_raw_text::ToRawText;
-use crate::dom::unicode_string::UnicodeStrExt;
 use crate::dom::{DomHandle, DomLocation, Range};
 use crate::{ComposerModel, ComposerUpdate, ListType, Location, UnicodeString};
 
@@ -89,24 +87,18 @@ where
     pub(crate) fn do_backspace_in_list(
         &mut self,
         parent_handle: &DomHandle,
-        location: usize,
     ) -> ComposerUpdate<S> {
         let parent_node = self.state.dom.lookup_node(parent_handle);
         let list_node_handle = parent_node.handle().parent_handle();
         if let DomNode::Container(parent) = parent_node {
             if parent.is_empty_list_item() {
-                // Store current Dom
-                self.push_state_to_history();
-                self.remove_list_item(
+                self.state.dom.extract_list_items(
                     &list_node_handle,
-                    location,
                     parent_handle.index_in_parent(),
-                    false,
+                    1,
                 );
-                self.create_update_replace_all()
-            } else {
-                self.do_backspace()
             }
+            self.do_backspace()
         } else {
             panic!("No list item found")
         }
@@ -133,7 +125,6 @@ where
     pub(crate) fn do_enter_in_list(
         &mut self,
         list_item_handle: &DomHandle,
-        current_cursor_global_location: usize,
         list_item_end_offset: usize,
     ) -> ComposerUpdate<S> {
         // Store current Dom
@@ -144,21 +135,19 @@ where
             if list_item_node.is_empty_list_item() {
                 // Pressing enter in an empty list item means you want to
                 // end the list.
-                self.remove_list_item(
+                self.state.dom.extract_list_items(
                     &list_handle,
-                    current_cursor_global_location,
                     list_item_handle.index_in_parent(),
-                    true,
+                    1,
                 );
             } else {
                 // Pressing enter in a non-empty list item splits this item
                 // into two.
-                self.slice_list_item(
-                    &list_handle,
-                    list_item_handle,
-                    current_cursor_global_location,
-                    list_item_end_offset,
-                );
+                self.state
+                    .dom
+                    .slice_list_item(list_item_handle, list_item_end_offset);
+                // Slicing always adds a single ZWSP.
+                self.increment_selection(1);
             }
             self.create_update_replace_all()
         } else {
@@ -289,73 +278,6 @@ where
         self.create_update_replace_all()
     }
 
-    fn slice_list_item(
-        &mut self,
-        list_handle: &DomHandle,
-        list_item_handle: &DomHandle,
-        location: usize,
-        list_item_end_offset: usize,
-    ) {
-        let list_item = self.state.dom.lookup_node_mut(list_item_handle);
-        let mut slice = list_item.slice_after(list_item_end_offset);
-        slice.as_container_mut().unwrap().add_leading_zwsp();
-        let list = self.state.dom.lookup_node_mut(list_handle);
-        let DomNode::Container(list) = list else { panic!("List node is not a container") };
-        if slice.text_len() == 0 {
-            list.insert_child(
-                list_item_handle.index_in_parent() + 1,
-                DomNode::new_list_item(vec![DomNode::new_zwsp()]),
-            );
-        } else {
-            list.insert_child(list_item_handle.index_in_parent() + 1, slice);
-        }
-        self.state.start = Location::from(location + 1);
-        self.state.end = Location::from(location + 1);
-    }
-
-    fn remove_list_item(
-        &mut self,
-        list_handle: &DomHandle,
-        current_cursor_global_location: usize,
-        li_index: usize,
-        insert_trailing_text_node: bool,
-    ) {
-        let list_node = self.state.dom.lookup_node_mut(list_handle);
-        if let DomNode::Container(list) = list_node {
-            let list_len = list.to_raw_text().len();
-            let li_len = list.children()[li_index].to_raw_text().len();
-            if list.children().len() == 1 {
-                // TODO: handle list items outside of lists
-                let parent = self.state.dom.parent_mut(list_handle);
-                parent.remove_child(list_handle.index_in_parent());
-                if parent.children().is_empty() {
-                    parent.append_child(DomNode::new_text(S::default()));
-                }
-                let new_location =
-                    Location::from(current_cursor_global_location - list_len);
-                self.state.start = new_location;
-                self.state.end = new_location;
-            } else {
-                list.remove_child(li_index);
-                if insert_trailing_text_node {
-                    let parent = self.state.dom.parent_mut(list_handle);
-                    // TODO: should probably append a paragraph instead
-                    parent.append_child(DomNode::new_zwsp());
-                    let new_location = Location::from(
-                        current_cursor_global_location - li_len + 1,
-                    );
-                    self.state.start = new_location;
-                    self.state.end = new_location;
-                } else {
-                    let new_location =
-                        Location::from(current_cursor_global_location - li_len);
-                    self.state.start = new_location;
-                    self.state.end = new_location;
-                }
-            }
-        }
-    }
-
     pub(crate) fn can_indent_handle(&self, handle: &DomHandle) -> bool {
         let parent = self.state.dom.parent(handle);
         if parent.is_list_item() {

crates/wysiwyg/src/composer_model/replace_text.rs

@@ -137,11 +137,8 @@ where
                             && l.kind == ListItem
                     })
                     .unwrap();
-                let current_cursor_global_location =
-                    leaf.position + leaf.start_offset;
                 self.do_enter_in_list(
                     &list_item.node_handle,
-                    current_cursor_global_location,
                     list_item.end_offset,
                 )
             }

crates/wysiwyg/src/composer_model/selection.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::ops::AddAssign;
+
 use crate::composer_model::menu_state::MenuStateComputeType;
 use crate::{ComposerModel, ComposerUpdate, Location, UnicodeString};
 
@@ -74,6 +76,12 @@ where
         let (s, e) = self.safe_selection();
         s == e
     }
+
+    /// Increment both the start and the end of the selection by given isize.
+    pub(crate) fn increment_selection(&mut self, rhs: isize) {
+        self.state.start.add_assign(rhs);
+        self.state.end.add_assign(rhs);
+    }
 }
 
 #[cfg(test)]

crates/wysiwyg/src/dom/dom_list_methods.rs

@@ -152,6 +152,25 @@ where
         }
         self.join_nodes_in_container(&handle.parent_handle());
     }
+
+    /// Slice list item at given handle and offset. A ZWSP node
+    /// is added at the beginning of the created list item.
+    ///
+    /// * `handle` - the list item handle.
+    /// * `offset` - offset at which the list item should be sliced
+    pub(crate) fn slice_list_item(
+        &mut self,
+        handle: &DomHandle,
+        offset: usize,
+    ) {
+        let list_item = self.lookup_node_mut(handle);
+        let mut slice = list_item.slice_after(offset);
+        slice.as_container_mut().unwrap().add_leading_zwsp();
+        let list = self.lookup_node_mut(&handle.parent_handle());
+        let DomNode::Container(list) = list else { panic!("List node is not a container") };
+        list.insert_child(handle.index_in_parent() + 1, slice);
+        self.join_nodes_in_container(&handle.parent_handle());
+    }
 }
 
 #[cfg(test)]
@@ -354,6 +373,22 @@ mod test {
         assert_eq!(ds(&dom), "abc<br />def<br />ghi");
     }
 
+    #[test]
+    fn slice_list_item() {
+        let mut dom = cm("<em>abcd</em>ef|").state.dom;
+        dom.wrap_nodes_in_list(
+            ListType::Ordered,
+            vec![&DomHandle::from_raw(vec![0]), &DomHandle::from_raw(vec![1])],
+        );
+        assert_eq!(ds(&dom), "<ol><li><em>~abcd</em>ef</li></ol>");
+
+        dom.slice_list_item(&DomHandle::from_raw(vec![0, 0]), 4);
+        assert_eq!(
+            ds(&dom),
+            "<ol><li><em>~abc</em></li><li><em>~d</em>ef</li></ol>"
+        );
+    }
+
     #[test]
     fn wrap_and_remove_lists() {
         list_roundtrips("abc<strong>de<em>f<br />gh</em>i</strong>jkl|");

crates/wysiwyg/src/dom/nodes/container_node.rs

@@ -408,36 +408,34 @@ where
         // Note: handle might not be set in cases where we are transforming a node
         // that is detached from the DOM. In that case it's fine to transform it
         // without worrying about handles.
-        let handle_is_set = self.handle().is_set();
+        fn insert_zwsp<S>(container: &mut ContainerNode<S>)
+        where
+            S: UnicodeString,
+        {
+            if container.handle().is_set() {
+                container.insert_child(0, DomNode::new_zwsp());
+            } else {
+                container.children.insert(0, DomNode::new_zwsp());
+            }
+        }
+
         let Some(first_child) = self.children.get_mut(0) else {
-            return false;
+            insert_zwsp(self);
+            return true;
         };
         match first_child {
             DomNode::Container(c) => c.add_leading_zwsp(),
             DomNode::Zwsp(_) => false,
-            DomNode::Text(t) => {
-                match (handle_is_set, t.data().is_empty()) {
-                    (true, true) => {
-                        self.replace_child(0, vec![DomNode::new_zwsp()]);
-                    }
-                    (true, false) => {
-                        self.insert_child(0, DomNode::new_zwsp());
-                    }
-                    (false, true) => {
-                        self.children[0] = DomNode::new_zwsp();
-                    }
-                    (false, false) => {
-                        self.children.insert(0, DomNode::new_zwsp());
-                    }
+            DomNode::Text(t) if t.data().is_empty() => {
+                if self.handle().is_set() {
+                    self.replace_child(0, vec![DomNode::new_zwsp()]);
+                } else {
+                    self.children[0] = DomNode::new_zwsp();
                 }
                 true
             }
-            DomNode::LineBreak(_) => {
-                if handle_is_set {
-                    self.insert_child(0, DomNode::new_zwsp());
-                } else {
-                    self.children.insert(0, DomNode::new_zwsp());
-                }
+            DomNode::Text(_) | DomNode::LineBreak(_) => {
+                insert_zwsp(self);
                 true
             }
         }
@@ -452,7 +450,15 @@ where
             return false;
         };
         match first_child {
-            DomNode::Container(c) => c.remove_leading_zwsp(),
+            DomNode::Container(c) => {
+                // Remove the entire container if all it contains is the ZWSP.
+                if c.text_len() == 1 && c.has_leading_zwsp() {
+                    self.remove_child(0);
+                    true
+                } else {
+                    c.remove_leading_zwsp()
+                }
+            }
             DomNode::Zwsp(_) => {
                 // Note: handle might not be set in cases where we are transforming a node
                 // that is detached from the DOM. In that case it's fine to transform it