D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid requested changes to this revision.
This revision now requires changes to proceed.
Restricted Application added a subscriber: okular-devel.

REPOSITORY
  R223 Okular

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

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

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

D10932: [Okular] Option to reset forms

[Henrik Fehlauer]

rkflx added a comment.


  In D10932#252571 , @aacid wrote:
  
  > In D10932#248618 , @rkflx wrote:
  >
  > > FWIW, Foxit Reader does have a Reset Form button, which upon clicking 
shows a dialog asking "This option will reset all form fields to their default 
values. You may lose some data. Are you sure?".
  > >
  > > Let me know if you have specific example documents I should test this 
applications with, to see how they solved some of the more complicated aspects.
  >
  >
  > Can you try it with 
https://cgit.kde.org/okular.git/plain/autotests/data/formSamples.pdf ? what 
does it do with the "Option buttons" on the top left?
  
  
  After opening the file, the first option is selected. Resetting goes back to 
that exact same state:
  
  F5822304: foxit-reader-forms.webm 

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid added a comment.


  In D10932#248786 , @ngraham wrote:
  
  > +1 for keeping this open and refining the behavior rather than abandoning 
it.
  
  
  How do you refine the behaviour in the case i mentioned where there are 
javascript actions associated with contents of the fields changing? What is the 
refined way to do that?

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid added a comment.


  In D10932#248618 , @rkflx wrote:
  
  > FWIW, Foxit Reader does have a Reset Form button, which upon clicking shows 
a dialog asking "This option will reset all form fields to their default 
values. You may lose some data. Are you sure?".
  >
  > Let me know if you have specific example documents I should test this 
applications with, to see how they solved some of the more complicated aspects.
  
  
  Can you try it with 
https://cgit.kde.org/okular.git/plain/autotests/data/formSamples.pdf ? what 
does it do with the "Option buttons" on the top left?

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Nathaniel Graham]

ngraham added a comment.


  +1 for keeping this open and refining the behavior rather than abandoning it.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Henrik Fehlauer]

rkflx added a comment.


  FWIW, Foxit Reader does have a Reset Form button, which upon clicking shows a 
dialog asking "This option will reset all form fields to their default values. 
You may lose some data. Are you sure?".
  
  Let me know if you have specific example documents I should test this 
applications with, to see how they solved some of the more complicated aspects.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama added a comment.


  I agree with what you have said, and I was thinking about some of it.
  Closing the feature will not upset me, I have learned a lot from working on 
it.
  I hope that I could make better contributions to Okular later.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular, aacid
Cc: cfeck, ngraham, aacid, #okular, michaelweghorn

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid added a comment.


  Sorry for the late answer.
  
  I've been thinking about this and as all the stuff random user ask for, it 
shows they have not really thought much about it, and it is probably my fault 
having set this as a junio job.
  
  What does "Reset" actually mean? Is clearing a field resetting it? What if 
the field had "BLA" as contents when you opened it? Wouldn't resetting mean 
going back to "BLA" instead of empty?
  
  Ok, so this could be solved by changing from "Reset Forms" to "Clear Forms".
  
  "Clear" has a more "make this empty" meaning.
  
  But how do you actually clear a Radio button? When one of the N buttons has 
to be selected by definition?
  
  One could say "ok, let's ignore radio buttons".
  
  But then the biggest problem shows up and is on change actions linked to 
forms. You can have javascript linked to changing contents on form fields, and 
for example you could have one that said "if text of field A is empty put text 
'BLA' on field B".
  
  What would be the correct output of running "Clear Forms"? Should field B 
contain "BLA" or not?
  
  And i'm going to say probably, but how do you actually achieve that 
programatically? I don't see a way since you'll go "set field A to be empty" -> 
"this triggers its execute change action" -> "set field B to empty"
  
  So my current thinking is closing the feature request as won't fix giving a 
version of the explanation written above.
  
  What do you think? Do you think what i say makes sense or am i saying stupid 
things?
  
  And if we were to throw away this code, how sad would you be? Have you at 
least learnt a bit about how testing/undo commands in Qt work?

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular, aacid
Cc: cfeck, ngraham, aacid, #okular, michaelweghorn

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama updated this revision to Diff 31204.
ahmadosama added a comment.


  Removing white spaces.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10932?vs=29771=31204

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/resetformstest.cpp
  core/document.cpp
  core/document.h
  core/documentcommands.cpp
  core/documentcommands_p.h
  part.rc
  ui/pageview.cpp
  ui/pageview.h

To: ahmadosama, #okular, aacid
Cc: cfeck, ngraham, aacid, #okular, michaelweghorn

