[okular] [Bug 476981] Binary Factory build #1575 is missing one .exe from /bin -- Configure backend doesn't work

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=476981

Andre Heinecke  changed:

   What|Removed |Added

 CC||aheine...@gnupg.org

--- Comment #3 from Andre Heinecke  ---
For the record we at GnuPG realized that this was an issue that is
unintentional and will remove the requirement for gpgme-w32-spawn.exe lying
next to the gpgme.dll in one of the next GPGME releases. Our upstream issue for
this is: https://dev.gnupg.org/T6818

-- 
You are receiving this mail because:
You are the assignee for the bug.

[okular] [Bug 408272] The default scaling method should be "Fit to Full Page"

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=408272

Andre Heinecke  changed:

   What|Removed |Added

 CC||aheine...@gnupg.org

--- Comment #12 from Andre Heinecke  ---
Can this be closed since
https://invent.kde.org/graphics/okular/-/merge_requests/700 Has been merged and
seems to provide the feature requested here?

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: Okular with GnuPG / Gpg4win

[Andre Heinecke]

Hi,

On Tuesday 16 May 2023 23:55:00 CEST Albert Astals Cid wrote:
> The text looks reasonable to me, but i guess we'd definitely want some input 
> from the KDE Promo folks. Want me to involve them or will you?

I already did start on that yesterday. 
https://marc.info/?l=kde-promo&m=168423425626159&w=2

Btw. my second mail in the thread gives some additional rationale why the GnuPG
 integration is useful for us so it might be worth a read.

> I can see that how that would make sense for translation but i don't see how 
> this particular wording makes sense to be in the Okular repository.
> 
> Imagine we get 10 different downstreams like you, we would need 10 different 
> strings.
> 
> One thing that comes to mind is we could come up with some kind of "these 
> functionalities have been disabled" based on FORCE_NOT_REQUIRED_DEPENDENCIES 
> have been set. (Or maybe just a generic one if any has been set?) 

I understand your point In Kleopatra we try to avoid that, too.

What do you think about two options:

option(OKULAR_UI_TITLE "Use an alternative title for the Okular UI. Please 
consider when using FORCE_NOT_REQUIRED_DEPENDENCIES" "Okular")
option(SHOW_REDUCED_FUNCTIONALITY_WARNING "Show a warning on first run and in 
the about dialog that this Okular comes with a reduced functionality set." 
((NOT FORCE_NOT_REQUIRED_DEPENDENCIES) AND (NOT FORCE_NOT_REQUIRED_DEPENDENCIES 
STREQUAL "")))

That way we could have a generic warning because I don't think
our users will understand what the libraries mean and explaining that would be
too much. And a distro that might want to make only a minor change
like not shipping CHM support for some reason can set this option to "OFF" to 
avoid
such a warning.

The reference to the Windows store should also be added with a
Q_OS_WIN ifdef and maybe for Linux with an "obtain from your Distribution".

OKULAR_UI_TITLE we can then use to bring in "Okular (GnuPG Edition)".

Probably best if I create an MR and we can discuss this there.

Best Regards,
Andre

-- 
GnuPG.com - a brand of g10 Code, the GnuPG experts.

g10 Code GmbH, Erkrath/Germany, AG Wuppertal HRB14459
GF Werner Koch, USt-Id DE215605608, www.g10code.com.

GnuPG e.V., Rochusstr. 44, D-40479 Düsseldorf.  VR 11482 Düsseldorf
Vorstand: W.Koch, B.Reiter, A.HeineckeMail: bo...@gnupg.org
Finanzamt D-Altstadt, St-Nr: 103/5923/1779.   Tel: +49-211-28010702

signature.asc
Description: This is a digitally signed message part.

Okular with GnuPG / Gpg4win

[Andre Heinecke]

Hi,

our integration of Okular and GnuPG (and later on GnuPG VS-Desktop) is 
nearly finished. Not everything is upstream yet but we see no roadblocks on the 
way that might cause us to abort so we would like to go ahead and announce 
this a bit more.

Attached is a first draft of a statement about why we started to work on this.


First of I want to say a big thank you to everyone who helped with reviewing 
etc. and for the excellent design of Okular which allowed a very modular 
build. 

But, this is a slight problem because we are targeting a high security 
environment we want to limit the attack surface as much as possible. This 
means that we have stripped down Okular quite a lot.

- It will have only the poppler generator.
- Basically no optional dependencies. (No JavaScript)
- No Phonon for Media (patches to cleanly make that optional are incoming, I 
have hacked it for now).

Additionally we carry some patches which allow us to strip down framework 
inter dependencies and brutally hack some parts like KIO to come for example 
without DBus support.

As such I think it would be unfair of us to call this just "Okular" and give 
you a possibly bad name. 

My suggestion is the following:
- Use the name "Okular (GnuPG Edition)" in user visible strings, like the 
start Menu, Window Title, About Dialog etc.
- Change the bug tracker URL to dev.gnupg.org for us (should be obvious).

And finally to add a Message Box on the first launch and add a Text in the 
about 
dialog to promote the full featured Okular which I draft as following:

--
Okular in general is a lightweight and highly secure document viewer for many 
document formats.

To reduce the attack surface even further the GnuPG Edition is stripped
down to only support PDF documents without any active content.

For the best User Experience you can safely install the fully featured Okular 
from the https://apps.microsoft.com/store/detail/okular/
9N41MSQ1WNM8">Microsoft Store
--

If this seems agreeable to you I would open a merge request regarding 
something like this as a build switch. I would like to have the text included 
upstream instead of patching it in for translation / wording support etc. 

I don't think that a parallel installation of two Okulars will make much sense 
except in very specific use cases (e.g. If you use Okular (GnuPG Edition) to 
open PDF's from Mails and the regular Okular as default). But it is possible 
and no Problem.


Best Regards,
Andre

-- 
GnuPG.com - a brand of g10 Code, the GnuPG experts.

g10 Code GmbH, Erkrath/Germany, AG Wuppertal HRB14459
GF Werner Koch, USt-Id DE215605608, www.g10code.com.

GnuPG e.V., Rochusstr. 44, D-40479 Düsseldorf.  VR 11482 Düsseldorf
Vorstand: W.Koch, B.Reiter, A.HeineckeMail: bo...@gnupg.org
Finanzamt D-Altstadt, St-Nr: 103/5923/1779.   Tel: +49-211-28010702
# First draft of an announcement regarding Okular in Gpg4win
# probably a bit too long for publication.


Okular to be added to Gpg4win / GnuPG VS-Desktop

With the Gpg4win 4.2.0 release in May, Okular will be added as an optional
component to the Gpg4win installer, in preparation to a later addition
to GnuPG VS-Desktop. This variant of Okular will feature direct integration
with GnuPG.

---> GnuPG VS-Desktop / Company introduction.

g10 Code GmbH is the company behind the matured Open Source workhorse
GnuPG. Recently we were able to convert this into a commercially successful
product with "GnuPG VS-Desktop", which consists mostly of GnuPG and
Kleopatra as the fronted. Together with an Outlook plugin on Windows and
the usual, excellent, KMail integration on Linux. Previously a recipient of
donations, g10 Code is now able to start giving back to the community and 
recently
became a patron of KDE.

GnuPG VS-Desktop is not only approved for officially restricted
file and mail encryption in Germany (Verschlusssachen – nur für den 
Dienstgebrauch), but
also in Europe and across the NATO for EU/NATO RESTRICTED documents. It
has a large customer base with hundreds of thousands installations
already across Europe and is easily purchasable in Germany
through either the large public sector IT suppliers or a framework contract 
with the
federal government.

The free of charge community versions of these packages (without the approval) 
are
available for Windows under www.gpg4win.org and https://gnupg.org/download/ 
(Look for
the AppImage).

---> Okular in General

Okular is probably the best open source document viewer there is. Due to its 
modular
architecture it combines the achievements of several document handling projects 
in
a single, accessible interface. It has recently been awarded the "Blue Angel" 
for
eco friendly software.

KDE Promo -> Please expand here :)

We consider Okular to have the highest security standards already, but to reduce
the attack surface even further our packaging will contain a stripped
down edition of Okular that only comes with PDF support and no support
for any active content. [1]

The 

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in builtin.js:99
> Can you try what adobe reader does? If it behaves the same it's more than fine

Acrobat saves the "internal text" as the annotation value and the formatted 
text in the "object"

To clarify (As I don't really know what the "object" is properly called):

If I enter "1234.56444" in a field that formats it as "$ 1,234.56" I find the 
following in the pdf:

  <>/ProcSet [ /PDF /Text ]>>/Subtype 
/Form/Type /XObject>>stream
  /Tx BMC 
  q
  1 1 132.0416 15.2125 re
  W
  n
  BT
  /Helv 12 Tf
  0 g
  2 4.155 Td
  ($ ) Tj
  9.996 0 Td
  (1,234.56) Tj
  ET
  Q
  EMC
  
  endstream
  endobj

And:

  <<
/AA <<
/F 55 0 R
/K 56 0 R
>>
/AP <<
/N 76 0 R
>>
/DA (/Helv 12 Tf 0 g)
/F 4
/FT /Tx
/MK <<
>>
/P 15 0 R
/Rect [ 121.683 763.617 255.724 780.829 ]
/Subtype /Widget
/T (us_currency_fmt)
/Type /Annot
/V (1234.56444)
  >>

Which leads to this behavior (which is IMO Ok) in okular:

F5931466: okular-saved-formatted.gif 

I don't think that there is API in poppler to set the fields text to a 
different value then the value of the annotation. My preference would be to say 
for now that the behavior of save/load is not perfect but it's usable. A user 
will probably be annoyed once but afterwards have learned how it behaves.

P.S. Here is the document as saved by Acrobat DC: F5931482: 
FieldFormat_filled.pdf 

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aheinecke wrote in builtin.js:99
> I'm not sure I understand the question. I understand it that you are 
> concerned that the actual value of the field is changed by formatting and the 
> user is then confused if he mixes the format when entering additional values.
> 
> The trick for that is that the number is not actually changed. As soon as you 
> focus in on the text field the text will change to the actual value. This is 
> also what Acrobat does.
> 
> So if you enter with dots but the form want's commas the number is 
> transformed according to your locale for formatting. But as soon as you edit 
> it to add more text it will be changed right back to what you originally 
> entered.
> 
> Here is an example that has most "predefined" format settings:
> 
> F5918086: FieldFormat.pdf 
> 
> Only the first three lines work in okular as they use the number format.
> 
> Another example would be:
> https://www.pbeakk.de/fileadmin/redakteure/contents/PDF/Formulare/form_01.pdf

Noticed while thinking about save/load of a formatted form that the behavior 
you are concerned about happens when we save and load. Because only the text is 
saved and not the internal text.

I doubt that this will be a huge issue though as you mostly fill out form 
fields once and if you save / load a filled form you probably don't remember 
what you had filled as "internal" value previously. But it's indeed confusing 
and a bad user experience in that case.

Do you have an Idea how we could save the internal text?

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in form.h:298
> Do we need these two to be virtual? i.e do you envision a case in which a 
> backend generator would want to reimplement them? It seems to me this is 
> mostly an "internal storage" area for the core and the backends don't really 
> care?

Saving / loading could be an issue. I'm not sure how we could save the internal 
text, but maybe someone ingenious will find a way.
And then it might be interesting to override these.

I mostly did it though to match the rest of the class. ;-)

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke added a comment.


  There is a side effect here that AF_SimpleCalculate now returns a localized 
number which will have group Seperators according to the system locale. See my 
inline comment about this.

INLINE COMMENTS

> calculatetexttest.cpp:89
>  QCOMPARE (fields[QStringLiteral ("AVG")]->text(), QStringLiteral( "20" 
> ));
> -QCOMPARE (fields[QStringLiteral ("Prod")]->text(), QStringLiteral( 
> "6000" ));
> +QCOMPARE (fields[QStringLiteral ("Prod")]->text(), QStringLiteral( 
> "6,000" ));
>  QCOMPARE (fields[QStringLiteral ("Min")]->text(), QStringLiteral( "10" 
> ));

Should just the calculate localize the number?

Acrobat does not do this. In Acrobat the result here is "6000".

But I think that for a KDE / Qt Application it is the correct behavior. As the 
number format can be set by the user through System Settings and we should 
respect that here when we convert a number to a string.

It is likely IMO that this is not a real world problem anyway as most forms 
with calculated values will have explicit number format functions.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 36278.
aheinecke added a comment.


  Force consistent locale in calculate text test to avoid
  different results.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13172?vs=36276&id=36278

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

AFFECTED FILES
  autotests/calculatetexttest.cpp
  autotests/data/simpleCalculate.pdf
  core/script/builtin.js
  core/script/kjs_util.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 36276.
aheinecke added a comment.


  Update the calculatetexttest and simpleCalculate PDF with
  formatting removed from the fields in simple calculate.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13172?vs=35087&id=36276

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

AFFECTED FILES
  autotests/calculatetexttest.cpp
  autotests/data/simpleCalculate.pdf
  core/script/builtin.js
  core/script/kjs_util.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke planned changes to this revision.
aheinecke added a comment.


  Just noticed that this breaks the calculatetexttest as I've accidentally also 
added Formatting (which Acrobat did automatically) on the calculated fields in 
that test document.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke added a comment.


  The test is D13588 

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13588: Add test for field number formatting

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: aacid.
aheinecke added a project: Okular.
Restricted Application added a subscriber: okular-devel.
aheinecke requested review of this revision.

REVISION SUMMARY
  This adds a test document with most formatting options
  directly accessible through acrobat.
  Currently only the number format functions are tested
  as they are the only ones implemented.

TEST PLAN
  Works for me.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  autotests/data/fieldFormat.pdf
  autotests/parttest.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke added a comment.


  Apologies for the delay. Some travel, some sick days and some other work and 
the weeks fly by ;-)
  I'm getting back to this now.

