-
Notifications
You must be signed in to change notification settings - Fork 250
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
adds a delete_all_highlights #953
base: development
Are you sure you want to change the base?
Conversation
Somes notes:
|
I created a new func to run a signle db query to delete all the highlights and call it directly from the Moreover, I see
It appears two lines are important in this method. One calls a DB query that will delete entries from the highlights table? and the other |
No, you just all |
Added this in the latest commit.
I think this will just get in the way of the workflow. But you're correct in that it's destructive. I think we could add a config to disable the warning. Please share your thoughts. @ahrm . |
This deletes all the highlights (not just the current document's). Also I still think a prompt is necessary because a user might accidentally click on it while browsing the |
I see it. I think the problem is with the DB query |
Yes, using |
Awesome. I still think a prompt is necessary because a user might accidentally click on it while browsing the : menu. Makes sense. I'll skip it for my build but include it for this PR. Do you already have something that prompts the user? (else I could create a |
No. |
All has been addressed. |
This is unnecessary:
It was used in the past when we did not use UUID to identify highlights, but now it is pointless. |
Removed. |
Please don't make irrelevant changes in your PRs. (the changes in |
@ahrm Hi! Are there any more problems with this PR? |
I find the
clear_current_document_drawings
andclear_current_page_drawings
functions incredibly useful. So I addeddelete_all_highlights
that does something similar for clearing all document highlights.