Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-15 Thread Aleix Pol Gonzalez

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

(Updated Oct. 15, 2014, 10:50 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.


Repository: kdeclarative


Description
---

Moves the caching types used in Plasma Workspace into a QuickAddons submodule.

Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
item. Uses the same strategies used in Plasma Framework for caching the 
textures.


Diffs
-

  src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
  src/quickaddons/CMakeLists.txt PRE-CREATION 
  src/quickaddons/imagetexturescache.h PRE-CREATION 
  src/quickaddons/imagetexturescache.cpp PRE-CREATION 
  src/quickaddons/managedtexturenode.h PRE-CREATION 
  src/quickaddons/managedtexturenode.cpp PRE-CREATION 
  tests/qiconitem_test.qml PRE-CREATION 
  src/CMakeLists.txt eb0dfd3 

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


Testing
---

I added some manual test (that was impossible to run before the patch). Also 
tested it in KRunner and Muon Discover, which use the component. Everything 
still works.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-14 Thread Vishesh Handa


 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer
 
 Aleix Pol Gonzalez wrote:
 I don't think it's a good idea to add a d-pointer there. It's a class to 
 encapsulate the object, I don't see why we should store more things in there.
 
 If needs changed, then I'd suggest to create another class.
 
 Marco Martin wrote:
 if it's exported, i prefer a dpointer anyways
 
 Aleix Pol Gonzalez wrote:
 Can we please discuss this? I really don't think we want to be so cheap 
 when it comes to memory usage. This means that each node in the scene will 
 take a payload for no much reason.

It's not only about memory usage. The memory gets defragmented as well.

Also, maybe considering this class is so small, you may want to inline the 
function definitions.


- Vishesh


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-14 Thread Vishesh Handa

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



src/quickaddons/imagetexturescache.h
https://git.reviewboard.kde.org/r/120550/#comment47668

Strange indentation.


- Vishesh Handa


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-14 Thread Marco Martin


 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer
 
 Aleix Pol Gonzalez wrote:
 I don't think it's a good idea to add a d-pointer there. It's a class to 
 encapsulate the object, I don't see why we should store more things in there.
 
 If needs changed, then I'd suggest to create another class.
 
 Marco Martin wrote:
 if it's exported, i prefer a dpointer anyways
 
 Aleix Pol Gonzalez wrote:
 Can we please discuss this? I really don't think we want to be so cheap 
 when it comes to memory usage. This means that each node in the scene will 
 take a payload for no much reason.
 
 Vishesh Handa wrote:
 It's not only about memory usage. The memory gets defragmented as well.
 
 Also, maybe considering this class is so small, you may want to inline 
 the function definitions.

heap fragmentation may be a valid argument, yeah


- Marco


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Aleix Pol Gonzalez

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

(Updated Oct. 13, 2014, 11:13 a.m.)


Review request for KDE Frameworks, David Edmundson and Marco Martin.


Changes
---

Added API documentation.
Moved ManagedTextureNode into a separate file.


Repository: kdeclarative


Description
---

Moves the caching types used in Plasma Workspace into a QuickAddons submodule.

Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
item. Uses the same strategies used in Plasma Framework for caching the 
textures.


Diffs (updated)
-

  src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
  src/quickaddons/CMakeLists.txt PRE-CREATION 
  src/quickaddons/imagetexturescache.h PRE-CREATION 
  src/quickaddons/imagetexturescache.cpp PRE-CREATION 
  src/quickaddons/managedtexturenode.h PRE-CREATION 
  src/quickaddons/managedtexturenode.cpp PRE-CREATION 
  tests/qiconitem_test.qml PRE-CREATION 
  src/CMakeLists.txt eb0dfd3 

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


Testing
---

I added some manual test (that was impossible to run before the patch). Also 
tested it in KRunner and Muon Discover, which use the component. Everything 
still works.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Marco Martin

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



src/quickaddons/imagetexturescache.h
https://git.reviewboard.kde.org/r/120550/#comment47603

would like some apidocs: at the beginning of the class some doc on why and 
how use this class, and just a bit of apidoc on loadTexture()


- Marco Martin


On Oct. 13, 2014, 11:13 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 11:13 a.m.)
 
 
 Review request for KDE Frameworks, David Edmundson and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Marco Martin

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

Ship it!


apart managedtexturenode needing a dpointer, looks good


src/quickaddons/managedtexturenode.h
https://git.reviewboard.kde.org/r/120550/#comment47616

even if will always remain just this member, just to me sure, it should be 
in a dpointer


- Marco Martin


On Oct. 13, 2014, 11:13 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 11:13 a.m.)
 
 
 Review request for KDE Frameworks, David Edmundson and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Aleix Pol Gonzalez


 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer

I don't think it's a good idea to add a d-pointer there. It's a class to 
encapsulate the object, I don't see why we should store more things in there.

If needs changed, then I'd suggest to create another class.


- Aleix


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


On Oct. 13, 2014, 11:13 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 11:13 a.m.)
 
 
 Review request for KDE Frameworks, David Edmundson and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Aleix Pol Gonzalez

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

(Updated Oct. 13, 2014, 5:54 p.m.)


Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.


Repository: kdeclarative


Description
---

Moves the caching types used in Plasma Workspace into a QuickAddons submodule.

Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
item. Uses the same strategies used in Plasma Framework for caching the 
textures.


Diffs
-

  src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
  src/quickaddons/CMakeLists.txt PRE-CREATION 
  src/quickaddons/imagetexturescache.h PRE-CREATION 
  src/quickaddons/imagetexturescache.cpp PRE-CREATION 
  src/quickaddons/managedtexturenode.h PRE-CREATION 
  src/quickaddons/managedtexturenode.cpp PRE-CREATION 
  tests/qiconitem_test.qml PRE-CREATION 
  src/CMakeLists.txt eb0dfd3 

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


Testing
---

I added some manual test (that was impossible to run before the patch). Also 
tested it in KRunner and Muon Discover, which use the component. Everything 
still works.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Marco Martin


 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer
 
 Aleix Pol Gonzalez wrote:
 I don't think it's a good idea to add a d-pointer there. It's a class to 
 encapsulate the object, I don't see why we should store more things in there.
 
 If needs changed, then I'd suggest to create another class.

if it's exported, i prefer a dpointer anyways


- Marco


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-13 Thread Aleix Pol Gonzalez


 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer
 
 Aleix Pol Gonzalez wrote:
 I don't think it's a good idea to add a d-pointer there. It's a class to 
 encapsulate the object, I don't see why we should store more things in there.
 
 If needs changed, then I'd suggest to create another class.
 
 Marco Martin wrote:
 if it's exported, i prefer a dpointer anyways

Can we please discuss this? I really don't think we want to be so cheap when it 
comes to memory usage. This means that each node in the scene will take a 
payload for no much reason.


- Aleix


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

2014-10-12 Thread Aleix Pol Gonzalez

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

Review request for KDE Frameworks, David Edmundson and Marco Martin.


Repository: kdeclarative


Description
---

Moves the caching types used in Plasma Workspace into a QuickAddons submodule.

Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
item. Uses the same strategies used in Plasma Framework for caching the 
textures.


Diffs
-

  src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
  src/quickaddons/CMakeLists.txt PRE-CREATION 
  src/quickaddons/imagetexturescache.h PRE-CREATION 
  src/quickaddons/imagetexturescache.cpp PRE-CREATION 
  tests/qiconitem_test.qml PRE-CREATION 
  src/CMakeLists.txt eb0dfd3 
  src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
  src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 

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


Testing
---

I added some manual test (that was impossible to run before the patch). Also 
tested it in KRunner and Muon Discover, which use the component. Everything 
still works.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel