D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26267#584400 , @ndavis wrote:
  
  > In D26267#584396 , 
@hpereiradacosta wrote:
  >
  > > -1:
  > >  if QWeakPointer::data() is obsoleted, (while needed in the code), I 
would object to adding toStrongRef, in between, since as pointed out by Antony, 
calling data immediately after brings no further security, and just results in 
some overhead (Weak to shared pointer). It means that either
  > >
  > > - weakPointer::data should not be obsoleted
  > > - the kdecoration api should be changed (to return shared pointers, or 
provide direct access to the data).
  >
  >
  > So this patch is not the correct thing to do at all, even if I make the 
changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate 
QWeakPointer::data() is an option, so it seems like changing KDecoration would 
be the correct thing to do. Does that seem correct to you?
  
  
  That's my understanding yes. As long as access to the true data is needed 
(here for signal/slot connections) by the customers of kdecoration, then 
kdecoration must provide this access, without roundtrips to shared pointers. 
  Others please correct me if I am wrong.

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Noah Davis]

ndavis added a comment.


  
  
  In D26267#584396 , 
@hpereiradacosta wrote:
  
  > -1:
  >  if QWeakPointer::data() is obsoleted, (while needed in the code), I would 
object to adding toStrongRef, in between, since as pointed out by Antony, 
calling data immediately after brings no further security, and just results in 
some overhead (Weak to shared pointer). It means that either
  >
  > - weakPointer::data should not be obsoleted
  > - the kdecoration api should be changed (to return shared pointers, or 
provide direct access to the data).
  
  
  So this patch is not the correct thing to do at all, even if I make the 
changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate 
QWeakPointer::data() is an option, so it seems like changing KDecoration would 
be the correct thing to do. Does that seem correct to you?

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  -1:
  if QWeakPointer::data() is obsoleted, (while needed in the code), I would 
object to adding toStrongRef, in between, since as pointed out by Antony, 
calling data immediately after brings no further security, and just results in 
some overhead (Weak to shared pointer). It means that either
  
  - weakPointer::data should not be obsoleted
  - the kdecoration api should be changed (to return shared pointers, or 
provide direct access to the data).

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Noah Davis]

ndavis added a subscriber: hpereiradacosta.
ndavis added a comment.


  Thank you for pointing those out. I don't have a lot of experience, so this 
helps a lot.

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Anthony Fieroni]

anthonyfieroni added a comment.


  `auto c = client().toStrongRef().data();`
  This is completely wrong, the idea behind shared / weak pointer is ability to 
know when non-owning resource goes out of scope. by calling toStrongRef you 
extend its lifetime if and only if it exists at time of call then holding this 
ref you ensure that resource will not be destroyed meanwhile. By holding data() 
it does nothing to extend resource lifetime but getting a pointer to well-known 
dying object. It should be
  `auto c = m_decoration.data()->client().toStrongRef();`
  Last but not least, c can be nullptr check against null should be performed 
as well. Exp
  
xcb_window_t windowId;
auto c = m_decoration.data()->client().toStrongRef();
if (c && (windowId = c->windowId())) {
} else {
hide();
}
  
  It should be added checks in all use places.

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma
Cc: anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Noah Davis]

ndavis created this revision.
ndavis added reviewers: Breeze, Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ndavis requested review of this revision.

REVISION SUMMARY
  As stated in https://doc.qt.io/qt-5/qweakpointer-obsolete.html :
  QWeakPointer::data() "Returns the value of the pointer being tracked by this 
QWeakPointer, without ensuring that it cannot get deleted. To have that 
guarantee, use toStrongRef(), which returns a QSharedPointer object."

REPOSITORY
  R31 Breeze

BRANCH
  replace-deprecated (branched from master)

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

AFFECTED FILES
  kdecoration/breezebutton.cpp
  kdecoration/breezedecoration.cpp
  kdecoration/breezedecoration.h
  kdecoration/breezesettingsprovider.cpp
  kdecoration/breezesizegrip.cpp

To: ndavis, #breeze, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart