Sys - Editor crashes every time parent setState is called

[Phil9l]

Every time I call setState() in the parent component which is pretty critical for me. I think it's initialized as late here.

Component looks like this when I'm calling setState.

Code snippet:

class Screen extends StatefulWidget {
  @override
  State<Screen> createState() => _ScreenState();
}

class _ScreenState extends State<Screen> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Form(
          child: ListView(
            children: [
              EditorToolbar.basic(
                controller: controller,
              ),
              Expanded(
                child: VisualEditor.basic(
                  controller: controller,
                ),
              ),
              OutlinedButton(
                  onPressed: () => {
                    setState();
                  },
                  child: const Text('Break')),
            ],
          ),
        ),
    );
  }
}

I only tried it in web.

Video: https://cdn.discordapp.com/attachments/972503769113829427/996005922193281127/Peek_2022-07-11_12-49.mp4

[adrian-moisa]

Thank you for reporting it! I will have a look as soon as time allows to identify the issue.

[kairan77]

Dear god, don't use setState please, it is a bad bad practice. 90% of the time you use setState you should probably use ValueNotifier/Stream/Publisher/ChangeNotifier/Bloc instead.

The contractual invariant is mathematically stricter and easier to reason with for all those alternatives but setState. Hence it is so much easier to write wrong code with setState than with the other alternatives.

To be more helpful, this kind of behavior usually has something to do with your data being prepared in the initState method or some future, and when setState is called, bear in mind your initState or future returning method is probably not rerun. Without seeing your code, I am 80% confident this probably has nothing to do with quill, but the overall data loading strategy of your own code, swap out quill and just work with the rest of your app to show the data somewhere on screen you will probably see the same error message. Double check how your data is loaded and use debugPrint on those lifecycle hooks to see what get called, you will probably find what is wrong.

[adrian-moisa]

@kairan77 Yes indeed, I agree with your feedback. I'd argue the same, don't use setState. Though I'm aware that a whole lot of people wont get it for a very long time and they will use setState. So, I need to make sure the editor stays alive. I just saw today the issue manifesting from code wrote by one of my juniors. It's going to happen a lot even with the best edu materials being available out there.

So the goal is to at least keep the editor running in this scenario, performance for sure will suffer (though not greatly for most usecases). I've traced the issue and found a flaw in my current refactoring effort. I'm currently working on a fix. I'll release an explanation once stable.

[Phil9l]

Dear god, don't use setState please, it is a bad bad practice. 90% of the time you use setState you should probably use ValueNotifier/Stream/Publisher/ChangeNotifier/Bloc instead.

Thanks for your feedback. I'm using bloc for bigger projects but this one is very small, just one form, so I decided to just use setState. I believe widget should't crash after a setState call even if it's not the best architectural decision.

So the goal is to at least keep the editor running in this scenario, performance for sure will suffer (though not greatly for most usecases). I've traced the issue and found a flaw in my current refactoring effort. I'm currently working on a fix. I'll release an explanation once stable.

Thanks for working on this, looking forward!

[kairan77]

So the goal is to at least keep the editor running in this scenario,

Spirit and Effort admired and appreciated!

[adrian-moisa]

Fixed issue with EditorController being reinitialised on setState(). Usually setState() should not be used. However there are scenarios when the host code might request the entire editor to update (for example when changing styles). Prior to this fix the editor was crashing completely. When a client app triggers setState() it also rebuilds the EditorController if the first controller instance is not cached by the developer. When a new controller is created a new internal state store object is created as well. In this state store we also keep references towards several important classes: ScrollController, FocusNode, EditorRenderer. The issue came from the fact that these references were not properly renewed. In many places of the code base we have code snippets that depend on the latest refs being available. The fix was to patch the newly created state store with the proper refs. In the old Quill Repo this was present but due to the lack of documentation this code got discarded. Now this fix restores this functionality but with the necessary changes to make it work within the refactored codebase of Visual editor.

Avoid setState()

Triggering setState() in the parent widget of the VisualEditor widget should be avoided as much as possible. Once you built the page, you don't want to trigger a rebuild of the entire editor. Even if Flutter is clever enough to avoid any major work in the rendering layer it still has to run change detection. And for a large document this adds up. Especially on low power devices such as smartphones. To avoid such scenarios it is recommended that you use the controller API to update the document instead of setState() on the editor parent widget.

Avoid setState() on text select

One common mistake is to react to the selection change in the editor by setting state in the parent. This will induce a needless build cycle in the editor. For example, in the Markers demo page you can see an editor and bellow it a stats panel with numbers indicating the selection extent. Notice that in the demo page implementation we have made special effort to avoid triggering setState() on the entire page when the selection changes. Our solution (one of many possible solutions) was to send the selection extend numbers via a stream to the sibling component that renders them.