search

singerdmx / flutter-quill

Rich text editor for Flutter
https://pub.dev/packages/flutter_quill
MIT License
2.6k stars 839 forks source link

feat!: release version 11.0.0 with breaking changes #2338

Closed EchoEllet closed 1 week ago

EchoEllet commented 3 weeks ago

Description

Prepare version 11.0.0 and introduce breaking changes, and cleanup with no new features.

To try the new version before it is released:

dependency_overrides:
  flutter_quill:
    git:
      url: https://github.com/singerdmx/flutter-quill.git
      ref: release/v11
  flutter_quill_extensions:
    git:
      url: https://github.com/singerdmx/flutter-quill.git
      ref: release/v11
      path: flutter_quill_extensions

See the migration guide to migrate from 10.0.0 to 11.0.0.

Related Issues

Type of Change

EchoEllet commented 3 weeks ago

@CatHood0 @AtlasAutocode @singerdmx

If you have the time, can you review this PR? See the migration guide if you want the short list of changes.

EchoEllet commented 3 weeks ago

Thank you all for the review.

Feel free to drop suggestions for further changes.

After clicking a toolbar button, focus is not restored to the editor window. Example: Type in some text. Click the Bold toolbar button. Focus is not restored to the editor window. User must click in the editor window to continue typing. Reason: editorFocusNode was removed from QuillController This is a Major Bug and must be fixed.

Adding the editor focus request was already a breaking change introduced in a minor version, I removed it since it's possible to implement in the dev project. Adding properties in the controller for the QuillSimpleToolbar is not ideal IMO, especially if it's the simple toolbar config.

User must click in the editor window to continue typing.

We shouldn't set it as the default since the behavior is different on mobile than on desktop. It's in the breaking behavior section, We could also provide a code snippet to revert to the old behavior.

QuillToolbar class no longer exists - need documentation on what to use and what configuration options to use. You are forcing users to use the QuillSimpleToolbar. Documentation states it was removed as not used which is not true. Anyone who used the original class is totally dead - users will expect a suggested replacement.

The migration guide has been updated to clarify it. I'm not sure why QuillSimpleToolbar is related.

I agree that the current docs could use more improvements with more code snippets. The custom toolbar page has been updated too.

I'm trying not to force users to use the QuillToolbar to write their toolbar, a minor reason why the provider and FlutterQuillLocalizationsWidget were removed.

Introduce alternative for `QuillSimpleToolbar`?

> I do not use the QuillSimpleToolbar as it is not suitable for my desktop application. I wanted to rewrite the toolbar, to provide something that's opinioned and works for most use cases, looks and functions better since now we have too many buttons, they take more than half of the screen on mobile, and are not platform adaptive on desktop. However, that doesn't solve the priorities issue.

The loss of the base implementation means I have to resurrect the old flutter quill code and create my own class. This is a major inconvenience!

I'm aware of the how inconvenience and it's unfortunate.

I didn't want to touch the QuillSimpleToolbar to keep it backward-compatible, and instead, provide a new toolbar widget, so I didn't have plans to remove this feature of base options.

Restoring it without the toolbar provider will introduce us to a lot of boilerplates, we have to pass them around the constructors and create a method that's the opposite of copyWith() which will provide its default value if the value is null for all the button options, it was already a buggy feature and I shouldn't introduce it, I didn't prioritizeQuillSimpleToolbar since on the long-term it's better to replace it. The toolbar is a small part of the library and shouldn't be a priority with our current issues.

Also, it's confusing to document that they need to wrap their toolbar with QuillToolbar, providing them the button options param and it only works if they're using the buttons of the simple toolbar.

I am sure you have considered these breaking changes and have an answer. I am giving a quick response that mimics users upgrading and they face a major amount of work in this upgrade.

I published this PR as a dev release since I'm still considering things and it might not be final, I appreciate the feedback and I'm looking for more feedback before releasing this as stable.

EchoEllet commented 3 weeks ago

I am giving a quick response that mimics users upgrading and they face a major amount of work in this upgrade.

What took the most time during the migration? IMO removing the base options will require time the most if the user depends on them and the simple toolbar buttons in their custom toolbar or QuillSimpleToolbar.

Some common changes such as the config class can be backward compatible:

@Deprecated('Use QuillEditorConfig instead.')
typedef QuillEditorConfigurations = QuillEditorConfig;

class QuillEditorConfig {}

Deprecating them rather than removing them can help users migrate quickly without reading the migration guide.

I would appreciate more feedback, as I want to understand the user's perspective on a production project.

EchoEllet commented 3 weeks ago

I think there is some confusion understanding the removal of QuillToolbar due to this invalid commit.

In the previous code docs, it uses QuillToolbar.simple, which is incorrect for a custom toolbar. Instead, you would use QuillToolbar. The name is confusing and is not documented properly.

The widget QuillToolbar is not a toolbar on its own like the QuillSimpleToolbar. It's a widget that only ensures to provide the localization delegate and the toolbar provider and that's no longer required with 11.0.0. You only need to remove it, and are not forced to use QuillSimpleToolbar.

I have updated the migration guide once again to clarify this.

EchoEllet commented 3 weeks ago

Desktop users expect the toolbar to be a part of the editor and the focus to remain within the editor so they can continue to type after clicking a toolbar button.

Old reply

