Replacing the KIcon type with a factory method and is frameworks a good idea at all?

Tue Sep 6 13:39:35 BST 2011

We've just had a long discussion about the future of KIcon in APIs,
which led into a discussion about what we're doing in frameworks at
all and why.

Several people don't think refactoring kdelibs into independent
frameworks is a good thing. Do we need to decide if it's worth it?

I'm posting the log for archival reasons mostly. There are several
misunderstandings in it, but those are mostly untangled later on.

The content of http://paste.kde.org/118765/ is:

namespace KIcon
{
  QIcon fromTheme(const QString &name)
  {
    return QIcon(new KIconEngine(...));
  }
}

Where the content of the function is intended to be 'what KIcon ctor does'

[13:08:26] <steveire> ogoffart: KIcon("foo") should be replaced with
QIcon::fromTheme("foo"), right?
[13:08:37] <steveire> How does QIcon::fromTheme work?>
[13:08:45] <ogoffart> steveire: yes.
[13:08:48] <steveire> Does it use a kde pluing?
[13:08:54] <steveire> plugin*
[13:09:01] <ogoffart> steveire: it has its own implementation of the
xdg icon spec
[13:09:15] <ogoffart> steveire: it uses a plugin only to know which
theme to load.
[13:09:27] <ogoffart> that is, to read kde settings.
[13:10:06] <steveire> So is KIconEngine also obsolete?
[13:10:24] <steveire> I guess this stuff might be in the spreadsheet from randa
[13:11:37] <ogoffart> steveire: yes.
[13:11:43] <steveire> The spreadsheet is mostly silent on that topic.
[13:11:54] <steveire> It just says Qt needs overlay support to replace KIcon.
[13:13:34] <ogoffart> steveire: but we discussed at the contributor
summit, and aseigo and dfaure said that nothing really use the
overlay. (Or if they use overlay, it is not something that is in the
tier1, 2 or 3, so this could go into a kde specific library of even in
the application itself)
[13:13:47] <steveire> Right.
[13:13:55] <dfaure> "nothing uses overlays" is not true, kfileitem.cpp does
[13:14:16] <dfaure> but I suppose we could develop something that uses
QIcon::fromTheme twice and combines the results
[13:14:22] <ogoffart> yes
[13:15:17] <-> 45PAALFRM is now known as miroslav
[13:17:35] <steveire> Why does mpyne recommend not using the official Qt?
[13:17:58] <DxSadEagle> does QIcon::fromTheme implement appropriate
appropriate caching & lazy loading?
[13:20:23] <pinotree> does QIcon::fromTheme() read also in the icon
theme of the component data?
[13:22:24] <pinotree> can QIcon::fromTheme() take a different
component data and/or icon loader to read icons from?
[13:23:16] <dfaure> the icon theme of the component data? a plugin
coming with its own icon theme?
[13:23:49] <pinotree> with own icons in a icon-theme tree under its
$datadir/icons
[13:24:15] <pinotree> this greatly avoids polluting the global icon
theme with app- or plugin-specific icons
[13:24:42] <dfaure> ah, "own icons", not a replacement icon theme.
[13:25:05] <pinotree> well, nothing forbids you to put a whole icon
theme there :)
[13:25:20] <dfaure> a feature actually used by.... ?
[13:25:37] <pinotree> ls -d /usr/share/kde4/apps/*/icons
[13:28:42] <-> atomo_yawn is now known as atomopawn
[13:29:56] <dfaure> ah I see, even if you don't have a full icon
theme, it's still organized like one
[13:30:31] <dfaure> ogoffart: should QIcon::fromTheme take an
additional optional argument with a base directory for the
app-specific icon theme?
[13:30:58] <steveire> I presume that stuff is built into the KIconEngine?
[13:31:20] <steveire> I could create KIcon::fromTheme which uses that,
but still use QIcon in the API
[13:31:43] <ogoffart> dfaure: you can only set the theme globaly with
QIcon::setThemeName or something
[13:32:02] <ogoffart> dfaure: then again, the default theme name is
fetch from the KDE config while running on KDE
[13:32:20] <dfaure> steveire: you wouldn't have access to the icon
theme loader in qt to add paths to it
[13:32:41] <dfaure> ogoffart: you didn't read the above discussion it
seems ;) This is about /usr/share/kde4/apps/kgpg/icons/hicolor/
[13:33:02] <dfaure> the icon theme name is hicolor just fine, the
issue is "for this specific icon, I want to also look into
/usr/share/kde4/apps/kgpg/icons
[13:33:37] <ogoffart> DxSadEagle: it has per-application cache
[13:36:13] <ogoffart> ah
[13:36:18] <ogoffart> dfaure: there is QIcon::setThemeSearchPaths
[13:36:44] <dfaure> ah.
[13:36:48] <ogoffart> again this is application global, but one can
add /usr/share/kde4/apps/kgpg there
[13:36:55] <ogoffart> in kgpg
[13:37:04] <dfaure> indeed.
[13:37:25] <steveire> '<dfaure> steveire: you wouldn't have access to
the icon theme loader in qt to add paths to it' I don't understand
this I think
[13:37:27] <ogoffart> the default is also fetch from the plugin
[13:38:00] <steveire> Looking at the implementation of KIcon it looks
like an empty class with a magic constructor
[13:38:01] <DxSadEagle> that's too global...
[13:38:05] <dfaure> steveire: QIcon::fromTheme uses a qt-internal-iconloader
[13:38:30] <steveire> Let me look at the code again :)
[13:38:36] <dfaure> steveire: KIcon uses KIconEngine uses KIconLoader,
but the discussion here is about porting that to QIcon::fromTheme....
[13:39:08] <dfaure> DxSadEagle: yeah it's too global for libs to add
their own search paths. But looking at /usr/share/kde4/apps/*/icons,
it seems only apps do that.
[13:39:15] <dfaure> libs just use icons from the actual icon theme...
[13:39:32] <steveire> dfaure: Yes. The point I'm trying to make is
only about using KIcon vs QIcon in the APIs
[13:39:33] <DxSadEagle> dfaure: and none of the things in there are parts?
[13:40:00] <steveire> KIcon::fromTheme would just do what the KIcon ctor does
[13:40:14] <dfaure> I don't understand the point of a "KIcon::fromTheme"
[13:40:19] <ogoffart> dfaure: so the plugin says the default search
path is:   KGlobal::dirs()->resourceDirs("icon");
[13:40:20] <dfaure> it's indeed what the ctor does ;)
[13:40:34] <dfaure> DxSadEagle: no, but that's a good point.
[13:40:44] <DxSadEagle> dfaure: even okular?
[13:40:58] <winterz> daitheflu, bcooksley-away:  we don't generate the
PyKDE dox.  Simon emails me a tarball containing the PyKDE dox and I
hang it up on api.kde.org.  So there are 2 problems:  1) we need to
copy over the PyKDE dox from the old EBN for the versions we have 2)
we need to nag Simon to send us PyKDE dox updates for KDE 4.7, KDE
4.6... maybe some others
[13:41:21] <dfaure> ogoffart: the plugin has no idea if we're about to
load an icon from a part or from an app
[13:41:37] <dfaure> DxSadEagle: there's only a
/usr/share/kde4/apps/okular/icons/hicolor/16x16/apps/okular-gv.png
[13:41:46] <dfaure> not sure if that's loaded by okular app or part
[13:41:51] <ogoffart> but how does KIcon does?
[13:42:34] <dfaure> ogoffart: by being passed the pointer to a
KIconLoader in the ctor
[13:42:35] <steveire> dfaure: From what I understand, KIcon::fromTheme
would allow us to take KIcon out of our APIs, and would have all the
features you're discussin glike app specific icons would already work
[13:43:01] <steveire> But if QIcon::fromTheme can be made to do that
too, maybe that's ok
[13:43:04] <dfaure> steveire: you can already use QIcon in the API,
and let people write KIcon("foo") when calling the API, no need for
KIcon::fromTheme (very redundant)
[13:43:15] <DxSadEagle> dfaure: I recall it being able to also load
stuff from plain app/kfoo/pics
[13:43:47] <dfaure> DxSadEagle: yes, pics. That's different, it's not
organized like a theme, you only need a QStandardPaths +
"kfoo/pics/foo.png" and pass the full path to QIcon
[13:45:28] <steveire> I guess.
[13:45:34] <DxSadEagle> dfaure: just that can currently be loaded by
loadIcon, too.
[13:46:19] <steveire> What I'd really like is to put some factory
method in a namespace instead of having a static method in the class
(no one can create an instance of a namespace)
[13:47:03] <DxSadEagle> and you're suggestiong while we're discussing
how global is insufficient? ;-)
[13:47:04] -*- dfaure isn't sure which namespace/class steveire is talking about
[13:49:15] <winterz> daitheflu, bcooksley-away: my sent-mail folder
says I sent such a nag to Simon on 31 July 2011.  and got no response.
 i'll re-nag one more time
