D27650: ItemContainer: disconnect signals in destructor

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:8f0da90f18ff: ItemContainer: disconnect signals in 
destructor (authored by alnikiforov, committed by davidedmundson).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27650?vs=76462&id=77089

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

AFFECTED FILES
  components/containmentlayoutmanager/itemcontainer.cpp

To: alnikiforov, ngraham, davidedmundson, mart
Cc: cfeck, anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[David Edmundson]

davidedmundson added a comment.


  Urgh, sorry.
  
  Thanks cfeck

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: cfeck, anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Christoph Feck]

cfeck added a comment.


  If this is still needed, please commit and clarify the status of the 
referenced bug.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: cfeck, anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov added a comment.


  In D27650#618492 , @davidedmundson 
wrote:
  
  > Can I have your full name + email address please.
  
  
  Aleksei Nikiforov 

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[David Edmundson]

davidedmundson added a comment.


  > No, AFAIK I don't have commit access.
  
  Can I have your full name + email address please.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov updated this revision to Diff 76462.
alnikiforov edited the summary of this revision.
alnikiforov added a comment.


  Got rid of two disconnects and updated summary formatting.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27650?vs=76439&id=76462

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

AFFECTED FILES
  components/containmentlayoutmanager/itemcontainer.cpp

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[David Edmundson]

davidedmundson added a comment.


  > No, AFAIK I don't have commit access.
  
  Ok, well I hope this is the first of many patches. I like working with people 
who can use gdb!
  
  Good stuff.

INLINE COMMENTS

> alnikiforov wrote in itemcontainer.cpp:68
> Should I remove these 2 disconnects or leave them as they are?

Lets get rid of them. Then it helps self-document that this the one left is 
doing something explicitly for a good reason.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov added inline comments.

INLINE COMMENTS

> davidedmundson wrote in itemcontainer.cpp:68
> Right, this one line makes a lot of sense.
> 
> I can imagine QQuickItem::~QuickItem changes parent, and calling 
> ItemContainer::setLayout after we've run our ItemContainers destructor means 
> all bets are completely off. I can believe an already destroyed QPointer 
> behaves weirdly.
> 
> Lets not change the other two. There's no chance of an event loop processing 
> a timer event after ItemContainer:::~ItemContainer before we're auto 
> disconnected.

Should I remove these 2 disconnects or leave them as they are?

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov added a comment.


  In D27650#618207 , @davidedmundson 
wrote:
  
  > > And this used QWeakPointer leads to premature destruction of object 
m_layout points to.
  >
  > Do you have commit access?
  
  
  Yep, original description was not entirely correct.
  
  No, AFAIK I don't have commit access.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[David Edmundson]

davidedmundson added a comment.


  > And this used QWeakPointer leads to premature destruction of object 
m_layout points to.
  
  Do you have commit access?

INLINE COMMENTS

> itemcontainer.cpp:68
> +disconnect(m_editModeTimer, &QTimer::timeout, this, nullptr);
> +disconnect(this, &QQuickItem::parentChanged, this, nullptr);
> +

Right, this one line makes a lot of sense.

I can imagine QQuickItem::~QuickItem changes parent, and calling 
ItemContainer::setLayout after we've run our ItemContainers destructor means 
all bets are completely off. I can believe an already destroyed QPointer 
behaves weirdly.

Lets not change the other two. There's no chance of an event loop processing a 
timer event after ItemContainer:::~ItemContainer before we're auto disconnected.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> itemcontainer.cpp:66
>  {
> +disconnect(m_sizeHintAdjustTimer, &QTimer::timeout, this, 
> &ItemContainer::sizeHintsChanged);
> +disconnect(m_editModeTimer, &QTimer::timeout, this, nullptr);

You can translate this calls to
`m_sizeHintAdjustTimer->disconnect(this)`

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov added a comment.


  Fix consists of following line:
  
disconnect(this, &QQuickItem::parentChanged, this, nullptr);
  
  Two other disconnects are just in case of similar issues arising and for code 
consistency.

REPOSITORY
  R120 Plasma Workspace

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

To: alnikiforov, ngraham, davidedmundson, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27650: ItemContainer: disconnect signals in destructor

[Aleksei Nikiforov]

alnikiforov updated this revision to Diff 76439.
alnikiforov retitled this revision from "Don't use guarded pointers for 
AppletsLayout" to "ItemContainer: disconnect signals in destructor".
alnikiforov edited the summary of this revision.
alnikiforov added a comment.


  Uploaded updated change

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27650?vs=76364&id=76439

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

AFFECTED FILES
  components/containmentlayoutmanager/itemcontainer.cpp

To: alnikiforov, ngraham, davidedmundson, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart