This patch adds the typewriter annotation tool in Okular with a new typewriter icon and the settings dialogue to configure annotation's font and the text input UI is same as that of inline note. It sets the intent to TypeWriter and background color transparent.
Details
Details
Test for forwards and backwards compatibility of #AARRGGBB and #RRGGBB color formats.
1.1. Test for backwards compatibility:
- open a test.txt file in older Okular (i.e. compiled from master branch)
- add some annotations (inline note, eclipse, ...)
- save the document to test.okular document archive
- unpack the okular archive unzip test.okular
- open metadata.xml and check if all <annotation> ...color=... </annotation> attributes are in #RRGGBB format
- pack the archive again zip test_rrggbb.okular content.xml test.txt metadata.xml
- open test_rrggbb.okular in Okular with D13203 applied
- check if annotations are painted with colors as expected. It should work!
1.2. Test for forwards compatibility:
- open a test.txt file in Okular with D13203 applied
- add some annotations (inline note, eclipse, ...)
- save the document to test.okular document archive
- unpack the okular archive unzip test.okular
- open metadata.xml and check if all <annotation>...color=...</annotation> attributes are in #AARRGGBB format
- pack the archive again zip test_aarrggbb.okular content.xml test.txt metadata.xml
- open test_aarrggbb.okular in older Okular (i.e. compiled from master branch)
- check if annotations are painted with colors as expected. It should work!
Diff Detail
Diff Detail
- Repository
- R223 Okular
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Hmm, what can this do that the regular text annotation tool can't?
Also, why land it immediately without waiting for review?
Comment Actions
@ngraham This is the part of my GSoC project: https://summerofcode.withgoogle.com/projects/#6053164340477952
I have committed the same to my separate branch: https://cgit.kde.org/okular.git/log/?h=gsoc2018_typewriter but don't know how it got landed! It needs to be reviewed first. Have I done something wrong?
Comment Actions
Ah, I see, you committed it to a branch, not to master.
Still, by committing anywhere, Phabricator figured that it had been landed.
Comment Actions
The gsoc* branches are meant to share and log small junks of GSoC progress frequently (even daily), prior to publishing a ready-to-review patch on phabricator. But there haven't been any intermediate GSoC commits during the past two weeks on gsoc2018_typewriter. D13203 on phabricator is just the same as c344c5e31b6a in the branch. So both diffs have the same hash and therefore got automatically linked.
If you don't push your work-in-progress frequently to gsoc2018_typewriter, I doubt there's a point in having that extra branch branch at all. You could equally work on a local branch only and submit a patch after few weeks to phabricator.
Comment Actions
If I use the new tool to typewrite onto a plain text file, I get opaque white background instead of transparent background. Transparency does work for PDF documents (i.e. rendered by poppler). In the former case the typewriter annotation is drawn in ui/pagepainter.cpp L.680. Can you checkout what's wrong there?
We should add something like "Transparent background working on any supported document type" to the testplan (or only test .pdf and .txt for a start, it's enough to test both generator and pagepainter drawing paths).
Comment Actions
The typewriter feature should ultimately get an UI similar to the Adobe Reader typewriter tool, but we decided to start with a less breaking and more working patch 😄 Therefore Dileep just reuses the inline note tool. The resulting annotation differs from inline note in making background always transparent and setting PDF field "intent" to FreeTextTypeWriter to ensure interoperability with other PDF tools. This fulfills the minimal requirements for a typewriter tool.
In upcoming steps we will try to make the UI more like the Adobe Reader typewriter (no popup window to enter text anymore, but type directly onto the document). That's really not an easy task. Dileep already requested for comments, but there was no response so far.
@ngraham Any ideas how to motivate people joining? Every opinion is appreciated. People with deep knowledge about pagepainter, FormWidgets, annotation subsystem and generators could especially help us finding a promising approach.
Comment Actions
The gsoc* branches are meant to share and log small junks of GSoC progress frequently (even daily), prior to publishing a ready-to-review patch on phabricator. But there haven't been any intermediate GSoC commits during the past two weeks on gsoc2018_typewriter. D13203 on phabricator is just the same as c344c5e31b6a in the branch. So both diffs have the same hash and therefore got automatically linked.
If you don't push your work-in-progress frequently to gsoc2018_typewriter, I doubt there's a point in having that extra branch branch at all. You could equally work on a local branch only and submit a patch after few weeks to phabricator.
Thank you so much for the info. From now on, I will frequently push my work to gsoc2018_typewriter.
Comment Actions
In ui/pagepainter.cpp L 668, we can comment out acolor.setAlpha( opacity ) as here the value of opacity is 255 but changing the L 659 or 668 should be consistent in the case of other annotations too. Is commenting out L 668 accepted?
Comment Actions
Hm, if you just comment L.668, how would you control transparency of ordinary inline notes and popup notes? Preferably without changing the look of existing annotations created in earlier Okular versions?
Let's reconsider. We have two transparency parameters for each annotation:
- Alpha channel of QColor Style::color
- double Style::opacity
Sadly it seems like PagePainter and Poppler currently have different interpretations of those two values:
- Poppler: Use alpha channel of Style::color to control transparency of background color. Use Style::opacity to control transparency of the whole appearance=border+background+font.
- PagePainter: Ignore alpha channel of Style::color. Use Style::opacity to control transparency of background color. No way to control transparency of the whole appearance=border+background+font.
This is a inconsistent. I think we could either fix this inconsistency in Poppler or PagePainter (more effort), or hardcode some kind of
if ( inplaceIntent == Okular::TextAnnotation::InplaceIntent ) acolor.setAlpha(0);
in PagePainter.
Any other ideas?
Comment Actions
How about changing ui/pagepainter.cpp L668 to
acolor.setAlpha( a->style().opacity() * a->style().color().alpha() );
It gives us transparent background for new typewriter annotations. It should be backwards/forwards compatible with *.okular documents from other Okular versions. And I believe it's more consistent to what happens in Poppler, because I assume in Poppler color[alpha] and opacity will also get multiplied to determine the final alpha of background color for FreeText, if both values are set. But haven't verified this assumption yet.
Comment Actions
How about changing ui/pagepainter.cpp L668 to
acolor.setAlpha( a->style().opacity() * a->style().color().alpha() );It gives us transparent background for new typewriter annotations. It should be backwards/forwards compatible with *.okular documents from other Okular versions. And I believe it's more consistent to what happens in Poppler, because I assume in Poppler color[alpha] and opacity will also get multiplied to determine the final alpha of background color for FreeText, if both values are set. But haven't verified this assumption yet.
I agree but why do we need to multiply alpha value and opacity to determine the final alpha of background color for FreeText in Poppler? It is already determined by alpha value solely. I mean isn't it enough to change the line to acolor.setAlpha( a->style().opacity() * a->style().color().alpha() ) as you suggested? Why do we need to change the Poppler's one?
Secondly, in my opinion, we should change L 628 to unsigned int opacity = (unsigned int)( a->style().color().alpha() * a->style().opacity() ) and here the variable opacity will determine the background color of FreeText.
Comment Actions
I didn't mean to change anything in Poppler. I meant it's the way it works in Poppler right now. I assume if both 'rg' operator and 'ca' entry are applied, alpha gets multiplied. Probably also depending on blend mode. I don't know how this works exactly, it's a longer read through AnnotFreeText::generateFreeTextAppearance and the spec (e.g. 11.6.3 Specifying Blending Colour and Blend Mode). Maybe you can figure it out on your own?
Secondly, in my opinion, we should change L 628 to unsigned int opacity = (unsigned int)( a->style().color().alpha() * a->style().opacity() ) and here the variable opacity will determine the background color of FreeText.
You're right, that's better.
Comment Actions
I didn't mean to change anything in Poppler. I meant it's the way it works in Poppler right now. I assume if both 'rg' operator and 'ca' entry are applied, alpha gets multiplied. Probably also depending on >blend mode. I don't know how this works exactly, it's a longer read through AnnotFreeText::generateFreeTextAppearance and the spec (e.g. 11.6.3 Specifying Blending Colour and Blend Mode). Maybe you >can figure it out on your own?
It's also complicated for me to comprehend but there is a blending mode function B(Cb,Cs) that multiplies the source color with the backdrop color. If not necessary, I won't go into the details.
Btw I have updated the revision to have typewriter background transparent in PagePainter.
Comment Actions
Typewriter annotations created with the initial typewriter tool have a border of width=1 (Okular started without existing okularpartrc). But we always want border width = 0 for typewriter, is it? It could even be hardcoded into drawing code.
This is because you set width = 0 only for EditAnnotToolDialog::m_stubann. But the initial tool entry created out of tools.xml does not use this stub annotation to determine properties. Only further typewriter tools created with the EditAnnotTool config dialog use the stub annotation. So my okularpartrc looks like this:
[Reviews] AnnotationTools=[...]<tool type="typewriter" id="10"><engine type="PickPoint" color="#000000" block="true"><annotation type="Typewriter" color="#00ffffff"/></engine></tool>,<tool type="typewriter" id="11" name="My own typewriter"><engine type="PickPoint" color="#000000" block="true"><annotation type="Typewriter" color="#00ffffff" width="0"/></engine></tool>
As you see, only the second typewriter has width=0. The first uses default width (=1).
Comment Actions
Hell! Sorry for this mistake again. Whenever I update the revision, I push the commit to the gsoc2018_typewriter branch too and the revision landed in need review state by that commit (both have the same hash).
@ltoscano May you please reopen this revision?
Should I push the commit to the gsoc branch or not? I usually update revision in this manner:
- git branch
*gsoc2018_typewriter
master
- vim file.cpp
- git commit -m "msg"
- git checkout master
- git merge gsoc2018_typewriter
- arc diff --update D13203
It sends two new commit, one created in the gsoc branch and one at the point of merging to master branch
- git reset --hard <Commit ID>
Here <Commit ID> is the last commit before merging gsoc2018_typewriter branch
- git checkout gsoc2018_typewriter
- git push origin gsoc2018_typewriter
And finally, pushing the commit to origin/gsoc2018_typewriter branch.
Can you suggest me where I'm wrong?
| ui/annotwindow.cpp | ||
|---|---|---|
| 262 ↗ | (On Diff #35332) | QColor(m_annot->style().color().name()) is a bit arcane way of saying "force alpha of the popup window background color to 1.0" 😄 If that's what you've actually intended? How about removing const of newcolor (not bad, local scope only) and do newcolor.setAlpha(1.);? const QColor newcolor = m_annot->style().color().isValid() ? QColor(m_annot->style().color().red(), m_annot->style().color().green(), m_annot->style().color().blue(), 255) : Qt::yellow; if you want to keep const. That would be more explicit and obvious. Btw., sorry for commenting on an older revision. But ui/annotwindow.cpp is not included in the latest revision. Phabricator seems to be a bit too smart for our intended workflow. |
Comment Actions
The revision is closed. I'm unable to update it. Maybe someone with admin privileges should reopen it?
| ui/annotwindow.cpp | ||
|---|---|---|
| 262 ↗ | (On Diff #35332) | I agree. It should be straightforward without any latent meaning. Updating the revision. |
| conf/editannottooldialog.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #35508) | So from now on ALL annotations (even ink, highlight, inline note etc) that are created by a user-defined tool will store color as #AARRGGBB in *.okular document archive, right? Formerly it was #RRGGBB. What will happen if user A with new Okular saves an inline note to mydoc.okular and sends mydoc.okular to user B who has an older Okular version. Will the older version be able to deal with #AARRGGBB? How is it vice versa, user B sending something to user A? If this change is indeed forwards and backwards compatible, I think it's good to go this way. We prepare other annotations types for transparency without destroying something. Then we should however also adapt the color=... attributes of the default tools in tools.xml to the new #AARRGGBB format, for the sake of consistency. |
| conf/editannottooldialog.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #35508) | There is no any problem in the newer Okular version as what I have found is that it respects both #RRGGBB and #AARRGGBB formats. I don't know about the older version but it should be both forward and backward compatible. And I agree there should be consistency in the color formats. |
| autotests/parttest.cpp | ||
|---|---|---|
| 117 ↗ | (On Diff #35977) | Please resolve local merge conflict before submitting the diff. |
| 1511 ↗ | (On Diff #35977) | Please resolve local merge conflict before submitting the diff. |
| 1563 ↗ | (On Diff #35977) | 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. |
| 1565 ↗ | (On Diff #35977) | Using CloseDialogHelper would be more RAII |
| 1582 ↗ | (On Diff #35977) | 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. |
| conf/editannottooldialog.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #35508) |
Test for forwards compatibility (def: "allows a system to accept input intended for a later version of itself") is easy.
Because tools.xml was not yet patched to #AARRGGBB for all annotations, just let's simulate this patch:
$ unzip test.okular
$ vi metadata.xml # do :%s/color="#/color="#00/g
$ zip test_aarrggbb.okular content.xml test.txt metadata.xml
$ okular test_aarrggbb.okular
Just tried it, it works. Can you please come up with similar test steps for backwards compatiblity (def: "allows for interoperability with an older legacy system") and perform the test? Reconsidering it, I'm not sure if it's worth the while to open a new Differential patch for the tools.xml alpha channel patch. Probably it's ok to incorporate it in D13203. But let's at least write both the forwards and backwards compatibility test steps down in the Test Plan field of D13203. It's yet empty. May you please do this? |
| 128 ↗ | (On Diff #35508) |
I'm not sure if I understand this question. Of course not all color entries that appear somewhere in okular source code 😃 We shall just be consistent with const QString color = m_stubann->style().color().name(QColor::HexArgb); So adapt everything to #AARRGGBB that will affect Okular::Annotation::style()->color(). Afaikt this are the color=... attributes of <annotation> elements in tools.xml. But not the color=... attributes of <engine> elements, because they don't affect Okular::Annotation::style()->color(). Are there other places? |
| autotests/parttest.cpp | ||
|---|---|---|
| 1582 ↗ | (On Diff #35977) | First of all, in CloseDialogHelper, m_clicked is always false as m_part->widget()->findChild<QDialog*>(); returns NULL for the QInputDialog being the activeModalWidget. So I didn't use it. Secondly, I agree that the low tests should be independent but I'm using QTimer technique in testTypewriterAnnotTool to grab the pointer to the active QInputDialog widget for which I need to connect the SIGNAL timeout() to the SLOT testDialogClosed which eventually becomes another test method. So I can't do it in only a single method instead need two different methods in which one of them becomes the slot of QTimer implementation. In this case. do you think I should do the same in the dedicated class like PartTestAnnotation? Thirdly, to verify if the annotation is created correctly, what parameter(s) should I check? |
| autotests/parttest.cpp | ||
|---|---|---|
| 1582 ↗ | (On Diff #35977) |
That's because QInputDialog is not a child of the part, but a "window" with no parent. You can slightly modify CloseDialogHelper and it will work: void closeDialog()
{
//QDialog *dialog = m_part->widget()->findChild<QInputDialog*>();
QWidget * dialog = qApp->activeModalWidget();Afaikt this is also compatible with the remaining locations where CloseDialogHelper is used. If you apply the change, CloseDialogHelper::m_part is no longer required and can be removed. Construct CloseDialogHelper with QDialogButtonBox::Cancel or QDialogButtonBox::Ok, to click either the Ok or the Cancel button.
The problem goes away if you use CloseDialogHelper.
A perfect UI test would be to check for the painted annotation. As you deferred it because it's too difficult, how about checking part->m_document->page(0)->annotations() for the expected typewriter annotation? |
Comment Actions
Adapt CloseDialogHelper for use with QInputDialog.
Close typewriter input dialog with ok button and test for expected typewriter annotation.
| conf/editannottooldialog.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #35508) |
No, there are not. I'm updating the revision with tools.xml having <annotation> elements color attributes incorporating #AARRGGBB color format. |
Comment Actions
Updating tools.xml <annotation> elements color attributes to incorporate #AARRGGBB color formats
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | Poppler::TextAnnotation::setTextFont is brand new poppler API (actually not yet merged). We still want to support builds with older poppler versions. So setTextFont needs to be surrounded by some #ifdef HAVE_POPPLER_0_xy #endif where 0_xy is the poppler release for which we assume the API to be released. Maybe 0_67, to be optimistic. To detect poppler version at build time, add a check_cxx_source_compiles for Poppler::TextAnnotation::setTextFont and define HAVE_POPPLER_0_67 accordingly in generators/poppler/CMakeLists.txt. Also add a cmakedefine to generators/poppler/CMakeLists.txt. You'll find existing examples for other versions if you look at these files. |
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | Ah, meant setTextColor, not setTextFont, sorry. Btw, (how) can I edit my inline comments here in case of typos? |
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) |
#ifdef HAVE_POPPLER_0_67 ppl_txtann->setTextColor( okl_txtann->textColor() ); #endif //HAVE_POPPLER_0_67
|
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | 1.) yes
|
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | I have added the following lines in generator/poppler/CMakeLists.txt: check_cxx_source_compiles("
#include <poppler-qt5.h>
#include <QColor>
int main()
{
Poppler::TextAnnotation *annot = new Poppler::TextAnnotation( Poppler::TextAnnotation::InPlace );
annot->setTextColor(Qt::blue);
return 0;
}
" HAVE_POPPLER_0_67)The typewriter text is created with the desired color but whenever I move the annotation, the text color falls back to "black". It means in `PopplerAnnotationProxy::notifyModification``, `ppl_txtann->setTextColor( okl_txtann->textColor() );`` is never get called. Why is this happening even after setting the flag in both CMakeLists.txt and annots.cpp? |
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | Is the flag really set (check in build/generators/poppler/config-okular-poppler.h)? If not: throw away the content of your 'build' directory and try again from scratch. |
| generators/poppler/annots.cpp | ||
|---|---|---|
| 182 ↗ | (On Diff #37378) | Hm, for me it works. By reading your comment I suppose you missed to add /* Defined if we have the 0.67 version of the Poppler library */ #cmakedefine HAVE_POPPLER_0_67 1 to config-okular-poppler.h.cmake, is it? From cmake hellp: #cmakedefine VAR ... will be replaced with either: #define VAR ... or: /* #undef VAR */ depending on whether VAR is set in CMake to any value not considered a false constant by the if() command |
Comment Actions
I'd like to split this into two new Diffs
- Add typewriter (everything until ID 36397)
- Add fontcolor support (the reminder, depends on "Add typwriter")
It's easier to review and test. And, the first part could land quite immediately, the second part depends on a pending poppler patch that needs some more research.
If there are no objections posted the next few days, I'll do it.
Comment Actions
Done. Let's jump over to D15204 and D15205 for further actions.
@dileepsankhla: Could you close D13203, to avoid confusion?