Merge lp:~adamreichold/qpdfview/extended-text-selection into lp:qpdfview

[Adam Reichold]

This branch extends the model to allow for extended text selections, i.e. an arbitrary boundary and the text contained within it, and implements this within the PDF model. It also modifies the PageItem so that it makes use of this selection if available, preferring it over the previous method of text extraction and also highlighting the boundary during the selection process.

The extended text extraction itself uses the same Poppler API as the cached text extraction and relies on the fact that the text boxes are provided in "approximately reading order" by Poppler already. Hence this might be much more complicated to implement in the other backends, e.g. DjVuLibre, but therefore it should stay optional IMHO.

Some of the points that need to be discussed IMHO are:

* Do we want to always use it if made available by the model, or maybe extend the copy-to-clipboard pop-up menu with additional options to explicitly select the method of text extraction instead?

* Is the performance sufficient or do we need to devise a more incremental computation? It feels alright on my machine but I did come to wrong conclusions based on this in the past. This also affects whether this should be put behind a configuration setting or into a separate rubber-band-mode.

* This currently works on a per-text-box, i.e. approximately per-word, level and could be extended to work on a per-character level but I suspect with significant overhead for either tracking character runs or already aggregating runs into a boundary and contained text.

* Should the boundary be processed further to provide more connected highlighting and if so how, e.g. computing (an approximation of) the convex hull?

[Adam Reichold (adamreichold)]

Also note that this does not make any use of tagged PDF features and hence its reliability w.r.t. complicated layouts is probably questionable...

[lp:~adamreichold/qpdfview/extended-text-selection]

1935. By Adam Reichold on 2015-07-09

Since PdfPage::text is now a fallback interface we can remove the separate Page::cachedText entry point which all other plugins forwarded to Page::text anyway.

1936. By Adam Reichold on 2015-07-09

Some minor cosmetic clean-ups to PDF text extraction and selection.

[Razi Alavizadeh (srazi)]

Hello Adam,

Also note that this does not make any use of tagged PDF features and hence
> its reliability w.r.t. complicated layouts is probably questionable...
>

Is related? With this branch it selects text of RTL documents in reversed
order.

CORRECT: این عشق خانمانسوز
INCORRECT: زوسنامناخ قشع نیا
I attached a PDF in Persian for testing.

Best regards,
Razi.

2015-07-09 22:46 GMT+04:30 Adam Reichold <email address hidden>:

> Also note that this does not make any use of tagged PDF features and hence
> its reliability w.r.t. complicated layouts is probably questionable...
> --
>
> https://code.launchpad.net/~adamreichold/qpdfview/extended-text-selection/+merge/264322
> You are requested to review the proposed merge of
> lp:~adamreichold/qpdfview/extended-text-selection into lp:qpdfview.
>

--
Alavizadeh, Sayed Razi
My Blog: http://pozh.org
Saaghar (نرم‌افزار شعر): http://saaghar.pozh.org/
Saaghar Fan Page: http://www.facebook.com/saaghar.p
Saaghar Mailing List: http://groups.google.com/group/saaghar

[Adam Reichold (adamreichold)]

Hello,

> Is related? With this branch it selects text of RTL documents in reversed
> order.
>
> CORRECT: این عشق خانمانسوز
> INCORRECT: زوسنامناخ قشع نیا
> I attached a PDF in Persian for testing.

I don't think this is related to tagged PDF since the previous method of text extraction did not use it as well. But it is obviously incorrect and a result of loosing some of the heuristics which where implemented within Poppler::Page::text and Poppler::Page::search.

What I wonder is though, this should affect the extended search dock in the same way as it uses the same method of text extraction? At least I don't understand how there could be a difference in behaviour...

From how I understand these things, "fixing" this by just calling QString::reverse if Document::wantsRightToLeft() will not be correct?

Best regards, Adam.

[Benjamin Eltzner (b-eltzner)]

Hi Adam,

I noticed the following:

* When selecting several lines of text, line breaks are not preserved
and not replaced by blank spaces, which leads to word concatenations.

* The selection tool is still a "frame", so when the text portion to be
selected starts to the right of the position where it ends one will
still have to copy unwanted text bits. Maybe the selection tool could be
adjusted to select "line wise" from the "button push position" to the
"button release position".

* I did not have any trouble concerning performance.

I agree that it would be favorable to be able to use the tagged PDF
features, however it seems that the poppler project maintainers are not
particularly interested in these. So I am really glad you propose this
implementation and are willing to take the burden of maintenance.

Cheers, Benjamin

[Razi Alavizadeh (srazi)]

Hello Adam,

What I wonder is though, this should affect the extended search dock in the
> same way as it uses the same method of text extraction? At least I don't
> understand how there could be a difference in behaviour...
>

Yes, indeed the extended search dock is affected, too. But I didn't report
it, because it is the bug of Poppler, and also the issue is bigger when we
speak about searching:

If you want search for RTL phrase "ABC" by Poppler then you have to reverse
it, i.e. you have to search for "CBA" and if it's a mixed phrase i.e "ABC
123" that "123" is an LTR phrase then you have to search for "CBA 123". see
the long standing bug [1].

From how I understand these things, "fixing" this by just calling
> QString::reverse if Document::wantsRightToLeft() will not be correct?
>
Well I have some problem and points here:
1- QString::reverse() (that we should implement it) is a solution when text
is RTL only.

2- What is reterned by "Document::wantsRightToLeft()" if there is both RTL
and LTR content?

3 And my problem: I compiled Poppler 0.29 and 0.34 but when linking with
both of them there is error about symbol not found :| for both of them it
says "m_document->textDirection()" is not found and for version 0.34 it
says "
Poppler::Page::SearchFlags" is not found, maybe its because I disable
"libcairo" when compiling? or Do I have set some flags?

Btw, I wrote a patch that fixes problem with extracting/searching [2].
Don't you think we can port this patch to Poppler's code as a patch for [1]?

[1] https://bugs.freedesktop.org/show_bug.cgi?id=2981
[2] https://code.launchpad.net/~srazi/qpdfview/extended-text-selection

Best Regards,
Razi.

2015-07-11 11:13 GMT+04:30 Adam Reichold <email address hidden>:

> Hello,
>
> > Is related? With this branch it selects text of RTL documents in reversed
> > order.
> >
> > CORRECT: این عشق خانمانسوز
> > INCORRECT: زوسنامناخ قشع نیا
> > I attached a PDF in Persian for testing.
>
> I don't think this is related to tagged PDF since the previous method of
> text extraction did not use it as well. But it is obviously incorrect and a
> result of loosing some of the heuristics which where implemented within
> Poppler::Page::text and Poppler::Page::search.
>
> What I wonder is though, this should affect the extended search dock in
> the same way as it uses the same method of text extraction? At least I
> don't understand how there could be a difference in behaviour...
>
> >From how I understand these things, "fixing" this by just calling
> QString::reverse if Document::wantsRightToLeft() will not be correct?
>
> Best regards, Adam.
> --
>
> https://code.launchpad.net/~adamreichold/qpdfview/extended-text-selection/+merge/264322
> You are requested to review the proposed merge of
> lp:~adamreichold/qpdfview/extended-text-selection into lp:qpdfview.
>

--
Alavizadeh, Sayed Razi
My Blog: http://pozh.org
Saaghar (نرم‌افزار شعر): http://saaghar.pozh.org/
Saaghar Fan Page: http://www.facebook.com/saaghar.p
Saaghar Mailing List: http://groups.google.com/group/saaghar

[Adam Reichold (adamreichold)]

Hello Razi,

Am 13.07.2015 um 17:33 schrieb S. Razi Alavizadeh:
> Yes, indeed the extended search dock is affected, too. But I didn't report
> it, because it is the bug of Poppler, and also the issue is bigger when we
> speak about searching:
>
> If you want search for RTL phrase "ABC" by Poppler then you have to reverse
> it, i.e. you have to search for "CBA" and if it's a mixed phrase i.e "ABC
> 123" that "123" is an LTR phrase then you have to search for "CBA 123". see
> the long standing bug [1].

> Btw, I wrote a patch that fixes problem with extracting/searching [2].
> Don't you think we can port this patch to Poppler's code as a patch for [1]?

I will look into what it would take to get this into Poppler, especially
reading up on the history of [1] seems necessary before submitting
anything. But could you rebase your work on qpdfview's trunk as a
separate merge request? (Because I will reject this merge and abandon
the branch for which I'll give an explanation in a separate message.)

>>From how I understand these things, "fixing" this by just calling
>> QString::reverse if Document::wantsRightToLeft() will not be correct?
>>
> Well I have some problem and points here:
> 1- QString::reverse() (that we should implement it) is a solution when text
> is RTL only.
>
> 2- What is reterned by "Document::wantsRightToLeft()" if there is both RTL
> and LTR content?

This is exactly what I was thinking of. So your bidirectional method
seems like the way to go. Is this a full implementation of the BiDi
algorithm?

> 3 And my problem: I compiled Poppler 0.29 and 0.34 but when linking with
> both of them there is error about symbol not found :| for both of them it
> says "m_document->textDirection()" is not found and for version 0.34 it
> says "
> Poppler::Page::SearchFlags" is not found, maybe its because I disable
> "libcairo" when compiling? or Do I have set some flags?

Have you checked that qpdfview is building against the correct Poppler
include path? This sounds like you try to build with "HAS_POPPLER_31"
but without adjustment of "INCLUDEPATH"? It should definitely be
independent of whether you enable Cario integration or not.

Best regards, Adam.

[Adam Reichold (adamreichold)]

Hello Benjamin,

Am 12.07.2015 um 13:30 schrieb Benjamin Eltzner:
> * When selecting several lines of text, line breaks are not preserved
> and not replaced by blank spaces, which leads to word concatenations.

Looking into this, Poppler::Page::textList gives us absolutely no
information other than the text (in whatever granularity) and its
bounding boxes. Everything else has to be synthesized from that. From
looking at how for example Okular does that, I do definitely not want to
implement comparable heuristics and will rather try to work on the
tagged PDF support in the Qt frontend of Poppler instead. Hence I will
reject this merge and abandon the related branch.

> * The selection tool is still a "frame", so when the text portion to be
> selected starts to the right of the position where it ends one will
> still have to copy unwanted text bits. Maybe the selection tool could be
> adjusted to select "line wise" from the "button push position" to the
> "button release position".

I agree, but it does not matter IMHO because of the above.

> I agree that it would be favorable to be able to use the tagged PDF
> features, however it seems that the poppler project maintainers are not
> particularly interested in these. So I am really glad you propose this
> implementation and are willing to take the burden of maintenance.

I do not think that favour has anything to do with it, but rather a
simple shortage of contributed labour. Tagged PDF support is a
significant project that puts considerable strain on the resources of
the Poppler project.

Best regards, Adam.

[Razi Alavizadeh (srazi)]

Hello Adam,

> I will look into what it would take to get this into Poppler, especially
> reading up on the history of [1] seems necessary before submitting
> anything. But could you rebase your work on qpdfview's trunk as a
> separate merge request? (Because I will reject this merge and abandon
> the branch for which I'll give an explanation in a separate message.)
>
Because you find existing patch for [1] a good start point then I don't
send the patch, feel free to ask it on your need.

This is exactly what I was thinking of. So your bidirectional method
> seems like the way to go. Is this a full implementation of the BiDi
> algorithm?
>
I think you already know that my patch is not bidi-algorithm and
bidi-algorithm is a very complicated thing, you can find its implementation
in QTSRC/gui/text/qtextengine.cpp.

Have you checked that qpdfview is building against the correct Poppler
> include path? This sounds like you try to build with "HAS_POPPLER_31"
> but without adjustment of "INCLUDEPATH"? It should definitely be
> independent of whether you enable Cario integration or not.
>
Interestingly it was disappeared when compiling against Qt5.

Best Regards,
Razi.

2015-07-13 21:41 GMT+04:30 Adam Reichold <email address hidden>:

> The proposal to merge lp:~adamreichold/qpdfview/extended-text-selection
> into lp:qpdfview has been updated.
>
> Status: Needs review => Rejected
>
> For more details, see:
>
> https://code.launchpad.net/~adamreichold/qpdfview/extended-text-selection/+merge/264322
> --
> You are requested to review the proposed merge of
> lp:~adamreichold/qpdfview/extended-text-selection into lp:qpdfview.
>

--
Alavizadeh, Sayed Razi
My Blog: http://pozh.org
Saaghar (نرم‌افزار شعر): http://saaghar.pozh.org/
Saaghar Fan Page: http://www.facebook.com/saaghar.p
Saaghar Mailing List: http://groups.google.com/group/saaghar

Unmerged revisions

1936. By Adam Reichold on 2015-07-09

Some minor cosmetic clean-ups to PDF text extraction and selection.

1935. By Adam Reichold on 2015-07-09

Since PdfPage::text is now a fallback interface we can remove the separate Page::cachedText entry point which all other plugins forwarded to Page::text anyway.

1934. By Adam Reichold on 2015-07-09

At least simplify the boundary and text of extended PDF selections.

1933. By Adam Reichold on 2015-07-09

Use proper text selection if made available by the model.

1932. By Adam Reichold on 2015-07-09

Extend model to allow for proper text selections

=== modified file 'sources/documentview.cpp'
--- sources/documentview.cpp	2015-07-05 14:37:58 +0000
+++ sources/documentview.cpp	2015-07-09 19:23:47 +0000
@@ -936,8 +936,8 @@
 
     const QRectF surroundingRect(x, rect.top(), width, rect.height());
 
-    const QString& matchedText = m_pages.at(page - 1)->cachedText(rect);
-    const QString& surroundingText = m_pages.at(page - 1)->cachedText(surroundingRect);
+    const QString& matchedText = m_pages.at(page - 1)->text(rect);
+    const QString& surroundingText = m_pages.at(page - 1)->text(surroundingRect);
 
     return qMakePair(matchedText, surroundingText);
 }
 
=== modified file 'sources/model.h'
--- sources/model.h	2015-06-27 06:35:40 +0000
+++ sources/model.h	2015-07-09 19:23:47 +0000
@@ -65,6 +65,15 @@
 
     };
 
+    struct Selection
+    {
+        QPainterPath boundary;
+        QString text;
+
+        Selection() : boundary(), text() {}
+
+    };
+
     class Annotation : public QObject
     {
         Q_OBJECT
@@ -115,7 +124,7 @@
         virtual QList< Link* > links() const { return QList< Link* >(); }
 
         virtual QString text(const QRectF& rect) const { Q_UNUSED(rect); return QString(); }
-        virtual QString cachedText(const QRectF& rect) const { return text(rect); }
+        virtual Selection* selectedText(const QRectF& rect) const { Q_UNUSED(rect); return 0; }
 
         virtual QList< QRectF > search(const QString& text, bool matchCase, bool wholeWords) const { Q_UNUSED(text); Q_UNUSED(matchCase); Q_UNUSED(wholeWords); return QList< QRectF >(); }
 
 
=== modified file 'sources/pageitem.cpp'
--- sources/pageitem.cpp	2015-06-02 18:15:29 +0000
+++ sources/pageitem.cpp	2015-07-09 19:23:47 +0000
@@ -75,6 +75,7 @@
     m_formFields(),
     m_rubberBandMode(ModifiersMode),
     m_rubberBand(),
+    m_selection(),
     m_annotationOverlay(),
     m_formFieldOverlay(),
     m_renderParam(),
@@ -457,6 +458,12 @@
     {
         m_rubberBand = QRectF(event->pos(), QSizeF());
 
+        if(m_rubberBandMode == CopyToClipboardMode)
+        {
+            const QRectF rect = m_transform.inverted().mapRect(m_rubberBand).normalized();
+            m_selection.reset(m_page->selectedText(rect));
+        }
+
         emit rubberBandStarted();
 
         update();
@@ -561,6 +568,12 @@
         {
             m_rubberBand.setBottomRight(event->pos());
 
+            if(m_rubberBandMode == CopyToClipboardMode)
+            {
+                const QRectF rect = m_transform.inverted().mapRect(m_rubberBand).normalized();
+                m_selection.reset(m_page->selectedText(rect));
+            }
+
             update();
 
             event->accept();
@@ -595,6 +608,8 @@
         m_rubberBandMode = ModifiersMode;
         m_rubberBand = QRectF();
 
+        m_selection.reset();
+
         emit rubberBandFinished();
 
         update();
@@ -730,6 +745,18 @@
 
 void PageItem::copyToClipboard(const QPoint& screenPos)
 {
+    QString text;
+
+    if(m_selection)
+    {
+        text = m_selection->text;
+    }
+    else
+    {
+        const QRectF rect = m_transform.inverted().mapRect(m_rubberBand).normalized();
+        text = m_page->text(rect);
+    }
+
     QMenu menu;
 
     QAction* copyTextAction = menu.addAction(tr("Copy &text"));
@@ -737,8 +764,6 @@
     const QAction* copyImageAction = menu.addAction(tr("Copy &image"));
     const QAction* saveImageToFileAction = menu.addAction(tr("Save image to &file..."));
 
-    const QString text = m_page->text(m_transform.inverted().mapRect(m_rubberBand));
-
     copyTextAction->setVisible(!text.isEmpty());
     selectTextAction->setVisible(!text.isEmpty() && QApplication::clipboard()->supportsSelection());
 
@@ -1271,6 +1296,18 @@
 
         painter->restore();
     }
+
+    if(m_selection)
+    {
+        painter->save();
+
+        painter->setTransform(m_transform, true);
+        painter->setCompositionMode(QPainter::CompositionMode_Multiply);
+
+        painter->fillPath(m_selection->boundary, QBrush(s_settings->pageItem().highlightColor()));
+
+        painter->restore();
+    }
 }
 
 } // qpdfview
 
=== modified file 'sources/pageitem.h'
--- sources/pageitem.h	2015-06-01 19:28:55 +0000
+++ sources/pageitem.h	2015-07-09 19:23:47 +0000
@@ -37,6 +37,7 @@
 namespace Model
 {
 struct Link;
+struct Selection;
 class Annotation;
 class FormField;
 class Page;
@@ -166,6 +167,8 @@
     RubberBandMode m_rubberBandMode;
     QRectF m_rubberBand;
 
+    QScopedPointer< Model::Selection > m_selection;
+
     void copyToClipboard(const QPoint& screenPos);
     void addAnnotation(const QPoint& screenPos);
 
 
=== modified file 'sources/pdfmodel.cpp'
--- sources/pdfmodel.cpp	2015-06-27 06:35:40 +0000
+++ sources/pdfmodel.cpp	2015-07-09 19:23:47 +0000
@@ -150,11 +150,59 @@
 typedef QSharedPointer< Poppler::TextBox > TextBox;
 typedef QList< TextBox > TextBoxList;
 
-QCache< const PdfPage*,  TextBoxList > textCache(1 << 12);
+QCache< const Poppler::Page*, TextBoxList > textCache(1 << 12);
 QMutex textCacheMutex;
 
 #define LOCK_TEXT_CACHE QMutexLocker mutexLocker(&textCacheMutex);
 
+TextBoxList textBoxes(QMutex* mutex, const Poppler::Page* page)
+{
+    {
+        LOCK_TEXT_CACHE
+
+        if(const TextBoxList* object = textCache.object(page))
+        {
+            return *object;
+        }
+    }
+
+    TextBoxList textBoxes;
+
+    {
+        QMutexLocker mutexLocker(mutex);
+
+        foreach(Poppler::TextBox* textBox, page->textList())
+        {
+            textBoxes.append(TextBox(textBox));
+        }
+    }
+
+    {
+        LOCK_TEXT_CACHE
+
+        textCache.insert(page, new TextBoxList(textBoxes), textBoxes.count());
+    }
+
+    return textBoxes;
+}
+
+inline void extendSelection(Selection* selection, const TextBox& textBox)
+{
+    selection->boundary.addRect(textBox->boundingBox());
+    selection->text.append(textBox->text());
+
+    if(textBox->hasSpaceAfter())
+    {
+        selection->text.append(QLatin1Char(' '));
+    }
+}
+
+inline void simplifySelection(Selection* selection)
+{
+    selection->boundary = selection->boundary.simplified();
+    selection->text = selection->text.simplified();
+}
+
 } // anonymous
 
 namespace qpdfview
@@ -293,7 +341,7 @@
     {
         LOCK_TEXT_CACHE
 
-        textCache.remove(this);
+        textCache.remove(m_page);
     }
 
     delete m_page;
@@ -420,46 +468,9 @@
 
 QString PdfPage::text(const QRectF& rect) const
 {
-    LOCK_PAGE
-
-    return m_page->text(rect).simplified();
-}
-
-QString PdfPage::cachedText(const QRectF& rect) const
-{
-    bool wasCached = false;
-    TextBoxList textBoxes;
-
-    {
-        LOCK_TEXT_CACHE
-
-        if(const TextBoxList* object = textCache.object(this))
-        {
-            wasCached = true;
-
-            textBoxes = *object;
-        }
-    }
-
-    if(!wasCached)
-    {
-        {
-            LOCK_PAGE
-
-            foreach(Poppler::TextBox* textBox, m_page->textList())
-            {
-                textBoxes.append(TextBox(textBox));
-            }
-        }
-
-        LOCK_TEXT_CACHE
-
-        textCache.insert(this, new TextBoxList(textBoxes), textBoxes.count());
-    }
-
     QString text;
 
-    foreach(const TextBox& textBox, textBoxes)
+    foreach(const TextBox& textBox, textBoxes(m_mutex, m_page))
     {
         if(!rect.intersects(textBox->boundingBox()))
         {
@@ -485,6 +496,38 @@
     return text.simplified();
 }
 
+Selection* PdfPage::selectedText(const QRectF& rect) const
+{
+    Selection* selection = new Selection();
+
+    bool firstRun = false;
+    TextBoxList currentRun;
+
+    foreach(const TextBox& textBox, textBoxes(m_mutex, m_page))
+    {
+        if(textBox->boundingBox().intersects(rect))
+        {
+            foreach(const TextBox& textBox, currentRun)
+            {
+                extendSelection(selection, textBox);
+            }
+
+            extendSelection(selection, textBox);
+
+            firstRun = true;
+            currentRun.clear();
+        }
+        else if(firstRun)
+        {
+            currentRun.append(textBox);
+        }
+    }
+
+    simplifySelection(selection);
+
+    return selection;
+}
+
 QList< QRectF > PdfPage::search(const QString& text, bool matchCase, bool wholeWords) const
 {
     LOCK_PAGE
 
=== modified file 'sources/pdfmodel.h'
--- sources/pdfmodel.h	2015-01-27 20:41:38 +0000
+++ sources/pdfmodel.h	2015-07-09 19:23:47 +0000
@@ -115,7 +115,7 @@
         QList< Link* > links() const;
 
         QString text(const QRectF& rect) const;
-        QString cachedText(const QRectF& rect) const;
+        Selection* selectedText(const QRectF& rect) const;
 
         QList< QRectF > search(const QString& text, bool matchCase, bool wholeWords) const;