diff --git a/alembic/versions/35f453946f1e_added_start_and_end_pos_to_note.py b/alembic/versions/35f453946f1e_added_start_and_end_pos_to_note.py new file mode 100644 index 00000000..b7775b2e --- /dev/null +++ b/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 ### diff --git a/bookworm/annotation/__init__.py b/bookworm/annotation/__init__.py index 9c953f59..b785e578 100644 --- a/bookworm/annotation/__init__.py +++ b/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" diff --git a/bookworm/annotation/annotation_dialogs.py b/bookworm/annotation/annotation_dialogs.py index c27fd4a0..1c69a5f9 100644 --- a/bookworm/annotation/annotation_dialogs.py +++ b/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): diff --git a/bookworm/annotation/annotation_gui.py b/bookworm/annotation/annotation_gui.py index 450fc6a7..7cc6577b 100644 --- a/bookworm/annotation/annotation_gui.py +++ b/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 diff --git a/bookworm/annotation/annotator.py b/bookworm/annotation/annotator.py index f468a950..9dacb365 100644 --- a/bookworm/annotation/annotator.py +++ b/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.""" diff --git a/bookworm/database/__init__.py b/bookworm/database/__init__.py index d6913fb8..344d7ed1 100644 --- a/bookworm/database/__init__.py +++ b/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 diff --git a/bookworm/database/models.py b/bookworm/database/models.py index e7fb8501..1cf0c135 100644 --- a/bookworm/database/models.py +++ b/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.""" diff --git a/tests/conftest.py b/tests/conftest.py index 32a4eff6..95ed2e39 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,8 +1,92 @@ +import os from pathlib import Path +import tempfile +import time import pytest +from sqlalchemy.orm import close_all_sessions +from sqlalchemy.pool import NullPool + +from bookworm.config import setup_config +from bookworm.database import init_database +from bookworm.database.models import Base +from bookworm.document.elements import Section +from bookworm.reader import EBookReader @pytest.fixture(scope="function", autouse=True) def asset(): yield lambda filename: str(Path(__file__).parent / "assets" / filename) + + +class DummyTextCtrl: + def SetFocus(self): + pass + + +class DummyView: + """Represents a mock for the bookworm view""" + def __init__(self): + self.title = "" + self.toc_tree = None + self.input_result = "" + self.insertion_point = 0 + self.state_on_section_change: Section = None + self.contentTextCtrl = DummyTextCtrl() + + def add_toc_tree(self, tree): + self.toc_tree = tree + + def set_text_direction(self, direction): + pass + + def set_title(self, title): + self.title = title + + def get_insertion_point(self): + return self.insertion_point + + def set_state_on_section_change(self, value: Section) -> None: + self.state_on_section_change = value + + def set_state_on_page_change(self, page): + pass + + def set_insertion_point(self, point: int) -> None: + self.insertion_point = point + + def go_to_position(self, start: int, end: int) -> None: + self.set_insertion_point(start) + + def go_to_webpage(self, url: str) -> None: + pass + + def show_html_dialog(self, markup: str, title: str) -> None: + pass + +@pytest.fixture() +def text_ctrl(): + yield DummyTextCtrl() + +@pytest.fixture +def view(text_ctrl): + v = DummyView() + v.contentTextCtrl = text_ctrl + yield v + +@pytest.fixture() +def engine(tmp_path): + temp_file = tmp_path / "test.db" + engine = init_database( + url=f"sqlite:///{temp_file}", + poolclass = NullPool + ) + yield engine + close_all_sessions() + engine.dispose() + +@pytest.fixture() +def reader(view, engine): + setup_config() + reader = EBookReader(view) + yield reader diff --git a/tests/test_annotator.py b/tests/test_annotator.py new file mode 100644 index 00000000..e2fb064e --- /dev/null +++ b/tests/test_annotator.py @@ -0,0 +1,20 @@ +import pytest + +from bookworm.annotation import NoteTaker +from bookworm.database.models import * +from bookworm.document.uri import DocumentUri + +from conftest import asset, reader + +def test_notes_can_not_overlap(asset, reader): + uri = DocumentUri.from_filename(asset('roman.epub')) + reader.load(uri) + annot = NoteTaker(reader) + # This should succeed + comment = annot.create(title='test', content="test", position=0, start_pos=0, end_pos=1) + # check if it overlaps at start_pos 0, end_pos 1, page_number 0 and position 0 + assert annot.overlaps(0, 1, 0, 0) == True + # Check if no selection with position 0 and page_number 0 overlaps with the existing annotation + assert annot.overlaps(None, None, 0, 0) + # This should not overlap + assert annot.overlaps(None, None, 0, 2) == False