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

Authored by aheinecke on Jan 23 2018, 1:51 PM.

Details

Summary

If a field is updated because of a calculate form action /
a script execution, not only refresh the rendered pixmap but
also the corresponding formWidget.

Test Plan

Unittest in separate revision. Tested it manually, too.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
aheinecke requested review of this revision.Jan 23 2018, 1:51 PM
aheinecke created this revision.
aheinecke planned changes to this revision.Jan 23 2018, 2:31 PM

This might not be necessary. If we use the calculated field as "event" object in the javascript it should update through the fields.setValue mechanism.

aheinecke updated this revision to Diff 25875.Jan 24 2018, 12:24 PM

Updated to work with an Event Object.

aacid added a subscriber: aacid.Feb 3 2018, 7:47 PM

Can you explain why we need this?

isn't this already working https://www.youtube.com/watch?v=S-zmHc3WUhs ?

Probably i'm misunderstanding something

aheinecke planned changes to this revision.Feb 6 2018, 8:49 AM

Mh for me this did not work when just setting the text on a field. Looking at JSField::fieldSetValue I probably missed the refreshPixmaps call after setting the value.

One difference here is that we change the Field through the Event object and not in pure JavaScript. But yes both ways should use the same mechanism. I look into it some more. Maybe some general "field changed through script" mechanism would be better to support stuff like changing visibility or ReadOnly state of fields. I'm not sure if the "refreshPixmaps" would already make this possible.

I had another look at this and I could not get it to work with refreshPixmaps and I don't understand how the FormWidget should be updated if the text of the underlying FormField changes. The only setTexts I see in the FormWidgets are in the Ctor or in the "slotHandleTextChangedByUndoRedo" I'm probably overlooking something :-/

So I tried it out and added another Field "JSSum" to the example PDF I'm using

which had a calculate script that modified a field value through a JavaScript assignment and it also did not work there:
So maybe there was a regression at some point?

Could you attach a sample PDF where this is working? Sorry I don't want to create additional work for you but I did not find one by searching for related bugs in BKO.

The Versions I tried were unmodified master (e06883d10) and Okular 0.26.1 which I have from my debian distro.

To be clear:
I'm still planning to make changes to this revision but I first want to figure out if there is an already working update mechanism between FormField and FormWidget that I have overlooked or if the one that was in place needs a fix.

Thanks,
Andre

aacid added a comment.Feb 7 2018, 11:19 PM

I had another look at this and I could not get it to work with refreshPixmaps and I don't understand how the FormWidget should be updated if the text of the underlying FormField changes. The only setTexts I see in the FormWidgets are in the Ctor or in the "slotHandleTextChangedByUndoRedo" I'm probably overlooking something :-/

So I tried it out and added another Field "JSSum" to the example PDF I'm using

which had a calculate script that modified a field value through a JavaScript assignment and it also did not work there:
So maybe there was a regression at some point?

Could you attach a sample PDF where this is working?

https://www.revenue.wi.gov/dorforms/2016-Form1f.pdf

Sorry I don't want to create additional work for you but I did not find one by searching for related bugs in BKO.

The Versions I tried were unmodified master (e06883d10) and Okular 0.26.1 which I have from my debian distro.

To be clear:
I'm still planning to make changes to this revision but I first want to figure out if there is an already working update mechanism between FormField and FormWidget that I have overlooked or if the one that was in place needs a fix.

Well you need the refresh pixmaps for sure, because at least right now when hiding the forms the image of the pdf has the wrong value, so that needs fixing.

As for the difference with the pdf i linked, i think i see the difference, the ones you linked the all the widgets are so you need to update the "result" widgets values.

Could you explore getting that value from the core of poppler and not setting the value in the ui manually? I mean if you force a pixmap refresh (by zooming in/out) the values of the rendered "image" update correctly so somewhere something knows the value changed. I'd prefer using that over going manually and setting the value "again"

Thanks,
Andre

Could you attach a sample PDF where this is working?

https://www.revenue.wi.gov/dorforms/2016-Form1f.pdf

Thanks. Still with current master this does not work for me as in the video. When I zoom in and out it works but not automatically through the refresh-pixmaps in the field setter. With current master its:

Maybe that one is optimized out somehow? I'll try to fix this with this change, too.

Well you need the refresh pixmaps for sure, because at least right now when hiding the forms the image of the pdf has the wrong value, so that needs fixing.
As for the difference with the pdf i linked, i think i see the difference, the ones you linked the all the widgets are so you need to update the "result" widgets values.

Ah, I understand it now better -> refresh pixmaps to update the "rendered" / result field values.

Could you explore getting that value from the core of poppler and not setting the value in the ui manually? I mean if you force a pixmap refresh (by zooming in/out) the values of the rendered "image" update correctly so somewhere something knows the value changed. I'd prefer using that over going manually and setting the value "again"

Yes indeed. I think we need something similar to refreshPixmaps for the input widgets too, some kind of "refreshWidgets" that then updates the widgets with the values from the poppler fields.

aheinecke updated this revision to Diff 26761.Feb 8 2018, 1:48 PM
aheinecke retitled this revision from [PATCH 2/4] Communicate calculate text change to formwidgets to [PATCH 2/4] Add refresh widgets if underlying field changes.
aheinecke edited the summary of this revision. (Show Details)

Proper refreshPixmap handling and a new "refreshWidget" signal instead of a very specialized signal name. Value is updated from poppler field and not carried by the signal.

Thanks. Still with current master this does not work for me as in the video. When I zoom in and out it works but not automatically through the refresh-pixmaps in the field setter. With current master its:

Maybe that one is optimized out somehow? I'll try to fix this with this change, too.

The reason for this was a very recent regression as noted in: https://phabricator.kde.org/R223:2d8b2c7e95927db1633ecb871ed4100c3e7e3833#inline-968

With the fix from that comment both my sample and your sample work nicely.

aacid added inline comments.Feb 9 2018, 11:56 PM
core/document.cpp
1153

I was thinking, can't we do setText emit the signal? so we don't really need to worry about forgetting to emit refreshFormWidget in case we end up implementing another function or something that does change the text of a form?

aheinecke added inline comments.Feb 12 2018, 9:31 AM
core/document.cpp
1153

Sure this would be nice but as I understand it this would mean making FormFieldText (or the general FormField) a QObject. As this is public API i shied away from such a solution.

Should I change the patch to make FormFieldText a QObject with a textChanged signal?

Haven't had time to try myself, but how all this works with undo/redo? Does stuff break or does it work nice?

core/document.h
1200

make this 1.4 which will be the actually released version

Haven't had time to try myself, but how all this works with undo/redo? Does stuff break or does it work nice?

Ah, no. Undo / Redo does not trigger a new evaluation of calculate actions. But this is not a regression. I'll look at it it in a different patch.

core/document.h
1200

Ok. But if we make the FormField a QObject we won't need it anymore :-)

Can you answer my question if making FormField a QObject would be ok? That would probably give us the best flexibility for future changes so I would like to do that. :-)

aacid added inline comments.Feb 13 2018, 8:37 AM
core/document.cpp
1153

API break is not a problem since i already had to break it recently.

But on the other hand it will probably open a can of works since all the other document signals about forms should probably be formfield signals too, so i'd say i'm leaning towards ntot changing this.

core/document.h
1200

I'm prety sure i answered that, but the answer is not there, i wonder if i was using two different windows to answer and that made one content get lost?

Anyhow,will answer below.

aheinecke updated this revision to Diff 27257.Feb 15 2018, 2:17 PM

Changed comment about version which added the new signal.

aheinecke marked 6 inline comments as done.Feb 15 2018, 2:18 PM

So I only changed the version to 1.4 here.

aheinecke updated this revision to Diff 27266.Feb 15 2018, 2:38 PM

Update to build event with non const page ref after change in D10073

aacid added inline comments.Feb 19 2018, 11:54 PM
core/document.cpp
1139 ↗(On Diff #25875)

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.

aheinecke added inline comments.Feb 20 2018, 12:44 PM
core/document.cpp
1139 ↗(On Diff #25875)

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.

aacid added inline comments.Feb 20 2018, 7:07 PM
core/document.cpp
1139 ↗(On Diff #25875)

right sorry i didn't see you did call setEvent null below

aacid accepted this revision.Feb 21 2018, 11:18 PM
This revision is now accepted and ready to land.Feb 21 2018, 11:18 PM
This revision was automatically updated to reflect the committed changes.