Changes between Version 7 and Version 12 of Ticket #12341

Aug 28, 2014, 1:48:25 PM (9 years ago)
Piotrek Koszuliński

Replying to comment:9:

The thing about variables is that they make relations between conditions. If one variable is used in two of them that gives you some information - e.g. that you must change both occurrences. Example from the last ticket I reviewed - you changed clipboardIdDataType in one place but forgot about tests e.g, because it's not used there. There you again check CKEDITOR.env.

What I ask you to do is to look at the code and find patterns. Then create a clipboard's env object that will contain flags which will be used in dev and in tests.

If the meaning of some condition is "because IE does it in its own way" and this is the single place where it happens - leave it. But I'm 100% sure that you will find more patterns, because there are 72 changes involving CKEDITOR.env in major already. But remember to that you're not bound to your narrow field - e.g. a condition used for choosing drop target is used at least few times in many places. So far it was used in widgets only, but it's spread around the code base recently.

Another example:

 if ( ) {
	return new this.dataTransfer( null, sourceEditor );

Are you certain that this is the only place where we exclude/include browsers that don't have full support for data transfer? I can see at least one more in addPasteListenersToEditable.

Look also for occurrences of 'Text' and 'URL'. There are two places where they are used together most likely for the same reason.

You need to remember that we will be maintaining this code for many years. And things that makes some sense now will be a total WTH in couple of months for all of us, including you. And since I found them confusing now, I'm sure that there's a room for improvements.


  • Ticket #12341

    • Property Status changed from assigned to review_failed
  • Ticket #12341 – Description

    v7 v12  
    66* In `initPasteDataTransfer` and `initDropDataTransfer` you expect first argument to be what? Because docs says it's native event, when it's not.
    77* I mentioned this during one of previous reviews and I noticed this again - we can't write conditional code based on repeating `` usage. This makes code hard to read and hard to maintain, because it's unclear what this condition means - why IE, why not other browsers, whether IE12, 13 too or not. You need to create meaningfull variables like `dataTransferSetCustomDataSupport` and use them. E.g. in `pasteDataFromClipboard` you created `htmlAlwaysInDataTransfer` - it's great, just make it global for this plugin and then create more of them.
    8 * `CKEDITOR.DATA_TRANSFER_INTERNAL` flags should start from 1 (1, 2, 4).
     8* `CKEDITOR.DATA_TRANSFER_INTERNAL` flags should start from 1 (1, 2, 3).
    1010TC1 (Chrome,FF):
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy