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

ClipboardMonitor causes background lag when enabling clipboard buttons #2421

Open
1 task done
Maksim-Nikolaev opened this issue Dec 25, 2024 · 13 comments · May be fixed by #2427
Open
1 task done

ClipboardMonitor causes background lag when enabling clipboard buttons #2421

Maksim-Nikolaev opened this issue Dec 25, 2024 · 13 comments · May be fixed by #2427
Labels
bug Something isn't working

Comments

@Maksim-Nikolaev
Copy link
Contributor

Maksim-Nikolaev commented Dec 25, 2024

Is there an existing issue for this?

Flutter Quill version

flutter_quill: ^11.0.0-dev.17 & flutter_quill_extensions: ^11.0.0-dev.7

Steps to reproduce

  1. Add QuillSimpleToolbar and pass QuillSimpleToolbarConfig with showClipboardCut, showClipboardCopy, or showClipboardPaste set to true
  2. Since it adds the button with Clipboard Service it causes performance issues:
    Source code for reference:
import 'dart:async';

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';

import '../../common/utils/widgets.dart';
import '../../editor_toolbar_controller_shared/clipboard/clipboard_service_provider.dart';
import '../../l10n/extensions/localizations_ext.dart';
import '../base_button/base_value_button.dart';
import '../simple_toolbar.dart';

enum ClipboardAction { cut, copy, paste }

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();
    }
  }
}

class QuillToolbarClipboardButton extends QuillToolbarToggleStyleBaseButton {
  QuillToolbarClipboardButton({
    required super.controller,
    required this.clipboardAction,
    super.options = const QuillToolbarToggleStyleButtonOptions(),

    /// Shares common options between all buttons, prefer the [options]
    /// over the [baseOptions].
    super.baseOptions,
    super.key,
  });

  final ClipboardAction clipboardAction;

  final ClipboardMonitor _monitor = ClipboardMonitor();

  @override
  State<StatefulWidget> createState() => QuillToolbarClipboardButtonState();
}

class QuillToolbarClipboardButtonState
    extends QuillToolbarToggleStyleBaseButtonState<
        QuillToolbarClipboardButton> {
  @override
  bool get currentStateValue {
    switch (widget.clipboardAction) {
      case ClipboardAction.cut:
        return !controller.readOnly && !controller.selection.isCollapsed;
      case ClipboardAction.copy:
        return !controller.selection.isCollapsed;
      case ClipboardAction.paste:
        return !controller.readOnly && (kIsWeb || widget._monitor.canPaste);
    }
  }

  void _listenClipboardStatus() => didChangeEditingValue();

  @override
  void addExtraListener() {
    if (widget.clipboardAction == ClipboardAction.paste) {
      widget._monitor.monitorClipboard(true, _listenClipboardStatus);
    }
  }

  @override
  void removeExtraListener(covariant QuillToolbarClipboardButton oldWidget) {
    if (widget.clipboardAction == ClipboardAction.paste) {
      oldWidget._monitor.monitorClipboard(false, _listenClipboardStatus);
    }
  }

  @override
  String get defaultTooltip => switch (widget.clipboardAction) {
        ClipboardAction.cut => context.loc.cut,
        ClipboardAction.copy => context.loc.copy,
        ClipboardAction.paste => context.loc.paste,
      };

  @override
  IconData get defaultIconData => switch (widget.clipboardAction) {
        ClipboardAction.cut => Icons.cut_outlined,
        ClipboardAction.copy => Icons.copy_outlined,
        ClipboardAction.paste => Icons.paste_outlined,
      };

  void _onPressed() {
    switch (widget.clipboardAction) {
      case ClipboardAction.cut:
        controller.clipboardSelection(false);
        break;
      case ClipboardAction.copy:
        controller.clipboardSelection(true);
        break;
      case ClipboardAction.paste:
        controller.clipboardPaste();
        break;
    }
    afterButtonPressed?.call();
  }

  @override
  Widget build(BuildContext context) {
    final childBuilder = this.childBuilder;
    if (childBuilder != null) {
      return childBuilder(
        options,
        QuillToolbarToggleStyleButtonExtraOptions(
          context: context,
          controller: controller,
          onPressed: _onPressed,
          isToggled: currentValue,
        ),
      );
    }

    return UtilityWidgets.maybeTooltip(
        message: tooltip,
        child: QuillToolbarIconButton(
          icon: Icon(
            iconData,
            size: iconSize * iconButtonFactor,
          ),
          isSelected: false,
          onPressed: currentValue ? _onPressed : null,
          afterPressed: afterButtonPressed,
          iconTheme: iconTheme,
        ));
  }
}
  1. Since the QuillToolbarClipboardButton was added to the Widget tree, it starts affecting the app performance & memory on the background, even if you are not actively copying or pasting with clipboard at the moment.
W/Looper  ( 4641): PerfMonitor longMsg : seq=69 plan=18:41:47.480 late=0ms wall=1570ms h=android.os.Handler c=io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0 procState=-1
W/Looper  ( 4641): PerfMonitor longMsg : seq=71 plan=18:41:48.479 late=572ms wall=1418ms h=android.os.Handler c=io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0 procState=-1
W/Looper  ( 4641): PerfMonitor longMsg : seq=73 plan=18:41:49.479 late=991ms wall=1410ms h=android.os.Handler c=io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0 procState=-1
W/Looper  ( 4641): PerfMonitor longMsg : seq=74 plan=18:41:50.559 late=1323ms wall=1427ms h=android.os.Handler c=io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0 procState=-1
...
...

Expected results

Expected Clipboard to not be as heavy for the application and to not cause performance issues.

Actual results

ClipboardService causes background lag when used.

@Maksim-Nikolaev Maksim-Nikolaev added the bug Something isn't working label Dec 25, 2024
@Maksim-Nikolaev
Copy link
Contributor Author

It is possible there is a better implementation of Clipboard Monitor. I will try to take a look myself and tweak some stuff to see if it is any better. Right now it re-instantiates the ClipboardService every second, which could be an issue.

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();
    }
  }
}

@EchoEllet EchoEllet changed the title Usage of ClipboardService causes background lag ClipboardMonitor causes background lag when enabling clipboard buttons Dec 25, 2024
@EchoEllet
Copy link
Collaborator

EchoEllet commented Dec 25, 2024

flutter_quill: ^11.0.0-dev.17

Did you encounter the issue with older versions in case you used them?

with showClipboardCut, showClipboardCopy, or showClipboardPaste set to true
Since it adds the button with Clipboard Service it causes performance issue

To clarify, the ClipboardService is used even when the clipboard toolbar buttons are disabled. The issue is likely with ClipboardMonitor.

I will disable those buttons as a breaking change in v11 since they were already not enabled and introduced as unexpected change in a minor release. Related lines.

I'm thinking about removing them since supporting them is not a priority, and it's better to support less well-done and maintained features.

@Maksim-Nikolaev

This comment was marked as resolved.

@EchoEllet

This comment was marked as outdated.

@Maksim-Nikolaev
Copy link
Contributor Author

What is the use-case for clipboard actions in the toolbar, or why is it needed?

Just a very convenient way to use some actions (especially paste). I have no problem implementing them on my own, but I'll give it a try to fix buttons / clipboard monitor, so those actions are available for everyone using the plugin from toolbar as well.

It's being used when copying an image, pasting an image (when flutter_quill_extension is utilized), and pasting rich text content from the system clipboard (as HTML).

After a second thought, I do use them for pasting images. I'll give it a proper test and come back to it later on

@EchoEllet

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@EchoEllet
Copy link
Collaborator

, it indeed causes a lot of lag and without debouncer / optimization it seems like a very heavy check when there is big clipboard content.

One possible solution is to listen to the system clipboard changes, which involves platform-specific code and might not be available on some platforms or has some restrictions.

await Clipboard.getData(Clipboard.kTextPlain)

I was considering replacing it with platform code in quill_native_bridge to solve the image paste issue, which was fixed in #2384 by pasting the image first.

I would suggest to allow the users to determine "canPaste" status on their own and take control of the button's state. Then the users can also implement their own error handling for "onPaste".

How would they solve it? It's not possible without platform code AFAIK, and I think users prefer something that's already implemented without much configuration.

For now, we should notify users about this issue before they enable those buttons.

@Maksim-Nikolaev

This comment was marked as resolved.

@Maksim-Nikolaev

This comment was marked as resolved.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Dec 26, 2024

And this is exactly the same how it is used in flutter quill => clipboard_service

BTW, the current solution needs improvement. The hasClipboardContent determines if it has copied something to the system clipboard and if the app can retrieve it. However, it will only check plain text. It's on my TODO list to improve it.

This is already implemented in flutter with the simple in-built import from "services".

The issue is how we can listen to clipboard state changes, and the current solution achieves that by running platform code every 1 second from the Dart side, which is not efficient.

For me it also feels like that they can implement their own "canPaste" (aka isEnabled) if they really need it to avoid periodic checks.

Assuming they have greater control,
how would they solve the issue? There is almost no cross-platform solution that's consistent.

I will check to see if it's possible using platform code.

@Maksim-Nikolaev
Copy link
Contributor Author

I think it comes to the discussion if the "canPaste" flag is even needed for the in-built "paste" button.

For my use case I would just completely remove the canPaste flag and make it always active, and handle the exceptions/empty clipboard when I actually interact with the button

@EchoEllet

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants