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

[pauliyobo]

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]

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.

[cary-rowen]

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]

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]

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]

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.

[cary-rowen]

I agree, Any updates?

[pauliyobo]

@cary-rowen feel free to try it out.

[cary-rowen]

Hi @pauliyobo
Sorry for taking the liberty of synchronizing the develop branch for you. I have only found one problem so far, as mentioned in my comment: the selected text range is missing a character when navigating comments.

[pauliyobo]

@cary-rowen Yeah, I forgot about that issue, my bad. It should be working now.

[cary-rowen]

Hi @pauliyobo

Another suggestion: for the warning dialog that detects overlapping comments, I think we could display it earlier — perhaps when the user is trying to add a comment, instead of allowing them to write the comment and then showing the dialog after they click 'Submit'. This way, it would help users avoid unnecessary effort upfront.

[pauliyobo]

@cary-rowen
Done. If a selection overlaps with existing notes, the user will be notified instead of asking for input.
Let me know if there's anything else I missed.

[cary-rowen]

Thank you @pauliyobo

I noticed that you seemed to have done a format fix, so some files that were not related to this PR were modified, what do you think about this? Are they worth doing in separate PRs or commits?
Doing so will keep the modification history of that PR clearer

Other than that I think this PR is ready

[pauliyobo]

Thanks.
I have went ahead and rebased the commit to not include the cosmetic changes just to facilitate review.
I'll go ahead and merge this.