Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[David Rosca]

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

(Updated July 12, 2016, 8:06 a.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Changes
---

Submitted with commit 9070f461bba483350c91528336a73ce68e41bde3 by David Rosca 
to branch master.


Repository: plasma-framework


Description
---

Clearing the pixmap is actually wrong, because in some cases the 
IconItem::updatePolish() is not called when changing visibility.


Diffs
-

  src/declarativeimports/core/iconitem.h e73829b 
  src/declarativeimports/core/iconitem.cpp 4f41b3b 

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


Testing
---

Tests still pass.

The bug (no icon rendered) can be sometimes experienced in task manager volume 
controls and systray popup pin button. This patch fixes it.


Thanks,

David Rosca

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[Marco Martin]

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


Ship it!




Ship It!

- Marco Martin


On July 9, 2016, 3:28 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated July 9, 2016, 3:28 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[Anthony Fieroni]

> On Юли 11, 2016, 11:10 преди обяд, Anthony Fieroni wrote:
> > src/declarativeimports/core/iconitem.cpp, line 483
> > 
> >
> > Or here to be
> > m_oldIconPixmap = QPixmap();
> > ?
> 
> David Rosca wrote:
> No, loadPixmap() does m_oldIconPixmap = m_iconPixmap, so this change 
> would have no effect.
> 
> Anthony Fieroni wrote:
> m_iconPixmap stays valid i.e. not be invisible, no?

Oh yes, i don't saw that line.
m_oldIconPixmap = m_iconPixmap


- Anthony


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


On Юли 9, 2016, 6:28 след обяд, David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated Юли 9, 2016, 6:28 след обяд)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[Anthony Fieroni]

> On Юли 11, 2016, 11:10 преди обяд, Anthony Fieroni wrote:
> > src/declarativeimports/core/iconitem.cpp, line 483
> > 
> >
> > Or here to be
> > m_oldIconPixmap = QPixmap();
> > ?
> 
> David Rosca wrote:
> No, loadPixmap() does m_oldIconPixmap = m_iconPixmap, so this change 
> would have no effect.

m_iconPixmap stays valid i.e. not be invisible, no?


- Anthony


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


On Юли 9, 2016, 6:28 след обяд, David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated Юли 9, 2016, 6:28 след обяд)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[David Rosca]

> On July 11, 2016, 7:52 a.m., Anthony Fieroni wrote:
> > If only remove clearing, animation still present?

There is no change in behavior. It still disables the animation when going from 
invisible to visible, it just fixes the bug with empty icon.


- David


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


On July 9, 2016, 3:28 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated July 9, 2016, 3:28 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[David Rosca]

> On July 11, 2016, 8:10 a.m., Anthony Fieroni wrote:
> > src/declarativeimports/core/iconitem.cpp, line 483
> > 
> >
> > Or here to be
> > m_oldIconPixmap = QPixmap();
> > ?

No, loadPixmap() does m_oldIconPixmap = m_iconPixmap, so this change would have 
no effect.


- David


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


On July 9, 2016, 3:28 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated July 9, 2016, 3:28 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[Anthony Fieroni]

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



If only remove clearing, animation still present?

- Anthony Fieroni


On Юли 9, 2016, 6:28 след обяд, David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128409/
> ---
> 
> (Updated Юли 9, 2016, 6:28 след обяд)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Clearing the pixmap is actually wrong, because in some cases the 
> IconItem::updatePolish() is not called when changing visibility.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h e73829b 
>   src/declarativeimports/core/iconitem.cpp 4f41b3b 
> 
> Diff: https://git.reviewboard.kde.org/r/128409/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass.
> 
> The bug (no icon rendered) can be sometimes experienced in task manager 
> volume controls and systray popup pin button. This patch fixes it.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Review Request 128409: IconItem: Use better approach to disable animation when going from invisible to visible

[David Rosca]

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

Review request for Plasma.


Repository: plasma-framework


Description
---

Clearing the pixmap is actually wrong, because in some cases the 
IconItem::updatePolish() is not called when changing visibility.


Diffs
-

  src/declarativeimports/core/iconitem.h e73829b 
  src/declarativeimports/core/iconitem.cpp 4f41b3b 

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


Testing
---

Tests still pass.

The bug (no icon rendered) can be sometimes experienced in task manager volume 
controls and systray popup pin button. This patch fixes it.


Thanks,

David Rosca

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