D23927: Improve naming of KTitleWidget icon methods

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D23927#548607 , @kossebau wrote:
  
  > Any chance you could pik this up and complete? Thing is, the deprecated API 
is also still used internally,so anything but deprecated. Actually the new API 
calls the "deprecated" one.
  
  
  Hmpf, I mixed up signatures and added "#if 
KWIDGETSADDONS_BUILD_DEPRECATED_SINCE(5, 63)" to the wrong method, all fine 
there, sorry for that noise.
  
  Still, complain about the API consistency still holds, please consider fixing 
it :)

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: kossebau, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Any chance you could pik this up and complete? Thing is, the deprecated API 
is also still used internally,so anything but deprecated. Actually the new API 
calls the "deprecated" one.
  
  I just tried to adapt the code to the new deprecation macros (forgot that 
this change has slipped in since I did D24468 
 and landed it).
  But I failed due to this reason, as #if 
KWIDGETSADDONS_BUILD_DEPRECATED_SINCE(5, 63) cannot be used for 
KTitleWidget::setPixmap() implementation due to that.

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: kossebau, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D23927#537585 , @vkrause wrote:
  
  > Moving even more towards a QIcon rather than a QPixmap API is a good thing 
for sure, I agree on that. I don't think going back is justified here though, 
especially since the QIcon-based API improves high DPI scalability support. If 
it's decided to revert this nevertheless, please note that this has a ripple 
effect on the frameworks already depending on this.
  
  
  The commit & its message claims to "Improve naming of KTitleWidget icon 
methods", which it does not. It is now actually resulting in wrong expectations.
  
  Using a QIcon as pixmap provider engine surely makes sense. That is why the 
old API had those overloads. But the method does not set an icon, it sets the 
pixmap by immediately having the QIcon generating the pixmap and then 
discarding the QIcon object. Which means, if the title widget gets scaled e..g 
by setting another level, the API user had no gain by passing a QIcon object.
  
  Please add a respective disclaimer to the API documentation for now, as the 
current documentation is misleading, as I can tell you by experience :)

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: kossebau, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Volker Krause]

vkrause added a comment.


  In D23927#537583 , @kossebau wrote:
  
  > When going to adapt some code to follow the deprecation, I now look though 
at some inconsistency in the API:
  >
  > - KTitleWidget has a property "pixmap"
  > - the `setPixmap` methods all set this pixmap, with overloads for 
convenience
  > - `setIcon` is without a getter and a documented property, the relation to 
the pixmap property is unclear just by the API names
  >
  >   With this in mind, I fear I consider this change a regression in API 
quality. I'd propose to revert this, or go the full mile and make "icon" a real 
property, deprecating the pixmap one. What do you think?
  
  
  Moving even more towards a QIcon rather than a QPixmap API is a good thing 
for sure, I agree on that. I don't think going back is justified here though, 
especially since the QIcon-based API improves high DPI scalability support. If 
it's decided to revert this nevertheless, please note that this has a ripple 
effect on the frameworks already depending on this.

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: kossebau, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Friedrich W. H. Kossebau]

kossebau added a comment.


  When going to adapt some code to follow the deprecation, I now look though at 
some inconsistency in the API:
  
  - KTitleWidget has a property "pixmap"
  - the `setPixmap` methods all set this pixmap, with overloads for convenience
  - `setIcon` is without a getter and a documented property, the relation to 
the pixmap property is unclear just by the API names
  
  With this in mind, I fear I consider this change a regression in API quality. 
I'd propose to revert this, or go the full mile and make "icon" a real 
property, deprecating the pixmap one. What do you think?

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: kossebau, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Volker Krause]

vkrause added inline comments.

INLINE COMMENTS

> dhaumann wrote in ktitlewidget.h:45
> Could you also fix the documentation here? This is typically copy & pasted 
> later ;)

Sorry, race condition on the push :) Will do that in a separate commit.

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> ktitlewidget.h:45
>  titleWidget->setText(i18n("Title"));
>  titleWidget->setPixmap(QIcon::fromTheme("screen").pixmap(22, 22));
>   * @endcode

Could you also fix the documentation here? This is typically copy & pasted 
later ;)

REPOSITORY
  R236 KWidgetsAddons

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

To: vkrause, dfaure
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:10753c73409d: Improve naming of KTitleWidget icon methods 
(authored by vkrause).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23927?vs=65979=65983

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

AFFECTED FILES
  src/ktitlewidget.cpp
  src/ktitlewidget.h

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

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

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23927: Improve naming of KTitleWidget icon methods

[Volker Krause]

vkrause created this revision.
vkrause added a reviewer: dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vkrause requested review of this revision.

REVISION SUMMARY
  As suggested by David in D23899 .

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

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

AFFECTED FILES
  src/ktitlewidget.cpp
  src/ktitlewidget.h

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns