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

adds a delete_all_highlights #953

Open
wants to merge 12 commits into
base: development
Choose a base branch
from

Conversation

nikinbaidar
Copy link

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.

@ahrm
Copy link
Owner

ahrm commented Jan 29, 2024

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_highlights in DocumentView, we can just call doc()->delete_all_highlights() from MainWidget.
  • delete_all_highlights is misleading, maybe a better name would be delete_all_current_document_highlights?
  • delete_all_highlights is inefficient, it performs n database queries, we should just delete all highlights in a single query.

@nikinbaidar
Copy link
Author

nikinbaidar commented Jan 30, 2024

* `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?

@ahrm
Copy link
Owner

ahrm commented Jan 30, 2024

No, you just all highlights.clear() to clear all of the document's highlights.

@nikinbaidar
Copy link
Author

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 .

@ahrm
Copy link
Owner

ahrm commented Jan 30, 2024

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.

@nikinbaidar
Copy link
Author

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?

@ahrm
Copy link
Owner

ahrm commented Jan 30, 2024

Yes, using document_path filed (it actually is not the path of the document, rather the document's checksum).

@nikinbaidar
Copy link
Author

Yes, using document_path filed (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.

@ahrm
Copy link
Owner

ahrm commented Jan 30, 2024

No.

@nikinbaidar
Copy link
Author

Somes notes

All has been addressed.

@ahrm
Copy link
Owner

ahrm commented Jan 31, 2024

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.

@nikinbaidar
Copy link
Author

Removed.

@ahrm
Copy link
Owner

ahrm commented Feb 3, 2024

Please don't make irrelevant changes in your PRs. (the changes in .gitignore and build_linux.sh).

@nikinbaidar
Copy link
Author

@ahrm Hi! Are there any more problems with this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants