[SuperEditor] Crashes when `editables` are not in correct order

[raulmabe-labhouse]

I tried to make the simplest editor I could, but instead stayed debugging for half a day why it was crashing.

I was initializing the editor this way (which didn't seem like an issue):

    document = MutableDocument(nodes: [ParagraphNode(id: Editor.createNodeId(), text: AttributedText())]);
    composer = MutableDocumentComposer();
    editor = Editor(
      editables: {
        Editor.composerKey: composer,
        Editor.documentKey: document,
      },
      requestHandlers: defaultRequestHandlers.cast(),
      reactionPipeline: defaultEditorReactions.cast(),
    );

But whenever I was deleting a node, the line at _styler_user_selection.dart L74 (final node = _document.getNodeById(viewModel.nodeId)!; was throwing a null type error.

The fix was including the document as an editable BEFORE the composer, like this:

    document = MutableDocument(nodes: [ParagraphNode(id: Editor.createNodeId(), text: AttributedText())]);
    composer = MutableDocumentComposer();
    editor = Editor(
      editables: {
        Editor.documentKey: document,
        Editor.composerKey: composer,
      },
      requestHandlers: defaultRequestHandlers.cast(),
      reactionPipeline: defaultEditorReactions.cast(),
    );

This issue is not on the docs, nor should be intended to be use like this.

Also, why does the user needs to include the document and the composer as editables? Should SuperEditor work without these editables? If the answer is not then why SuperEditor does not include them by default?

---

I was building a reproducible example but couldn't make it compile...

pubspec.yaml

  super_editor:
    git:
      url: https://github.com/superlistapp/super_editor.git
      ref: stable
      path: super_editor

dependency_overrides:
  super_text_layout:
    git:
      url: https://github.com/superlistapp/super_editor.git
      path: super_text_layout
      ref: stable

  attributed_text:
    git:
      url: https://github.com/superlistapp/super_editor.git
      path: attributed_text
      ref: stable

Compile Error

Error (Xcode): ../../.pub-cache/git/super_editor-1e39961a3c0ab6bc20f32dda3a6e873b9f26e99b/super_editor/lib/src/infrastructure/platforms/mac/mac_ime.dart:149:30: Error: Too many positional arguments: 1 allowed, but 2 found.

[matthew-carroll]

nor should be intended to be use like this.

I agree that the ordering of a Set should not cause a bug like this. We'll need to review the ordering and either make the order irrelevant, or switch to a List to make it clear that order matters.

I was building a reproducible example but couldn't make it compile...

You may need to run flutter pub upgrade to ensure you have the latest commit on stable for all those overridden packages.

Also, why does the user needs to include the document and the composer as editables? Should SuperEditor work without these editables? If the answer is not then why SuperEditor does not include them by default?

Real editors exist to alter document content. If the developer who is using SuperEditor doesn't pass a document in, then that developer won't be able to do anything with the document. Similarly, any editor that one is likely to build will want access to the user's selection, which is held in the composer. Therefore, in practice, a developer will always want to provide those from outside of SuperEditor.

The one confusing detail, however, is that the Document and the DocumentComposer must be given to the Editor object, and also the SuperEditor widget. This might change in the future. It's not clear to me yet what the best approach is.

This issue is not on the docs...

We are in the middle of major changes to SuperEditor. The next release will be the biggest change since the original release. We haven't kept the docs up to date. We'll need to do a full review and update before the next release, along with a migration guide. But we're waiting until we feel more confident about the new APIs that we've introduced.

IF you'd like to update any of the guide code to the current state of the API, I'd be happy to review a PR for that.

[matthew-carroll]

@rutvik110 do you wanna give this ticket a try, while I'm working on the scroll system for your SuperTextField work?

[rutvik110]

Sure! I'll check on it today.

[rutvik110]

@matthew-carroll So I was able to diagnose the root cause of this issue.

With the following order of passing editables to Editor,

 editables: {
        Editor.composerKey: composer,
        Editor.documentKey: document,
      },

This is related to how the SingleColumnLayoutPresenter handles updating the _phaseViewModels it contains and passes to SingleColumnLayoutSelectionStyler that then applies visual selections to document components present within the passed view model.

Currently on document change we do construct a new view model but with the above order of editables, the document change isn't received within presenter till we finish the reaction for the composer on the given request, and we don't have access to the latest view model that reflects changes in the document nodes.

And build phase triggered by the reaction of the composer is where the SingleColumnLayoutSelectionStyler (_styler_user_selection.dart) is dealing with the old SingleColumnLayoutViewModel and tries to apply selection stylings to nodes which are deleted.

Proposed solution:

1. As you suggested we could either move to a list to assert that order is always document > composer,
2. Or we can make a slight change within SingleColumnLayoutPresenter that would update view models if it's not up-to date with the latest document changes, specifically change in the document nodes. A simple update based on second approach that fixes this issue looks like below where we do an additional check on view models components wrt to latest document nodes, and update view model if the nodes has been changed.

_presenter.dart

SingleColumnLayoutPresenter{
  ...
  .....

  SingleColumnLayoutViewModel _createNewViewModel() {
    editorLayoutLog.fine("Running layout presenter pipeline");
    // (Re)generate all dirty phases.
    SingleColumnLayoutViewModel? newViewModel = _getCleanCachedViewModel();

    if (newViewModel == null || newViewModel._componentViewModels.length != _document.nodes.length ) {
      // The document changed. All view models were invalidated. Create a
      // new base document view model.
  ...
  ......

The first approach does seems the most straightforward but with the assumption that users will always pass a single document and a single composer whose orders we can assert. But as this is likely to change in the near future as you mentioned above, we may wanna deal with this at SingleColumnLayoutPresenter level.

Wdyt?

[rutvik110]

@matthew-carroll Could you please share ur thoughts on the above comment?

[matthew-carroll]

@rutvik110 I see that you provided a lot of info about the presenter and the view model. Unfortunately, I can't tell what you're trying to say in the explanation of the problem. Can you boil down the problem to one or two sentences that describe the relationship between the Set of Editables in the Editor and why that Set order is causing a crash within the presenter or view model?

The goal with any situation like this is to provide a clear explanation that doesn't require me, or someone else, to go hunting through code. The current explanation doesn't really expose a lot of useful information without me looking up each class and file you mentioned.

[rutvik110]

ah yah, so whenever we receive a request that affects the Editor, each Editable within the editables responds to the start and end of that request.

The reaction from the document editable at the end of this request is responsible for propagating any changes to the Document to the places whose internal state depends on Document, like SingleColumnLayoutPresenter. And now the consecutive editables may perform actions that may need to be aware of this Document change in their reaction flow like when composer interacts with the SingleColumnLayoutPresenter at the end of the request.

If the order is reversed where the composer reacts to the request before document, then certain parts of composer's reaction flow aren't aware of the document change yet, like SingleColumnLayoutPresenter which is causing the issue in our case.

[rutvik110]

Atm beside the current case of deletion of nodes, I'm not aware of any other scenarios where the order of the editables is important.

But given that the Document change is something important that every part of the SuperEditor that depends on it needs to be aware of asap, we may just wanna make sure the order is always document > composer.

[matthew-carroll]

This is still really tough to follow.

The reaction from the document editable at the end of this request

^ I'm not sure what this means. Reactions are their own artifact. I'm not sure what "reaction from the document" means.

is responsible for propagating any changes to the Document to the places whose internal state depends on Document

^ I don't know what this means.

And now the consecutive editables may perform actions that may need to be aware of this Document change in their reaction flow like when composer interacts with the SingleColumnLayoutPresenter at the end of the request

^ Again, I can't follow what this means.

Perhaps if a general description is too confusing, can you try describing a single execution path, beginning from an editor request, and ending with the exception?

[rutvik110]

The reaction from the document editable at the end of this request

By reaction I meant the .onTransactionStart or .onTransactionEnd methods on Editable which are called at the beginning and end of the request respectively on every individual Editable in editables, when dealing with the received request in Editor, .

is responsible for propagating any changes to the Document to the places whose internal state depends on Document

Those are actually the places that are listening for changes on the Document. Basically, the .onTransactionEnd on document editable, notifies all listeners of the Document change.

One of the places we're listening for Document change is within SingleColumnLayoutPresenter which is responsible for constructing a new SingleColumnLayoutViewModel which is used by SingleColumnLayoutSelectionStyler for styling the selection within the super editor.

When the order is reversed and a selection in an document is deleted(*basically document nodes are deleted), the view model within SingleColumnLayoutPresenter isn't updated wrt the deleted nodes in a Document before it's used by SingleColumnLayoutSelectionStyler, where it tries to access the missing nodes from the Document during styling process but fails to do so as those are not present.

This happens as a result of calling the .onTransactionEnd method on the composer editable before the document editable, which results in a call to update the styling for document nodes.

@matthew-carroll Let me know if that clears things up. If not I'll try to share the general flow of the program if that helps.

[matthew-carroll]

Sounds like this entire conversation can be reduced to the following:

This happens as a result of calling the .onTransactionEnd method on the composer editable before the document editable, which results in a call to update the styling for document nodes.

^ As an example, when it comes to relaying the root cause of a problem, this is the kind of thing that's helpful. This cuts through all those other names and relationships that confused things and focuses on the precise place where the issue originates.

So the order of editables set impacts the order of in which onTransactionEnd is called on each editable, which then impacts the order of their listeners.

My initial thought is that things listening to the end of transactions shouldn't allow themselves to be impacted by the order in which they're called.

Why is the presenter impacted by this? Is it because the presenter only invalidates the selection stages of the pipeline when the composer changes, but it invalidates the whole view model when the document changes?

[rutvik110]

Why is the presenter impacted by this? Is it because the presenter only invalidates the selection stages of the pipeline when the composer changes, but it invalidates the whole view model when the document changes?

Yes, that's correct.

From the SingleColumnLayoutPresenter docs,

/// When the [document] changes, the entire pipeline is re-run to produce
/// a new [SingleColumnLayoutViewModel].

[matthew-carroll]

Ok. I don't know if there's a great option, but here's my thinking.

Single transaction notification

The first thing that comes to mind is that we could move the transaction listener to the Editor, itself. That would result in only a single notification when a transaction ends. Unfortunately, that notification wouldn't let listeners know which editable(s) changed. Therefore, we'd excessively invalidate everything, even if only the selection changed.

Document versioning

Another option is to version the Document. For example, we could add the following to Document:

class Document {
Object get version => // implement

  bool hasChangedSince(Object version) {
    // implement
  }
}

We can make the version an int and increment it every time the document mutates.

With the version number, the presenter can compare its document version number at an appropriate time to decide if it should invalidate the layout view model.

Unfortunately, this version number means that anything which depends upon the document is now responsible for both listening for changes, and comparing version numbers at other times. I don't think it will be clear when something should listen for changes vs compare the version number.

Also, I think this would still suffer the order of operations problem we already have, because the Composer will run all of its listeners before the Document has a chance to update its version number.

Make both changes?

If we centralize transaction listening on the Editor, so there's only one such event, and we version the Document, then the presenter's single listener on the Editor transaction could check the Document version.

This would solve our order of operations problem and solve the presenter situation.

However, it's unclear how listeners would know if other editables changed. For example, did the Composer change in any way? Without adding versioning to the composer, there's no way to know. Also, the Composer isn't a single structure like a Document. The Composer holds the user's selection but also edit modes, like enabling "bold" text insertion. A listener might care about selection changes, but not about toggling "bold" on and off. So a single version number for the Composer wouldn't be great.

Timing alteration

I don't have a specific proposal in this area, but it's worth noting that if there were a frame of separation between listeners invalidating themselves, and listeners taking their action, this wouldn't be an issue. For example, imagine that the presenter didn't re-run the styler immediately, and instead waited until the next frame. If it did that, then both the Composer and the Document change listeners could run, mark things dirty, wait until the next frame, and then recreate the view model.

We could make that change to the presenter, but how would other developers know to do that? Would we be solving an Editor problem within the Presenter? Or is this actually a Presenter problem, which should be solved in the Presenter?

Let me know your thoughts.

[rutvik110]

The first thing that comes to mind is that we could move the transaction listener to the Editor, itself. That would result in only a single notification when a transaction ends.

Does this mean something like SingleColumnLayoutPresenter which is impacted by changes to the document and composer, will only listen for changes from Editor on transaction end? But within the Editor itself, we would still be running the .onTransactionEnd on every Editable at the end of that transaction before notifying the Editor listeners, right?

If that's the case, then this would partially solve the problem in the places where we know that the order of editable is relevant like in case of SingleColumnLayoutPresenter but in the places where we don't know whether order will impact or not, how would one decide whether to directly listen for changes on document or composer, or depend on Editor itself for both?

Unfortunately, this version number means that anything which depends upon the document is now responsible for both listening for changes, and comparing version numbers at other times. I don't think it will be clear when something should listen for changes vs compare the version number.

That def will be the case. One thing we could do is explicitly mention the cases under which one should check for document version? That might help to make it clear whether the user should directly listen for document changes or check the version number.

Though I'm worried that adding versioning the Editable(s) might add an little overhead for us to manage? Like in case of Composer, we may need to have multiple versions related to its different parts, and another question that may arise as we go forward with this is whether versioning be enforced for other Editable(s) that either we/user will create or it's only relevant to solving this issue?

We could make that change to the presenter, but how would other developers know to do that? Would we be solving an Editor problem within the Presenter? Or is this actually a Presenter problem, which should be solved in the Presenter?

I don't think this is a Presenter problem. Anything that might wanna react to document changes could suffer from the same issue as it's not aware of how and when each Editable it depends on notifies its listeners, and it shouldn't need to be. I think it's the Editor responsibility to make sure things are executed in an order which doesn't affect anything.

[matthew-carroll]

Does this mean something like SingleColumnLayoutPresenter which is impacted by changes to the document and composer, will only listen for changes from Editor on transaction end? But within the Editor itself, we would still be running the .onTransactionEnd on every Editable at the end of that transaction before notifying the Editor listeners, right?

The answer to the first question is "Yes", the answer to the second question is "No". Based on what I was describing, Editables would never notify any listeners. Only the Editor would notify listeners that a transaction ended, and then listeners would be able to go inspect whatever they want.

[matthew-carroll]

I don't think this is a Presenter problem. Anything that might wanna react to document changes could suffer from the same issue as it's not aware of how and when each Editable it depends on notifies its listeners, and it shouldn't need to be. I think it's the Editor responsibility to make sure things are executed in an order which doesn't affect anything.

I'm not convinced of this yet. The place where the Presenter's problem begins is with the Presenter's cached view model, which is essentially a cache of document information. In general, with any listener system, it's possible that changes are reported in any order. Listeners don't really have any guarantee of execution order within a single call stack, or within a single notification loop.

Moreover, imagine that we switched from an Editable Set to a List to enforce priority. That might solve the Presenter's issue because the Presenter happens to include a selection decision that depends upon the document. But what if some other object has a document decision that depends upon the selection. That object would require the exact opposite notification order. At that point, there wouldn't be anything we could do to solve those conflicting requirements.

[rutvik110]

That's true.

Does this then concludes that this is the problem at Presenter level? based on the fact that the order of listeners being called from Editor isn't fixed and also taking into account the last point you mentioned here.

[matthew-carroll]

I think we should probably solve this within the Presenter. I think that when the Presenter receives a change notification from Document and Composer and probably anything else, the Presenter should mark itself dirty, but should wait until the end of the current frame to re-compute the view model.

Do you want to give that a try?

I think the behavior would be:

• Presenter receives a change notification
• Presenter marks the appropriate phase dirty
• If a style pass has been scheduled, return
• Check if a frame is currently scheduled
  • If it is, add a post frame callback to run a style pass
  • If not, schedule a frame with a callback to run a style pass
• Set a flag that says a style pass is scheduled, so other change notifications don't schedule any more style passes

[rutvik110]

Cool. Will give it a go.

On Mon, Oct 23, 2023, 10:37 AM Matt Carroll @.***> wrote:

I think we should probably solve this within the Presenter. I think that when the Presenter receives a change notification from Document and Composer and probably anything else, the Presenter should mark itself dirty, but should wait until the end of the current frame to re-compute the view model.

Do you want to give that a try?

I think the behavior would be:

• Presenter receives a change notification
• Presenter marks the appropriate phase dirty
• If a style pass has been scheduled, return
• Check if a frame is currently scheduled
  • If it is, add a post frame callback to run a style pass
  • If not, schedule a frame with a callback to run a style pass
• Set a flag that says a style pass is scheduled, so other change notifications don't schedule any more style passes

— Reply to this email directly, view it on GitHub https://github.com/superlistapp/super_editor/issues/1505#issuecomment-1774443830, or unsubscribe https://github.com/notifications/unsubscribe-auth/APRQL6VIN6C64IZK4K2AETTYAX3QZAVCNFSM6AAAAAA57NSCCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZUGQ2DGOBTGA . You are receiving this because you were mentioned.Message ID: @.***>

[rutvik110]

@matthew-carroll The above solution does seem to solve the issue mentioned here by @raulmabe-labhouse, but this solution also seems to have introduced another issue when calculating DocumentSelectionLayout in the next frame.

https://github.com/superlistapp/super_editor/assets/65209850/c84400e6-f8f9-4e82-bc74-cb1dbbc7245b

Towards the end of the above demo, you'll see that when I try to delete a BoxComponent, it results in an error when trying to calculate the new DocumentSelectionLayout while still working with the deleted box component.

So far I'm not able to get an clear idea on what's the root cause is here and how I can approach this issue. Would you like to get on a quick meet where we can discuss this?

I'm working on this issue on 1505_fix_editables_order branch btw.

[matthew-carroll]

@rutvik110 Yes, we can get on a call. I've got a bunch of calls today, but maybe we can jump on a call tomorrow (Tuesday for me)

[rutvik110]

That works.

[matthew-carroll]

I had a call with @rutvik110 tonight. We determined that postponing the document layout viewmodel calculation to the end of the frame wasn't going to work because the Presenter listeners were likely being called at the beginning of a frame. This resulted in an attempt to build the document layout with a layout viewmodel that was stale.

Instead, @rutvik110 is going to adjust the change notification system so that the Editor sends change notifications instead of individual Editables like MutableDocument and MutableDocumentComposer. When the Editor sends a change notification at the end of a transaction, that change notification will include a Set<String>, which identifies all Editables that changed during the transaction.

After the change, each individual listener will receive only a single change notification, instead of possibly receiving multiple change notifications. In the case of the Presenter, the Presenter will be able to invalidate all relevant style phases at the same time, because the Presenter will be notified about all changes within a single change notification. At that point, the bug in this ticket will be resolved.

[matthew-carroll]

@rutvik110 did we merge this change in? Or is this change still in the works?

[rutvik110]

Not yet. I've to get on it.

[matthew-carroll]

I moved this to @angelosilvestre because he has some bandwidth.

[rutvik110]

That's alright.