ahrm
adds a delete_all_highlights
I find the clear_current_document_drawings and clear_current_page_drawings functions incredibly useful. So I added delete_all_highlights that does something similar for clearing all document highlights.
Somes notes:
- Since this is a very destructive command maybe we should show a message to the user to confirm they want to do this so they don't accidentally delete all their data.
- There is no need for
delete_all_highlightsinDocumentView, we can just calldoc()->delete_all_highlights()fromMainWidget. delete_all_highlightsis misleading, maybe a better name would bedelete_all_current_document_highlights?delete_all_highlightsis inefficient, it performsndatabase queries, we should just delete all highlights in a single query.
* `delete_all_highlights` is inefficient, it performs `n` database queries, we should just delete all highlights in a single query.
I created a new func to run a signle db query to delete all the highlights and call it directly from the delete_all_highlights() function as db_manager->()delete_all_current_doc_highglights in input.cpp but it does not delete anything at all, at first (unless I close sioyek). Highlights are gone if I reopen sioyek.
Moreover, I see
void Document::delete_highlight_with_index(int index) {
Highlight highlight_to_delete = highlights[index];
db_manager->delete_highlight(highlight_to_delete.uuid);
highlights.erase(highlights.begin() + index);
is_annotations_dirty = true;
}
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 highlights.erase does what? What happens (or what should happen) when I skip the highlights.erase? Will I still to run highlight.erase() in a loop?
No, you just all highlights.clear() to clear all of the document's highlights.
No, you just all
highlights.clear()to clear all of the document's highlights.
Added this in the latest commit.
- Since this is a very destructive command maybe we should show a message to the user to confirm they want to do this so they don't accidentally delete all their data
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 : menu.
This deletes all the highlights (not just the current document's).
I see it. I think the problem is with the DB query DELETE FROM highlights; which delete everything in the highlights table. Correct me if I'm wrong but I think all the docs use shareddb to store highlights, is there any way to trace only the current doc highlights?
Yes, using document_path filed (it actually is not the path of the document, rather the document's checksum).
Yes, using
document_pathfiled (it actually is not the path of the document, rather the document's checksum).
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 bool prompt_user() in utils.h.
This is unnecessary:
ss << std::setprecision(10) <<
It was used in the past when we did not use UUID to identify highlights, but now it is pointless.
Please don't make irrelevant changes in your PRs. (the changes in .gitignore and build_linux.sh).