INLINE COMMENTS

> aacid wrote in builtin.js:99
> Not having a document to test makes this a bit harder, but let's say i write 
> a number in a form, the form then changes that number to have dots as 
> separator format and then i just go and add a number at the beginning/end of 
> the "text" of that form. When it comes back here stringToNumber will expect 
> commas and not dots and then everything will break?

I'm not sure I understand the question. I understand it that you are concerned 
that the actual value of the field is changed by formatting and the user is 
then confused if he mixes the format when entering additional values.

The trick for that is that the number is not actually changed. As soon as you 
focus in on the text field the text will change to the actual value. This is 
also what Acrobat does.

So if you enter with dots but the form want's commas the number is transformed 
according to your locale for formatting. But as soon as you edit it to add more 
text it will be changed right back to what you originally entered.

Here is an example that has most "predefined" format settings:

F5918086: FieldFormat.pdf 

Only the first three lines work in okular as they use the number format.

Another example would be:
https://www.pbeakk.de/fileadmin/redakteure/contents/PDF/Formulare/form_01.pdf

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke added a comment.


  Yes, I plan to add a test for this.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke updated this revision to Diff 35089.
aheinecke added a comment.


  - Simplyfy page lookup in processFormatAction
  - Clarify comment about the need to refresh calculated
  
  fields in processFormatAction.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13171?vs=35027&id=35089

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

AFFECTED FILES
  core/document.cpp
  core/document.h
  core/documentcommands.cpp
  core/form.cpp
  core/form.h
  core/script/event.cpp
  core/script/event_p.h
  core/script/kjs_field.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 35087.
aheinecke added a comment.


  Remove spurious comment

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13172?vs=35086&id=35087

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

AFFECTED FILES
  core/script/builtin.js
  core/script/kjs_util.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 35086.
aheinecke added a comment.


  Removed to currency and extended numberToString to handle this, too.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13172?vs=35028&id=35086

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

AFFECTED FILES
  core/script/builtin.js
  core/script/kjs_util.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke planned changes to this revision.
aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in builtin.js:91
> QLocale::toString ?

*facepalm* I thought that didn't do separators.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13170: Add test for additional form actions

[Andre Heinecke]

aheinecke added a comment.


  In D13170#270082 , @aheinecke 
wrote:
  
  > In D13170#269657 , @aacid wrote:
  >
  > > QTest::qWait( 100 ); is bad as you probably already guessed since it's 
"machine dependant".
  >
  >
  > Yeah, looked fishy ;-) Although stuff like that is used in part test on 
other places so i've copied it.
  
  
  Well Ok, only in the testAnnotWindow,.. so it's not really used in other 
places.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13170: Add test for additional form actions

[Andre Heinecke]

aheinecke updated this revision to Diff 35084.
aheinecke added a comment.


  Use QTRY instead of QTest::wait. This is acutally much faster.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13170?vs=35022&id=35084

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

AFFECTED FILES
  autotests/data/additionalFormActions.pdf
  autotests/parttest.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13170: Add test for additional form actions

[Andre Heinecke]

aheinecke added a comment.


  In D13170#269657 , @aacid wrote:
  
  > QTest::qWait( 100 ); is bad as you probably already guessed since it's 
"machine dependant".
  
  
  Yeah, looked fishy ;-) Although stuff like that is used in part test on other 
places so i've copied it.
  
  > In those cases you awnt to use the QTRY_ versions, i.e. QTRY_VERIFY2 
instead of QVERIFY2
  
  I'll try.
  
  > Also you probably want QCOMPARE instead of QVERIFY if your QVERIFY has a == 
inside, because that way when it fails it already says the expected: actual: 
values without you having to construct the string (in this case we want 
QTRY_COMPARE to be able to remove the wait() calls).
  
  I used QVERIFY2 with the equality operator because there is no QCOMPARE2 for 
QVERIFY So I can't add a message. And if I use such a helper function I need to 
know the actual failure line.
  
  > Hope i made sense :D
  
  Yes, thanks :-)

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke added a comment.


  In D13171#269646 , @aacid wrote:
  
  > I'm not very happy of the internalText method/storage in form, ideally, the 
text in core/form should always be the internal text, and if there's a 
formatted text that needs to be different that should only be as part of the 
widget itself. i.e. the FormLineEdit would do something like
  >
  > setText( formatText( form->text() ) );
  >
  > Do you think it could be implemented this way?
  
  
  Sadly I don't think so. I would be happier with such a solution, too, but we 
need to have the formatted text as part of the real form data so that it is 
properly printed and shown even if the widget is not visible. Actually the 
"internalText" is kind of more a "UI" thing while the formatted text is the 
real thing.

INLINE COMMENTS

> aacid wrote in document.cpp:4297
> You can simplify the loop to p->formFields().contains(fft);

ack.

> aacid wrote in document.cpp:4341
> I don't understand this, can you elaborate why we need to update a form if 
> the new and old text are the same?

If the field was calculated we need to update it anyway because we no longer 
refresh it in DocumentPrivate::recalculateForms. This avoids a double refresh 
in case the field is formatted.

Old text here is before formatting, but after calculate. New text is after 
formatting, but still after calculate. Even if the formatting did not change 
anything we have to update because of the calculate.

If I remember correctly this happened to me during development when the Format 
script raised an exception. I shall clarify the comment.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in formwidgets.cpp:1078
> Can you please add that explanation either as a comment somewhere in the code 
> or as part of the git commit? It'll make it easier to find next time someone 
> looks at the code and wonders why it has an "else " like i did.

In the comment in line 1052ff I tried to do just that. A problem with the 
macros is that you cant properly do inline comments.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: okular-devel, aacid, ngraham

D13172: Add AFNumber_Format and l10n AFSimple_Calculate

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: aacid.
aheinecke added a project: Okular.
Restricted Application added a subscriber: okular-devel.
aheinecke requested review of this revision.

