Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[office] Add regression tests for PDFDocumentPage. #101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[office] Add regression tests for PDFDocumentPage. #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f41cbe
[office] Add regression tests for PDFDocumentPage.
dcaliste Nov 24, 2016
46fc21a
ploum
dcaliste Mar 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,7 @@ install(FILES org.sailfish.office.service DESTINATION ${CMAKE_INSTALL_PREFIX}/sh
install(FILES org.sailfish.office.xml DESTINATION ${CMAKE_INSTALL_PREFIX}/share/dbus-1/interfaces)
install(FILES sailfish-office.privileges DESTINATION ${CMAKE_INSTALL_PREFIX}/share/mapplauncherd/privileges.d)

install(FILES tests/tst_PDFDocumentPage.qml DESTINATION /opt/tests/sailfish-office)
install(FILES tests/data/broken.pdf DESTINATION /opt/tests/sailfish-office/data)
install(FILES tests/data/protected.pdf DESTINATION /opt/tests/sailfish-office/data)
install(FILES tests/data/sample.pdf DESTINATION /opt/tests/sailfish-office/data)
10 changes: 10 additions & 0 deletions pdf/pdfjob.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include <QtMath>
#include <QUrlQuery>
#include <QDebug>
#include <poppler-qt5.h>

LoadDocumentJob::LoadDocumentJob(const QString &source)
Expand Down Expand Up @@ -115,6 +116,15 @@ void RenderPageJob::run()
{
Q_ASSERT(m_document);

if (m_document->isLocked()) {
qWarning() << QStringLiteral("Rendering of a locked document.");
return;
}
if (m_index >= m_document->numPages()) {
qWarning() << QStringLiteral("Rendering of page %1 in a doument with %2 pages.").arg(m_index).arg(m_document->numPages());
return;
}

Poppler::Page *page = m_document->page(m_index);
QSizeF size = page->pageSizeF();
float scale = 72.0f * (float(m_width) / size.width());
Expand Down
9 changes: 9 additions & 0 deletions pdf/pdfrenderthread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,16 @@ void PDFRenderThreadQueue::processPendingJob()
case PDFJob::LoadDocumentJob:
d->loadFailure = false;
break;
case PDFJob::UnLockDocumentJob:
job->m_document = d->document;
break;
default:
// Avoid treating rendering jobs of a locked document
// because of a race condition when reusing pdfcanvas.cpp
// with the same PdfDocument instance while changing the actual
// PDF it is working on.
if (d->document->isLocked())
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default case leaks memory? Should warn? Could also note code changes in commit message and why they were done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is dirty fallback solution for the following issue:

  • load a valid PDF file and render it;
  • load a locked PDF file with the same PdfDocument instance (by calling setDocument());
  • having a race condition in PdfCanvas that may call updatePaintNode() from the rendering thread before the page count is reset to zero by line 758.
    In that situation you may have some rendering job in the queue before the document is unlocked, which segfault later in pdfjob.cpp#61 when calling page() on a non-existing one.

What do you think ? Is it better to try to cure this race condition, that doesn't happen in practice because we're not reusing the same PdfDocument instance each time ; or should I add a comment here mentioning the reason ?

Besides, writing this, I notice that I should convert the Q_ASSERT() I've added in pdfjob.cpp#61 as a return statement for the same race condition reason with a document smaller than the previous one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For things that cannot happen in normal operation of the application I wouldn't mind if there are small workarounds avoiding wrong behavior in testing. However, those should be clearly marked with comments, maybe even doing qWarning(), so it's possible to catch the problem if application behavior ever changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added a comment before the test. I've also added some qWarning() in pdfjob.cpp to signal something unusual happening (and avoid segfaults also).

job->m_document = d->document;
break;
}
Expand Down
12 changes: 12 additions & 0 deletions plugin/PDFDocumentPage.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ import "PDFStorage.js" as PDFStorage
DocumentPage {
id: base

// Group of accessors properties. Can be used to control the PDFDocumentPage
// from outside or for testing purposes.
property alias document: pdfDocument
property alias placeHolder: placeHolderLoader
property alias toolbar: toolbar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commonly not fond of adding very much code used only for testing, but can have some. Just thinking it could be marked for such usage so it's not accidentally removed as cleanup. Maybe could also have all testability properties of the file grouped together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to tune the tests so added accessors may have a sense from a public API point of view, like exposing the toolbar item so any external user of the PDFDocumentPage item may control better the toolbar behaviour for their own needs.

Well, for the moment, I've simply added a comment on every added accessors…

property var _settings // Handle save and restore the view settings using PDFStorage
property ContextMenu contextMenuLinks
property ContextMenu contextMenuText
Expand Down Expand Up @@ -90,6 +96,7 @@ DocumentPage {
}

Loader {
id: placeHolderLoader
parent: base
sourceComponent: (pdfDocument.failure || pdfDocument.locked) ? placeholderComponent : null
anchors.verticalCenter: parent.verticalCenter
Expand Down Expand Up @@ -171,6 +178,11 @@ DocumentPage {

property Notification notice

// Group of accessors properties. Can be used to control the toolbar
// from outside or for testing purposes.
property alias searchIconized: search.iconized
property alias searchText: search.text

width: parent.width
height: base.orientation == Orientation.Portrait
|| base.orientation == Orientation.InvertedPortrait
Expand Down
40 changes: 18 additions & 22 deletions plugin/PDFView.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ SilicaFlickable {
property alias selectionDraggable: selectionView.draggable
property bool canMoveBack: (_contentYAtGotoLink >= 0)

// Accessor currently used for testing purposes.
property bool scrolling: scrollAnimation.running || quickScrollAnimation.running

property bool scaled: pdfCanvas.width != width
property QtObject _feedbackEffect

Expand Down Expand Up @@ -97,12 +100,8 @@ SilicaFlickable {
_searchIndex = index

var match = searchDisplay.itemAt(index)
var cX = match.x + match.width / 2. - width / 2.
cX = Math.max(0, Math.min(cX, pdfCanvas.width - width))
var cY = match.y + match.height / 2. - height / 2.
cY = Math.max(0, Math.min(cY, pdfCanvas.height - height))

scrollTo(Qt.point(cX, cY), match.page, match)
scrollTo(Qt.point(match.x + match.width / 2. - width / 2.,
match.y + match.height / 2. - height / 2.), match.page, match)
}

function nextSearchMatch() {
Expand All @@ -122,24 +121,27 @@ SilicaFlickable {
}

function scrollTo(pt, pageId, focusItem) {
// Ensure that pt can be reached.
var ctX = Math.max(0, Math.min(contentWidth - width, pt.x))
var ctY = Math.max(0, Math.min(contentHeight - height, pt.y))
if ((pt.y < base.contentY + base.height && pt.y > base.contentY - base.height)
&& (pt.x < base.contentX + base.width && pt.x > base.contentX - base.width)) {
scrollX.to = pt.x
scrollY.to = pt.y
scrollX.to = ctX
scrollY.to = ctY
scrollAnimation.focusItem = (focusItem !== undefined) ? focusItem : null
scrollAnimation.start()
} else {
var deltaY = pt.y - base.contentY
var deltaY = ctY - base.contentY
if (deltaY < 0) {
deltaY = Math.max(deltaY / 2., -base.height / 2.)
} else {
deltaY = Math.min(deltaY / 2., base.height / 2.)
}
leaveX.to = (base.contentX + pt.x) / 2
leaveX.to = (base.contentX + ctX) / 2
leaveY.to = base.contentY + deltaY
returnX.to = pt.x
returnY.from = pt.y - deltaY
returnY.to = pt.y
returnX.to = ctX
returnY.from = ctY - deltaY
returnY.to = ctY
quickScrollAnimation.pageTo = pageId
quickScrollAnimation.focusItem = (focusItem !== undefined) ? focusItem : null
quickScrollAnimation.start()
Expand Down Expand Up @@ -439,20 +441,14 @@ SilicaFlickable {
if (left !== undefined && left >= 0.) {
scrollX = rect.x + left * rect.width - ( leftSpacing !== undefined ? leftSpacing : 0.)
}
if (scrollX > contentWidth - width) {
scrollX = contentWidth - width
}
// Adjust vertical position.
scrollY = rect.y + (top === undefined ? 0. : top * rect.height) - ( topSpacing !== undefined ? topSpacing : 0.)
if (scrollY > contentHeight - height) {
scrollY = contentHeight - height
}
return Qt.point(Math.max(0, scrollX), Math.max(0, scrollY))
return Qt.point(scrollX, scrollY)
}
function goToPage(pageNumber, top, left, topSpacing, leftSpacing) {
var pt = contentAt(pageNumber, top, left, topSpacing, leftSpacing)
contentX = pt.x
contentY = pt.y
contentX = Math.max(0, Math.min(contentWidth - width, pt.x))
contentY = Math.max(0, Math.min(contentHeight - height, pt.y))
}
// This function is the inverse of goToPage(), returning (pageNumber, top, left).
function getPagePosition() {
Expand Down
1 change: 1 addition & 0 deletions plugin/SearchBarItem.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ BackgroundItem {
property real iconizedWidth
property bool searching
property alias searchProgress: progressBar.progress
property alias text: searchField._searchText

property real _margin: Math.max((iconizedWidth - searchIcon.width) / 2., 0.)

Expand Down
12 changes: 12 additions & 0 deletions rpm/sailfish-office.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,22 @@ Summary: Translation source for %{name}
License: GPLv2
Group: System/Base

%package tests
Summary: Unitary tests for %{name}
Requires: qt5-qtdeclarative-import-qttest
Requires: qt5-qtdeclarative-devel-tools
Requires: %{name} = %{version}-%{release}


%description
%{summary}.

%description ts-devel
%{summary}.

%description tests
%{summary}.


%files
%defattr(-,root,root,-)
Expand All @@ -54,6 +63,9 @@ Group: System/Base
%files ts-devel
%{_datadir}/translations/source/*.ts

%files tests
/opt/tests/sailfish-office/tst_PDFDocumentPage.qml
/opt/tests/sailfish-office/data/*.pdf

%prep
%setup -q -n %{name}-%{version}
Expand Down
Empty file added tests/data/broken.pdf
View file
Open in desktop
Empty file.
Binary file added tests/data/protected.pdf
View file
Open in desktop
Binary file not shown.
Binary file added tests/data/sample.pdf
View file
Open in desktop
Binary file not shown.
Loading