Clean up ClipboardStatusNotifier #98951
Conversation
| void handleCut(TextSelectionDelegate delegate, ClipboardStatusNotifier? clipboardStatus) { | ||
| void handleCut(TextSelectionDelegate delegate) { | ||
| delegate.cutSelection(SelectionChangedCause.toolbar); | ||
| clipboardStatus?.update(); |
There was a problem hiding this comment.
It should be the delegate's job to call update on clipboardStatus since it is the owner. There may also be use case that cutSelection does not write thing into clipboard due to other short-circuited condition
| void handleCopy(TextSelectionDelegate delegate, ClipboardStatusNotifier? clipboardStatus) { | ||
| void handleCopy(TextSelectionDelegate delegate) { | ||
| delegate.copySelection(SelectionChangedCause.toolbar); | ||
| clipboardStatus?.update(); |
|
|
||
| bool _disposed = false; | ||
| /// True if this instance has been disposed. | ||
| bool get disposed => _disposed; |
There was a problem hiding this comment.
This is used by the listeners to decide whether they should call removeListener when they are disposed
This is no longer needed because removeListener is allowed to be called after it is disposed now
There was a problem hiding this comment.
I'm happy to see this going away 👍
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
This looks cleaner and I agree we shouldn't have the selection toolbar depend on the clipboard status unless it actually uses "paste".
| Widget build(BuildContext context) { | ||
| // Don't render the menu until the state of the clipboard is known. | ||
| if (widget.handlePaste != null && _clipboardStatus!.value == ClipboardStatus.unknown) { | ||
| if (widget.handlePaste != null && widget.clipboardStatus?.value == ClipboardStatus.unknown) { |
There was a problem hiding this comment.
What if clipboardStatus is null?
There was a problem hiding this comment.
if it is null it will skip this if condition and still build the toolbar. We only want to skip if we need to build the paste button( has handlePaste and has _clipboardStatus) and the status is unknown
|
|
||
| bool _disposed = false; | ||
| /// True if this instance has been disposed. | ||
| bool get disposed => _disposed; |
There was a problem hiding this comment.
I'm happy to see this going away 👍
This reverts commit c74a646.
…utter#99361)" This reverts commit 64d9ea6.
) This reverts commit c74a646.
* Revert "Clean up ClipboardStatusNotifier (#98951)" (#99361) This reverts commit c74a646. * Revert "Draggable can be accepted when the data is null" (#99419) * Cherrypick engine to 32395e4ab410df3b7d05b0f90d2601c5c7d27024 Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> Co-authored-by: Kate Lovett <katelovett@google.com>
* Clean up ClipboardStatusNotifier * fix * fix test * more refactaaaaagit add .
) This reverts commit c74a646.
The main motivation of this change is that other widget no longer need to create ClipboardStatusNotifier to use SelectionOverlay if they don't need paste functionality. Furthermore, it should not be TextSelectionOverlay's job to refresh the ClipboardStatus to notify itself, it should be whoever handles cut/paste to refresh the clipboard status. It also allows more flexibility if one does not rely on system clipboard.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.