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

aacid accepted this revision. This revision is now accepted and ready to land. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10865 To: aheinecke, #okular, aacid Cc: aacid, michaelweghorn, ngraham

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

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

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

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:

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

aacid added inline comments. INLINE COMMENTS > aheinecke wrote in pageview.cpp:1002 > 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

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

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=29471 REVISION DETAIL https://phabricator.kde.org/D10865 AFFECTED FILES

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

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

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

aacid added inline comments. INLINE COMMENTS > aheinecke wrote in formwidgets.cpp:310 > 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(

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

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

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

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

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

aacid added a comment. BTW next time please use arc so phabricator shows the context of the diff. INLINE COMMENTS > formwidgets.cpp:304 > m_widget->clearFocus(); > -m_widget->setVisible( visible ); > +m_widget->setVisible( visible && !m_ff->isReadOnly() ); > return

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

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

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

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

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