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 batch sub-types #10967

Closed
scofalik opened this issue Dec 6, 2021 · 2 comments · Fixed by #11009
Closed

Introduce batch sub-types #10967

scofalik opened this issue Dec 6, 2021 · 2 comments · Fixed by #11009
Assignees
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@scofalik
Copy link
Contributor

scofalik commented Dec 6, 2021

Recently, we discussed some problems connected with remote operations. In general, at the moment, there's no flag or easy way to check whether given batch contains operations created locally or coming from a remote client through real-time collaboration.

Related tickets are #9233, #10582. Solving autosave issue will also be helpful in revision history feature.

Proposed solution:

At the moment, we have Batch#type property. AFAIR, we have only one batch type, i.e. transparent and it has very wide meaning. Because of that, we can't really use this information. For example, remote operations are only in transparent batch, but this is not the only usage for transparent batches. So, we can't use this information in determining how the algorithm should work if we want to target remote operations only.

Instead we propose dropping type property in the favor of a set of more precise flags, e.g.:

  • Batch#isLocal (default true),
  • Batch#isTyping (default false, replacement for Input#isInput),
  • Batch#isTransparent (default false, not sure if needed),
  • Batch#isUndo (default false, not sure if needed).
  • Batch#isUndoable (kind of opposite to today's 'transparent' batch type, as we most probably introduced transparent batches for undo reasons, this is just an idea).

Those flags would be set the same way type is set at the moment:

model.enqueueChange( { isLocal: false }, writer => { ... } );

Using 'transparent' as batch type in those calls would be deprecated.

Questions for refinement:

  1. What about situations where isLocal = false batch has local operations? This could happen if RTC operations triggered a post-fixer.
    1. These are still changes related to remote operations.
    2. They should not be undoable (guaranteed today by being added to a transparent batch).
    3. The question is if such changes should trigger autosave?
  2. Where do we use transparent batches? Where do we set them and where do we read Batch#type?
  3. What flags/subtypes do we need?
  4. Do we still need a generic isTransparent batch subtype? If not, what do to with model.enqueueChange( 'transparent', ... ) calls (we had an idea that they should be mapped to { isTransparent: true }. If there's no isTransparent then we probably need isUndoable and set it correctly.
  5. Should one flag enforce another and how? E.g. isLocal = false means isTransparent = true. (or isUndoable = false).

My answers for now:

  1. The save is probably not needed. If post-fixer happens it means that something changed locally too, so there will be original autosave callback for that.
  2. There are several places where we use transparent batches: undo, autoformat, image upload, typing, data init. However, nothing too complicated.
  3. I'd go with the minimal set for start, i.e. isLocal, isTyping, isTransparent.
  4. I'd go with isTransparent generic subtype because we don't know how it is used in 3rd party code. isTransparent subtype is the closest to what is available right now, so let's stick with it.
  5. I don't see big reason to do this. isLocal will be used only in one place and AFAICS this is the only thing that we would enforce.
@scofalik scofalik added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:engine squad:collaboration Issue to be handled by the Collaboration team. labels Dec 6, 2021
@scofalik scofalik self-assigned this Dec 8, 2021
@scofalik
Copy link
Contributor Author

scofalik commented Dec 14, 2021

Where transparent batch is created:

  • Data controller - autoparagraphing and setting data,
  • Undo - undo batches are transparent,
  • Changing image status during upload,
  • Users markers,
  • Remote operations in collaboration.

Where transparent batch is checked:

  • Undo - ignore some batches, clear undo stack on init (uses transparent batch for that :|),
  • Change buffer - don't reset typing batch on transparent batches (mostly cares about remote operations and maybe image upload status changes),
  • Block and inline autoformat + text watcher - I assume this is to filter remote operations triggering those features maybe to also filter undo,
  • Track changes - skip batches coming from undo in one of the mechanisms.

I hope I didn't miss anything.

For me, it looks like a mixed bag of everything. It feels that it hold together by a coincidence, because some uses and checks are separate groups of scenarios that do not intersect.

From these uses and checks, only change buffer case seems to me like it is more general and would like to ignore some general kind of "transparent" batches. Other than that we have clear groups of uses and checks. Except of change buffer case, I now think that general isTransparent option may be actually problematic.

I propose:

  • isUndo - batch created by undo/redo features,
  • isUndoable / canBeUndone / canUndo / isBackground - batch that should be skipped by undo. isBackground sounds close to isTransparent but we explicitly write in documentation that this means that it will be skipped by undo and nothing more,
  • isLocal - this was already agreed on.

Old new Batch( 'transparent' ) and model.enqueueChange( 'transparent', ... ) calls will set isUndoable to false.

@Reinmar
Copy link
Member

Reinmar commented Dec 14, 2021

  • isUndoable

This, to make it clear (isBackground is as bad as isTransparent in this case) and also start with is*.

scofalik added a commit that referenced this issue Jan 11, 2022
Feature: Replaced `Batch#type` with a set of flags: `isUndoable`, `isLocal`, `isUndo`, `isTyping` which better represent the batch type. `Batch` constructor and `Model#enqueueChange()` now expect an object. Closes #10967.

Fix (typing): Fixed editor crash when an unrecognized transformation was given in configuration (as a string).

Other (typing): Typing feature will now create batches with `isTyping` set to `true`.

Other (undo): Undo feature will now create batches with `isUndo` set to `true`.

BREAKING CHANGE (engine): `Batch#type` has been deprecated. It will always return `'default'` value and reading it will log a warning in the console. Use `Batch#isUndoable`, `#isLocal`, `#isUndo` and `#isTyping` instead.

BREAKING CHANGE (typing): `Input#isInput()` has been removed. Use `Batch#isTyping` instead.

MINOR BREAKING CHANGE (engine): String value for `Batch` type and `Model#enqueueChange()` is now deprecated. Using string value will log a warning in the console. Use an object value instead. For more information, refer to the API documentation.
@AnnaTomanek AnnaTomanek added this to the iteration 50 milestone Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. 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.

3 participants