Review Request 119977: Mark setProgramIconName() as deprecated, it did not do anything

[Albert Astals Cid]

> On ago. 28, 2014, 5:02 p.m., Albert Astals Cid wrote:
> > Errr, what do you mean it doesn't do anything?
> > 
> > It's used http://lxr.kde.org/source/frameworks/kxmlgui/src/kaboutapplicationdialog.cpp?v=kf5-qt5
> > http://lxr.kde.org/source/frameworks/kjobwidgets/src/kuiserverjobtracker.cpp?v=kf5-qt5
> > http://lxr.kde.org/source/frameworks/kconfigwidgets/src/kstandardaction.cpp?v=kf5-qt5
> 
> Jonathan Riddell wrote:
>     Ug, so all these classes expect a property to be set in QApplication which may be set by KAboutData if you remember to call setProgramIconName(), but not used by any part of Qt?  Should these should be ported to QApplication::windowIcon?
> 
> Albert Astals Cid wrote:
>     Which QApplication property are you talking about?
>     
>     QString KAboutData::programIconName() const
>     {
>         return d->programIconName.isEmpty() ? componentName() : d->programIconName;
>     }
>     
>     KAboutData &KAboutData::setProgramIconName(const QString &iconName)
>     {
>         d->programIconName = iconName;
>         return *this;
>     }
> 
> Jonathan Riddell wrote:
>     yes that's the one, it sets a variable in KAboutData and sets a property in QCoreApplication.  But not the same property as is actually used by Qt to set the icon.  most app authors won't expect to have to call a second method to set the icon especially one that doesn't actually set the icon.
> 
> Albert Astals Cid wrote:
>     I think it's a much better idea to have a Q_COREAPP_STARTUP_FUNCTION in kguiaddons to set the icon based on that property and document that setProgramIconName will set the icon if linking to kguiaddons it than deprecating something that we use.
>     
>     Or even better implement support for applicationIconName in Qt itself.
> 
> Jonathan Riddell wrote:
>     Those both seems excessive.  This property adds no value to Qt, it should just be scrapped.
> 
> Albert Astals Cid wrote:
>     Adds value to ourselves, there's lots of places where we pass the application icon by name, not by QIcon as QApplication has, how are you going to change the use of 
>     http://lxr.kde.org/source/frameworks/kjobwidgets/src/kuiserverjobtracker.cpp?v=kf5-qt5
>     
>     To use a QIcon there?
> 
> Alexander Volkov wrote:
>     -QString programIconName = QCoreApplication::instance()->property("applicationIconName").toString();
>     +QString programIconName = QGuiApplication::windowIcon().name();
> 
> Albert Astals Cid wrote:
>     Problem is that nothing guarantees QIcon will have a name().
> 
> Jonathan Riddell wrote:
>     Nothing guarantees there is a applicationIconName property, especially since setting it doesn't actually set the icon so most app developers will not use it
> 
> Albert Astals Cid wrote:
>     Define "it doesn't actually set the icon", because it sets it for me in all the places i need, sure it doesn't set QGuiApplication::windowIcon(), but what's that useful for?
> 
> Jonathan Riddell wrote:
>     It seems flakey.  When I tried it with a template programme from kapptemplate it didn't work.  It did work for Harald but then after reinstalling he says it stopped working.  Best to avoid it I think and use the API which Qt provides for us.
> 
> Alexander Volkov wrote:
>     There is only one place in KF5 where you really need an icon name as a string: KUiServerJobTracker.
>     Yes, there is no guarantee that a programmer will set windowIcon with name(), but also there is no guarantee that he will use KAboutData::setProgramIconName.
>     So there can be only one. And the reason why it should be QGuiApplication::setWindowIcon() is that there is no reasonable way to set window icon from kcoreaddons.
>     You have proposed to do it in kguiaddons but what if I don't want to use kguiaddons? Why do I have to care if my app is using kguiaddons somehow?
>     Or write:
>     KAboutData::setProgramIconName("mysuperapp");
>     QGuiApplication::setWindowIcon(QIcon::fromTheme("mysuperapp"));
>     and then think "what was that?"
> 
> Alexander Volkov wrote:
>     And I suppose there should be an intermediate commit for KUiServerJobTracker.
>     It should use QGuiApplication::windowIcon().name() when "applicationIconName" property is empty.

Ok, i withdraw my objection.


- Albert


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


On ago. 28, 2014, 4:39 p.m., Jonathan Riddell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119977/
> -----------------------------------------------------------
> 
> (Updated ago. 28, 2014, 4:39 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> Mark setProgramIconName() as deprecated, it did not do anything since
> kcoreaddons no longer uses QtGui so it can not set the icon.
> 
> BUG: 337938
> 
> 
> Diffs
> -----
> 
>   src/lib/kaboutdata.h 2d8bd5645150a57739e94f9f71b112b20ec0e01f 
>   src/lib/kaboutdata.cpp 5ad81de814c123f50b17bb542331459e15649f4b 
> 
> Diff: https://git.reviewboard.kde.org/r/119977/diff/
> 
> 
> Testing
> -------
> 
> compiled a program made with kapptemplate 4.14.1 which uses this function, successfully gives a warning during compile
> 
> 
> Thanks,
> 
> Jonathan Riddell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140902/7be57d1b/attachment.html>