feat(ereader): wire highlighting into EreaderLayoutManager (R7)

core/highlight.py was 249 lines of fully implemented, fully tested code
that nothing could reach. EreaderLayoutManager had no highlight API, so
highlighting was not available through the library's own interface - it
was tested in isolation and otherwise dead.

Adds to the manager:

    highlight_point(point, color, note, tags)   tap to highlight a word
    highlight_range(start, end, ...)            drag to highlight a span
    remove_highlight(id) / clear_highlights()
    list_highlights() / get_highlights_for_current_page()

Highlights default to the bookmarks directory, so a document's reading
state lives in one place rather than two.

A Highlight carried only pixel bounds, which describe the one rendering
they were taken from: change the font scale or page size and they no
longer point at anything. Highlight now also records the
RenderingPosition of the page it was made on, and page association goes
through that instead of through bounds overlap. The field is optional and
read with .get, so existing stores load unchanged - they simply never
match a page, which is the honest answer for a highlight whose only
anchor is stale pixels.

Also removes the persistence duplication the review called out.
BookmarkManager and HighlightManager each had their own copy of "make the
directory, read the file, swallow and print on failure". Both now use
core/persistence.py, which logs with exc_info instead of printing and
catches specific exceptions rather than bare Exception. File names and
formats are unchanged, so nothing needs migrating.

Tests cover point and range highlighting, colour/note/tag round trips,
misses returning None, page scoping across navigation, persistence across
a restart, and that a corrupt highlight file does not stop a book from
opening.

902 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pyWebLayout/core/highlight.py

@@ -6,12 +6,16 @@ managing highlight collections, and rendering highlights on pages.
 """
 
 from __future__ import annotations
+import logging
 from dataclasses import dataclass
 from typing import List, Tuple, Optional, Dict, Any
 from enum import Enum
-import json
 from pathlib import Path
 
+from pyWebLayout.core.persistence import ensure_dir, read_json, write_json
+
+logger = logging.getLogger(__name__)
+
 
 class HighlightColor(Enum):
     """Predefined highlight colors with RGBA values"""
@@ -44,6 +48,12 @@ class Highlight:
     start_word_index: Optional[int] = None  # Word index in document (if available)
     end_word_index: Optional[int] = None
 
+    # Where in the document this highlight lives, as a serialized
+    # RenderingPosition. `bounds` are pixel coordinates on one particular
+    # rendering, so they stop matching as soon as the font scale or page size
+    # changes; this survives repagination and is what page association uses.
+    position: Optional[Dict[str, Any]] = None
+
     # Metadata
     note: Optional[str] = None  # Optional annotation
     tags: List[str] = None  # Optional categorization tags
@@ -63,6 +73,7 @@ class Highlight:
             'text': self.text,
             'start_word_index': self.start_word_index,
             'end_word_index': self.end_word_index,
+            'position': self.position,
             'note': self.note,
             'tags': self.tags,
             'timestamp': self.timestamp
@@ -78,6 +89,7 @@ class Highlight:
             text=data['text'],
             start_word_index=data.get('start_word_index'),
             end_word_index=data.get('end_word_index'),
+            position=data.get('position'),
             note=data.get('note'),
             tags=data.get('tags', []),
             timestamp=data.get('timestamp')
@@ -100,12 +112,9 @@ class HighlightManager:
             highlights_dir: Directory to store highlight data
         """
         self.document_id = document_id
-        self.highlights_dir = Path(highlights_dir)
+        self.highlights_dir = ensure_dir(highlights_dir)
         self.highlights: Dict[str, Highlight] = {}  # id -> Highlight
 
-        # Create directory if it doesn't exist
-        self.highlights_dir.mkdir(parents=True, exist_ok=True)
-
         # Load existing highlights
         self._load_highlights()
 
@@ -178,34 +187,22 @@ class HighlightManager:
 
     def _save_highlights(self) -> None:
         """Persist highlights to disk"""
-        try:
+        write_json(self._get_filepath(), {
-            filepath = self._get_filepath()
-            data = {
             'document_id': self.document_id,
             'highlights': [h.to_dict() for h in self.highlights.values()]
-            }
+        })
-
-            with open(filepath, 'w') as f:
-                json.dump(data, f, indent=2)
-        except Exception as e:
-            print(f"Error saving highlights: {e}")
 
     def _load_highlights(self) -> None:
         """Load highlights from disk"""
+        data = read_json(self._get_filepath(), {})
         try:
-            filepath = self._get_filepath()
-            if not filepath.exists():
-                return
-
-            with open(filepath, 'r') as f:
-                data = json.load(f)
-
             self.highlights = {
                 h['id']: Highlight.from_dict(h)
                 for h in data.get('highlights', [])
             }
-        except Exception as e:
+        except (AttributeError, TypeError, KeyError):
-            print(f"Error loading highlights: {e}")
+            logger.warning("Highlight file %s is not in the expected shape; ignoring it",
+                           self._get_filepath(), exc_info=True)
             self.highlights = {}
 
 
@@ -213,16 +210,18 @@ def create_highlight_from_query_result(
     result,
     color: Tuple[int, int, int, int] = HighlightColor.YELLOW.value,
     note: Optional[str] = None,
-    tags: Optional[List[str]] = None
+    tags: Optional[List[str]] = None,
+    position: Optional[Dict[str, Any]] = None
 ) -> Highlight:
     """
     Create a highlight from a QueryResult.
 
     Args:
-        result: QueryResult from query_pixel or query_range
+        result: QueryResult from query_point or query_range
         color: RGBA color tuple
         note: Optional annotation
         tags: Optional categorization tags
+        position: Serialized RenderingPosition of the page the result came from
 
     Returns:
         Highlight instance
@@ -243,6 +242,7 @@ def create_highlight_from_query_result(
         bounds=bounds,
         color=color,
         text=text,
+        position=position,
         note=note,
         tags=tags or [],
         timestamp=time()

pyWebLayout/core/persistence.py

@@ -0,0 +1,59 @@
+"""
+Small JSON-file helpers shared by the per-document stores.
+
+BookmarkManager and HighlightManager both keep a JSON file per document under a
+directory, and both had their own copy of "make the directory, try to read it,
+swallow and print on failure". The duplication is the point of this module; the
+file formats themselves stay owned by each store.
+"""
+
+from __future__ import annotations
+
+import json
+import logging
+from pathlib import Path
+from typing import Any
+
+logger = logging.getLogger(__name__)
+
+
+def ensure_dir(path: str | Path) -> Path:
+    """Return `path` as a Path, creating it and any missing parents."""
+    directory = Path(path)
+    directory.mkdir(parents=True, exist_ok=True)
+    return directory
+
+
+def read_json(path: Path, default: Any) -> Any:
+    """
+    Read JSON from `path`, returning `default` if it is missing or unreadable.
+
+    A corrupt store must not stop a book from opening, so failures are logged
+    and swallowed. `default` is returned as given, so pass a fresh mutable if
+    the caller intends to mutate it.
+    """
+    if not path.exists():
+        return default
+
+    try:
+        with open(path, 'r', encoding='utf-8') as handle:
+            return json.load(handle)
+    except (OSError, ValueError):
+        logger.warning("Could not read %s; ignoring its contents", path, exc_info=True)
+        return default
+
+
+def write_json(path: Path, data: Any) -> bool:
+    """
+    Write `data` to `path` as JSON.
+
+    Returns True on success. Failures are logged rather than raised: losing a
+    bookmark is not a reason to take down the reader.
+    """
+    try:
+        with open(path, 'w', encoding='utf-8') as handle:
+            json.dump(data, handle, indent=2)
+        return True
+    except (OSError, TypeError, ValueError):
+        logger.error("Could not write %s", path, exc_info=True)
+        return False

pyWebLayout/layout/ereader_manager.py

@@ -8,9 +8,7 @@ into a unified, easy-to-use API.
 
 from __future__ import annotations
 from typing import List, Dict, Optional, Tuple, Any, Callable
-import json
 import logging
-from pathlib import Path
 
 from .ereader_layout import RenderingPosition, ChapterNavigator, ChapterInfo
 from .page_buffer import BufferedPageRenderer
@@ -20,6 +18,9 @@ from pyWebLayout.concrete.image import RenderableImage
 from pyWebLayout.style.page_style import PageStyle
 from pyWebLayout.style.fonts import BundledFont
 from pyWebLayout.layout.document_layouter import image_layouter
+from pyWebLayout.core.highlight import Highlight, HighlightColor, HighlightManager, \
+    create_highlight_from_query_result
+from pyWebLayout.core.persistence import ensure_dir, read_json, write_json
 
 logger = logging.getLogger(__name__)
 
@@ -38,8 +39,7 @@ class BookmarkManager:
             bookmarks_dir: Directory to store bookmark files
         """
         self.document_id = document_id
-        self.bookmarks_dir = Path(bookmarks_dir)
+        self.bookmarks_dir = ensure_dir(bookmarks_dir)
-        self.bookmarks_dir.mkdir(exist_ok=True)
 
         self.bookmarks_file = self.bookmarks_dir / f"{document_id}_bookmarks.json"
         self.position_file = self.bookmarks_dir / f"{document_id}_position.json"
@@ -49,29 +49,23 @@ class BookmarkManager:
 
     def _load_bookmarks(self):
         """Load bookmarks from file"""
-        if self.bookmarks_file.exists():
+        data = read_json(self.bookmarks_file, {})
         try:
-                with open(self.bookmarks_file, 'r') as f:
-                    data = json.load(f)
             self._bookmarks = {
                 name: RenderingPosition.from_dict(pos_data)
                 for name, pos_data in data.items()
             }
-            except Exception as e:
+        except (AttributeError, TypeError, KeyError):
-                print(f"Failed to load bookmarks: {e}")
+            logger.warning("Bookmark file %s is not in the expected shape; ignoring it",
+                           self.bookmarks_file, exc_info=True)
             self._bookmarks = {}
 
     def _save_bookmarks(self):
         """Save bookmarks to file"""
-        try:
+        write_json(self.bookmarks_file, {
-            data = {
             name: position.to_dict()
             for name, position in self._bookmarks.items()
-            }
+        })
-            with open(self.bookmarks_file, 'w') as f:
-                json.dump(data, f, indent=2)
-        except Exception as e:
-            print(f"Failed to save bookmarks: {e}")
 
     def add_bookmark(self, name: str, position: RenderingPosition):
         """
@@ -128,11 +122,7 @@ class BookmarkManager:
         Args:
             position: Current reading position
         """
-        try:
+        write_json(self.position_file, position.to_dict())
-            with open(self.position_file, 'w') as f:
-                json.dump(position.to_dict(), f, indent=2)
-        except Exception as e:
-            print(f"Failed to save reading position: {e}")
 
     def load_reading_position(self) -> Optional[RenderingPosition]:
         """
@@ -141,13 +131,14 @@ class BookmarkManager:
         Returns:
             Last reading position or None if not found
         """
-        if self.position_file.exists():
+        data = read_json(self.position_file, None)
+        if data is None:
+            return None
         try:
-                with open(self.position_file, 'r') as f:
-                    data = json.load(f)
             return RenderingPosition.from_dict(data)
-            except Exception as e:
+        except (TypeError, KeyError):
-                print(f"Failed to load reading position: {e}")
+            logger.warning("Position file %s is not in the expected shape; ignoring it",
+                           self.position_file, exc_info=True)
             return None
 
 
@@ -171,7 +162,8 @@ class EreaderLayoutManager:
                  document_id: str = "default",
                  buffer_size: int = 5,
                  page_style: Optional[PageStyle] = None,
-                 bookmarks_dir: str = "bookmarks"):
+                 bookmarks_dir: str = "bookmarks",
+                 highlights_dir: Optional[str] = None):
         """
         Initialize the ereader layout manager.
 
@@ -182,6 +174,8 @@ class EreaderLayoutManager:
             buffer_size: Number of pages to cache in each direction
             page_style: Custom page styling (uses default if None)
             bookmarks_dir: Directory to store bookmark files
+            highlights_dir: Directory to store highlights. Defaults to
+                bookmarks_dir, so a document's reading state lives in one place.
         """
         self.blocks = blocks
         self.page_size = page_size
@@ -196,6 +190,8 @@ class EreaderLayoutManager:
         self.renderer = BufferedPageRenderer(blocks, page_style, buffer_size, page_size)
         self.chapter_navigator = ChapterNavigator(blocks)
         self.bookmark_manager = BookmarkManager(document_id, bookmarks_dir)
+        self.highlight_manager = HighlightManager(
+            document_id, highlights_dir if highlights_dir is not None else bookmarks_dir)
 
         # Current state
         self.current_position = RenderingPosition()
@@ -848,6 +844,104 @@ class EreaderLayoutManager:
         """
         return self.bookmark_manager.list_bookmarks()
 
+    # ------------------------------------------------------------------
+    # Highlights
+    #
+    # A Highlight carries pixel bounds, which belong to the one rendering it
+    # was taken from: change the font scale or page size and they no longer
+    # describe anything. Each highlight therefore also records the
+    # RenderingPosition of the page it was made on, and page association goes
+    # through that rather than through the bounds.
+    # ------------------------------------------------------------------
+
+    def highlight_point(self,
+                        point: Tuple[int, int],
+                        color: Tuple[int, int, int, int] = HighlightColor.YELLOW.value,
+                        note: Optional[str] = None,
+                        tags: Optional[List[str]] = None) -> Optional[Highlight]:
+        """
+        Highlight whatever is at a point on the current page.
+
+        Args:
+            point: (x, y) in page coordinates, as delivered by a tap
+            color: RGBA fill, e.g. one of HighlightColor
+            note: Optional annotation
+            tags: Optional categorization tags
+
+        Returns:
+            The stored Highlight, or None if nothing was at that point.
+        """
+        result = self.get_current_page().query_point(point)
+        if result is None or result.object_type == "empty":
+            return None
+
+        return self._store_highlight(result, color, note, tags)
+
+    def highlight_range(self,
+                        start: Tuple[int, int],
+                        end: Tuple[int, int],
+                        color: Tuple[int, int, int, int] = HighlightColor.YELLOW.value,
+                        note: Optional[str] = None,
+                        tags: Optional[List[str]] = None) -> Optional[Highlight]:
+        """
+        Highlight the text between two points on the current page.
+
+        Args:
+            start: (x, y) where the selection began
+            end: (x, y) where the selection ended
+            color: RGBA fill, e.g. one of HighlightColor
+            note: Optional annotation
+            tags: Optional categorization tags
+
+        Returns:
+            The stored Highlight, or None if the range selected no text.
+        """
+        selection = self.get_current_page().query_range(start, end)
+        if not selection.results:
+            return None
+
+        return self._store_highlight(selection, color, note, tags)
+
+    def _store_highlight(self, result, color, note, tags) -> Highlight:
+        """Build a Highlight from a query result and persist it."""
+        highlight = create_highlight_from_query_result(
+            result, color=color, note=note, tags=tags,
+            position=self.current_position.to_dict())
+        self.highlight_manager.add_highlight(highlight)
+        return highlight
+
+    def remove_highlight(self, highlight_id: str) -> bool:
+        """
+        Remove a highlight.
+
+        Args:
+            highlight_id: ID of the highlight to remove
+
+        Returns:
+            True if it existed and was removed
+        """
+        return self.highlight_manager.remove_highlight(highlight_id)
+
+    def list_highlights(self) -> List[Highlight]:
+        """Get every highlight in this document."""
+        return self.highlight_manager.list_highlights()
+
+    def get_highlights_for_current_page(self) -> List[Highlight]:
+        """
+        Get the highlights made on the page currently being displayed.
+
+        Matched on the recorded RenderingPosition, so this stays correct across
+        font changes; highlights saved before the position field existed have
+        no position and are never matched.
+        """
+        current = self.current_position.to_dict()
+        return [h for h in self.highlight_manager.list_highlights()
+                if h.position == current]
+
+    def clear_highlights(self) -> None:
+        """Remove every highlight in this document."""
+        self.highlight_manager.clear_all()
+
     def get_reading_progress(self) -> float:
         """
         Get reading progress as a percentage.

tests/layout/test_ereader_highlights.py

@@ -0,0 +1,168 @@
+"""
+Tests for the highlight API on EreaderLayoutManager (R7).
+
+core/highlight.py was fully implemented and tested but unreachable: the manager
+had no highlight API, so highlighting could not be used through the library's
+own interface. These tests cover the wiring, not the dataclass - that is
+tests/core/test_highlight.py.
+"""
+
+import pytest
+
+from pyWebLayout.core.highlight import Highlight, HighlightColor
+from pyWebLayout.io.readers.html_extraction import parse_html_string
+from pyWebLayout.layout.ereader_manager import EreaderLayoutManager
+
+
+@pytest.fixture
+def manager(tmp_path):
+    blocks = parse_html_string(
+        "<p>" + " ".join(f"word{i}" for i in range(300)) + "</p>")
+    manager = EreaderLayoutManager(blocks, page_size=(400, 600),
+                                   document_id="highlights",
+                                   bookmarks_dir=str(tmp_path))
+    yield manager
+    manager.shutdown()
+
+
+def text_points(page, limit=None):
+    """Points on the rendered page that land on a text object."""
+    found = []
+    for y in range(0, 120, 2):
+        for x in range(0, 400, 2):
+            result = page.query_point((x, y))
+            if result is not None and result.object_type == "text" and result.text:
+                found.append((x, y))
+                if limit and len(found) >= limit:
+                    return found
+    return found
+
+
+@pytest.fixture
+def point_on_text(manager):
+    page = manager.get_current_page()
+    page.render()
+    return text_points(page, limit=1)[0]
+
+
+class TestHighlightPoint:
+    def test_highlighting_a_word_returns_a_stored_highlight(self, manager, point_on_text):
+        highlight = manager.highlight_point(point_on_text)
+
+        assert isinstance(highlight, Highlight)
+        assert highlight.text
+        assert manager.list_highlights() == [highlight]
+
+    def test_colour_note_and_tags_are_kept(self, manager, point_on_text):
+        highlight = manager.highlight_point(
+            point_on_text, color=HighlightColor.GREEN.value,
+            note="a note", tags=["review"])
+
+        assert highlight.color == HighlightColor.GREEN.value
+        assert highlight.note == "a note"
+        assert highlight.tags == ["review"]
+
+    def test_highlighting_empty_space_returns_none(self, manager):
+        manager.get_current_page().render()
+
+        assert manager.highlight_point((399, 599)) is None
+        assert manager.list_highlights() == []
+
+    def test_the_originating_position_is_recorded(self, manager, point_on_text):
+        highlight = manager.highlight_point(point_on_text)
+
+        assert highlight.position == manager.current_position.to_dict()
+
+
+class TestHighlightRange:
+    def test_a_selection_spans_multiple_words(self, manager):
+        page = manager.get_current_page()
+        page.render()
+        points = text_points(page)
+
+        highlight = manager.highlight_range(points[0], points[-1])
+
+        assert highlight is not None
+        assert len(highlight.text.split()) > 1
+        assert len(highlight.bounds) > 1
+
+    def test_a_selection_hitting_no_text_returns_none(self, manager):
+        manager.get_current_page().render()
+
+        assert manager.highlight_range((398, 596), (399, 599)) is None
+
+
+class TestHighlightsAreScopedToTheirPage:
+    def test_current_page_highlights_do_not_leak_across_pages(self, manager, point_on_text):
+        manager.highlight_point(point_on_text)
+        assert len(manager.get_highlights_for_current_page()) == 1
+
+        manager.next_page()
+
+        assert manager.get_highlights_for_current_page() == []
+        assert len(manager.list_highlights()) == 1, "still in the document, just not here"
+
+    def test_returning_to_the_page_finds_it_again(self, manager, point_on_text):
+        highlight = manager.highlight_point(point_on_text)
+        manager.next_page()
+        manager.previous_page()
+
+        assert manager.get_highlights_for_current_page() == [highlight]
+
+
+class TestPersistence:
+    def test_highlights_survive_a_restart(self, manager, point_on_text, tmp_path):
+        highlight = manager.highlight_point(point_on_text, note="kept")
+        manager.shutdown()
+
+        reopened = EreaderLayoutManager(
+            manager.blocks, page_size=(400, 600), document_id="highlights",
+            bookmarks_dir=str(tmp_path))
+        try:
+            restored = reopened.list_highlights()
+            assert len(restored) == 1
+            assert restored[0].id == highlight.id
+            assert restored[0].note == "kept"
+            assert restored[0].position == highlight.position
+        finally:
+            reopened.shutdown()
+
+    def test_highlights_share_the_bookmarks_directory_by_default(self, manager,
+                                                                 point_on_text, tmp_path):
+        manager.highlight_point(point_on_text)
+
+        assert (tmp_path / "highlights_highlights.json").exists()
+
+    def test_removing_a_highlight_persists(self, manager, point_on_text, tmp_path):
+        highlight = manager.highlight_point(point_on_text)
+
+        assert manager.remove_highlight(highlight.id) is True
+        assert manager.remove_highlight(highlight.id) is False
+
+        reopened = EreaderLayoutManager(
+            manager.blocks, page_size=(400, 600), document_id="highlights",
+            bookmarks_dir=str(tmp_path))
+        try:
+            assert reopened.list_highlights() == []
+        finally:
+            reopened.shutdown()
+
+    def test_clear_removes_everything(self, manager, point_on_text):
+        manager.highlight_point(point_on_text)
+
+        manager.clear_highlights()
+
+        assert manager.list_highlights() == []
+
+    def test_a_corrupt_store_does_not_stop_the_book_opening(self, tmp_path):
+        (tmp_path / "broken_highlights.json").write_text("{not json")
+        blocks = parse_html_string("<p>hello world</p>")
+
+        manager = EreaderLayoutManager(blocks, page_size=(400, 600),
+                                       document_id="broken",
+                                       bookmarks_dir=str(tmp_path))
+        try:
+            assert manager.list_highlights() == []
+            assert manager.get_current_page() is not None
+        finally:
+            manager.shutdown()