fix(annotation): Allow comments to target text selections (#280)

* Fixed if condition in order to explicitly check whether the engine passed is None

* Added new alembic revision

* Added support for comments targeting selected text

* Handling comments that target part of a line in the GUI

* Improved selection precision and whenever a selection overlaps with a comment that points to it said comment will be edited

* Added dummyview implementation to test services

* Further improved text fixtures

* Added more relevant tests

* Modified comment ordering logic

* Revised tests

* Eliminated TODO item

* Update bookworm/annotation/__init__.py

Co-authored-by: Rowen <manchen_0528@outlook.com>

* Overlapping issues will be reported before the user can be prompted to insert a new note

* Fixed import error

* Fixed import Error

* Cleaned up dead code

---------

Co-authored-by: cary-rowen <manchen_0528@outlook.com>

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,11 @@ 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:
+            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:
                 evtdata["comment"] = True
-                break
         wx.CallAfter(self._process_caret_move, evtdata)
 
     def _process_caret_move(self, evtdata):
@@ -252,8 +259,9 @@ 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
+                comment_msg = _("Has comment")
+                to_speak.append(comment_msg)
             speech.announce(" ".join(to_speak), False)
 
     def get_annotation(self, annotator_cls, *, foreword):
@@ -262,7 +270,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 +288,8 @@ 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 = 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

@@ -203,6 +203,21 @@ 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)
+        if comments.overlaps(start_pos, end_pos, self.reader.current_page, insertionPoint):
+            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.")
+            )
         comment_text = self.view.get_text_from_user(
             # Translators: the title of a dialog to add a comment
             _("New Comment"),
@@ -212,9 +227,10 @@ def onAddNote(self, event):
         )
         if not comment_text:
             return
-        note = NoteTaker(self.reader).create(
-            title="", content=comment_text, position=insertionPoint
+        note = comments.create(
+            title="", content=comment_text, position=insertionPoint, start_pos=start_pos, end_pos=end_pos
         )
+
         self.service.style_comment(self.view, insertionPoint)
         if _with_tags:
             # add tags

bookworm/annotation/annotator.py

@@ -2,6 +2,7 @@
 
 from dataclasses import astuple, dataclass
 from enum import IntEnum, auto
+from typing import Optional
 
 import sqlalchemy as sa
 
@@ -12,7 +13,6 @@
 log = logger.getChild(__name__)
 # The bakery caches query objects to avoid recompiling them into strings in every call
 
-
 @dataclass
 class AnnotationFilterCriteria:
     book_id: int = 0
@@ -141,6 +141,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,13 +235,112 @@ def delete_orphan_tags(cls):
             session.delete(otag)
         session.commit()
 
+class PositionedAnnotator(TaggedAnnotator):
+    """Annotations which are positioned on a specific text range"""
 
-class NoteTaker(TaggedAnnotator):
+    def overlaps(self, start: Optional[int], end: Optional[int], page_number: int, position: int) -> bool:
+        """
+        Determines whether an annotation overlaps with  a given position
+        The criterias used to check for the position are the following:
+        - If a selection is present, represented by start and end, then it is checked
+        - If no selection is present the insertion point is used to determine if the annotation overlaps
+        """
+        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,
+                    )
+                )
+            )
+        ]
+        return self.session.query(model).filter_by(book_id = self.current_book.id).filter(sa.or_(*clauses)).one_or_none() is not None
+
+
+class NoteTaker(PositionedAnnotator):
     """Comments."""
 
     model = Note
 
 
+    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."""