Similar to how the autotest (editformtext.cpp) tests the form contents, I implemented the reset form option in a similar way by creating a function to reset each widget separately .
BUG: 288042
Details
Details
Diff Detail
Diff Detail
- Repository
- R223 Okular
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
I have created an auto test but it is not working, in the test class I created a page view class that takes a document pointer in its constructor then it calls the reset function in the pageview class.
I am thinking of a way of getting it to work other than creating a reset function in the document class.
This comment was removed by ahmadosama.
Comment Actions
I added an auto-test for this option, the test is working fine on all the created test cases.
Comment Actions
Have you thought how this should interact with the undo/redo history stack?
| autotests/resetformstest.cpp | ||
|---|---|---|
| 183 | what does this test do? | |
| part.cpp | ||
| 3359 ↗ | (On Diff #29396) | Why did you add this? |
| ui/pageview.cpp | ||
| 731 | Why are you calling this? | |
| 4333 | why do you set the text every single time this is called instead of on construction time? | |
| 4433 | What's the point of all this one single line functions? | |
| autotests/resetformstest.cpp | ||
|---|---|---|
| 183 | This test is to make sure that the reset button is working correctly. | |
| part.cpp | ||
| 3359 ↗ | (On Diff #29396) | I added it to show the "reset forms" button in the displayed message, beside the "Show Forms" button. |
| ui/pageview.cpp | ||
| 731 | 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. | |
| 4333 | Oh yes, I am going to edit this. | |
| 4433 | I did this so that if I want to change the reset function's implementation later, | |
Comment Actions
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.
Comment Actions
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.
| part.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #29396) | 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. | |
| part.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #29396) | 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. |
Comment Actions
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.
| ui/pageview.h | ||
|---|---|---|
| 59 | whitespace | |
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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?
Comment Actions
After opening the file, the first option is selected. Resetting goes back to that exact same state:
Comment Actions
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.