REVISION SUMMARY
  This adds utility functions to util to work with QLocale
  for formatting and Number handling.
  
  At least until we support AFNumber_Keystroke to restrict
  what a user enters in Number input fields it is good
  behavior to expect the user to enter Numbers in the system's
  Locale.
  
  AFNumber_Format is new for formatting and uses the
  Locale util functions.

TEST PLAN
  Needs unit test

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  core/script/builtin.js
  core/script/kjs_util.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13171: Add support for form text formatting

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: aacid.
aheinecke added a project: Okular.
Restricted Application added a subscriber: okular-devel.
aheinecke requested review of this revision.

REVISION SUMMARY
  With formatting there is an internal value, which represents
  the true value of a field additionaly to the normal,
  visible, text.
  
  For fields which have formatting rules these might differ
  and for calculations the internal value is used. The behavior
  to format on focus in / focus out events is similar to
  that of Acrobat reader.

TEST PLAN
  Needs unit test

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  core/document.cpp
  core/document.h
  core/documentcommands.cpp
  core/form.cpp
  core/form.h
  core/script/event.cpp
  core/script/event_p.h
  core/script/kjs_field.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D13170: Add test for additional form actions

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: aacid.
aheinecke added a project: Okular.
Restricted Application added a subscriber: okular-devel.
aheinecke requested review of this revision.

REVISION SUMMARY
  This tests the newly added additional widget actions.

TEST PLAN
  Passes for me. Is my first GUI test, maybe the timing
  needs to be slower for slower machines?

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  autotests/data/additionalFormActions.pdf
  autotests/parttest.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke updated this revision to Diff 35015.
aheinecke added a comment.


  Don't eat the MouseRelease event if it is outside the widget

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12665?vs=35014&id=35015

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

AFFECTED FILES
  core/annotations.h
  core/form.cpp
  core/form.h
  core/form_p.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/formfields.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: okular-devel, aacid, ngraham

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> CMakeLists.txt:97
> +}
> +" HAVE_POPPLER_0_65)
> +

Out of curiosity: Why is this and the other "check compiles" tests for released 
versions of poppler not:

  if (Poppler_VERSION VERSION_GREATER "0.64.99")
set (HAVE_POPPLER_0_65 1)
  endif()

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: okular-devel, aacid, ngraham

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke updated this revision to Diff 35014.
aheinecke added a comment.


  - Fixed precedence of activation action over mouse release action.
  - Only activate mouse released if release is inside the widget.
  - Fixed check for Poppler 0.65

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12665?vs=33810&id=35014

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

AFFECTED FILES
  core/annotations.h
  core/form.cpp
  core/form.h
  core/form_p.h
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/formfields.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: okular-devel, aacid, ngraham

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke planned changes to this revision.
aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in formwidgets.cpp:1073
> This still triggers if you press the button inside a form, move the mouse 
> outside the form and release, not sure this is "according to spec", it says 
> "An action to be performed when the mouse button is released inside the 
> annotation’s active area."

> This still triggers if you press the button inside a form, move the mouse 
> outside the form and release, not sure this is "according to spec", it says 
> "An action to be performed when the mouse button is released inside the 
> annotation’s active area."

Right. I also tested with Acrobat and it only triggers if the mouse is released 
in the annotation area.

> aacid wrote in formwidgets.cpp:1078
> Are you sure this should be an else? Why should activation action only be 
> signaled if there's no mouse release action?
> 
> Also, before we only did activation action for buttons, but now we do for 
> lots of other forms, is that on purpose?

> Are you sure this should be an else? Why should activation action only be 
> signaled if there's no mouse release action?

Good question. I checked in the spec and it should be the other way around. I 
was confused because Adobe Acrobat DC sets the "Mouse Released" action as the 
"A" entry of the annotation dictonary. This is parsed by poppler as the 
"Additional Action"

Page 649 (Table 8.44) in the spec Says about the mouse released event:
Note: For backward compatibility the A entry in an annotation dictionary, if 
present, takes precedence over this entry.

So it should be the other way around. Only execute the mouseReleased action if 
there is no activation action and execute the activation action otherwise.
I'll change it.

Checkboxes are handled differently because they trigger in the 
"doActivateAction" so that the action can also be triggered by scripts changing 
the checked state.

> Also, before we only did activation action for buttons, but now we do for 
> lots of other forms, is that on purpose?

Yes this is on purpose. The activation action is not bound to buttons. I did 
not find it clearly stated in the spec but If I add a Mouse Release action on a 
textfield in Acrobat DC it is added as a Mouse Release action.

And the Example from bug306818 uses a mouse action on a read only text field to 
hide the warning in there.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: okular-devel, aacid, ngraham

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke updated this revision to Diff 35008.
aheinecke added a comment.


  Include duplicate cals from editFormList and editFormCombo

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12825?vs=33987&id=35008

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

AFFECTED FILES
  core/document.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke added a comment.


  I'll upload a new patch. (sorry for delaying my okular work, I'm currently 
still a bit swamped by the efail hype)

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke added a comment.


  Ugh, Sorry for overlooking these.
  
  Yes. They also call notifyFormChanges in their redo.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke added a comment.


  To clarify:
  
  d->m_undoStack->push( uc );
  
  results in:
  EditFormTextCommand::redo()
  
  https://cgit.kde.org/okular.git/tree/core/documentcommands.cpp#n513
  
  which calls:
  
  m_docPriv->notifyFormChanges( m_pageNumber );
  
  https://cgit.kde.org/okular.git/tree/core/document.cpp#n3435
  
  That triggers the recalculate. So it is duplicated here.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke added a comment.


  I did not know when adding f0a80a67 
 
that pushing to the undoStack automatically executes the redo function.

REPOSITORY
  R223 Okular

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

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12825: Fix recalculating forms twice

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: aacid.
Restricted Application added a project: Okular.
Restricted Application added a subscriber: okular-devel.
aheinecke requested review of this revision.

REVISION SUMMARY
  As a side effect of f0a80a67 
 a 
recalculation was triggered
  by the notifyFormChanges called in EditFormTextCommand::redo,
  which is called for each edit. So calculation was done twice.

TEST PLAN
  The calculate forms test still passes.

REPOSITORY
  R223 Okular

BRANCH
  master

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

AFFECTED FILES
  core/document.cpp

To: aheinecke, aacid
Cc: okular-devel, ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke updated this revision to Diff 33810.
aheinecke added a comment.


  - Removed a spurious emit when calling signalAction.
  - Updated to API of update poppler patch.
  - Fixed leaking the additional actions.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12665?vs=33630&id=33810

BRANCH
  master

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

AFFECTED FILES
  core/annotations.h
  core/form.cpp
  core/form.h
  core/form_p.h
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke updated this revision to Diff 33630.
aheinecke added a comment.


  Changed mouseRelease event to fallback to activationAction
  Some formatting.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12665?vs=33497&id=33630

BRANCH
  master

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

AFFECTED FILES
  core/form.h
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke planned changes to this revision.
aheinecke added a comment.


  MouseReleased indeed needs some different handling. It only works in the 
tests for the Button's as they trigger the "MouseReleased" action as the 
activation action.
  
  As we need a generic way to trigger activation actions for any Form Element I 
think it would be best to trigger the "ActivationAction" in the MouseReleased 
handler for those forms elements that do not have a way to trigger their 
activation action, yet.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: ngraham, aacid

D12665: Support additional widget actions in PDF Forms

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This adds support for actions associated with form fields
  through corresponding annotation widgets.

TEST PLAN
  Still needs a unit test, only tested manually with
  the document attached in the task.

REPOSITORY
  R223 Okular

BRANCH
  master

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

AFFECTED FILES
  core/form.h
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: ngraham, aacid

[okular] [Bug 306818] Okular cannot close instructions overlay in PDF

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=306818

--- Comment #8 from Andre Heinecke  ---
This example is extra fun...
Although it uses the Hide Actions, which we now support, the trigger is
different and not yet supported.

The "Help" overlays are text edits with an actionMousePressed annotation. This
is similar to Bug 307304 (where there are FocusIn / FocusOut) action triggers .
I've opened a Task for this, as I really think that it's important to handle
these triggers in forms: https://phabricator.kde.org/T8627

But for added fun: The Text edits with the help text are Read Only (thats why
they look so strange if you Show Forms). So we now have a use case to show
Widgets for Read Only form fields, because they could have actions attached.
:-/

-- 
You are receiving this mail because:
You are the assignee for the bug.

[okular] [Bug 306855] Support Javascript

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=306855

Andre Heinecke  changed:

   What|Removed |Added

 Blocks|306818  |


Referenced Bugs:

https://bugs.kde.org/show_bug.cgi?id=306818
[Bug 306818] Okular cannot close instructions overlay in PDF
-- 
You are receiving this mail because:
You are the assignee for the bug.

[okular] [Bug 306818] Okular cannot close instructions overlay in PDF

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=306818

Andre Heinecke  changed:

   What|Removed |Added

 Depends on|306855  |
   Version Fixed In||1.5.0
 Status|CONFIRMED   |RESOLVED
 CC||aheine...@intevation.de
 Resolution|--- |FIXED

--- Comment #5 from Andre Heinecke  ---
This sounds totally similar to the change in https://phabricator.kde.org/T8274
where the "veraeusserungsanzeige.pdf" linked in the issue also had such an
overlay and removed it after clicking it.

I can't find the example here either through wayback machine or by googling
after the name so I'm not 100% sure (the link no longer works). But I'm
confident enough from the description to resolve this. Please reopen if you
have an example where it does not work.


Referenced Bugs:

https://bugs.kde.org/show_bug.cgi?id=306855
[Bug 306855] Support Javascript
-- 
You are receiving this mail because:
You are the assignee for the bug.

D10932: [Okular] Option to reset forms

[Andre Heinecke]

aheinecke added a comment.


  Reset-Form Actions are specified in Adobe's PDF Reference as:
  
  > A reset-form action resets selected interactive form fields to their 
default values;
  >  that is, it sets the value of the V entry in the field dictionary to that 
of the DV entry
  >  (see Table 8.69 on page 675). If no default value is defined for a field, 
its V entry is
  >  removed. For fields that can have no value (such as pushbuttons), the 
action has
  >  no effect.
  
  It can further be specified in that action which fields to reset.
  
  IMO a reset should be implemented in Popper by adding a "reset" function to 
fields, which takes the default value into account. This could then save us 
from having to propagate the default value through the layers.
  
  This does not appear to necessarily be undoable (Foxit does not appear to 
have it undoable either).
  
  My plan for this would be to implement the Reset Form FormAction. Then create 
a "Fixed" QAction which uses a virtual FormAction that would affect all fields.
  
  The behavior could be tested against the reset action of a button and mimic 
the behavior of Acrobat Reader.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular, aacid
Cc: aheinecke, rkflx, cfeck, ngraham, aacid, #okular

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke added a comment.


  In D11609#253518 , @sander wrote:
  
  > > I've tried to create a test document with Adobe Acrobat Pro DC but ended 
up
  > >  frustrated and unable to create Movie or Rendition Actions (except for 
the Page Open Action).
  >
  > Would using LaTeX and the 'multimedia' package help here?
  
  
  Googling around it at least brought me to 
http://pages.uoregon.edu/noeckel/PDFmovie.html This page at least describes 
some formats and with an example document with which I was able to create 
rendition actions. And even trigger the debug output I added here.
  But I was unable to get the rendition action to work (even without having it 
as a chained action) in Okular. The page open play works fine.
  
  Here is my try:
  F5822001: movie-edit.pdf 
  
  > I know it create Movie _annotations_ . Is that the same as movie _actions_?
  
  No. From the PDF reference on movie actions:
  
  > The contents of a movie action dictionary are identical to those of a movie 
activa-
  >  tion dictionary (see Table 9.31 on page 785), with the additional entries 
shown in
  >  Table 8.59. The contents of the activation dictionary associated with the 
movie
  >  annotation provide the default values. Any information specified in the 
movie ac-
  >  tion dictionary overrides these values.
  
  So there is a difference.
  
  To be honest, I think that Movie actions and Renditon actions are deprecated, 
don't fully work and are exotic. 
  The most common usecase appears to be to have the Annotations on page open 
etc, which works.
  So I think we can, with good conscience, skip the support for them in this 
new feature here.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: sander, aacid, ngraham

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke updated this revision to Diff 33048.
aheinecke added a comment.


  Removed unused deletion prevention for media links.
  Noted instead that nextActions containing MovieActions or
  RenditionActions are unsupported.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11609?vs=32471&id=33048

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

AFFECTED FILES
  autotests/visibilitytest.cpp
  core/action.cpp
  core/action.h
  core/document.cpp
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  generators/poppler/generator_pdf.cpp

To: aheinecke, #okular
Cc: aacid, ngraham

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke added a comment.


  Right, I overlooked this a bit and assumed that the references were resolved 
on demand.
  But I think we could also say that we should resolve the MediaLinkReferences 
of the next actions, too.
  
  Looking at this more I see the problem that if we have a movie action
  as a next action it would be executed but the media links won't be resolved.
  I'm not really sure if this would be bad (crash) or if the second action just 
