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 · 12 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 · 12 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
Copy link
Contributor Author

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

I used the previous versions but cannot confirm with certainty that it was also present.

I'll be looking into Clipboard monitor and try to fix them on myself and will open a PR if I have success.

Thanks for clarifying that Clipboard service is actually being used even when the buttons are not visible, I was not aware of this and it actually reduces the scope for my investigation :)

Also, Merry Christmas 🎄

@EchoEllet
Copy link
Collaborator

I used the previous versions but cannot confirm with certainty that it was also present

The PR #1843 could be related, disabling the clipboard toolbar buttons should fix the issue.

I was not aware of this and it actually reduces the scope for my investigation :)

It might be worth trying quill_super_clipboard since it will replace the quill_native_bridge clipboard implementation with super_clipboard to confirm whether this is an issue with the DefaultClipboardService.

for clarifying that Clipboard service is actually being used even when the buttons are not visible

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).

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

Thanks
Merry Christmas 🎄

You're welcome. I'm familiar with those parts (except #1843), so let me know if you have questions.

@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
Copy link
Collaborator

I will disable those buttons as a breaking change in v11

Change introduced (7abe6d2).

@Maksim-Nikolaev
Copy link
Contributor Author

Maksim-Nikolaev commented Dec 26, 2024

I have concluded my review and can give some more details:

  1. The issue is only present for "Paste" button.
  2. PerfMonitor messages were complaining only about await Clipboard.getData(Clipboard.kTextPlain); function when there was a big enough content: image or a lot of text. *The function itself is Flutter's own implementation and works as intended.
    When I then checked for very small text ("Hello World") it was fine and not complaining
  3. Since the timer was periodic and was checking that every 1 second, 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 way to optimize that would be to introduce a flag if clipboard content is already being retrieved and do not trigger the function again in that case and while it will improve the performance by a lot in some cases, it is still not enough in others and feels a little bit useless since the timer is running constantly.

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".
Then we don't need periodic timer and the function will only be called during the actual use of it, which seems optimal to me.

Let me know if you would agree and then I can prepare a PullRequest with changes.

@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
Copy link
Contributor Author

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.

Exactly. I was trying to see if there are available solutions but it feels like an overkill

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.

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

import 'package:flutter/services.dart';

...

await Clipboard.getData(Clipboard.kTextPlain);

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

  /// If the Clipboard is not empty or has something to paste
  Future<bool> get hasClipboardContent async {
    final clipboardData = await Clipboard.getData(Clipboard.kTextPlain);
    return clipboardData != null;
  }

image

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

  1. Enable "canPaste" always by default
  2. Allow pasting their own state to "canPaste" flag
  3. If "paste" fails or no content found, they can handle it on their own (for example display a snackbar saying "no clipboard content").

@Maksim-Nikolaev
Copy link
Contributor Author

You can paste this code to dartpad to verify that it is available.

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

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  void getClipboardContent() async {
    final ClipboardData? clipboardData =
        await Clipboard.getData(Clipboard.kTextPlain);
    final String? copiedText = clipboardData?.text;

    print("Clipboard content: ${copiedText}");
  }

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        body: Center(
          child: OutlinedButton(
            onPressed: getClipboardContent,
            child: Text("Print clipboard content"),
          ),
        ),
      ),
    );
  }
}

@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

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