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

fix: Allow comments to targets sections of a line instead of the whole #280

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

pauliyobo
Copy link
Collaborator

Link to issue number:

fixes #205

Summary of the issue:

Up until now, bookworm allowed to add notes, though it was possible to do so only by targeting the whole line

Description of how this pull request fixes the issue:

This PR makes it so that whenever you add a new comment, the comment will target the selected text, if any. Otherwise it fals back to the default behaviour, which is to target the whole line.

Testing performed:

Manual testing

Known issues with pull request:

It's not really an issue with the PR itself, but since this PR makes changes to the database, backing it up is advised before testing the change, unless you know how to use alembic to downgrade a revision.

@cary-rowen would this solve your issue?

P.S.
Due to the change introduced it is now possible to overlap two comments in the same line, as well as having multiple comments in the same line but in different positions.
Whenever two or more comments overlap, this will be reported with the message "Has N comments" where N is the number of comments.
If this is confusing, perhaps we can just prevent notes from overlapping.

@cary-rowen
Copy link
Collaborator

Hi @pauliyobo
Thanks for fixing this.
I did some testing and found the following issue:

  1. When I add comment to the selected content, the commented content has one more character: for example, the following text: "123456" When I selected '23' and successfully added a comment, I checked the comment and found that the commented text was '234'.
  2. Taking the above example, when I press F9 to navigate to the comment, the entire line will to be selected instead of the actual commented text.

IMO, regarding multiple comments within the same line,

  1. If the user wants to add a comment but selects the same text area as last time, we can assume that the user wants to edit the last comment content. We display the last comment in the add comment textbox.
  2. If the user selects a different text area, albeit within the same line, we should probably allow this use case.

… comment that points to it said comment will be edited
@cary-rowen
Copy link
Collaborator

Hi @pauliyobo

It's more precise now.

I guess you haven't had time to fix this yet:

when I press F9 to navigate to the comment, the entire line will to be selected instead of the actual commented text.

Many thanks

@pauliyobo
Copy link
Collaborator Author

Sorry for the silence. As you noticed, I managed to tackle the selection precision and overlapping. Handling the comment cycling with f8 is a bit trickier because we have to keep into account overlapping comments, as well as comments that target the whole line. I'll be tackling this next.

@cary-rowen
Copy link
Collaborator

This does seem a bit tricky since we allow overlapping comments. If we take a step back, it seems easy enough that we don't allow overlapping comments.

@pauliyobo
Copy link
Collaborator Author

We could prevent comments to overlap. I believe quotes already do this.
Unless there is an use case that requires them to overlap, although nothing comes to mind right now.

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.

When adding notes please only target the selected text and not the entire line.
2 participants