won't work.
  
  I've tried to create a test document with Adobe Acrobat Pro DC  but ended up
  frustrated and unable to create Movie or Rendition Actions (except for the 
Page Open Action).
  I was unable to add Media that was compatible with the "Play Media (Acrobat 5 
compatible)" and
  "Play Media (Acrobat 6 compatible)" actions, which I expect to be
  Movie / Rendition actions.
  
  The only thing I could do with added Movies was to create "RichMediaExecute" 
actions,
  which are (as far as i can tell) not supported by poppler. So it looks to me 
that Movie / Rendition actions
  are somewhat deprecated by Adobe Acrobat.
  
  As we don't have a testcase I think its probably better to just leave it 
unsupported. I'll update the patch accordingly.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, ngraham

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke updated this revision to Diff 32471.
aheinecke added a comment.


  Changed to handle deletion of the link actions more explicit
  and to compile against the commited poppler patch.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11609?vs=30302&id=32471

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

AFFECTED FILES
  autotests/visibilitytest.cpp
  core/action.cpp
  core/action.h
  core/document.cpp
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  generators/poppler/generator_pdf.cpp

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11597: Add test for visibility changes

[Andre Heinecke]

aheinecke updated this revision to Diff 32469.
aheinecke added a comment.


  Fixed visibilitytest.pdf which was somehow broken with the
  last update.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11597?vs=31744&id=32469

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/data/visibilitytest.pdf
  autotests/visibilitytest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

[okular] [Bug 307304] Javascript in forms not working correctly

[Andre Heinecke]

https://bugs.kde.org/show_bug.cgi?id=307304

Andre Heinecke  changed:

   What|Removed |Added

   Assignee|okular-devel@kde.org|aheine...@intevation.de

--- Comment #7 from Andre Heinecke  ---
The problem here is that the SUM fields start readOnly / invisible. The
calculation works and if you set the fields to visible by hand they are updated
correctly.


In the PDF itself I find such scriptlets:
{
var summe = this.getField("Summe Spalte 1").value;

if(summe <= "0")
{
   this.getField("Summe Spalte 1").display = display.hidden;
} 
else
this.getField("Summe Spalte 1").display = display.visible;
}

Currently there is no support for the display property and display object.
These scripts are also not executed. Poppler currently does not parse the
scriplets. pdfinfo -js does not show them.

Looking at them in Adobe shows that the visibility scriptlets should be
executed when a field is deactivated. I guess that means something like
"Editing finished". The trigger has the Key "Bl".

So:
1. Parse "Bl" Triggered actions in poppler.
2. Execute them in Okular.
3. Add basic support for the display object / property in Okulars JavaScript.


I'll look into it.

-- 
You are receiving this mail because:
You are the assignee for the bug.

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke added a subscriber: aacid.
aheinecke added inline comments.

INLINE COMMENTS

> generator_pdf.cpp:475
> +// poppler links
> +popplerLink->setNextLinks( QVector< Poppler::Link * >() );
> +}

@aacid When you commited the corresponding poppler patch you did not add this 
API.

The problem here is:

- In the LinkPrivate dtor we delete the nextLinks list.
- createLinkFromPopplerLink deletes the parsed the link.

My solution for this was to add API to change the nextLinks so that they can be 
"taken" from a Link.

Maybe in poppler we could add "Poppler::Link::takeNextLinks()" to have a more 
explicit API for that?

Alternatively changing "createLinkFromPopplerLink" to optionally not delete the 
link it parses? I think that is a bit more error prone as we have to make sure 
we catch everything. For example the movie and rendition actions have their own 
deletion.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11597: Add test for visibility changes

[Andre Heinecke]

aheinecke updated this revision to Diff 31744.
aheinecke added a comment.


  Added nasty circular next actions

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11597?vs=30278&id=31744

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/data/visibilitytest.pdf
  autotests/visibilitytest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11608: Add support for Next actions following an action

[Andre Heinecke]

aheinecke abandoned this revision.
aheinecke added a comment.


  Please see the freedesktop issue for the latest patch.

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11596: Add support for dynamic visibility

[Andre Heinecke]

aheinecke updated this revision to Diff 31736.
aheinecke added a comment.


  Changed from single targetName to QVector targets API

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11596?vs=30277&id=31736

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

AFFECTED FILES
  core/form.cpp
  core/form.h
  core/script/kjs_field.cpp
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  generators/poppler/formfields.h
  generators/poppler/generator_pdf.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11594: Add support for hide action

[Andre Heinecke]

aheinecke abandoned this revision.
aheinecke added a comment.


  Please see the freedesktop issue for the last version of this patch. It was 
updated there.

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11595: Qt5: Allow setting of visibility

[Andre Heinecke]

aheinecke abandoned this revision.
aheinecke added a comment.


  This was pushed by Albert with poppler rev 3e040896

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11608: Add support for Next actions following an action

[Andre Heinecke]

aheinecke added a comment.


  Freedesktop bug is: https://bugs.freedesktop.org/show_bug.cgi?id=105759

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11594: Add support for hide action

[Andre Heinecke]

aheinecke added a comment.


  Freedesktop bugzilla entry for this is: 
https://bugs.freedesktop.org/show_bug.cgi?id=105758

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11595: Qt5: Allow setting of visibility

[Andre Heinecke]

aheinecke added a comment.


  Done: https://bugs.freedesktop.org/show_bug.cgi?id=105757
  
  I'm leaving the differential open here (and will update it if neccessary) so 
that it is clear in the task which patches belong together and will close this 
revision once it was accepted to poppler.

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D11609: Add support for chained / next actions

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This adds support for multiple actions following each
  other through the "Next" value of Action dictionaries.

TEST PLAN
  Activates the corresponding part of the visibilitytest.

REPOSITORY
  R223 Okular

BRANCH
  master

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

AFFECTED FILES
  autotests/visibilitytest.cpp
  core/action.cpp
  core/action.h
  core/document.cpp
  generators/poppler/CMakeLists.txt
  generators/poppler/generator_pdf.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11608: Add support for Next actions following an action

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  Next actions are action dictionaries or an array
  of action dictonaries. "Next" is an entry in the
  general action dictionary.
  
  These actions are supposed to be performed after each other.
  So that a single button press can for example
  both trigger a Hide action and a JavaScript action.

TEST PLAN
  Using visibilitytest autotest in okular.

BRANCH
  master

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

