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

! Support clipboard actions from the toolbar. #1843

Merged
merged 23 commits into from
May 3, 2024

Conversation

AtlasAutocode
Copy link
Collaborator

Description

  • Enables clipboard actions to be initiated from toolbar buttons. Previously, actions could only be initiated by keyboard shortcuts or from the popup menu after selecting some text, neither of which are user friendly.

  • Fixes errors in copy-paste operations involving selection of partial attribute nodes. Previously, copying a selection with a mix of attribute types resulted in an incorrect result when pasted into the same document. For example: enter the text 'bolder plainer' and select 'ld plain'. When pasted, the correct result should be 'ld plain' but the current release shows 'ld plain', there are 6 bold characters rather than the expected 2.

  • Allows checking if the quill document is readOnly so that direct changes will not be made when the editor is in readOnly mode.

Related Issues

Improvements

Provides better user control of clipboard actions, particularly for desktop systems.

Additional notes

Breaking change: readonly flag moved from QuillEditorConfigurations to QuillController to enable readOnly state to be determined from outside of the editor. There are no effects if editor never sets readOnly.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • I have run the commands in ./scripts/before_push.sh and it all passed successfully

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

Douglas Ward and others added 23 commits March 31, 2024 16:06
… call to class function afterButtonPressed to allow default call to base button settings

quill_icon_button: L26 build for isSelected updated to call afterButtonPressed = same as if not selected
QuillController _updateSelection removed param=source because not used; added new param insertNewline when true set tog to style of preceding char (last entered); updated replaceText to call _updateSelection for NL
document collectStyle:  Selecting the start of a line, user expects the style to be the visible style of the line including inline styles
insert at start of line uses style for line
Fix: Toolbar drop buttons clipped when !multiRowsDisplay
# Conflicts:
#	lib/src/models/config/editor/editor_configurations.dart
@EchoEllet
Copy link
Collaborator

EchoEllet commented Sep 29, 2024

Breaking change: readonly flag moved from QuillEditorConfigurations to QuillController to enable readOnly state to be determined from outside of the editor. There are no effects if editor never sets readOnly.

This breaking change was released as a minor version and doesn't exist in QuillController or QuillControllerConfigurations when creating the controller using the QuillController.basic.

Also need changes related to #1836

We should not break things that often (was even more often in versions 8 and 9). Will release them as breaking change releases even if it's a minor breaking change.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Sep 29, 2024

Breaking change: readonly flag moved from QuillEditorConfigurations to QuillController to enable readOnly state to be determined from outside of the editor

@AtlasAutocode It seems this was already possible, the user can create their own _readOnly and manage it from their toolbar and pass it to the QuillEditor. We should have more consideration before adding new features and that's something you and I agree on. I have introduced some changes or maybe many without creating an issue or discussing it, reviewing them with caution.

@AtlasAutocode
Copy link
Collaborator Author

At the time this was done, I found that toolbar buttons did not have access to the toolbar that contains them, or the editor, and hence did not have access to the read-only state maintained by the user.

I have just researched this and eventually found requireQuillEditorConfigurations which is not documented anywhere, very hard to find and barely used anywhere. This would place read-only back in the configurations which should be const?

I would argue that configurations define how the application works and, as such, should be static, const and never change.

In contrast, read-only is dynamic and can change frequently. As such, the controller is a better place for this. But this is your decision.

You are making a lot of changes as you review and reorganize the code with a view to stabilizing it. I commend your activity, but it leaves me uncertain as to where I can be of use.

@EchoEllet
Copy link
Collaborator

this is your decision.

This is a community-driven project and is not a project managed all by one person.

requireQuillEditorConfigurations

This feature will be removed in the next breaking change release, it's only used internally and not meant to be used by user projects. It has design issues and full of bugs and is inefficient. It's similar to how Flutter widgets work internally though for our use case, we should not use it.

This feature is possible without requireQuillEditorConfigurations either but if we're talking about having the clipboard actions built-in in the user project, then this change is probably necessary.

The user can define their own _readOnly, update it from the toolbar, and pass to the QuillEditorConfigurations. However, my question is, if this is not a commonly requested feature, we should have at least introduced it in non-breaking change way.

As such, the controller is a better place for this. But this is your decision.
it leaves me uncertain as to where I can be of use.

It may sound like I'm against though actually, I'm with your change, your change itself doesn't have any issues, we should have discussed this, opened an issue, done more planning, and definitely avoided breaking changes in a new minor version, the output the user will get make it hard to debug the issues.

Confirming to semver guidelines is essential knowing that pub integrates with semver.

In contrast, read-only is dynamic and can change frequently. As such, the controller is a better place for this. But this is your decision.
I would argue that configurations define how the application works and, as such, should be static, const and never change.

We do have another parameter in the configuration that will prevent the use of the const keyword, is the performance our main concern or it's a matter of preference when making this decision? I suggested moving the controller since it always prevents the const keyword, I expect a negligible impact on performance and there are many aspects where we can improve the performance.

The issue is that things are being added randomly without understanding them, and we can't understand all of them if docs are missing.

At the moment I can't reply properly since I'm tight with other tasks which is why this message is not quite clear to the point.

Copy link
Collaborator

@EchoEllet EchoEllet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to finish #2230 (except lack of Windows support issue and error handling). A few things here could use some minor improvements.

@@ -59,6 +59,7 @@ class _QuillScreenState extends State<QuillScreen> {

@override
Widget build(BuildContext context) {
_controller.readOnly = _isReadOnly;
Copy link
Collaborator

@EchoEllet EchoEllet Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not change this in the build method since it is called more often, users use the example as a source of using the library. This can impact performance more than not using the const keyword in QuillEditorConfigurations.

We should focus on important aspects that improve the performance noticeably.

@@ -107,6 +107,9 @@ class QuillSimpleToolbarConfigurations extends QuillSharedToolbarProperties {
this.showSearchButton = true,
this.showSubscript = true,
this.showSuperscript = true,
this.showClipboardCut = true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Showing new buttons in a minor version is also a breaking change. We should focus on having less buttons and an opinioned toolbar that fit the needs of most of our users, noticed that currently, the toolbar takes more than half of the screen, which is an issue I introduced in 2023 when I introduced support for Material 3.

We should make it take less space, with less buttons, and instead have sections or groups where each group have related buttons, users can navigate between them, we should not touch the QuillSimpleToolbar anymore and introduce a new toolbar unless most developers want something different, currently, we don't have any reports regarding the toolbar though I noticed some developers already started to develop their our toolbar. We should introduce a new one instead of changing this. Or maybe provide those buttons so they can use them somewhere different than the toolbar.

The toolbar is one of the things that really need improvements. The UI is important for the end user.

@@ -407,20 +407,20 @@ base class Line extends QuillContainer<Leaf?> {
final data = queryChild(offset, true);
var node = data.node as Leaf?;
if (node != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related? Not sure why we need it for this feature.

Comment on lines +477 to +479
// Notify toolbar buttons directly with attributes
Map<String, Attribute> toolbarButtonToggler = const {};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the controller should not have anything related or specific to the toolbar. Should annotate most of the newly added properties in the last year as experimental as we're removing them as we discover issues and more users start using them.

@@ -65,7 +65,6 @@ void main() {
configurations: QuillEditorConfigurations(
controller: controller,
// ignore: avoid_redundant_argument_values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably update our analysis_options.yaml and also remove those comments.

@@ -27,7 +27,6 @@ void main() {
configurations: QuillEditorConfigurations(
controller: controller,
// ignore: avoid_redundant_argument_values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment, some lints in analysis_options.yaml need to be removed.

}

/// Returns whether paste operation was handled here.
/// updateEditor is called if paste operation was successful.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be more specific about this, it looks like it return true in read-only mode, and the doc comment indicate that this is a successful clipboard operation where it ignored the call due to readOnly.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Oct 1, 2024

doesn't exist in QuillController or QuillControllerConfigurations when creating the controller using the QuillController.basic() factory constructor.

We really need to solve this issue ASAP, see this comment.

Comment on lines +480 to +483
/// Clipboard caches last copy to allow paste with styles. Static to allow paste between multiple instances of editor.
static String _pastePlainText = '';
static List<OffsetValue> _pasteStyleAndEmbed = <OffsetValue>[];

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static to allow paste between multiple instances of editor.

This might not be the expected behavior? The copied image URL is not shared across different instances:

ImageUrl? _copiedImageUrl;
ImageUrl? get copiedImageUrl => _copiedImageUrl;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting across lists is broken "Justify win width" tooltip should just be "Justify"
3 participants