-
Notifications
You must be signed in to change notification settings - Fork 7
T/48 #71
Conversation
src/inputcommand.js
Outdated
* @protected | ||
* @member {typing.ChangeBuffer} #_buffer | ||
*/ | ||
this._buffer = new ChangeBuffer( editor.document, editor.config.get( 'typing.undoStep' ) || 20 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be given to the command as an option, not taken by it from the editor. A class like a command should depend on the world in as little details as possible.
src/input.js
Outdated
} ); | ||
} | ||
|
||
/** | ||
* Handles DOM mutations. | ||
* | ||
* @param {Array.<engine.view.Document~MutatatedText|engine.view.Document~MutatatedChildren>} mutations | ||
* @param {Array.<module:engine/view/document~MutatatedText|module:engine/view/document~MutatatedChildren>} mutations | ||
* @param {module:engine/controller/editingcontroller~EditingController} viewSelection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong type.
src/inputcommand.js
Outdated
* @param {String} [options.text=''] Text to be inserted. | ||
* @param {module:engine/model/range~Range} [options.range=null] Range in which the text is inserted. Defaults | ||
* to first range in the current selection. | ||
* @param {module:engine/model/position~Position} [options.selectionAnchor] Selection anchor which will be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command should not need to accept the selection anchor (I don't even know how it's used). It should calculate the result selection position based on the given target range to remove and text which it's inserting. At least... I hope that it can be done this way ;>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simple case it works as you described. But there are cases like e.g. spell checking and probably it was the reason that modelSelectionPosition was introduced at the first place.
If there is text like athat
and spell checking is used to replaced it with that
, based on mutation there is range like [a]that
and inserted text is empty. I am not sure if it is possible to calculate proper selection position based on that (I assume after replacing the position should be after replaced/corrected word)?
I may propose changing the option name to e.g. targetPosition
which may be a little more meaningful, unless ofc there is a way to correctly calculate selection in those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you replace athat
with that
and according to the engine, the selection should be after "that", then the passes options.range
should be [athat]
and options.text = 'that'
. The input command will remove the whole "athat", insert "that" and place the selection after "that". You don't need options.selectionAnchor
for that.
I understand that this may not be fully intuitive at first, but think about this from OT perspective. If you're fixing the word's "athat" spelling you're replacing it with "that". The whole word, not just a single letter. This may be crucial when resolving some conflicting changes and, in general, is more semantical.
Hence, try to get rid of selectionAnchor
. I wrote before that I'm not sure if it's doable, though, because I haven't checked input feature's implementation. Why did you implement it in the first place? Was it already strictly required by what the mutation observer produces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was relying too much on what information we already have calculated based on mutations.
For case described above the mutation provided by mutation observer is:
newText: "foo that bar"
node: Text
oldText: "foo athat bar"
type: "text"
I assume based only on this information it is not possible to reliable calculate the range and text which will be passed to InputCommand
, unless we may assume that the range (and text) will consist of all words which were modified by mutation. If not I think the mutation observer should be modified to provide some additional information in mutations.
The other spellchecking cases to consider are:
Foo hous a
-> Foo house a
Foo hous e
-> Foo house
Foo athat
-> Food that
Foo xhat
-> Foo chat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, forgot about viewSelection
which is passed along with mutations, will see if it will help solve this one :)
src/input.js
Outdated
} | ||
this.editor.execute( 'input', { | ||
text: mutation.newText.substr( firstChangeAt, insertions ), | ||
range: deletions > 0 ? ModelRange.createFromPositionAndShift( modelPos, deletions ) : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the consts out of this object literal. Those variables can make the code more readable (and easier to debug) thanks to their names.
src/inputcommand.js
Outdated
/** | ||
* The input command. Used by the {@link module:typing/input~Input input feature} to handle typing. | ||
* | ||
* @extends core.command.Command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old notation. Now should be @extends {module:blebleble~Command}
.
src/inputcommand.js
Outdated
* Typing's change buffer used to group subsequent changes into batches. | ||
* | ||
* @protected | ||
* @member {typing.ChangeBuffer} #_buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old type notation as well.
src/inputcommand.js
Outdated
* | ||
* @param {Object} [options] The command options. | ||
* @param {String} [options.text=''] Text to be inserted. | ||
* @param {module:engine/model/range~Range} [options.range=null] Range in which the text is inserted. Defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the range could default to first selection's range if not passed. Will be pretty useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, it is stated in the comment: Range in which the text is inserted. Defaults to first range in the current selection. However, it is no stated in the param specs, should it be like [options.range] plus description
or something like [options.range=document.selection.getFirstRange()]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be [options.range] plus description
. Can also be the latter. But definitely not =null
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense, I'll go with first one.
Pushed changes suggested by @Reinmar. There are two things which may require further discussion or some changes, see: #71 (comment) and #71 (comment). |
src/input.js
Outdated
* | ||
* @member {Number} #insertedCharacterCount | ||
* @type {module:typing/changebuffer~ChangeBuffer} #buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be @member
.
Corrected the docs. |
I commented on the two aspects you mentioned. The most important bit is to try to avoid the 3rd option. Also, you'll need to resolve conflicts now. PS. You may not know yet – you shouldn't rebase a branch once it's after any review because this will break comments on GH. So merge master to it. |
I moved |
I fixed the API docs a bit. I didn't like one thing – the fact that Another thing is that the range doesn't have to be checked too. I don't see a case today in which selection could not have any range in it. The document selection will return so called "default range" and if you provide any custom selection, with no range inside, let the code throw. At least we'll know the situation occurs so we'll be able to fix it. Now, it's a silent error. |
Of course, let's do that unless we already see that there may be no range or no text :D. Let's then think how to handle this situation. |
Well, it does not look like making much sense indeed. But without checking if text was passed and passing non-collapsed range at the same time to the command will result in removing the content within the range without inserting anything. So we should decide if it is an acceptable behavior or some error should be thrown if text was not passed. |
src/inputcommand.js
Outdated
const range = options.range || doc.selection.getFirstRange(); | ||
const resultPosition = options.resultPosition; | ||
|
||
let textInsertions = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
Made |
Feature: Introduced
InputCommand
used for typing handling. Closes ckeditor/ckeditor5#3058.