Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 12, 2016, 12:55 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Olivier Goffart.


Changes
---

Submitted with commit 0abf1b7a148cf6b27caea01a329631e0f1daa983 by David Rosca 
to branch master.


Bugs: 365130
https://bugs.kde.org/show_bug.cgi?id=365130


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[Anthony Fieroni]

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



Is this same as https://bugs.kde.org/show_bug.cgi?id=365355

- Anthony Fieroni


On July 8, 2016, 3:24 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 8, 2016, 3:24 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Bugs: 365130
> https://bugs.kde.org/show_bug.cgi?id=365130
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Faure]

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


Ship it!




Ship It!

- David Faure


On July 8, 2016, 12:24 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 8, 2016, 12:24 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Bugs: 365130
> https://bugs.kde.org/show_bug.cgi?id=365130
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[Olivier Churlaud]

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



Compiled and tested on archlinux: it fixed 
https://sourceforge.net/p/texstudio/bugs/1837/ which was related to bug 365130.

Not completely though, as the icon of the app is not shown in the taskbar (but 
it might be another bug). Looking forward for the release.

- Olivier Churlaud


On July 8, 2016, 2:24 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 8, 2016, 2:24 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Bugs: 365130
> https://bugs.kde.org/show_bug.cgi?id=365130
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 8, 2016, 12:24 p.m.)


Review request for KDE Frameworks and Olivier Goffart.


Bugs: 365130
https://bugs.kde.org/show_bug.cgi?id=365130


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[Olivier Churlaud]

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



I think this would fix that as well ? 
https://bugs.kde.org/show_bug.cgi?id=365130

- Olivier Churlaud


On July 8, 2016, 12:52 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 8, 2016, 12:52 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 8, 2016, 10:52 a.m.)


Review request for KDE Frameworks and Olivier Goffart.


Changes
---

QIconEngine::IsNullHook value is 3, use it with Qt < 5.7


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs (updated)
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[Olivier Goffart]

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




src/kiconengine.cpp (line 164)


Ideally, one would want that even if compiled with older Qt, this works as 
Qt is upgraded.

The way to do that would be to hardcode the numerical value of 
QIconEngine::IsNullHook when compiling with older Qt.


- Olivier Goffart


On July 7, 2016, 9:55 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 7, 2016, 9:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

> On July 7, 2016, 9:59 p.m., David Faure wrote:
> > autotests/kiconengine_unittest.cpp, line 69
> > 
> >
> > What's the problem if name() returns "invalid-icon-name" here?

```
bool QIcon::hasThemeIcon(const QString &name)
{
QIcon icon = fromTheme(name);

return icon.name() == name;
}
```

And QIcon::name() returns just the QIconEngine::iconName(). Qt's default icon 
engine returns empty iconName() when the icon cannot be found.


> On July 7, 2016, 9:59 p.m., David Faure wrote:
> > src/kiconengine.cpp, line 119
> > 
> >
> > I'm worried that this might do a (slow?) lookup every time it's called?

It's only called from QIcon::name() and QIcon::hasThemeIcon(), so it shouldn't 
be problem. Maybe the lookup can be done only once and mIconName emptied when 
the icon is not found, if it would be needed?


- David


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


On July 7, 2016, 9:55 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 7, 2016, 9:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Faure]

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



Thanks for looking into this!


autotests/kiconengine_unittest.cpp (line 69)


What's the problem if name() returns "invalid-icon-name" here?



src/kiconengine.cpp (line 119)


I'm worried that this might do a (slow?) lookup every time it's called?


- David Faure


On July 7, 2016, 9:55 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 7, 2016, 9:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

Review request for KDE Frameworks.


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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