[13:52:10] <ervin> dfaure: I predict KIcon being used in the api if
the type is still there though ;)
[13:52:29] <dfaure> ervin: not if the tiers prevent it...
[13:52:46] <ervin> dfaure: it's better if there's no KIcon type really
[13:53:01] <ervin> dfaure: ok, sorry, thought as well about apps internals
[13:53:14] <ervin> in doubt they'll go for K* types always
[13:53:42] <ervin> we'd just make it harder for app code to move into libs IMO
[13:53:49] <ervin> since they'd have to adapt to the type
[13:54:06] <ervin> a convenience ctor doesn't justify the existence of
a type imho
[13:54:15] <ogoffart> ervin: you rather have a KIcon macro?  :-)
[13:54:36] <ervin> ogoffart: I prefer steveire's proposal at that point
[13:54:47] <ervin> namespace with a factory method
[13:55:45] <steveire> http://paste.kde.org/118765/
[13:56:12] <steveire> Just for clarity :)
[13:57:05] <dfaure> breaks compat big way, because we can't have KIcon
as both a class and a namespace
[13:57:33] <dfaure> so every single piece of code currently doing
KIcon("foo") will break.
[13:57:59] <dfaure> I'm all for making the kdelibs-frameworks API as
clean as possible, but at the same time we should make the porting
minimal, just like Qt5 promises minimal SIC.
[13:58:33] <steveire> We can 1) Use Bertjans porting to for that
(there is going to be porting and a porting tool anyway) or 2) Use a
namespace of a different name and keep the KIcon class
[13:58:34] <ervin> I'm fine with a different namespace name
[13:58:49] <steveire> I prefer option 1
[13:58:58] <dfaure> "We" can use a complex tool, but not every kde app
author out there.
[13:58:59] <DxSadEagle> steveire: porting tool, hihi. Were you around
for KDE4 port? ;-)
[13:59:03] <ervin> the thing is the namespace proposed would live in
the ui integration lib, KIcon really has to move to a kde4support
[13:59:06] <dfaure> So I much prefer option 2.
[13:59:12] <steveire> We have yet to see what the realities of Qt 5
porting will be
[13:59:27] <steveire> DxSadEagle: Nope, I wasn't
[13:59:41] <dfaure> steveire: yes, the future is always a bit
uncertain. But that's no reason to make it worse.
[13:59:47] <ervin> agreed
[14:00:12] <steveire> Meh. Ok. I don't feel so strongly about it to argue it.
[14:01:02] <steveire> ervin: solid unit tests require kio. Can I
comment those tests out until either the tests are ported or kio is in
a tier somewhere?
[14:01:10] <pinotree> i just wonder why is using k-api so bad these days
[14:01:24] <steveire> pinotree: It creates a walled garden
[14:01:33] <ervin> steveire: hm, which one?
[14:02:00] <steveire> If my api is MyClass::someMethod(QUrl), a Qt
developer can use it. If it is MyClass::someMethod(KUrl) they can't
[14:02:12] <steveire> ervin: The last one in the CMakeLists file
[14:02:13] <ervin> steveire: might be something from the past, tried
to build without linking to it?
[14:02:28] <pinotree> steveire: so what's the problem with kicon? it's
a qicon, no?
[14:02:29] <steveire> I want to make solid part of the new build system
[14:02:41] <steveire> pinotree: It is indeed a QIcon.
[14:02:56] <steveire> MyClass::someMethod(QIcon) vs
MyClass::someMethod(KIcon) Same issue
[14:03:13] <pinotree> you're picking a different issue
[14:03:31] <pinotree> kicon is a qicon with the added points of using
kde's icon loader, what's bad in it that you wanted to dismantle it?
[14:03:57] <ervin> steveire: give it a try without linking to kio, if
that doesn't work then yes just disable it for the time being, it's
not an autotest anyway
[14:04:05] <steveire> ervin: Ok
[14:04:19] <daitheflu> winterz: thank you for the information :)
[14:04:19] <steveire> mbensi is solid maintainer now, right?
[14:04:28] <ervin> steveire: nope
[14:04:34] <ervin> steveire: afiestas is
[14:04:38] <steveire> Ah, ok
[14:04:48] <ervin> steveire: mbensi (aka [Nef]) maintains one of the
libsolid backends
[14:05:08] <steveire> pinotree: I'm not sure how I'm picking a different issue
[14:05:18] <pinotree> <steveire> MyClass::someMethod(QIcon) vs
MyClass::someMethod(KIcon) Same issue
[14:05:22] <steveire> Right.
[14:05:23] <-> atomopawn is now known as atomo_yawn
[14:05:32] <steveire> For me the issue is having KIcon in the api
[14:05:49] <pinotree> as parameter/return value or as class in general?
[14:05:49] <steveire> If that's not the issue, then what is? How is it
different?
[14:05:56] <steveire> pinotree: Yes
[14:06:03] <steveire> Sorry
[14:06:07] <pinotree> "A or B?" "yes"
[14:06:09] <steveire> Parameter or return value
[14:06:44] <pinotree> then i don't see why kicon("...") usages should
be replaced with qicon::fromtheme()
[14:06:55] <pinotree> or why kicon as class should go away
[14:07:13] <steveire> If KIcon remains as a non-deprecated class, then
use creep will make it re-appear in APIs
[14:07:39] <pinotree> oh please
[14:07:43] <steveire> I'm also not certain replacing it with
QIcon::fromTheme is the right thing to do anymore
[14:08:12] <steveire> You think my use-creep argument is ridiculus?
[14:08:31] <steveire> ridiculous*
[14:08:32] <pinotree> it is ridicolous if you use it as argument for
dismantling kicon
[14:08:48] <steveire> Why?
[14:08:54] <steveire> What is dismantling?
[14:09:13] <pinotree> ok, s/dismantling/removing/
[14:09:50] <ervin> well, on top of that, an extra type can't be
justified only by having ctors in its API really
[14:10:34] <steveire> What is the disadvantage of removing it? It is a
porting step? Is that the only one? Is that big enough to justify not
removing/deprecating it?
[14:11:11] <pinotree> steveire: let me reverse the question: why do
you want to remove kicon as class, instead of just use qicon in public
api?
[14:11:32] <steveire> If it remains as a class people will use it as a
class and put it in APIs
[14:11:47] <steveire> That creates a walled garden
[14:12:04] <ervin> and again, design wise it made no sense to create
it in the first place
[14:12:19] <steveire> It means that developers using Qt can't use our
stuff without using 'all' of our stuff
[14:12:23] <pinotree> so kde apps cannot use kde classes because it
creates a walled garden for qt developers?
[14:12:26] <ervin> this inheriting + ctor fetish is just so 90s :-)
[14:12:37] <steveire> ervin: I fully agree
[14:13:03] <steveire> I really hate the 'we must inherit' stuff
[14:13:20] <steveire> pinotree: I'm talking about frameworks
[14:13:26] <pinotree> i really hate the 'we must use only classes in qt' stuff
[14:13:43] <steveire> If kde apps feel good about using KIcon then so
be it. But there's no reason for it
[14:13:55] <-> Quintasan_ is now known as Quintasan
[14:14:03] <pinotree> the fact that *you* see no good reasons, does
not mean there aren't
[14:14:13] <steveire> Do you see any?
[14:14:39] <pinotree> i already said that almost one hour ago
[14:14:47] <steveire> I don't propose removing KIcon completely, just
removing it from APIs and deprecating it
[14:15:05] <pinotree> don't deprecate it, it is useful for kde stuff
[14:15:09] <ervin> pinotree: come on, stop painting things in black,
kicon is first and foremost a design mistake as api, we've the
opportunity to fix it
[14:15:13] <steveire> pinotree: I've also repeated myself on several
points. Enlighten me again please
[14:15:30] <ervin> you'd get the exact same service with factory methods
[14:16:13] <pinotree> ervin: thanks for portraiting me as i am not,
i'll stop here.
[14:16:52] <ervin> pinotree: well I felt ignored, or at least the
design point on the inheritance there :)
[14:17:10] <pinotree> i always find interesting when people jump from
technical arguments to "you suck"-like personal arguments
[14:17:11] <steveire> So "it's useful for kde stuff" is the reason for
keeping it?
[14:17:36] <pinotree> steveire: read again what i said. qicon as it is
it is not enough
[14:17:40] <svuorela> KIcon has, as I've heard one big advantage.
cross-application caching.
[14:17:54] <steveire> Where? In KIconEngine?
[14:18:01] <pinotree> <pinotree> does QIcon::fromTheme() read also in
the icon theme of the component data?
[14:18:06] <pinotree> → no
[14:18:09] <pinotree> <pinotree> can QIcon::fromTheme() take a
different component data and/or icon loader to read icons from?
[14:18:09] <steveire> If we create a QIcon witha KIconEngine, then we
get the same feature.
[14:18:12] <pinotree> → no
[14:18:14] <dfaure> I think there's a misunderstanding here. Steve
isn't proposing to ditch KIconLoader, just to replace KIcon("foo")
with KIconSomething::fromTheme("foo")
[14:18:15] <steveire> Read kicon.cpp
[14:18:33] <dfaure> pinotree: so there is no feature loss in this proposal
[14:18:33] <pinotree> dfaure: isn't that worse?
[14:18:37] <steveire> Or rather, SomeNamespace::someMethod("foo")
[14:18:51] <dfaure> pinotree: only an API change, where there is no
QIcon subclass, so the only way to pass an icon to a method is const
QIcon& icon
[14:18:54] <DxSadEagle> like someMehtod  = SmallIconSet? ;-)
[14:19:11] <dfaure> DxSadEagle: haha ;) kde2 is back ;)
[14:19:46] -*- svuorela isn't old enough to catch david and maksim's jokes.
[14:19:55] <montel_> :)
[14:19:57] <dfaure> svuorela: look at kiconloader.h ;)
[14:19:58] <steveire> pinotree: I'm not proposing replacing KIcon with
QIcon::fromTheme.
[14:20:09] <pinotree> dfaure: if all people claimed that qt5 and kde5
are "mostly SC", then i don't see how such a unreasoned change would
comply to that
[14:20:23] <dfaure> pinotree: mostly SC means keeping KIcon in a layer
that apps can use
[14:20:27] <pinotree> steveire: <steveire> ogoffart: KIcon("foo")
should be replaced with QIcon::fromTheme("foo"), right? ← one hour ago
[14:20:49] <steveire> pinotree: There has been an hour of discussion
since then, and even a pastebinned API ?
[14:21:16] <steveire> And talk about a method in a namespace, and talk
about using KIconEngine
[14:21:31] <steveire> And talk about how the only thing KIcon is is a ctor
[14:21:50] <steveire> The KIcon type doesn't have carry any benefits.
[14:21:58] <pinotree> steveire: i *know*, but then don't claim that
you didn't propose such stuff, when you really did (not knowing
anything about what kicon does at all)
[14:22:28] <kdepepo> QIcon myIcon = KUI::icon("foo");
[14:22:46] <kdepepo> where KUI is the namespace
[14:22:49] <steveire> I didn't say I didn't propose it. I said I
*don't*. It's subtle, maybe a bit too subtle. But I have not been
discussing fromTheme in some time
[14:23:34] <pinotree> and if i didn't start asking those two
questions, you would have kept your idea of replacing it, honestly
[14:23:39] <steveire> I have been disussing a method in a namespace
which *does* use KIconEngine and has the exact features KIcon
currently has
[14:24:12] <steveire> pinotree: Yes. But you did bring up that point.
Almost immediately the discussion changed to not using
QIcon::fromTheme
[14:24:24] <steveire> We're now discussing the discussion, which is far too meta
[14:24:27] <pinotree> all of this because "there must not be
k-classes, otherwise the poot qt developers cry"... sad
[14:24:46] <steveire> Let's get back to the topic kdepepo just posted.
What's the propblem with it?
[14:25:36] <pinotree> that i don't care, easy
[14:25:46] <steveire> huh?
[14:26:14] <steveire> The problem is that you don't care about the
snippit posted?
[14:26:40] <steveire> pinotree: That is indeed what frameworks is
about. There have been several discussions about it. Feel free to
bring it up on k-c-d and change the direction of the people doing the
work
[14:26:41] <pinotree> no, i don't care about this whole "making kde a
qt subsidiary" thing
[14:27:00] <pinotree> steveire: i'ts pretty clear the direction cannot
be changed at all
[14:27:17] <steveire> Ok.
[14:27:31] <pinotree> that leaves me the question why i'm still
here... let's fix this up
[14:27:35] <steveire> I'll just post this log for archival purposes to
k-c-d then.
[14:27:38] <-- pinotree (~pino at kde/pino) has left #kde-devel ("farewell")
[14:27:48] <steveire> We'll see if anything comes of it
[14:28:26] <dfaure> not sure a discussion about a meta discussion will
bring anything ;)
[14:28:58] <ervin> dfaure: likely not
[14:29:11] <ervin> well, maybe some breakthrough in metaphysics
[14:29:37] <dfaure> steveire: IMHO post an api proposal, not a
meta-meta-discussion
[14:29:55] -*- Jucato inserts http://xkcd.com/917/ for some
lighthearted humor ....
[14:30:13] <dfaure> steveire: the discussion about ditching
KIconLoader is a complex one, but indeed not the one that happened in
the last hour. Instead your proposal is only a "small" api change, and
keeping KIcon as compat, I see no issue there.
[14:30:15] <DxSadEagle> steveire: you're basically saying we should
favor hypothetical Qt developers over actual KDE app developers.
[14:30:31] <dfaure> DxSadEagle: no, we're saying that independent libs
make everyone happy
[14:30:39] <DxSadEagle> dfaure: do they now
[14:31:12] -*- DxSadEagle wonders about how much code will get
acicdentally moved to Qt without all copyright owners' permissions?
[14:31:28] <steveire> If they don't, then we can stop the frameworks
refactoring. I guess we need to decide before we do more work on it
[14:31:40] <steveire> DxSadEagle: I predict none
[14:31:41] <ervin> I wonder why everyone focuses on the emotional
fantasy of kde vs qt, while really KIcon is easy: design wise a type
inheriting from QIcon for the features offered makes no sense
[14:32:15] <DxSadEagle> But hey, my technical PoV leans more towards
forking Qt4.8 than reorganizing kdelibs.
[14:32:30] <steveire> dfaure: Ok, to be clear (and for hte archived
discussions): I absolutely do not support removing KIconLoader
anymore.
[14:32:59] <steveire> But I know you can see that.
[14:33:02] <dfaure> DxSadEagle: code-into-Qt sounds like a different
issue from "making the frameworks independent from each other".
[14:33:13] <dfaure> steveire: yep.
[14:33:34] <kdepepo> r1251673 ???
[14:33:49] <DxSadEagle> dfaure: it's not entirely, since you're trying
to use more Qt stuff while simultaneously not regressing
functionality/causing massive SC issues.
[14:34:09] <dfaure> fun, heh? ;-)
[14:34:38] <dfaure> sorry, gotta go, talk to you guys tomorrow
[14:35:18] <DxSadEagle> IMHO: poorly timed with the toolkit going in a
rather iffy direction.