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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
dac1cf6
Removed ClipboardMonitor from ClipboardAction.paste type button
Maksim-Nikolaev Jan 5, 2025
fb3f2d8
Update CHANGELOG.md
Maksim-Nikolaev Jan 5, 2025
dfeb9c2
Removed unused imports & added const keyword
Maksim-Nikolaev Jan 5, 2025
6ba6119
Revert "Removed ClipboardMonitor from ClipboardAction.paste type button"
Maksim-Nikolaev Jan 5, 2025
dc4019a
Revert "Removed unused imports & added const keyword"
Maksim-Nikolaev Jan 5, 2025
f2f3d86
added enableClipboardPaste to QuillSimpleToolbarConfig
Maksim-Nikolaev Jan 5, 2025
7727f08
Reverted 5 seconds to 1 second periodic timer
Maksim-Nikolaev Jan 5, 2025
eb0ddeb
moved enableClipboardPaste to button config
Maksim-Nikolaev Jan 5, 2025
ac00d45
Fix monitor enabling & disabling
Maksim-Nikolaev Jan 5, 2025
956b2d2
dartformat
Maksim-Nikolaev Jan 5, 2025
b6e343d
remove enableClipboardPaste from toolbar config
Maksim-Nikolaev Jan 5, 2025
297262a
Update CHANGELOG.md
Maksim-Nikolaev Jan 9, 2025
25c5273
Merge branch 'master' of https://github.com/singerdmx/flutter-quill
Maksim-Nikolaev Jan 9, 2025
f34bf49
Update CHANGELOG.md
Maksim-Nikolaev Jan 9, 2025
74f2cb1
Adjusted comments & documentation
Maksim-Nikolaev Jan 9, 2025
2ce564c
dart format
Maksim-Nikolaev Jan 9, 2025
3f6ed16
docs: improve CHANGELOG for consistency
EchoEllet Jan 10, 2025
284ab26
docs: improve doc comment of enableClipboardPaste
EchoEllet Jan 10, 2025
420a629
docs: minor changes to comments
EchoEllet Jan 10, 2025
b9d4d93
Merge branch 'master' into master
EchoEllet Jan 10, 2025
05215b3
revert: remove the addition of const keyword to QuillToolbarClipboard…
EchoEllet Jan 10, 2025
70512c7
Updated code readability with getter shouldUseClipboardMonitor
Maksim-Nikolaev Jan 10, 2025
ee7fcbf
Make the _shouldUseClipboardMonitor getter private
Maksim-Nikolaev Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- `enableClipboardPaste` flag in `QuillToolbarClipboardButton` to determine if the button defaults to `null,` which will use `ClipboardMonitor`, which checks every second if the clipboard has content to paste [#2427](https://github.com/singerdmx/flutter-quill/pull/2427).
- Croatian (hr) language translation.

### Changed

- Prevent `ClipboardMonitor` from doing multiple simultaneous clipboard checks to improve performance [#2427](https://github.com/singerdmx/flutter-quill/pull/2427) which is used when the clipboard paste button (`QuillToolbarClipboardButton`) is enabled in the toolbar.

## [11.0.0-dev.18] - 2025-01-06

### Changed
Expand Down
91 changes: 72 additions & 19 deletions lib/src/toolbar/buttons/clipboard_button.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,38 @@ class ClipboardMonitor {
bool get canPaste => _canPaste;
Timer? _timer;

bool _isCheckingClipboard = false;

void monitorClipboard(bool add, void Function() listener) {
if (kIsWeb) return;

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

Future<void> _update(void Function() listener) async {
// If the clipboard is already being checked, return early.
if (_isCheckingClipboard) {
return;
}

_isCheckingClipboard = true;

final clipboardService = ClipboardServiceProvider.instance;

if (await clipboardService.hasClipboardContent) {
_canPaste = true;

listener();
}

_isCheckingClipboard = false;
}
}

Expand All @@ -49,15 +65,15 @@ class QuillToolbarClipboardButton extends QuillToolbarToggleStyleBaseButton {

final ClipboardAction clipboardAction;

final ClipboardMonitor _monitor = ClipboardMonitor();

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

class QuillToolbarClipboardButtonState
extends QuillToolbarToggleStyleBaseButtonState<
QuillToolbarClipboardButton> {
final ClipboardMonitor _monitor = ClipboardMonitor();

@override
bool get currentStateValue {
switch (widget.clipboardAction) {
Expand All @@ -66,23 +82,54 @@ class QuillToolbarClipboardButtonState
case ClipboardAction.copy:
return !controller.selection.isCollapsed;
case ClipboardAction.paste:
return !controller.readOnly && (kIsWeb || widget._monitor.canPaste);
return !controller.readOnly &&
(kIsWeb ||
(widget.options.enableClipboardPaste ?? _monitor.canPaste));
}
}

void _listenClipboardStatus() => didChangeEditingValue();

@override
void didUpdateWidget(QuillToolbarClipboardButton oldWidget) {
super.didUpdateWidget(oldWidget);

// Default didUpdateWidget handler, otherwise simple flag change didn't stop the monitor.
if (oldWidget.controller != controller) {
oldWidget.controller.removeListener(didChangeEditingValue);
removeExtraListener(oldWidget);
controller.addListener(didChangeEditingValue);
addExtraListener();
currentValue = currentStateValue;
}
// The controller didn't change, but enableClipboardPaste did.
else if (widget.clipboardAction == ClipboardAction.paste) {
final isTimerActive = _monitor._timer?.isActive ?? false;

// Enable clipboard monitoring if not active and should be monitored.
if (shouldUseClipboardMonitor && !isTimerActive) {
_monitor.monitorClipboard(true, _listenClipboardStatus);
}
// Disable clipboard monitoring if active and should not be monitored.
else if (!shouldUseClipboardMonitor && isTimerActive) {
_monitor.monitorClipboard(false, _listenClipboardStatus);
}

currentValue = currentStateValue;
}
}

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

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

Expand All @@ -100,6 +147,11 @@ class QuillToolbarClipboardButtonState
ClipboardAction.paste => Icons.paste_outlined,
};

bool get shouldUseClipboardMonitor {
EchoEllet marked this conversation as resolved.
Show resolved Hide resolved
return widget.clipboardAction == ClipboardAction.paste &&
widget.options.enableClipboardPaste == null;
}

void _onPressed() {
switch (widget.clipboardAction) {
case ClipboardAction.cut:
Expand Down Expand Up @@ -131,16 +183,17 @@ class QuillToolbarClipboardButtonState
}

return UtilityWidgets.maybeTooltip(
message: tooltip,
child: QuillToolbarIconButton(
icon: Icon(
iconData,
size: iconSize * iconButtonFactor,
),
isSelected: false,
onPressed: currentValue ? _onPressed : null,
afterPressed: afterButtonPressed,
iconTheme: iconTheme,
));
message: tooltip,
child: QuillToolbarIconButton(
icon: Icon(
iconData,
size: iconSize * iconButtonFactor,
),
isSelected: false,
onPressed: currentValue ? _onPressed : null,
afterPressed: afterButtonPressed,
iconTheme: iconTheme,
),
);
}
}
8 changes: 8 additions & 0 deletions lib/src/toolbar/config/base_button_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class QuillToolbarBaseButtonOptions<T, I> {
this.afterButtonPressed,
this.tooltip,
this.iconTheme,
this.enableClipboardPaste,
this.childBuilder,
});

Expand All @@ -65,6 +66,13 @@ class QuillToolbarBaseButtonOptions<T, I> {
/// Use custom theme
final QuillIconTheme? iconTheme;

/// Determines if the paste button is enabled. The button is disabled and cannot be clicked if set to `false`.
///
/// Defaults to [ClipboardMonitor] in case of `null`, which checks if the clipboard has content to paste every second and only then enables the button, indicating to the user that they can paste something.
///
/// Set it to `true` to enable it even if the clipboard has no content to paste, which will do nothing on a press.
final bool? enableClipboardPaste;

Comment on lines +69 to +75
Copy link
Collaborator

@EchoEllet EchoEllet Jan 10, 2025

Choose a reason for hiding this comment

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

  • I'm confused, why is this change in the QuillToolbarBaseButtonOptions? This is the base for all buttons, and any configuration specific to the paste button should be in QuillToolbarClipboardButton instead. All the other buttons don't need this.
  • It's necessary to document if this is only applicable if the clipboard action is ClipboardAction.paste. Since the QuillToolbarClipboardButton is the same widget for cut, copy, and paste buttons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please take care of the location where to put the "enableClipboardPaste" flag?

ClipboardButtons use "QuillToolbarToggleStyleButtonOptions" and there is no "QuillToolbarPasteButton" class.
I'm not 100% familiar with how to introduce it without changes to be breaking.

  final QuillToolbarToggleStyleButtonOptions clipboardCut;
  final QuillToolbarToggleStyleButtonOptions clipboardCopy;
  final QuillToolbarToggleStyleButtonOptions clipboardPaste;

Copy link
Collaborator

@EchoEllet EchoEllet Jan 10, 2025

Choose a reason for hiding this comment

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

ClipboardButtons use "QuillToolbarToggleStyleButtonOptions".

It looks like the QuillToolbarClipboardButton doesn't have a custom config class (QuillToolbarClipboardButtonConfig), I'm not sure why but it shouldn't even if it doesn't require extra parameters. I will create a config class for this button and you can move this flag there instead.

here is no "QuillToolbarPasteButton" class.

I updated my message, I meant the config class of QuillToolbarClipboardButton which is missing.

Copy link
Collaborator

@EchoEllet EchoEllet Jan 10, 2025

Choose a reason for hiding this comment

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

ntroduce it without changes to be breaking.

You're right, it's not possible without any breaking in case users were already using QuillToolbarToggleStyleButtonOptions for those buttons (see #2433).

To be honest I'm not motivated to work on anything related to QuillSimpleToolbar, the design is a pain to work with and almost always confusing despite it's not very complex, our long plan is to completely rework the toolbar with a new modern one, better API design and usability while keeping the old toolbar backward compatible.

If #2433 merged, you can access enableClipboardPaste inside QuillToolbarClipboardButtonState:

widget._options.enableClipboardPaste;

But again, wouldn't it be better to mark this feature as experimental, rework on it later (to avoid breaking)? It's almost not a priority at least IMO. Working on it is not a good time investment which is why I updated README.md to include a note about contributing: https://github.com/singerdmx/flutter-quill#-contributing

@CatHood0 Do you think it worth introducing this breaking (changing options from QuillToolbarToggleStyleButtonOptions to QuillToolbarClipboardButtonOptions and slightly inconsistent API) to support disabling ClipboardMonitor introduced in #1843.

I have already disabled those buttons by default and mark them as experimental.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But again, wouldn't it be better to mark this feature as experimental, rework on it later (to avoid breaking)? It's almost not a priority at least IMO. Working on it is not a good time investment

I can understand the pain and now I realize more how hard it is to maintain open source project. I discovered that the paste button caused extreme lag in the app I'm working on, therefore I thought I'd contribute.

Otherwise in my case the usage of such button can be simplified to calling "controller.clipboardPaste" and can be achieved with custom button.

Once again, really appreciate your time and patience with this PR and I hope I don't cause too much issues for you

Copy link
Collaborator

@EchoEllet EchoEllet Jan 10, 2025

Choose a reason for hiding this comment

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

I don't cause too much issues for you

This is a project issue and not exclusive to this PR.

I realize more how hard it is to maintain open source project.

It's not very hard if we have a good solid foundation from the start, but there was a time when we kept merging PRs without much review and also introducing changes without much consideration or any tests at all, and that made it harder for us to fix those issues or maintain the project.

For example, the magnifier feature was introduced and merged quickly that people started depending on it (see #2406 for a list of regressions and issues), which caused major issues that affected the editor experience, and reverting to the old behavior is no longer trivial (see #2413), introducing breaking changes caused many issues for the app developers. That's why I think adding new features is not a priority, especially those that add more on the surface for more issues, this is not an issue with your code.

/// If you want to display a different widget based using a builder
final QuillToolbarButtonOptionsChildBuilder<T, I> childBuilder;
}
Expand Down
1 change: 1 addition & 0 deletions lib/src/toolbar/config/buttons/toggle_style_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ class QuillToolbarToggleStyleButtonOptions
super.afterButtonPressed,
super.iconTheme,
super.childBuilder,
super.enableClipboardPaste,
});
}
Loading