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

[Albert Astals Cid]

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

[Andre Heinecke]

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 LAST UPDATE
  https://phabricator.kde.org/D10865?vs=29471=29566

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageviewutils.cpp

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

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

[Andre Heinecke]

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: aacid, michaelweghorn, ngraham

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

[Albert Astals Cid]

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 working with formWidgets that have dangling pointers to deleted 
> fields in them.

Ok, so now that setCanBeFilled doesn't access the form, do we really need this 
extra if?

REPOSITORY
  R223 Okular

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

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

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

[Andre Heinecke]

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
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp
  ui/pageviewutils.cpp

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

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

[Andre Heinecke]

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 the field is readonly, it won't be shown, so the enabled status 
> of it doesn't matter, no?

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.

REPOSITORY
  R223 Okular

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

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

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

[Albert Astals Cid]

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( Okular::AllowFillForms 
> );
> 
> Returns false.
> Then Okular will show all FormWidgets which are not readOnly disabled. I'm 
> not sure if that makes much sense but as there is the extra code with 
> "setCanBeFilled" I thought I should better leave that behavior because 
> someone intended it that way at some point ;-)

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?

REPOSITORY
  R223 Okular

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

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

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

[Andre Heinecke]

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
  https://phabricator.kde.org/D10865?vs=28113=28706

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp
  ui/pageviewutils.cpp

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

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

[Andre Heinecke]

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 COMMENTS

> aacid wrote in 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.

Yes, Indeed. I'll update the Differential accordingly.

> aacid wrote in formwidgets.cpp: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?

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( Okular::AllowFillForms );

Returns false.
Then Okular will show all FormWidgets which are not readOnly disabled. I'm not 
sure if that makes much sense but as there is the extra code with 
"setCanBeFilled" I thought I should better leave that behavior because someone 
intended it that way at some point ;-)

> aacid wrote in pageview.cpp:1002
> If we remove the use of visiblity as i commented already we can get this back 
> to how it was?

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.

REPOSITORY
  R223 Okular

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

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

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

[Albert Astals Cid]

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 hadfocus;

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.

> formwidgets.cpp:310
>  {
> -m_widget->setEnabled( fill && m_canBeEnabled );
> +m_widget->setEnabled( fill && !m_ff->isReadOnly() );
>  }

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?

> aheinecke wrote in pageview.cpp:1002
> Without this change the PartTest::testSaveAsUndoStackForms would segfault 
> because the underlying fields are destroyed but setCanBeFilled now accesses 
> them to check for readOnly.

If we remove the use of visiblity as i commented already we can get this back 
to how it was?

REPOSITORY
  R223 Okular

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

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

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

[Andre Heinecke]

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 underlying fields are destroyed but setCanBeFilled now accesses 
them to check for readOnly.

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

[Andre Heinecke]

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

[Andre Heinecke]

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 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.

TEST PLAN
  Tested manually and with a unittest which is part of the series.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  ui/formwidgets.cpp
  ui/formwidgets.h
  ui/pageview.cpp

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