T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

ma2ciek · 2017-04-07T09:12:47Z

Suggested merge commit message (convention)

Feature: Introduced DataTransfer#files property. Changed the clipboardInput event and added the inputTranformation event. Closes ckeditor/ckeditor5#2656.

ma2ciek · 2017-04-07T09:27:50Z

src/clipboard.js

@@ -95,7 +95,7 @@ export default class Clipboard extends Plugin {
 	 * @inheritDoc
 	 */
 	static get pluginName() {
-		return 'clipboard/clipboard';


@Reinmar Not sure whether this should be a clipboard or clipboard/clipboard

See the documentation of Plugin.pluginName. It should be clipboard/clipboard – one is the package name (meaningful part of it) and the other is the module inside it.

pjasiun · 2017-04-11T09:55:41Z

Note that this PR also introduced "files" in the dataTransfer object. It should be mentioned in the commit message.

pjasiun · 2017-04-11T09:56:18Z

Also the files property miss documentation.

pjasiun · 2017-04-11T10:00:28Z

src/clipboard.js

@@ -179,34 +191,28 @@ export default class Clipboard extends Plugin {
 }

 /**
- * Fired with a content which comes from the clipboard (was pasted or dropped) and
+ * Fired with a `dataTransfer`, which comes from the clipboard (was pasted or dropped) and
 * should be processed in order to be inserted into the editor.
 * It's part of the {@link module:clipboard/clipboard~Clipboard "clipboard pipeline"}.


This is observers event now. Docs should be moved to the clipboard observer.

ma2ciek · 2017-04-11T11:42:34Z

Note that this PR also introduced "files" in the dataTransfer object. It should be mentioned in the commit message.

Fixed.

Added some docs improvements.

Reinmar · 2017-04-11T13:25:55Z

You know guys that the #files prop makes for a perfect separate PR? ;) But ok... I'll review both things.

Reinmar · 2017-04-11T13:30:16Z

src/clipboardobserver.js

+			// Prevent page refreshing.
+			data.preventDefault();
+
+			doc.fire( 'input', { dataTransfer: data.dataTransfer } );


This event is really confusing. There's a nativeinput event. So this one would need to be called clipboardInput. Is it ok?

That's fine for me.

Reinmar · 2017-04-11T13:31:59Z

src/clipboardobserver.js

+		doc.on( 'drop', _handleInput, { priority: 'low' } );
+
+		function _handleInput( evt, data ) {
+			// Prevent page refreshing.


This is not true. This line doesn't prevent page refreshing. It prevents the default action (which only in case of file dropping can be page reload). No reason to comment it.

Reinmar · 2017-04-11T13:32:42Z

src/clipboardobserver.js

+		doc.on( 'paste', _handleInput, { priority: 'low' } );
+		doc.on( 'drop', _handleInput, { priority: 'low' } );
+
+		function _handleInput( evt, data ) {


No reason to prefix this function name with _.

Reinmar · 2017-04-11T13:33:37Z

src/clipboardobserver.js

-		this.domEventType = [ 'paste', 'copy', 'cut' ];
+		this.domEventType = [ 'paste', 'copy', 'cut', 'drop' ];
+
+		doc.on( 'paste', _handleInput, { priority: 'low' } );


This is unsafe and incorrect. this.listenTo() should be used. Those listeners should be gone together with the observer.

Reinmar · 2017-04-11T13:35:07Z

src/datatransfer.js

@@ -42,4 +50,26 @@ export default class DataTransfer {
 	setData( type, data ) {
 		this._native.setData( type, data );
 	}
+
+	*_getFiles() {


Why is this function a generator if above we do Array.from() anyway? It doesn't make sense.

Besides, it needs at least minimal docs. Or even better – move it out of the prototype as it doesn't make sense here (unless for testing purposes, but then document this too).

That's because of the historical reasons. There were no files at the very beginning because there are some performance problems with reading files handlers, but then I realized that you need to get files handlers anyway to check if there are files, so the property was created. That's true that there is not a reason to keep it as a generator.

Reinmar · 2017-04-11T13:35:24Z

src/datatransfer.js

+		if ( files.length ) {
+			yield* files;
+		}
+		// // Chrome have empty DataTransfer.files, but let get files through the items interface.


Doubled //.

Reinmar · 2017-04-11T13:36:18Z

src/datatransfer.js

+		}
+	}
+
+	get types() {


Properties should be before methods and public stuff must be before protected and private stuff. Plus, it misses docs.

Reinmar · 2017-04-11T13:37:13Z

src/datatransfer.js

+		/**
+		 * The array of files created from the native `DataTransfer#files` and `DataTransfer#items` objects.
+		 *
+		 * @public


It's also @readonly.

Reinmar · 2017-04-11T13:38:04Z

src/datatransfer.js

@@ -19,6 +19,14 @@ export default class DataTransfer {
 		 * @member {DataTransfer} #_native
 		 */
 		this._native = nativeDataTransfer;
+
+		/**
+		 * The array of files created from the native `DataTransfer#files` and `DataTransfer#items` objects.


This is confusing – it indicates that #files is a combination of these two native properties while it's either one or another.

Reinmar · 2017-04-11T13:38:29Z

src/clipboard.js

+ *		this.listenTo( editor.editing.view, 'input', ( evt, data ) => {
+ *			const content = customTransform( data.dataTransfer.get( 'text/html' ) );
+ *			const transformedContent = transform( content );
+ * 			data.dataTransfer.set( 'text/html', transformedContent );


Indentation.

Reinmar · 2017-04-11T13:47:53Z

src/clipboard.js

@@ -50,7 +64,7 @@ import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/html
 * At this stage the pasted content can be processed by the features. E.g. a feature which wants to transform
 * a pasted text into a link can be implemented in this way:
 *
- *		this.listenTo( editor.editing.view, 'clipboardInput', ( evt, data ) => {
+ *		this.listenTo( editor.plugins.get( 'clipboard/clipboard' ), 'inputTransformation', ( evt, data ) => {


Hm... I'm not sure that it makes sense to showcase this method of retrieving a plugin. It may not be loaded and such code would break. OTOH, showing the right way would make this code unnecessarily long, so perhaps we can leave this.

Reinmar · 2017-04-11T14:21:37Z

R- for now. I haven't checked all but the rest should be cosmetics.

ma2ciek · 2017-04-11T15:24:20Z

It's ready for the review once again.

Feature: Introduced DataTransfer#files property. Change the clipboard input pipeline. Closes #16. BREAKING CHANGE: The `clipboardInput` event now contains only `DataTransfer`, no `content` there anymore. The separate `inputTransformation` event was introduced for the content transformations.

Piotr Jasiun and others added 7 commits March 20, 2017 11:06

Added getFiles and getTypes to the data transfer object.

82032ab

Added drop basic handling.

658c7a7

Split clipboardInput event into input and clipboardInput.

8d848cc

Fire inputTransformation on clipboard plugin. Fix bugs.

f583d0c

Aligned tests to the new API.

960b8e9

Fixed coverage in datatransfer.

4031792

Fixed code coverage in clipboardobserver.

0a96e79

ma2ciek added the status:review- label Apr 7, 2017

ma2ciek requested review from Reinmar and pjasiun April 7, 2017 09:12

ma2ciek added 2 commits April 7, 2017 11:19

Merge branch 'master' into t/16

c8299d7

Fixed pluginName method.

2ddf971

ma2ciek commented Apr 7, 2017

View reviewed changes

ma2ciek added 5 commits April 7, 2017 13:43

Fixed clipboard plugin name.

4080c9e

Added first batch of docs changes.

bf6b5fe

Added missing event docs.

c980d48

Added more docs.

81e9376

Fixed invalid event names.

845c2ba

ma2ciek removed the status:review- label Apr 7, 2017

pjasiun reviewed Apr 11, 2017

View reviewed changes

pjasiun added the status:review- label Apr 11, 2017

Fixed docs and manual tests.

fdc1d77

ma2ciek changed the title ~~T/16 Split the clipboardInput event into the input and inputTranformation events~~ T/16 Introduce DataTransfer#files property and split the clipboardInput event into the input and inputTranformation events Apr 11, 2017

ma2ciek removed the status:review- label Apr 11, 2017

Merge branch 'master' into t/16

25958e2

Reinmar reviewed Apr 11, 2017

View reviewed changes

szymonkups mentioned this pull request Apr 11, 2017

T/1 Upload initial implementation. ckeditor/ckeditor5-upload#11

Merged

Reinmar added the status:review- label Apr 11, 2017

ma2ciek added 2 commits April 11, 2017 16:58

Renamed event. Switched listening method to listenTo.

3c160f8

Fixed DataTransfer class.

8d38cfc

ma2ciek changed the title ~~T/16 Introduce DataTransfer#files property and split the clipboardInput event into the input and inputTranformation events~~ T/16 Introduce DataTransfer#files property and split the clipboardInput event into the clipboardInput and inputTranformation events Apr 11, 2017

ma2ciek changed the title ~~T/16 Introduce DataTransfer#files property and split the clipboardInput event into the clipboardInput and inputTranformation events~~ T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event Apr 11, 2017

ma2ciek removed the status:review- label Apr 11, 2017

pjasiun merged commit 16e8e2a into master Apr 12, 2017

pjasiun deleted the t/16 branch April 12, 2017 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

ma2ciek commented Apr 7, 2017 •

edited by pomek

Loading

ma2ciek Apr 7, 2017 •

edited

Loading

Reinmar Apr 7, 2017 •

edited

Loading

pjasiun commented Apr 11, 2017

pjasiun commented Apr 11, 2017

pjasiun Apr 11, 2017

ma2ciek commented Apr 11, 2017

Reinmar commented Apr 11, 2017

Reinmar Apr 11, 2017

pjasiun Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

pjasiun Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar Apr 11, 2017

Reinmar commented Apr 11, 2017

ma2ciek commented Apr 11, 2017

T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

Conversation

ma2ciek commented Apr 7, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

ma2ciek Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Reinmar Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

pjasiun commented Apr 11, 2017

pjasiun commented Apr 11, 2017

Choose a reason for hiding this comment

ma2ciek commented Apr 11, 2017

Reinmar commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Apr 11, 2017

ma2ciek commented Apr 11, 2017

ma2ciek commented Apr 7, 2017 •

edited by pomek

Loading

ma2ciek Apr 7, 2017 •

edited

Loading

Reinmar Apr 7, 2017 •

edited

Loading