AFFECTED FILES
  poppler/Link.cc
  poppler/Link.h
  qt5/src/poppler-link-private.h
  qt5/src/poppler-link.cc
  qt5/src/poppler-link.h
  qt5/src/poppler-page.cc

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11594: Add support for hide action

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> Link.h:468
> +
> +  // According to spec the target can be either:
> +  // a) A text string containing the fully qualified name of the target

I've looked at the spec but did not understand what should be happening with 
"An indirect reference to an annotation dictionary" and as a result I did not 
understand what "An array of such dictionaries" would look like. My guess is 
that it would be similar to the Annotation Ref in a Movie.

But as I could not come up with a test for this using Adobe DC and did not have 
at an example I did not add API for this and only added API for what I fully 
understood and could test.

My reasoning is that it is better not to have API, so that it can later be 
added, instead of wrong API that has to be carried around for compatibility.

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11597: Add test for visibility changes

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  The test only tests what is currently supported so it does
  not yet test radio buttons and action chains. But
  the visibilitytest.pdf already provides these, too.

REPOSITORY
  R223 Okular

BRANCH
  master

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/data/visibilitytest.pdf
  autotests/visibilitytest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11596: Add support for dynamic visibility

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This adds the hidden property to JavaScript fields and
  uses it to implement support for HideAction.

TEST PLAN
  Unit test in the next commit.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  core/form.cpp
  core/form.h
  core/script/kjs_field.cpp
  generators/poppler/CMakeLists.txt
  generators/poppler/formfields.cpp
  generators/poppler/formfields.h
  generators/poppler/generator_pdf.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11595: Qt5: Allow setting of visibility

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  Extends Qt5 API to allow setting visibility flags

TEST PLAN
  Autotest in Okular

BRANCH
  master

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

AFFECTED FILES
  qt5/src/poppler-form.cc
  qt5/src/poppler-form.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D11594: Add support for hide action

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  The hide action can be used to show / hide fields.

TEST PLAN
  Unit test using this action in okular

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

AFFECTED FILES
  poppler/Link.cc
  poppler/Link.h
  qt5/src/poppler-annotation.cc
  qt5/src/poppler-link.cc
  qt5/src/poppler-link.h
  qt5/src/poppler-page.cc

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke closed this revision.
aheinecke added a comment.


  Thanks!

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

To: aheinecke, #okular, aacid
Cc: aacid, michaelweghorn, ngraham

D10869: [5/5] Add test for read only set and checkbox calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 29920.
aheinecke added a comment.


  Added test for save / load of readOnly values

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10869?vs=28711&id=29920

BRANCH
  master

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

AFFECTED FILES
  autotests/data/checkbox_ro.pdf
  autotests/parttest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke added a comment.


  With this change saving / loading the read only state works.

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke updated this revision to Diff 29917.
aheinecke added a comment.


  Added object dict / xref update so that the Read Only state is saved.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10864?vs=28112&id=29917

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

AFFECTED FILES
  poppler/Form.cc
  poppler/Form.h
  qt5/src/poppler-form.cc
  qt5/src/poppler-form.h

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke added a comment.


  I've started to look at saving this on Friday. I'm a bit confused as the 
ReadOnly is a Field value and not a Widget Annotation value and the Adobe Spec 
says that it is ignored for Widget Annotations.
  
  Saving a document with Adobe after changing a ReadOnly field to a Read/Write 
seems to add a new Widget Annotation for the Field, though.
  
  As I don't really have experience working with the PDF File Format / poppler 
internals I'm a bit out of my depths here. I'll spend some more hours trying to 
understand it better today.

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke updated this revision to Diff 29566.
aheinecke added a comment.


  Remove superfluous check / reorder in PageView::notifySetup
  
  As noted by aacid we don't need the check anymore now that
  setCanBeFilled does not access the underlying fields.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10865?vs=29471&id=29566

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageviewutils.cpp

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in pageview.cpp:1002
> Ok, so now that setCanBeFilled doesn't access the form, do we really need 
> this extra if?

No. :-)

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke updated this revision to Diff 29471.
aheinecke added a comment.


  Removed check for readOnly in setCanBeFilled

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10865?vs=28706&id=29471

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp
  ui/pageviewutils.cpp

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in formwidgets.cpp:310
> sure, if allowfillforms is false, we will call setCanBeFilled with false and 
> it will be setEnabled to false.
> 
> What I am asking is why do we need to call isReadOnly here. As far as i 
> understand if the field is readonly, it won't be shown, so the enabled status 
> of it doesn't matter, no?

Oh, Sorry. I misunderstood your first comment and thought you questioned the 
need for the allowFillForms in general.

As for the read only check according to git blame this check was added ( 
8e70c16f 
 ) 
at a time where readOnly fields were visible but disabled and now that they are 
invisible it should indeed no longer be needed.

I tried to think of a way how a readOnly field could / should be visible but 
disabled and also can't think of a way. I'll remove the check.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10869: [5/5] Add test for read only set and checkbox calculate

[Andre Heinecke]

aheinecke updated this revision to Diff 28711.
aheinecke added a comment.


  No changes, only updated for context.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10869?vs=28120&id=28711

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

AFFECTED FILES
  autotests/data/checkbox_ro.pdf
  autotests/parttest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10868: [4/5] Fix CheckBox script usage

[Andre Heinecke]

aheinecke updated this revision to Diff 28710.
aheinecke added a comment.


  No changes, only updated to get context

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10868?vs=28119&id=28710

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

AFFECTED FILES
  core/script/kjs_field.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10867: [3/5] Add support to set read only from scripts

[Andre Heinecke]

aheinecke marked an inline comment as done.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10867: [3/5] Add support to set read only from scripts

[Andre Heinecke]

aheinecke updated this revision to Diff 28708.
aheinecke added a comment.


  Added @since marker to setReadOnly

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10867?vs=28115&id=28708

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

AFFECTED FILES
  core/form.cpp
  core/form.h
  core/script/kjs_field.cpp
  generators/poppler/formfields.cpp
  generators/poppler/formfields.h

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10866: [2/5] Implement generic version of Form Widgets refresh

[Andre Heinecke]

aheinecke updated this revision to Diff 28707.
aheinecke added a comment.


  Updated without setVisibility implicit handling of isReadOnly

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10866?vs=28114&id=28707

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke updated this revision to Diff 28706.
aheinecke added a comment.


  Removed implicit readOnly handling in setVisiblitiy and updated
  callers instead.
  
  Also the differential is now published with arcanist ;-)

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10865?vs=28113&id=28706

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp
  ui/pageviewutils.cpp

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke added a comment.


  In D10865#217744 , @aacid wrote:
  
  > BTW next time  please use arc so phabricator shows the context of the diff.
  
  
  Apologies, I'll try. While I like phabricator I'm not very skilled with 
