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.
Details
Details
- Reviewers
aacid - Group Reviewers
Okular - Maniphest Tasks
- T8097: Support for read only changes and checkbox values in scripts
- Commits
- R223:42717e1ae8f2: Access readOnly state of FormWidgets dynamically
Tested manually and with a unittest which is part of the series.
Diff Detail
Diff Detail
- Repository
- R223 Okular
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| ui/pageview.cpp | ||
|---|---|---|
| 1002 ↗ | (On Diff #28113) | Without this change the PartTest::testSaveAsUndoStackForms would segfault because the underlying fields are destroyed but setCanBeFilled now accesses them to check for readOnly. |
Comment Actions
BTW next time please use arc so phabricator shows the context of the diff.
| ui/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. | |
| 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? | |
| ui/pageview.cpp | ||
| 1002 ↗ | (On Diff #28113) | If we remove the use of visiblity as i commented already we can get this back to how it was? |
Comment Actions
Apologies, I'll try. While I like phabricator I'm not very skilled with arcanist, yet. :-}
| ui/formwidgets.cpp | ||
|---|---|---|
| 304 | Yes, Indeed. I'll update the Differential accordingly. | |
| 310 | As I understand it there is a case where Forms are bisible, but disabled. const bool allowfillforms = d->document->isAllowed( Okular::AllowFillForms ); Returns false. | |
| ui/pageview.cpp | ||
| 1002 ↗ | (On Diff #28113) | 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. |
Comment Actions
Removed implicit readOnly handling in setVisiblitiy and updated
callers instead.
Also the differential is now published with arcanist ;-)
| ui/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? | |
| ui/formwidgets.cpp | ||
|---|---|---|
| 310 | 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. | |
| ui/pageview.cpp | ||
|---|---|---|
| 1002 ↗ | (On Diff #28113) | Ok, so now that setCanBeFilled doesn't access the form, do we really need this extra if? |
| ui/pageview.cpp | ||
|---|---|---|
| 1002 ↗ | (On Diff #28113) | No. :-) |
Comment Actions
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.