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

alembic/versions/35f453946f1e_added_start_and_end_pos_to_note.py

@@ -0,0 +1,32 @@
+"""Added start and end pos to Note
+
+Revision ID: 35f453946f1e
+Revises: 85028f013e6d
+Create Date: 2024-12-01 11:46:44.985313
+
+"""
+from typing import Sequence, Union
+
+from alembic import op
+import sqlalchemy as sa
+import bookworm
+
+# revision identifiers, used by Alembic.
+revision: str = '35f453946f1e'
+down_revision: Union[str, None] = '85028f013e6d'
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column('note', sa.Column('start_pos', sa.Integer(), nullable=True))
+    op.add_column('note', sa.Column('end_pos', sa.Integer(), nullable=True))
+    # ### end Alembic commands ###
+
+
+def downgrade() -> None:
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column('note', 'end_pos')
+    op.drop_column('note', 'start_pos')
+    # ### end Alembic commands ###

bookworm/annotation/__init__.py

@@ -170,11 +170,16 @@ def onKeyUp(self, event):
                 speech.announce(no_annotation_msg)
                 sounds.invalid.play()
                 return
-            self.reader.go_to_page(comment.page_number, comment.position)
-            self.view.select_text(*self.view.get_containing_line(comment.position))
+            start_pos, end_pos = (comment.start_pos, comment.end_pos)
+            is_whole_line = (start_pos, end_pos) == (None, None)
+            self.reader.go_to_page(comment.page_number, comment.position if is_whole_line else end_pos)
+            if is_whole_line:
+                self.view.select_text(*self.view.get_containing_line(comment.position))
+            else:
+                self.view.select_text(start_pos, end_pos)
             reading_position_change.send(
                 self.view,
-                position=comment.position,
+                position=comment.position if is_whole_line else end_pos,
                 # Translators: spoken message when jumping to a comment
                 text_to_announce=_("Comment: {comment}").format(
                     comment=comment.content
@@ -226,9 +231,17 @@ def onCaretMoved(self, event):
                 evtdata["line_contains_highlight"] = True
                 break
         for comment in NoteTaker(self.reader).get_for_page():
-            if comment.position in pos_range:
-                evtdata["comment"] = True
-                break
+            start_pos, end_pos = (comment.start_pos, comment.end_pos)
+            # If the comment has a selection, we check if the caret position is inside the selection. Otherwise, we check that the ocmment position is in the pos_range, typically the whole line.
+            condition = (start_pos <= position <= end_pos) if (start_pos, end_pos) != (None, None) else  comment.position in pos_range
+            if condition:
+                # Previously comment would have the value True
+                # Now, since it is possible that two comments overlap, we set how many comments are present in the given position
+                # TODO: Understand whether htis is worth it, or if we should prevent comments from overlapping
+                if "comment" in evtdata:
+                    evtdata["comment"] += 1
+                else:
+                    evtdata["comment"] = 1
         wx.CallAfter(self._process_caret_move, evtdata)
 
     def _process_caret_move(self, evtdata):
@@ -252,8 +265,15 @@ def _process_caret_move(self, evtdata):
                 # Translators: spoken message indicating the presence of an annotation when the user navigates the text
                 to_speak.append(_("Line contains highlight"))
             if "comment" in evtdata:
-                # Translators: spoken message indicating the presence of an annotation when the user navigates the text
-                to_speak.append(_("Has comment"))
+                # Translators: Text that appears when only a comment is present
+                single_comment_msg = _("Has comment")
+                # Translators: text that appears if multiple comments are present
+                multiple_comments_msg = _("Has {} comments")
+                comments = evtdata["comment"]
+                if comments == 1:
+                    to_speak.append(single_comment_msg)
+                else:
+                    to_speak.append(multiple_comments_msg.format(comments))
             speech.announce(" ".join(to_speak), False)
 
     def get_annotation(self, annotator_cls, *, foreword):
@@ -262,7 +282,9 @@ def get_annotation(self, annotator_cls, *, foreword):
                 annotator_cls.__name__, annotator_cls(self.reader)
             )
         page_number = self.reader.current_page
-        start, end = self.view.get_containing_line(self.view.get_insertion_point())
+        # start, end = self.view.get_containing_line(self.view.get_insertion_point())
+        start = self.view.get_insertion_point()
+        end = start
         if foreword:
             annot = annotator.get_first_after(page_number, end)
         else:
@@ -278,7 +300,9 @@ def on_book_unload(self, sender):
 
     @classmethod
     def comments_page_handler(cls, sender, current, prev):
-        comments = NoteTaker(sender).get_for_page()
+        comments = NoteTaker(sender)
+        # comments.update_ranges(current)
+        comments = comments.get_for_page()
         if comments.count():
             if config.conf["annotation"][
                 "audable_indication_of_annotations_when_navigating_text"

bookworm/annotation/annotation_dialogs.py

@@ -576,7 +576,11 @@ class CommentsDialog(AnnotationWithContentDialog):
     def go_to_item(self, item):
         super().go_to_item(item)
         self.service.view.set_insertion_point(item.position)
-
+        start_pos, end_pos = (item.start_pos, item.end_pos)
+        if (start_pos, end_pos) != (None, None):
+            # We have a selection, let's select the text
+            self.service.view.contentTextCtrl.SetSelection(start_pos, end_pos)
+
 
 class QuotesDialog(AnnotationWithContentDialog):
     def go_to_item(self, item):

bookworm/annotation/annotation_gui.py

@@ -15,7 +15,7 @@
     GenericAnnotationWithContentDialog,
     QuotesDialog,
 )
-from .annotator import Bookmarker, NoteTaker, Quoter
+from .annotator import Bookmarker, NoteTaker, Quoter, AnnotationOverlapsError
 
 log = logger.getChild(__name__)
 
@@ -203,18 +203,41 @@ def onAddNamedBookmark(self, event):
     def onAddNote(self, event):
         _with_tags = wx.GetKeyState(wx.WXK_SHIFT)
         insertionPoint = self.view.contentTextCtrl.GetInsertionPoint()
+        start_pos, end_pos = self.view.contentTextCtrl.GetSelection()
+        # if start_pos and end_pos are equal, there is no selection
+        # see: https://docs.wxpython.org/wx.TextEntry.html#wx.TextEntry.GetSelection
+        if start_pos == end_pos:
+            start_pos, end_pos = (None, None)
+        else:
+            # We have to set end_pos to end_pos-1 to avoid selecting an extra character
+            end_pos -= 1
+        comments = NoteTaker(self.reader)
+        # We check if there is already a comment that precisely overlaps with the selection
+        # If it exists, we edit it directly
+        comment = comments.get_for_selection(start_pos, end_pos)
+        value = comment.content if comment else ""
         comment_text = self.view.get_text_from_user(
             # Translators: the title of a dialog to add a comment
             _("New Comment"),
             # Translators: the label of an edit field to enter a comment
             _("Comment:"),
             style=wx.OK | wx.CANCEL | wx.TE_MULTILINE | wx.CENTER,
+            value=value,
         )
         if not comment_text:
             return
-        note = NoteTaker(self.reader).create(
-            title="", content=comment_text, position=insertionPoint
-        )
+        note = None
+        try:
+            note = comments.create(
+                title="", content=comment_text, position=insertionPoint, start_pos=start_pos, end_pos=end_pos
+            )
+        except AnnotationOverlapsError:
+            return self.view.notify_user(
+                _("Error"),
+                # Translator: Message obtained whenever another note is overlapping the selected position
+                _("Another note is currently overlapping the selected position.")
+            )
+
         self.service.style_comment(self.view, insertionPoint)
         if _with_tags:
             # add tags

bookworm/annotation/annotator.py

@@ -12,6 +12,11 @@
 log = logger.getChild(__name__)
 # The bakery caches query objects to avoid recompiling them into strings in every call
 
+class AnnotationOverlapsError(Exception):
+    pass
+
+class QuoteOverlapsError(Exception):
+    pass
 
 @dataclass
 class AnnotationFilterCriteria:
@@ -141,6 +146,7 @@ def get_for_section(self, section_ident=None, asc=False):
     def get(self, item_id):
         return self.model.query.get(item_id)
 
+
     def get_first_after(self, page_number, pos):
         model = self.model
         clauses = (
@@ -234,12 +240,126 @@ def delete_orphan_tags(cls):
             session.delete(otag)
         session.commit()
 
+class PositionedAnnotator(TaggedAnnotator):
+    """Annotations which are positioned on a specific text range"""
+
+    def create(self, **kwargs):
+        start = kwargs.get('start_pos')
+        end = kwargs.get('end_pos')
+        page_number = kwargs.get('page_number', 0)
+        position = kwargs.get('position', 0)
+        model = self.model
+        clauses = [
+            sa.and_(
+                model.start_pos.is_not(None),
+                model.end_pos.is_not(None),
+                model.start_pos == start,
+                model.end_pos == end,
+                model.page_number == page_number,
+            ),
+            sa.and_(
+                model.page_number == page_number,
+                model.position == position
+            ),
+            sa.and_(
+                model.start_pos.is_not(None),
+                model.end_pos.is_not(None),
+                model.page_number == page_number,
+                sa.or_(
+                    sa.and_(
+                        model.start_pos == position,
+                        model.end_pos == position,
+                    ),
+                    sa.and_(
+                        model.start_pos <= position,
+                        model.end_pos >= position,
+                    )
+                )
+
+            )
+        ]
+        if self.session.query(model).filter_by(book_id = self.current_book.id).filter(sa.or_(*clauses)).one_or_none():
+            raise AnnotationOverlapsError
+        return super().create(**kwargs)
 
-class NoteTaker(TaggedAnnotator):
+class NoteTaker(PositionedAnnotator):
     """Comments."""
 
     model = Note
 
+    def update_ranges(self, page) -> None:
+        """
+        Start_pos and end_poss are None whenever the comments handle the whole text
+        Since there is currently no way to dynamically set the correct values at runtime, we'll handle it here
+        """
+        for note in self.session.query(self.model).filter_by(book_id=self.current_book.id).filter(self.model.start_pos == None, self.model.end_pos == None, self.model.page_number == self.reader.current_page).all():
+            start_pos, end_pos = self.reader.view.get_containing_line(note.position)
+            note.start_pos = start_pos
+            note.end_pos = end_pos - 1
+            self.session.add(note)
+        self.session.commit()
+
+    def get_for_selection(self, start: int, end: int):
+        model = self.model
+        return self.session.query(model).filter_by(book_id = self.current_book.id).filter(model.start_pos == start, model.end_pos == end, model.page_number == self.reader.current_page).one_or_none()
+
+    def get_first_after(self, page_number, pos):
+        model = self.model
+        clauses = (
+            sa.and_(
+                model.page_number == page_number,
+                # sa.or_(
+                model.start_pos > pos,
+                model.start_pos.is_not(None),
+                # ),
+            ),
+            sa.and_(
+                model.page_number == page_number,
+                model.position > pos,
+            ),
+            sa.and_(
+                model.page_number == page_number,
+                model.end_pos.is_(None),
+                model.position > pos,
+            ),
+            model.page_number > page_number,
+        )
+        return (
+            self.session.query(model)
+            .filter_by(book_id=self.current_book.id)
+            .filter(sa.or_(*clauses))
+            .order_by(
+                model.page_number.asc(),
+                sa.nulls_first(model.start_pos.asc()),
+                sa.nulls_first(model.end_pos.asc()),
+                model.position.asc(),
+            )
+            .first()
+        )
+
+    def get_first_before(self, page_number, pos):
+        model = self.model
+        clauses = (
+            sa.and_(
+                model.page_number == page_number,
+                model.start_pos < pos,
+                model.start_pos.is_not(None),
+            ),
+            sa.and_(
+                model.page_number == page_number,
+                model.position  < pos,
+                model.start_pos.is_(None)
+            ),
+            model.page_number < page_number,
+        )
+        return (
+            self.session.query(model)
+            .filter_by(book_id=self.current_book.id)
+            .filter(sa.or_(*clauses))
+            .order_by(model.page_number.desc())
+            .order_by(sa.nulls_last(model.end_pos.desc()))
+            .first()
+        )
 
 class Quoter(TaggedAnnotator):
     """Highlights."""

bookworm/database/__init__.py

@@ -27,11 +27,11 @@ def get_db_url() -> str:
     db_path = os.path.join(get_db_path(), "database.sqlite")
     return f"sqlite:///{db_path}"
 
-def init_database(engine = None, url: str = None) -> bool:
+def init_database(engine = None, url: str = None, **kwargs) -> bool:
     if not url:
         url = get_db_url()
-    if not engine:
-        engine = create_engine(get_db_url())
+    if engine == None:
+        engine = create_engine(url, **kwargs)
     log.debug(f"Using url {url} ")
     with engine.connect() as conn:
         context = MigrationContext.configure(conn)
@@ -61,5 +61,5 @@ def init_database(engine = None, url: str = None) -> bool:
     Base.session = scoped_session(
         sessionmaker(engine, autocommit=False, autoflush=False)
     )
-    return True
+    return engine
 

bookworm/database/models.py

@@ -22,6 +22,7 @@
 
 class DocumentUriDBType(types.TypeDecorator):
     """Provides sqlalchemy custom type for the DocumentUri."""
+    cache_ok = True
 
     impl = types.Unicode
 
@@ -264,7 +265,11 @@ def text_column(cls):
 class Note(TaggedContent):
     """Represents user comments (notes)."""
     __tablename__ = "note"
-
+    # Like Quote, a note can have a start and an end position
+    # difference is that they are allowed to be None, and if so, it means they are targeting the whole line
+    start_pos = sa.Column(sa.Integer, default = None)
+    end_pos = sa.Column(sa.Integer, default = None)
+
 
 class Quote(TaggedContent):
     """Represents a highlight (quote) from the book."""