arcanist, yet. :-}

INLINE COMMENTS

> aacid wrote in formwidgets.cpp:304
> i would really prefer this to be moved to whoever calls setVisiblity, it's 
> kind of weird for setVisiblity to sometimes not do what you ask it to do.

Yes, Indeed. I'll update the Differential accordingly.

> aacid wrote in formwidgets.cpp:310
> Do we need this at all? the widget won't ge visible when it's readonly, so 
> what do we care if it's enabled or not?

As I understand it there is a case where Forms are bisible, but disabled.
This is if in "PageView::notifySetup" the check for:

  const bool allowfillforms = d->document->isAllowed( Okular::AllowFillForms );

Returns false.
Then Okular will show all FormWidgets which are not readOnly disabled. I'm not 
sure if that makes much sense but as there is the extra code with 
"setCanBeFilled" I thought I should better leave that behavior because someone 
intended it that way at some point ;-)

> aacid wrote in pageview.cpp:1002
> If we remove the use of visiblity as i commented already we can get this back 
> to how it was?

No, setCanBeFilled accesses isReadOnly. This crashes if the formWidgets are not 
yet updated with the new fields.

I also think that it is better to only modify the field here and not earlier to 
avoid working with formWidgets that have dangling pointers to deleted fields in 
them.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10869: [5/5] Add test for read only set and checkbox calculate

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10869: [5/5] Add test for read only set and checkbox calculate

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  It's a real world screnario to have checkboxes that toggle
  the read only state for other fields.
  
  The test is a parttest because we need to trigger activiation
  action when changing the field, this is handled through the
  formwidgetscontroller.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  autotests/data/checkbox_ro.pdf
  autotests/parttest.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10868: [4/5] Fix CheckBox script usage

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10868: [4/5] Fix CheckBox script usage

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This implements setting / getting the value of buttons,
  which is important for checkboxes in scripts. It also moves
  the checkbox activate action after the value is set so that
  the correct value is used when the activation script is
  executed.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  core/script/kjs_field.cpp
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10867: [3/5] Add support to set read only from scripts

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10867: [3/5] Add support to set read only from scripts

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This makes it possible to set the read only state from
  scripts. Requires Poppler master with the patch attached to the parent task.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  core/form.cpp
  core/form.h
  core/script/kjs_field.cpp
  generators/poppler/formfields.cpp
  generators/poppler/formfields.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10866: [2/5] Implement generic version of Form Widgets refresh

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  The FormWidgetInterface now has a generic slotRefresh that
  refreshes what all widgets have in common. This is the read-only state for 
now.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10866: [2/5] Implement generic version of Form Widgets refresh

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> pageview.cpp:1002
>  }
> -else
> +else if ( !( setupFlags & Okular::DocumentObserver::UrlChanged ) 
> )
>  {

Without this change the PartTest::testSaveAsUndoStackForms would segfault 
because the underlying fields are destroyed but setCanBeFilled now accesses 
them to check for readOnly.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10865: [1/5] Access readOnly state of FormWidgets dynamically

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  This is more of a cleanup patch that removes the obsolete m_canBeEnabled
  member variable which was a leftover IMO from a time where readOnly fields 
were
  shown as disabled. readOnly fields are invisible, not disabled, and the code 
no longer assumes that
  readOnly does not change over time.

TEST PLAN
  Tested manually and with a unittest which is part of the series.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke created this revision.
aheinecke added a reviewer: Okular.
aheinecke added a project: Okular.
aheinecke requested review of this revision.

REVISION SUMMARY
  Read only is modifiable from AcroForm scripts. As such
  we need to add setting it to the data model of Poppler.
  
  -
  
  I know that this is not the right place for a poppler patch but until we have 
discussed the rest of the series I think it makes sense to attach it to the 
task.

TEST PLAN
  Manually tested and with a unit test that will be part of the task.

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

AFFECTED FILES
  poppler/Form.cc
  poppler/Form.h
  qt5/src/poppler-form.cc
  qt5/src/poppler-form.h

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10864: Poppler: Add read only setter for form fields

[Andre Heinecke]

aheinecke added a task: T8097: Support for read only changes and checkbox 
values in scripts.

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

To: aheinecke, #okular
Cc: michaelweghorn, ngraham, aacid

D10073: [PATCH 1/4] Add JavaScript Event Object handling

[Andre Heinecke]

aheinecke updated this revision to Diff 27658.
aheinecke added a comment.


  Removed check for page in field wrapping.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10073?vs=27264&id=27658

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

AFFECTED FILES
  CMakeLists.txt
  core/script/event.cpp
  core/script/event_p.h
  core/script/executor_kjs.cpp
  core/script/executor_kjs_p.h
  core/script/kjs_event.cpp
  core/script/kjs_event_p.h
  core/scripter.cpp
  core/scripter.h

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10547: Recalculate forms after command form changes

[Andre Heinecke]

aheinecke updated this revision to Diff 27600.
aheinecke retitled this revision from "[PATCH] Recalculate forms after command 
form changes" to "Recalculate forms after command form changes".
aheinecke edited the test plan for this revision.
aheinecke added a comment.


  Extended CalculateTextTest to include undo / redo.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10547?vs=27267&id=27600

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

AFFECTED FILES
  autotests/calculatetexttest.cpp
  core/document.cpp

To: aheinecke, #okular
Cc: aacid, ltoscano, michaelweghorn, ngraham

D10048: [PATCH 2/4] Add refresh widgets if underlying field changes

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in document.cpp:1139
> This is problematic since it will leave a dangling event pointer for any 
> Action::Script action executed through Document::processAction that doesn't 
> come from this function.

Sorry I can't follow you here.

If an action is processed from somewhere else the scripter won't have an event 
set and the event pointer in the scripter is null.

If the processAction is triggered here for text fields (and probably more in 
the future) we "reset" the event pointer after it was set in the scripter in 
line 1148 after the action is processed and keep using the event here until it 
goes out of scope and is destroyed by the shared pointer.

I don't see a codepath were we leave it dangling.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham

D10073: [PATCH 1/4] Add JavaScript Event Object handling

[Andre Heinecke]

aheinecke added inline comments.

INLINE COMMENTS

> aacid wrote in kjs_field.cpp:217
> Why the new if?

It is not assured in kjs_event.cpp eventGetSource and eventGetTarget that the 
targetPage / sourcePage is not null. This depends a how the event object is set 
up.

Although I think that sourcePage "should" be set when source is set and 
targetPage should be set if target is set there is no guarantee for this and so 
I think a check here is warranted.

REPOSITORY
  R223 Okular

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

To: aheinecke, #okular
Cc: aacid, michaelweghorn, ngraham