Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
Added an option for ClipboardPaste button to disable ClipboardMonitor #2427
Changes from 20 commits
dac1cf6
fb3f2d8
dfeb9c2
6ba6119
dc4019a
f2f3d86
7727f08
eb0ddeb
ac00d45
956b2d2
b6e343d
297262a
25c5273
f34bf49
74f2cb1
2ce564c
3f6ed16
284ab26
420a629
b9d4d93
05215b3
70512c7
ee7fcbf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a private getter (name suggestion:
shouldUseClipboardMonitor
) that is onlytrue
whenenableClipboardPaste
isnull
and then update related parts such as those to make use of this to make it easier and faster to understand from other developers's perspective. Also, it makes it easier to track related logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it. Note that it should also check if the clipboard action is paste, it costs nothing but a bit more specific.
I adjusted the code to replace such statements, as well as the readability for didUpdateWidget function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QuillToolbarBaseButtonOptions
? This is the base for all buttons, and any configuration specific to the paste button should be inQuillToolbarClipboardButton
instead. All the other buttons don't need this.ClipboardAction.paste
. Since theQuillToolbarClipboardButton
is the same widget for cut, copy, and paste buttons.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.I updated my message, I meant the config class of
QuillToolbarClipboardButton
which is missing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
insideQuillToolbarClipboardButtonState
: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
fromQuillToolbarToggleStyleButtonOptions
toQuillToolbarClipboardButtonOptions
and slightly inconsistent API) to support disablingClipboardMonitor
introduced in #1843.I have already disabled those buttons by default and mark them as experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a project issue and not exclusive to this PR.
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.