D13203: Add Typewriter annotation tool in Okular

[Tobias Deiminger]

tobiasdeiminger added inline comments.

INLINE COMMENTS

> parttest.cpp:117
>          void testAnnotWindow();
> +<<<<<<< HEAD
>          void testAdditionalActionTriggers();

Please resolve local merge conflict before submitting the diff.

> parttest.cpp:1511
>  
> +<<<<<<< HEAD
>  // Helper for testAdditionalActionTriggers

Please resolve local merge conflict before submitting the diff.

> parttest.cpp:1563
> +
> +	typewriterButton->click();
> +

From here on the code belongs logically to the next test case, is it? In a mixed purpose test like class PartTest, imho test methods should work independent from one another. I.e., in testDialogClosed don't assume that you've opened dialog in testTypewriterAnnotTool. Iow tests should be idempotent. If you want to retain state across several verifications, do everything in one method or create a new dedicated class PartTestAnnotations where it's more obvious to couple test methods tightly.

> parttest.cpp:1565
> +
> +	m_timer->start(1000);
> +

Using CloseDialogHelper would be more RAII

> parttest.cpp:1582
> +	}
> +	QVERIFY(m_clicked);
> +}

The QInputDialog is pure Qt functionality, just closing it is not that interesting in a typewriter test case. The more interesting thing is to verify if annotation is created correctly after closing.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D13203

To: dileepsankhla, tobiasdeiminger
Cc: ltoscano, ngraham, tobiasdeiminger, aacid, okular-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180611/511720a3/attachment.html>