Opened 8 years ago

Closed 8 years ago

#4445 closed New Feature (fixed)

Support callback param for CKEDITOR.editor::setData

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.1
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description (last modified by Garry Yao)

In order to comply with the asynchronous nature of 'setData' in wysiwyg mode, the user need to listen for a internal "contentDom" event for the very early access to a fully loaded document, we should provide a simpler form API like editor.setData( data, callback ).

Correspondingly, the 'afterSetData' event which already existed now should be fired right before we invoke this callback function, so user could make their choice between these two approaches.

Attachments (3)

4445.patch (3.5 KB) - added by Garry Yao 8 years ago.
4445_2.patch (3.5 KB) - added by Garry Yao 8 years ago.
4445_3.patch (3.2 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by Garry Yao

Description: modified (diff)
Owner: set to Garry Yao
Status: newassigned

Tidied up the description a bit.

Changed 8 years ago by Garry Yao

Attachment: 4445.patch added

comment:2 Changed 8 years ago by Garry Yao

Keywords: Review? added

comment:3 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

There are several issues to be considered here:

  1. Coding style: this thing is happening frequently, so it's better to point it out. The curly braces are optional for if/else/while/for blocks only if the block code is composed by a single line of code. Therefore line 590 in _source/core/editor.js is wrong.
  1. The "contentDom" solution pointed out in the ticket description is actually available for the WYSIWYG mode only. We must still consider that setData can be called in any mode, including Source mode. At this point, it makes sense introducing a new event, called "dataReady" that is thrown by each of the mode plugins. This event could be used by the setData function to call the callback.
  1. The editor._.isLoadingData trick is not needed anymore at this point, as we must guarantee that the "dataReady" event will be always fired, so we can safely use it only.
  1. I don't get the point about having to 300ms delay. This is not a "short delay" in any way, and quite undesirable.

comment:4 Changed 8 years ago by Garry Yao

Milestone: CKEditor 3.1CKEditor 3.0.1

Changed 8 years ago by Garry Yao

Attachment: 4445_2.patch added

comment:5 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Coming with a new patch targeting Fred's proposal.

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I'm not being able to apply the patch on _source/plugins/sourcearea/plugin.js and _source/core/editor.js, probably due to the "?" at the start of the files. Can you please update your local copy and create the patch again, without those changes at line 1 in those files.

Also, instead of purely call callback(), lets do instead callback.call( editor ), just to make it more flexible.

Changed 8 years ago by Garry Yao

Attachment: 4445_3.patch added

comment:7 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Ticket Test was updated at : http://ckeditor.t/tt/4445/1(2).html

comment:8 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

The above TC, other than pointing to the wrong URL, doesn't fail currently. The page actually gets broken. works well after the patch though.

comment:9 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed
Type: BugNew Feature

Fixed with [4354] (including the TC fixes).

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