Opened 10 years ago

Closed 10 years ago

#3367 closed New Feature (fixed)

Introduce range iterator API

Reported by: Garry Yao Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description (last modified by Garry Yao)

There're many cases when we need to traverse the range from the start boundary node to the end boundary node, it's archiveable by the following codes:

var boundaryNodes = range.getBoundaryNodes();
		var firstNode = boundaryNodes.startNode;
		var lastNode = boundaryNodes.endNode.getNextSourceNode( true );
var node = firstNode.getNextSourceNode();
while( node )
{
	if ( node.getName() == 'span'  && currentNode.getAttribute( '_fck_bookmark' ) )
	{
		node = node.getNextSourceNode();
		continue;
	}
	// Processing logic...
	node = node.getNextSourceNode();
}

It's cumbersome due to at least two reasons:

  1. The collapsed range problem reported at #3292;
  2. It make code DUP at every plugin for the same logic;

It's necessary to introduce an API for this feature with dom iterator, which already holds a method for iterating over paragraphs. So after the API, other plugins should walking through the range as easy as using the iterator.

Attachments (7)

3367.patch (14.2 KB) - added by Frederico Caldeira Knabben 10 years ago.
3367_2.patch (21.5 KB) - added by Frederico Caldeira Knabben 10 years ago.
test_walker_guard.patch (3.5 KB) - added by Garry Yao 10 years ago.
Unit Test Case
3367_3.patch (24.7 KB) - added by Frederico Caldeira Knabben 10 years ago.
test_range_checkStartOfBlock.patch (2.7 KB) - added by Garry Yao 10 years ago.
Unit Test Case
test_walker_stopAtBody.patch (1.5 KB) - added by Garry Yao 10 years ago.
Unit Test Case
3367_4.patch (827 bytes) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by Garry Yao

Description: modified (diff)

Fred said he'll take care this ticket and refacting task after this fix will be handled by other new opening tickets.

comment:2 Changed 10 years ago by Frederico Caldeira Knabben

Status: newassigned
Type: BugNew Feature

Actually, the idea here is not really replicating the DOM Iterator, but the DOM Walker. I mean, making it possible to traverse all nodes available inside a range boundaries.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 3367.patch added

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added

This patch brings an important change to the DOM Walker concept. It's now limited to a range boundaries, instead of a start/end nodes pair. It makes the walker much more powerful and easy to control.

The few points currently using the walker have been modified to reflect the changes. Several parts of the editor code can be rewritten, simplified and optimized after it.

I've left part of the code commented. Those are features we are not using right now in the editor, but we may need them in the future. As we're uncertain of it at this moment it's better to just leave them there.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 3367_2.patch added

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

This new patch adds the TC file.

comment:5 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

The patch is quite good, while there's two issues:

  1. The default guard function logic on L48 and L62 of document boundary check is invalid.
  2. The guard is not performed on specifically the boundary nodes, Please check the attached TC for this problem.
  3. Some iteration state could be further cached instead of been calculated every iteration, e.g. L71-L82.
  4. The walker should provide a 'reset' function to facilitate reusing of the walker on same range.

Changed 10 years ago by Garry Yao

Attachment: test_walker_guard.patch added

Unit Test Case

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 3367_3.patch added

Changed 10 years ago by Garry Yao

Unit Test Case

comment:6 in reply to:  5 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

I've appended the new tests to the patch.

Replying to garry.yao:

  1. Some iteration state could be further cached instead of been calculated every iteration, e.g. L71-L82.

I've considered that the guard function may change during the iteration. Let's leave it this way for now, just to be safe.

  1. The walker should provide a 'reset' function to facilitate reusing of the walker on same range.

We don't need it into the code currently. It should be introduced if found needed later, but only if the recreation of the object is really hard to achieve.

comment:7 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

Another bug been identified at #3477, please check test_range_checkStartOfBlock.patch for it, though it's not related to the walker core.

comment:8 in reply to:  7 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review- removed

Replying to garry.yao:

Another bug been identified at #3477, please check test_range_checkStartOfBlock.patch for it, Let's fix the problem at dedicated ticket.

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3463].

comment:10 Changed 10 years ago by Garry Yao

Keywords: Review+ removed
Resolution: fixed
Status: closedreopened

Minor issue that document boundary check is invalid, see the attached test_walker_stopAtBody.patch.

Changed 10 years ago by Garry Yao

Unit Test Case

Changed 10 years ago by Garry Yao

Attachment: 3367_4.patch added

comment:11 Changed 10 years ago by Garry Yao

Keywords: Review? added

comment:12 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:13 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: reopenedclosed

Garry's patch has been committed with [3465].

Please do not reopen tickets for related fixes, opening new tickets instead.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy