E-book viewer: Fix last read position (and bookmarks in general) being inaccurate for some books. Consequences: Bookmarks creaetd with this version of calibre will not work with previous calibre releases. Uses the CFI specification from the EPUB 3 standard.

src/calibre/ebooks/oeb/iterator.py

@@ -26,6 +26,8 @@ from calibre.constants import filesystem_encoding
 TITLEPAGE = CoverManager.SVG_TEMPLATE.decode('utf-8').replace(\
         '__ar__', 'none').replace('__viewbox__', '0 0 600 800'
         ).replace('__width__', '600').replace('__height__', '800')
+BM_FIELD_SEP = u'*|!|?|*'
+BM_LEGACY_ESC = u'esc-text-%&*#%(){}ads19-end-esc'
 
 def character_count(html):
     '''
@@ -273,27 +275,62 @@ class EbookIterator(object):
 
     def parse_bookmarks(self, raw):
         for line in raw.splitlines():
+            bm = None
             if line.count('^') > 0:
                 tokens = line.rpartition('^')
                 title, ref = tokens[0], tokens[2]
-                self.bookmarks.append((title, ref))
+                try:
+                    spine, _, pos = ref.partition('#')
+                    spine = int(spine.strip())
+                except:
+                    continue
+                bm = {'type':'legacy', 'title':title, 'spine':spine, 'pos':pos}
+            elif BM_FIELD_SEP in line:
+                try:
+                    title, spine, pos = line.strip().split(BM_FIELD_SEP)
+                    spine = int(spine)
+                except:
+                    continue
+                # Unescape from serialization
+                pos = pos.replace(BM_LEGACY_ESC, u'^')
+                # Check for pos being a scroll fraction
+                try:
+                    pos = float(pos)
+                except:
+                    pass
+                bm = {'type':'cfi', 'title':title, 'pos':pos, 'spine':spine}
+
+            if bm:
+                self.bookmarks.append(bm)
 
     def serialize_bookmarks(self, bookmarks):
         dat = []
-        for title, bm in bookmarks:
+        for bm in bookmarks:
-            dat.append(u'%s^%s'%(title, bm))
+            if bm['type'] == 'legacy':
-        return (u'\n'.join(dat) +'\n').encode('utf-8')
+                rec = u'%s^%d#%s'%(bm['title'], bm['spine'], bm['pos'])
+            else:
+                pos = bm['pos']
+                if isinstance(pos, (int, float)):
+                    pos = unicode(pos)
+                else:
+                    pos = pos.replace(u'^', BM_LEGACY_ESC)
+                rec = BM_FIELD_SEP.join([bm['title'], unicode(bm['spine']), pos])
+            dat.append(rec)
+        return (u'\n'.join(dat) +u'\n')
 
     def read_bookmarks(self):
         self.bookmarks = []
         bmfile = os.path.join(self.base, 'META-INF', 'calibre_bookmarks.txt')
         raw = ''
         if os.path.exists(bmfile):
-            raw = open(bmfile, 'rb').read().decode('utf-8')
+            with open(bmfile, 'rb') as f:
+                raw = f.read()
         else:
             saved = self.config['bookmarks_'+self.pathtoebook]
             if saved:
                 raw = saved
+        if not isinstance(raw, unicode):
+            raw = raw.decode('utf-8')
         self.parse_bookmarks(raw)
 
     def save_bookmarks(self, bookmarks=None):
@@ -306,18 +343,15 @@ class EbookIterator(object):
                 zf = open(self.pathtoebook, 'r+b')
             except IOError:
                 return
-            safe_replace(zf, 'META-INF/calibre_bookmarks.txt', StringIO(dat),
+            safe_replace(zf, 'META-INF/calibre_bookmarks.txt',
+                    StringIO(dat.encode('utf-8')),
                     add_missing=True)
         else:
             self.config['bookmarks_'+self.pathtoebook] = dat
 
     def add_bookmark(self, bm):
-        dups = []
+        self.bookmarks = [x for x in self.bookmarks if x['title'] !=
-        for x in self.bookmarks:
+                bm['title']]
-            if x[0] == bm[0]:
-                dups.append(x)
-        for x in dups:
-            self.bookmarks.remove(x)
         self.bookmarks.append(bm)
         self.save_bookmarks()
 

src/calibre/gui2/viewer/bookmarkmanager.py

@@ -31,6 +31,7 @@ class BookmarkManager(QDialog, Ui_BookmarkManager):
             bookmarks = self.bookmarks[:]
         self._model = BookmarkTableModel(self, bookmarks)
         self.bookmarks_table.setModel(self._model)
+        self.bookmarks_table.resizeColumnsToContents()
 
     def delete_bookmark(self):
         indexes = self.bookmarks_table.selectionModel().selectedIndexes()
@@ -80,7 +81,7 @@ class BookmarkManager(QDialog, Ui_BookmarkManager):
             if not bad:
                 bookmarks = self._model.bookmarks[:]
                 for bm in imported:
-                    if bm not in bookmarks and bm[0] != 'calibre_current_page_bookmark':
+                    if bm not in bookmarks and bm['title'] != 'calibre_current_page_bookmark':
                         bookmarks.append(bm)
                 self.set_bookmarks(bookmarks)
 
@@ -105,13 +106,14 @@ class BookmarkTableModel(QAbstractTableModel):
 
     def data(self, index, role):
         if role in (Qt.DisplayRole, Qt.EditRole):
-            ans = self.bookmarks[index.row()][0]
+            ans = self.bookmarks[index.row()]['title']
             return NONE if ans is None else QVariant(ans)
         return NONE
 
     def setData(self, index, value, role):
         if role == Qt.EditRole:
-            self.bookmarks[index.row()] = (unicode(value.toString()).strip(), self.bookmarks[index.row()][1])
+            bm = self.bookmarks[index.row()]
+            bm['title'] = unicode(value.toString()).strip()
             self.emit(SIGNAL("dataChanged(QModelIndex, QModelIndex)"), index, index)
             return True
         return False

src/calibre/gui2/viewer/documentview.py

@@ -4,7 +4,7 @@ __copyright__ = '2008, Kovid Goyal kovid@kovidgoyal.net'
 __docformat__ = 'restructuredtext en'
 
 # Imports {{{
-import os, math, glob, sys, zipfile
+import os, math, glob, zipfile
 from base64 import b64encode
 from functools import partial
 
@@ -313,10 +313,14 @@ class Document(QWebPage): # {{{
         self.javascript('goto_reference("%s")'%ref)
 
     def goto_bookmark(self, bm):
-        bm = bm.strip()
+        if bm['type'] == 'legacy':
-        if bm.startswith('>'):
+            bm = bm['pos']
-            bm = bm[1:].strip()
+            bm = bm.strip()
-        self.javascript('scroll_to_bookmark("%s")'%bm)
+            if bm.startswith('>'):
+                bm = bm[1:].strip()
+            self.javascript('scroll_to_bookmark("%s")'%bm)
+        elif bm['type'] == 'cfi':
+            self.page_position.to_pos(bm['pos'])
 
     def javascript(self, string, typ=None):
         ans = self.mainFrame().evaluateJavaScript(string)
@@ -367,40 +371,9 @@ class Document(QWebPage): # {{{
     def elem_outer_xml(self, elem):
         return unicode(elem.toOuterXml())
 
-    def find_bookmark_element(self):
-        mf = self.mainFrame()
-        doc_pos = self.ypos
-        min_delta, min_elem = sys.maxint, None
-        for y in range(10, -500, -10):
-            for x in range(-50, 500, 10):
-                pos = QPoint(x, y)
-                result = mf.hitTestContent(pos)
-                if result.isNull(): continue
-                elem = result.enclosingBlockElement()
-                if elem.isNull(): continue
-                try:
-                    ypos = self.element_ypos(elem)
-                except:
-                    continue
-                delta = abs(ypos - doc_pos)
-                if delta < 25:
-                    return elem
-                if delta < min_delta:
-                    min_elem, min_delta = elem, delta
-        return min_elem
-
-
     def bookmark(self):
-        elem = self.find_bookmark_element()
+        pos = self.page_position.current_pos
-
+        return {'type':'cfi', 'pos':pos}
-        if elem is None or self.element_ypos(elem) < 100:
-            bm = 'body|%f'%(float(self.ypos)/(self.height*0.7))
-        else:
-            bm = unicode(elem.evaluateJavaScript(
-                'calculate_bookmark(%d, this)'%self.ypos).toString())
-            if not bm:
-                bm = 'body|%f'%(float(self.ypos)/(self.height*0.7))
-        return bm
 
     @property
     def at_bottom(self):

src/calibre/gui2/viewer/main.py

@@ -513,17 +513,18 @@ class EbookViewer(MainWindow, Ui_EbookViewer):
                     self.load_path(self.iterator.spine[spine_index])
 
     def goto_bookmark(self, bm):
-        m = bm[1].split('#')
+        spine_index = bm['spine']
-        if len(m) > 1:
+        if spine_index > -1 and self.current_index == spine_index:
-            spine_index, m = int(m[0]), m[1]
+            if self.resize_in_progress:
-            if spine_index > -1 and self.current_index == spine_index:
+                self.view.document.page_position.set_pos(bm['pos'])
-                self.view.goto_bookmark(m)
             else:
-                self.pending_bookmark = bm
+                self.view.goto_bookmark(bm)
-                if spine_index < 0 or spine_index >= len(self.iterator.spine):
+        else:
-                    spine_index = 0
+            self.pending_bookmark = bm
-                    self.pending_bookmark = None
+            if spine_index < 0 or spine_index >= len(self.iterator.spine):
-                self.load_path(self.iterator.spine[spine_index])
+                spine_index = 0
+                self.pending_bookmark = None
+            self.load_path(self.iterator.spine[spine_index])
 
     def toc_clicked(self, index):
         item = self.toc_model.itemFromIndex(index)
@@ -700,6 +701,14 @@ class EbookViewer(MainWindow, Ui_EbookViewer):
         self.view.load_path(path, pos=pos)
 
     def viewport_resize_started(self, event):
+        old, curr = event.size(), event.oldSize()
+        if not self.window_mode_changed and old.width() == curr.width():
+            # No relayout changes, so page position does not need to be saved
+            # This is needed as Qt generates a viewport resized event that
+            # changes only the height after a file has been loaded. This can
+            # cause the last read position bookmark to become slightly
+            # inaccurate
+            return
         if not self.resize_in_progress:
             # First resize, so save the current page position
             self.resize_in_progress = True
@@ -747,9 +756,10 @@ class EbookViewer(MainWindow, Ui_EbookViewer):
                 _('Enter title for bookmark:'), text=bm)
         title = unicode(title).strip()
         if ok and title:
-            pos = self.view.bookmark()
+            bm = self.view.bookmark()
-            bookmark = '%d#%s'%(self.current_index, pos)
+            bm['spine'] = self.current_index
-            self.iterator.add_bookmark((title, bookmark))
+            bm['title'] = title
+            self.iterator.add_bookmark(bm)
             self.set_bookmarks(self.iterator.bookmarks)
 
     def set_bookmarks(self, bookmarks):
@@ -759,12 +769,12 @@ class EbookViewer(MainWindow, Ui_EbookViewer):
         current_page = None
         self.existing_bookmarks = []
         for bm in bookmarks:
-            if bm[0] == 'calibre_current_page_bookmark' and \
+            if bm['title'] == 'calibre_current_page_bookmark':
-                                self.get_remember_current_page_opt():
+                if self.get_remember_current_page_opt():
-                current_page = bm
+                    current_page = bm
             else:
-                self.existing_bookmarks.append(bm[0])
+                self.existing_bookmarks.append(bm['title'])
-                self.bookmarks_menu.addAction(bm[0], partial(self.goto_bookmark, bm))
+                self.bookmarks_menu.addAction(bm['title'], partial(self.goto_bookmark, bm))
         return current_page
 
     def manage_bookmarks(self):
@@ -784,9 +794,10 @@ class EbookViewer(MainWindow, Ui_EbookViewer):
             return
         if hasattr(self, 'current_index'):
             try:
-                pos = self.view.bookmark()
+                bm = self.view.bookmark()
-                bookmark = '%d#%s'%(self.current_index, pos)
+                bm['spine'] = self.current_index
-                self.iterator.add_bookmark(('calibre_current_page_bookmark', bookmark))
+                bm['title'] = 'calibre_current_page_bookmark'
+                self.iterator.add_bookmark(bm)
             except:
                 traceback.print_exc()
 

src/calibre/gui2/viewer/position.py

@@ -67,10 +67,16 @@ class PagePosition(object):
 
     def restore(self):
         if self._cpos is None: return
-        if isinstance(self._cpos, (int, float)):
+        self.to_pos(self._cpos)
-            self.document.scroll_fraction = self._cpos
-        else:
-            self.scroll_to_cfi(self._cpos)
         self._cpos = None
 
+    def to_pos(self, pos):
+        if isinstance(pos, (int, float)):
+            self.document.scroll_fraction = pos
+        else:
+            self.scroll_to_cfi(pos)
+
+    def set_pos(self, pos):
+        self._cpos = pos
+