Introducing this feature across all platforms, including mobile, in a minor version with behavior changes and without clear documentation is problematic. In #1876, testing was done only on the desktop, which led to added properties in the controller that complicate usage and make it hard to revert or fully understand. Example code snippet: ```dart final controller = QuillController.basic(); final editorFocusNode = FocusNode(); QuillSimpleToolbar( controller: controller, config: QuillSimpleToolbarConfig( buttonOptions: QuillSimpleToolbarButtonOptions( clearFormat: QuillToolbarClearFormatButtonOptions( afterButtonPressed: editorFocusNode.requestFocus, ), // For all buttons ), ), ); QuillEditor.basic( controller: controller, config: const QuillEditorConfig(), focusNode: editorFocusNode, ); ``` Adding this to every button is inefficient. So I will try to restore the `base` option since I don't plan on improving the `QuillSimpleToolbar`. Might restore the `QuillSimpleToolbarProvider` and restrict it for internal usage this time. Thank you for bringing this up.

The base parameter has been added back differently without the toolbar provider inherited widget, see the code snippet to use the old behavior.

EchoEllet commented 3 weeks ago

If you have a code snippet to achieve this, please provide it.

The base button options have been restored, and it supports the buttons of flutter_quill_extensions too:

  final editorFocusNode = FocusNode();
  QuillSimpleToolbar(
    config: QuillSimpleToolbarConfig(
      buttonOptions: QuillSimpleToolbarButtonOptions(
        base: QuillToolbarClearFormatButtonOptions(
          afterButtonPressed: editorFocusNode.requestFocus,
        ),
      ),
    ),
  );
  QuillEditor.basic(
    focusNode: editorFocusNode,
  );

See this section for more details with code snippet using a custom toolbar. The migration guide has been updated to include more details with code snippets to revert to the old behavior in the breaking behavior section.

then you could have simply made this an option rather than removing the functionality completely.

Reason of this change

**Note: This is not related to the PR directly but to explain a reason for a change.** **I'm grateful for all of your work over the last few months.** Removing the `base` option only requires more code to implement and doesn't make it impossible to implement, the main goal of this major release is to clean up some of the changes that were introduced since `7.5.0` and until now. Now that it has been restored, it requires very few changes, the migration guide provides code snippets for changes that break the behavior of the latest stable version (`10.8.5`), you have added this feature recently and I didn't see any reports about the previous behavior. My main issue is the following: ### The addition of the editor focus node was the `QuillController`. You also added the [`QuillSimpleToolbarConfig` and `QuillEditorConfig` in the `QuillController`](https://github.com/singerdmx/flutter-quill/blob/master/lib/src/controller/quill_controller.dart#L63-L70), in the [`Document`](https://github.com/singerdmx/flutter-quill/blob/master/lib/src/document/document.dart#L251-L261) too as public API. We have discussed previously that we don't properly understand the current architecture and we're making changes to the current one which is not enough documented. Having a good structure is currently a priority. ### The default editor focus shouldn't added without a major update and clear docs.

Reason with details

When users run `flutter pub upgrade` all dependencies will be updated to their latest compatible version, and this shouldn't change the current behavior unless there is a strong reason to. With major releases, users will need to manually update the `pubspec.yaml` and read the `CHANGELOG.md` which includes the migration guide. So that they are aware of the breaking changes, especially the ones that change behavior without requiring them to update their existing code. Refer to the [updating package dependencies](https://docs.flutter.dev/packages-and-plugins/using-packages#updating-package-dependencies) page for more details.

### It hasn't been tested on mobile and is more common on desktop than mobile Depending on the use case and requirements of the developer, it can be unsuitable. They will have to override the default `afterButtonPressed` to achieve this functionality with `() {}` to revert the behavior, as [you stated before](https://www.github.com/singerdmx/flutter-quill/pull/2235#issuecomment-2364588955), features should not be added if one person needs them. ### I haven't done enough code reviews
Details to an old issue

I haven't done enough to review PRs recently, merging things and then fixing them later is not ideal either especially since we don't have good code coverage. I have asked questions regarding merged PRs including yours and I couldn't get any answers after a month. I understand you don't have enough time which is why I stopped mentioning you in issue comments and decided to take action but release it as a dev release, and will wait more for reviews. **No need to rush, take your time.**

### Not a commonly requested feature I do follow all issues from time to another on GitHub, Slack, and previously on Telegram and I didn't see anyone requiring this feature before. A feature that's frequently requested doesn’t mean it should be always implemented. For instance, embedding tables and mentions is often requested though we should prioritize features with caution as you [pointed out](https://www.github.com/singerdmx/flutter-quill/pull/2238#pullrequestreview-2312706901). There are no reasons to make this a default, especially since this minor feature is not commonly requested and it's possible to implement with the minimal code snippet above.

EchoEllet commented 1 week ago

It's time to review and merge this PR into master, but not yet publish it as stable. I want more user feedback and I'm considering changes like disabling rich text paste by default (can be enabled with one line, see #2350) and reintroducing spell check as experimental (removed in v11).

To summary our discussion:

QuillToolbar class no longer exists

The QuillToolbar was used to create a custom toolbar while being able to use the buttons of flutter_quill, now it's no longer required and users can use the buttons directly, it has been removed. The docs has been updated to clarify this.

Quill Toolbar no longer works

It work as before, no changes were introduced, the only minor breaking change that introduced is not requesting the keyboard focus by default, and that has been already documented before, the migration guide has been updated to provide code snippets to revert on both custom and simple toolbar.

I used the original class in my app to create a custom toolbar specifically for desktop. I do not use the QuillSimpleToolbar as it is not suitable for my desktop application. The loss of the base implementation means I have to resurrect the old flutter quill code and create my own class. This is a major inconvenience!

It has been restored to make the migration smoother and it supports the custom button embed too (integrate with flutter_quill_extensions's buttons).

The pre-release has been published again on pub.dev

EchoEllet commented 1 week ago

This PR focuses on breaking changes and prioritizes issues needed in a major release.

Other changes will be addressed separately before publishing the stable release of v11.

Most changes are minor, primarily removals (e.g., CHANGELOG.md files, examples). It's not final though I don't expect new bugs.

Two minor questions: