Ticket #5268 (closed New Feature: fixed)

Opened 5 years ago

Last modified 4 years ago

Port DragResizeTable to V3

Reported by: alfonsoml Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.2
Keywords: Confirmed Review+ Cc:

Description

Several people have requested the upgrade of this plugin

Attachments

5268.patch (12.9 KB) - added by garry.yao 4 years ago.
5268_2.patch (11.5 KB) - added by garry.yao 4 years ago.
5268_3.patch (12.8 KB) - added by garry.yao 4 years ago.

Change History

Changed 4 years ago by garry.yao

comment:1 Changed 4 years ago by garry.yao

  • Owner set to garry.yao
  • Keywords Review? added
  • Status changed from new to assigned
  • Milestone set to CKEditor 3.3

The patch recreated the v2 plugin with the same functionality.

comment:2 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

comment:3 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed
  • This one is supposed to be an extra plugin, not included in the default list of plugins.
  • Let's also separate things a bit, to not make the core bloated. The buildTableMap function can even be appended to the CKEDITOR.tools object, but it should still be defined in the tabletools plugin. That plugin will then enter in the requires list for the tableresize plugin.

In the plugin code:

  • We don't need to have those functions defined in the private "_" property of the ColumnResizer class. I believe most of them can be private functions in the main anonymous function.
  • The class name should be columnResizer.
  • There are some jslint warnings in the code.
  • It happens to have decimal precision in the sizes, like "42.5px". Let's round them to have only integers.

Changed 4 years ago by garry.yao

comment:4 follow-up: ↓ 5 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

We don't need to have those functions defined in the private "_" property of the ColumnResizer class.

I don't see problems in this case, since they're event handlers that bind to the columnResizer instance, scoping them as closure bring runtime performance penalty as well.

comment:5 in reply to: ↑ 4 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed

Replying to garry.yao:

I don't see problems in this case, since they're event handlers that bind to the columnResizer instance,

The benefit of having them our of the "_" is that the code will compact much better on release. Other than having the function names reduced, their calls will result very shorter.

scoping them as closure bring runtime performance penalty as well.

Yes, a book theory. You'll never be able to demonstrate it in this specific case.

Changed 4 years ago by garry.yao

comment:6 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

New patch with the following points addressed:

  1. Performance: Pillar discovery algorithm reduced from O(n2) to linear with table pillar caching enable;
  2. Better code packings, 0.8K reduced by utilizing module pattern.

comment:7 Changed 4 years ago by fredck

  • Keywords Review+ added; Review? removed

The performance enhancements are noticeable, and wonderful. Congratulations for it.

Please have this one committed into the 3.4.x branch.

comment:8 Changed 4 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [5572] on 3.4.x branch.

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