D10932: [Okular] Option to reset forms

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> pageview.h:59
>  {
> -Q_OBJECT
> +Q_OBJECT
>  

whitespace

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular, aacid
Cc: cfeck, ngraham, aacid, #okular, michaelweghorn

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama updated this revision to Diff 29771.
ahmadosama added a comment.


  I have done the required changes.
  I created the EditWidgetsCommand class to undo/redo the reset action of all 
widgets by a single click, also I moved the core implementation of the reset 
function to the Document class, and I updated the reset auto test to include 
the undo/redo and the test is working fine on the created tests.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10932?vs=29396=29771

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/resetformstest.cpp
  core/document.cpp
  core/document.h
  core/documentcommands.cpp
  core/documentcommands_p.h
  part.cpp
  part.h
  part.rc
  ui/pageview.cpp
  ui/pageview.h

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama added inline comments.

INLINE COMMENTS

> aacid wrote in part.cpp:3359
> I don't think that's a good idea, do you think resetting the forms is as 
> common as showing/hiding them?

I am going to remove it, the user will be able to trigger the reset action from 
the "view" drop down menu in the menu bar.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid added a comment.


  In D10932#225740 , @ahmadosama 
wrote:
  
  > Regarding the undo/redo stacks, the undo/redo command will not undo the 
whole reset, it will undo widgets somehow randomly.
  >  RadioButtons in the same page will be in the same undo command, as for 
TextLineEdit each single widget will be added separately to the undo/redo stack.
  >
  > So triggering the reset action will result in adding several commands to 
the undo stack not just one, I am currently thinking of how to make the whole 
undo in a single command in the stack.
  
  
  So how this typically works is a QUndoCommand that holds all the other 
QUndoCommand
  
  > Also, the "Reset Forms" and "Show Forms/Hide Forms" buttons are stacked 
beside each other, the user can easily mistake by clicking the "Reset" button 
instead of the "Show Forms" Button.
  
  I'd just remove it from the top bar, i don't see why we need reset forms in 
there.
  
  > I am currently thinking about fixing this too.

INLINE COMMENTS

> ahmadosama wrote in part.cpp:3359
> I added it to show the "reset forms" button in the displayed message, beside 
> the "Show Forms" button.

I don't think that's a good idea, do you think resetting the forms is as common 
as showing/hiding them?

> part.rc:50
>  
> +
>

You need to increase the version number at the top of the file.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama added a comment.


  Regarding the undo/redo stacks, the undo/redo command will not undo the whole 
reset, it will undo widgets somehow randomly.
  RadioButtons in the same page will be in the same undo command, as for 
TextLineEdit each single widget will be added separately to the undo/redo stack.
  
  So triggering the reset action will result in adding several commands to the 
undo stack not just one, I am currently thinking of how to make the whole undo 
in a single command in the stack.
  
  Also, the "Reset Forms" and "Show Forms/Hide Forms" buttons are stacked 
beside each other, the user can easily mistake by clicking the "Reset" button 
instead of the "Show Forms" Button.
  I am currently thinking about fixing this too.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama added inline comments.

INLINE COMMENTS

> aacid wrote in resetformstest.cpp:182
> what does this test do?

This test is to make sure that the reset button is working correctly.

> aacid wrote in part.cpp:3359
> Why did you add this?

I added it to show the "reset forms" button in the displayed message, beside 
the "Show Forms" button.

> aacid wrote in pageview.cpp:731
> Why are you calling this?

I am going to remove it, I was writing code similar to that of the toggle forms 
action when I started working on this option.

> aacid wrote in pageview.cpp:4333
> why do you set the text every single time this is called  instead of on 
> construction time?

Oh yes, I am going to edit this.

> aacid wrote in pageview.cpp:4433
> What's the point of all this one single line functions?

I did this so that if I want to change the reset function's implementation 
later,  
anyway I am going to remove those functions.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Albert Astals Cid]

aacid added a comment.


  Have you thought how this should interact with the undo/redo history stack?

INLINE COMMENTS

> resetformstest.cpp:182
> +{
> +verifyTextForm( m_textLineForm );
> +}

what does this test do?

> part.cpp:3359
>  m_formsMessage->addAction( m_pageView->toggleFormsAction() );
> -
> +m_formsMessage->addAction( m_pageView->resetFormsAction() );
>  // ensure history actions are in the correct state

Why did you add this?

> pageview.cpp:731
> +d->aResetForms->setEnabled( false );
> +resetFormWidgets( false );
> +

Why are you calling this?

> pageview.cpp:4333
> +if(d->aResetForms)
> +d->aResetForms->setText( i18n( "Reset Forms" ) );
> +if( d->aResetForms && on)

why do you set the text every single time this is called  instead of on 
construction time?

> pageview.cpp:4433
> +{
> +document->editFormButtons( pageNumber, formButtons, newButtonStates);
> +}

What's the point of all this one single line functions?

REPOSITORY
  R223 Okular

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

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama updated this revision to Diff 29396.
ahmadosama edited the test plan for this revision.
ahmadosama added a comment.


  I added an auto-test for this option, the test is working fine on all the 
created test cases.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10932?vs=29292=29396

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/resetformstest.cpp
  part.cpp
  part.h
  part.rc
  ui/pageview.cpp
  ui/pageview.h

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

D10932: [Okular] Option to reset forms

[Ahmad Osama]

ahmadosama retitled this revision from "Bug 288042 - Option to reset forms 
(Implemented using FormFields)" to "[Okular] Option to reset forms".
ahmadosama edited the summary of this revision.
ahmadosama edited the test plan for this revision.

REPOSITORY
  R223 Okular

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

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