Hi all, I'm noticing I'm somewhat confused about which coordinate system belongs in which layers. I'm doing a quick investigation, and documenting the results here, to hopefully spark a discussion about which of these we actually need, which can be better hidden to prevent accidents etc. As near as I can tell, there are effectively two "classes" of positions available: 1. Low level positioning Directly available from PositionIterator. The most common one used is Simple Coordinates + filtered elements. This does NOT correspond to possible cursor positions as an extra layer of filtering is required for that. A position iterator is available from: gui.SelectionMover.createPositionIterator PositionIterator has the following offsets available PositionIterator.offset() - "Simple Coordinates" - 8 usages in prod code, scattered - Filters out empty text nodes. - Only ever instantiated from within SelectionMover, which configures the iterator to also ignore cursor & edit info nodes - Adjacent text nodes count as 1 position to the parent container e.g., <p>" " <editinfo/> " B" "C"|</p>, the cursor just after "C" would report an offset of 1 PositionIterator.domOffset() - "Filtered Coordinates" - 1 usage in entire codebase - Filters out empty text nodes. - Only ever instantiated from within SelectionMover, which configures the iterator to also ignore cursor & edit info nodes - Individual text nodes are counted e.g., <p>" " <editinfo/> " B" "C"|</p>, the cursor just after "C" would report an offset of 2 PositionIterator.unfilteredDomOffset() - "Unfiltered Coordinates" - 7 usages, all in SelectionMover - Filters out nothing. All nodes are counted - Individual text nodes are counted e.g., <p>" " <editinfo/> " B" "C"|</p>, the cursor just after "C" would report an offset of 4 PositionIterator.textOffset - "Aggregated Texts Coordinates" - 1 usage in the entire codebase IN A TEST - Returns the offset within a text node as if the text node was connected to all prior siblings e.g., <p>" " <editinfo/> " B" "C"|</p>, the cursor just after "C" would report an offset of 3 PositionIterator supplies the following two methods for setting it's position PositionIterator.setPosition - 7 usages in prod code, scattered - Takes a filtered container and filtered offset (note, this is Simple Coordinates) - This method CANNOT take Filtered Coordinates (will return the wrong node or run out of children) - This method CANNOT take Aggregated Text Coordinates (will likely throw an error that the offset is longer than the container) - Doesn't check that container is actually valid to pass the filter. If it isn't the iterator starts in a funny spot. PositionIterator.setUnfilteredPosition - 2 usages, scattered - Takes an unfiltered container and unfiltered offset (note, this is Unfiltered Coordinates) - Immediately jumps to the next filtered container & offset - This method cannot take any other coordinates, and will return the wrong position if provided them 2. Cursor-level positioning - "Cursor Steps" Manipulated using the SelectionMover. This indicates the number of cursor steps to move in a direction or count. These are *all* run through the TextPositionFilter which is required for basically every single call to the selection mover. Available from: OdtCursor.getStepCounter. Some things I've noticed: - Most operations work in "Cursor Steps" (OpAddCursor, OpMoveCursor, OpRemoveCursor) - The only way to move a cursor is using it's counting patterns generally, which always require the filter (which is the same for everything), and a root node often (again, usually the same for everything). - Cursor especially exposes a lot of unsafe methods for operating on (e.g., direct access to nodes etc., the returned range selection is in terms of unfiltered container & offsets "Unfiltered Coordinates") - SelectionMover.isPositionWalkable is only ever called once. - It is kind of difficult to go from an arbitrary node + offset to steps & back. E.g., each createOp* command starts with similar sets of calls asking for cursor positions and friends. Next, most op execution begins with "get my cursor, get my cursors position, get my position filter, now start counting" Some things that can be done: - Eliminate extraneous systems, and reduce to two primary coordinates (my vote is that the primary two are Steps & Unfiltered Coordinates). All others should be removed or hidden inside the class that requires them. - Fix iterators to work safely with both systems, and ideally provide a way for the iterator to verify that it's been given the right system, or automatically detect and adapt to the provided type - Figure out who owns a SelectionMover, a NodeFilter, Cursor etc.. Restructure these so that arguments are not continually passed around (e.g., odtDocument filter is passed to everyone, even though its static and central) - Specify the primary communication coordinates for various classes (e.g., "Steps", Unfiltered coordinates). Ideally each class should speak only one coordinate system (with perhaps a little bit of translation) If no-one has major concerns/issues/thoughts, I'll gradually refactor some parts of the stack carefully and submit them tiny bits at a time to try and keep the review workload small and the regression rate tiny. But, either way figured it was worth getting some shared understanding before I started about what the complexity I'm trying to address is, and what I think we should work towards :-) Cheers, Philip