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

Added an option for ClipboardPaste button to disable ClipboardMonitor #2427

Merged
merged 27 commits into from
Jan 21, 2025

Conversation

Maksim-Nikolaev
Copy link
Contributor

@Maksim-Nikolaev Maksim-Nikolaev commented Jan 5, 2025

Description

According to the issue #2421 ClipboardMonitor was causing lag if "paste" button was enabled in QuillSimpleToolbar.

That was because of the periodic 1 second check for Clipboard content. In case with large strings or images in clipboard, it would lag out the application and slow down everything drastically.

class ClipboardMonitor {
  bool _canPaste = false;
  bool get canPaste => _canPaste;
  Timer? _timer;

  void monitorClipboard(bool add, void Function() listener) {
    if (kIsWeb) return;
    if (add) {
      _timer = Timer.periodic(
          const Duration(seconds: 1), (timer) => _update(listener));
    } else {
      _timer?.cancel();
    }
  }

  Future<void> _update(void Function() listener) async {
    final clipboardService = ClipboardServiceProvider.instance;
    if (await clipboardService.hasClipboardContent) {
      _canPaste = true;
      listener();
    }
  }
}

This is not a breaking change, however it will change the behavior of availability for "paste" button for QuillSimpleToolbar since it always be enabled (except of readonly mode).

Related Issues

Type of Change

  • 🛠️ Bug fix: Resolves an issue without changing current behavior.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Jan 5, 2025

Thanks for your contribution.

This is not a breaking change

A breaking change is not only the kind that requires users to update their code to compile, but to also keep the current state without any behavior breaking.

Changing the default of a value is also a breaking change.

What would happen if the user pressed the paste button and there was nothing to paste?

I suggest providing the option to override the canPaste state instead, so they don't need to depend on ClipboardMonitor.

I'm with the idea of disabling something that's causing issues. However, users should have the option to revert to the old behavior in case they still want ClipboardMonitor with a signle boolean parameter.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@EchoEllet

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev Maksim-Nikolaev changed the title Removed ClipboardMonitor from ClipboardAction.paste type button Added an option for ClipboardPaste button to disable ClipboardMonitor Jan 5, 2025
@Maksim-Nikolaev

This comment was marked as resolved.

@singerdmx
Copy link
Owner

@EchoEllet can we merge this?

@EchoEllet
Copy link
Collaborator

Updated ClipboardMonitor periodic timer from 1 second to 5 seconds

This is still a breaking change.
Could you minimize the changes to only override the can paste state (whether the button is enabled)?

I added bool? enableClipboardPaste in lib\src\toolbar\config\simple_toolbar_config.dart for QuillSimpleToolbarConfig

This option should be only in the button config, not the toolbar config.

can we merge this?

I will work on it soon. However, it's not a high-priorty issue given that I have disabled the buttons by default (revert to the default behavior).

@Maksim-Nikolaev

This comment was marked as resolved.

lib/src/toolbar/buttons/clipboard_button.dart Outdated Show resolved Hide resolved
lib/src/toolbar/config/base_button_options.dart Outdated Show resolved Hide resolved
lib/src/toolbar/buttons/clipboard_button.dart Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@EchoEllet

This comment was marked as outdated.

@Maksim-Nikolaev

This comment was marked as resolved.

EchoEllet added a commit that referenced this pull request Jan 17, 2025
EchoEllet added a commit that referenced this pull request Jan 17, 2025
* feat!: add config class for clipboard action buttons

* docs: update CHANGELOG and migration guide

* chore: add a TODO for #2427
@EchoEllet
Copy link
Collaborator

EchoEllet commented Jan 17, 2025

info • Constructors in '@immutable' classes should be declared as 'const' • lib/src/toolbar/buttons/clipboard_button.dart:62:3 • prefer_const_constructors_in_immutables
info • Field overrides a field inherited from 'QuillToolbarBaseButtonOptions' • lib/src/toolbar/config/buttons/clipboard_button_options.dart:28:15 • overridden_fields
info • The member 'enableClipboardPaste' overrides an inherited member but isn't annotated with '@OverRide' • lib/src/toolbar/config/buttons/clipboard_button_options.dart:28:15 • annotate_overrides

Could you confirm if you have rebased your branch?

$ git remote add upstream [email protected]:singerdmx/flutter-quill.git
$ git fetch upstream
$ git rebase upstream/master # Fix conflicts if any. Follow the git CLI instructions.
$ git push --force # Force push if conflicts were solved or didn't occur.

@Maksim-Nikolaev

This comment was marked as resolved.

EchoEllet added a commit that referenced this pull request Jan 18, 2025
…ates that this is only applicable for the paste button
@Maksim-Nikolaev
Copy link
Contributor Author

Could you rebase your branch once again?

Rebased again.

Please fix the analysis issue. https://dart.dev/tools/linter-rules/prefer_const_constructors_in_immutables

Added const to QuillToolbarClipboardButton constructor.

@EchoEllet EchoEllet merged commit bfbbfc0 into singerdmx:master Jan 21, 2025
7 checks passed
@EchoEllet
Copy link
Collaborator

Thank you for your patience and contributions, please consider creating issues or reaching out before working on new changes. Have a good day!

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.

ClipboardMonitor causes background lag when enabling clipboard buttons
3 participants