Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a way to check if changes were introduced by typing #3158

Closed
scofalik opened this issue Jul 22, 2019 · 8 comments · Fixed by ckeditor/ckeditor5-typing#213
Closed
Assignees
Labels
package:typing type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@scofalik
Copy link
Contributor

I think it would be useful to have a way to check if given changes were done through typing. I see it as a method on Typing or InputCommand instance.

This could be solved in one of two ways:

  1. Add special batch type (yeah, we made precedence with undo :P).
  2. Store all InputCommand batches in some kind of a WeakSet.

Then, we could use it like this:

modelDocument.on( 'change', ( evt, batch ) => {
    if ( typingPlugin.isTyping( batch ) ) {
        // ...
    }
} );
modelDocument.registerPostFixer( writer => {
    if ( typingPlugin.isTyping( writer.batch ) ) {
        // ...
    }
} );

When this is done, we should fix changes introduced by this PR: https://github.com/ckeditor/ckeditor5-typing/pull/213/files

@Reinmar
Copy link
Member

Reinmar commented Jul 22, 2019

Sounds really interesting. 👍

@jodator
Copy link
Contributor

jodator commented Jul 22, 2019

I'm also in favor of such change. There are many cases in which we need such check and some standardized way for this would be awesome.

I like the typing batch type (and possibly create undo batch types also).

@Reinmar
Copy link
Member

Reinmar commented Jul 22, 2019

When this is done, we should fix changes introduced by this PR: https://github.com/ckeditor/ckeditor5-typing/pull/213/files

If this PR needs to add some API that we'd be removing after closing this ticket, I'd do that right now.

@Reinmar
Copy link
Member

Reinmar commented Jul 22, 2019

I like the typing batch type (and possibly create undo batch types also).

I don't know the situation and I can't tell if this makes sense in a long run – whether we'll not need dozens of types and/or those types will become unreliable for some reason (by getting weaker meaning or something). Once we'll start introducing them there will be no way back.

2. Store all InputCommand batches in some kind of a WeakSet.

So, unless you're really sure about the new types, I'd go for local changes, like the proposed here.

@jodator jodator self-assigned this Jul 22, 2019
@jodator
Copy link
Contributor

jodator commented Jul 22, 2019

Quick questions about API:

  1. Shouldn't we check isInput() vs isTyping() - technically we would like to check user input as in Input plugin.
  2. Resolving the isInput() vs isTyping() will resolve where the method lands. For me it looks like inputPlugin.isInput( batch ) - other wise as proposed by @scofalik.
  3. Should we expose the InputCommand.batches? I'd say this is protected but OTOH the InputCommand.buffer is exposed.

@Reinmar
Copy link
Member

Reinmar commented Jul 22, 2019

Ad 1. Typing = input + delete. So we can even have multiple methods for that in multiple places.

Ad 3. What would be the purpose of exposing it?

@jodator
Copy link
Contributor

jodator commented Jul 23, 2019

Ad 3. What would be the purpose of exposing it?

None probably. I'd leave that as private.

@scofalik
Copy link
Contributor Author

Probably not worth exposing right now. I use input buffer batches in track changes (and AFAIR they are used somewhere else too) but that's the "current batch" and this is an important property. Old batches, stored in a WeakMap are probably not useful.

scofalik referenced this issue in ckeditor/ckeditor5-typing Jul 24, 2019
Feature: Introduced `Input#isInput()`. Closes #214. Fixed the `TextTransformation` feature so it willl trigger only for typing changes. Closes #208.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 26 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:typing labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:typing type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants