Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-19 Thread David Rosca 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/
---

(Updated Feb. 19, 2016, 12:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Changes
---

Submitted with commit 82e172cc9e96bb530a591310ba6d8c294dc9f597 by David Rosca 
to branch master.


Bugs: 358849
http://bugs.kde.org/show_bug.cgi?id=358849


Repository: plasma-framework


Description
---

Regression from 344dbeb938

BUG: 358849


Diffs
-

  src/plasmaquick/appletquickitem.h 1e0174a 
  src/plasmaquick/appletquickitem.cpp 28f1eb5 
  src/plasmaquick/private/appletquickitem_p.h 1f99d2f 

Diff: https://git.reviewboard.kde.org/r/127073/diff/


Testing
---

Fixed for example plasmoid from bug.
Would be better if there was a way to fix it without adding another timer, 
though.


Thanks,

David Rosca

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-19 Thread David Rosca 


> On Feb. 19, 2016, 12:02 p.m., David Edmundson wrote:
> > src/plasmaquick/appletquickitem.cpp, lines 572-573
> > 
> >
> > can you get rid of this. 
> > 
> > I think it's trying to do what your patch does, only it doesn't work. 
> > (p.isValid() is always false)

Yes, I guess it tries to create the Layout, but it doesn't really work. I think 
it was valid for some items when I tested it, but it still didn't have any 
effect.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92552
---


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-19 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92552
---


Fix it, then Ship it!





src/plasmaquick/appletquickitem.cpp (lines 572 - 573)


can you get rid of this. 

I think it's trying to do what your patch does, only it doesn't work. 
(p.isValid() is always false)


- David Edmundson


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-15 Thread David Rosca 


> On Feb. 15, 2016, 4:29 p.m., David Edmundson wrote:
> > src/plasmaquick/appletquickitem.cpp, line 736
> > 
> >
> > if that's the case, filter on event->child()->metaObject.className()
> > 
> > rather than creating a new event for every single child

Unfortunately that doesn't work, it just shows as QObject.

>From doc:
>QEvent::ChildAdded and QEvent::ChildRemoved events are sent to objects when 
>children are added or removed. In both cases you can only rely on the child 
>being a QObject, or if isWidgetType() returns true, a QWidget. (This is 
>because, in the ChildAdded case, the child is not yet fully constructed, and 
>in the ChildRemoved case it might have been destructed already).


If the applet has attached Layout, it is generally the first and only child 
that will get through the conditionals and actually creates the timer.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92406
---


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-15 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92406
---




src/plasmaquick/appletquickitem.cpp (line 736)


if that's the case, filter on event->child()->metaObject.className()

rather than creating a new event for every single child


- David Edmundson


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-15 Thread Anthony Fieroni 


> On Фев. 15, 2016, 12:06 след обяд, Marco Martin wrote:
> > src/plasmaquick/appletquickitem.cpp, line 55
> > 
> >
> > why splitting it in a separate method if you don't call it from 
> > anywhere else?
> 
> David Rosca wrote:
> Because d-pointer is accessed from `childEvent` which is called from 
> AppletQuickItemPrivate constructor (`qmlObject` parented to `q`), and at that 
> point `d` is dangling pointer.

David Rosca +1, this is why, when you see *this* in contructor initialization 
list aways can be dangerous :)


- Anthony


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92387
---


On Фев. 14, 2016, 1:48 преди обяд, David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Фев. 14, 2016, 1:48 преди обяд)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-15 Thread Marco Martin 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92387
---


Fix it, then Ship it!




fine with an issue


src/plasmaquick/appletquickitem.cpp (line 55)


why splitting it in a separate method if you don't call it from anywhere 
else?


- Marco Martin


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-15 Thread Marco Martin 


> On Feb. 13, 2016, 11:33 p.m., Kai Uwe Broulik wrote:
> > src/plasmaquick/appletquickitem.h, line 149
> > 
> >
> > This looks binary incompatible.
> > 
> > Also, add Q_DECL_OVERRIDE
> 
> David Rosca wrote:
> Looks fine to me (according to policies on techbase), besides this is not 
> public header.

I'm not installing those headers (probably going to change) but we have to keep 
them BC anyways.
what is doing is reimplmenting an already existing virtual.
from the techbase page:

"As already explained, you can safely reimplement a virtual function defined in 
one of the base classes only if it is safe that the programs linked with the 
prior version call the implementation in the base class rather than the derived 
one."

so "should" be fine?


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92328
---


On Feb. 13, 2016, 11:48 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:48 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-13 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92328
---




src/plasmaquick/appletquickitem.h (line 149)


This looks binary incompatible.

Also, add Q_DECL_OVERRIDE



src/plasmaquick/appletquickitem.cpp (line 739)


The () are optional, dunno if we have a policy for this, Qt requires them 
anytime.


- Kai Uwe Broulik


On Feb. 13, 2016, 11:23 nachm., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:23 nachm.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-13 Thread David Rosca 


> On Feb. 13, 2016, 11:33 p.m., Kai Uwe Broulik wrote:
> > src/plasmaquick/appletquickitem.h, line 149
> > 
> >
> > This looks binary incompatible.
> > 
> > Also, add Q_DECL_OVERRIDE

Looks fine to me (according to policies on techbase), besides this is not 
public header.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/#review92328
---


On Feb. 13, 2016, 11:23 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127073/
> ---
> 
> (Updated Feb. 13, 2016, 11:23 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 358849
> http://bugs.kde.org/show_bug.cgi?id=358849
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Regression from 344dbeb938
> 
> BUG: 358849
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.h 1e0174a 
>   src/plasmaquick/appletquickitem.cpp 28f1eb5 
>   src/plasmaquick/private/appletquickitem_p.h 1f99d2f 
> 
> Diff: https://git.reviewboard.kde.org/r/127073/diff/
> 
> 
> Testing
> ---
> 
> Fixed for example plasmoid from bug.
> Would be better if there was a way to fix it without adding another timer, 
> though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127073: AppletQuickItem: Fix finding own attached layout

2016-02-13 Thread David Rosca 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127073/
---

(Updated Feb. 13, 2016, 11:48 p.m.)


Review request for Plasma.


Changes
---

Q_DECL_OVERRIDE


Bugs: 358849
http://bugs.kde.org/show_bug.cgi?id=358849


Repository: plasma-framework


Description
---

Regression from 344dbeb938

BUG: 358849


Diffs (updated)
-

  src/plasmaquick/appletquickitem.h 1e0174a 
  src/plasmaquick/appletquickitem.cpp 28f1eb5 
  src/plasmaquick/private/appletquickitem_p.h 1f99d2f 

Diff: https://git.reviewboard.kde.org/r/127073/diff/


Testing
---

Fixed for example plasmoid from bug.
Would be better if there was a way to fix it without adding another timer, 
though.


Thanks,

David Rosca

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel