> On Aug. 3, 2016, 7:38 a.m., David Rosca wrote: > > Why does it break for SNIs? It first tries QIcon::fromTheme and if it fails > > (isNull()) fallbacks to original QIcon. This patch just switched it, why > > does it make a difference? > > > > The problem I see here is that if a custom icon loader is used which uses > > static icon name (let's say "plasma" as we have it in breeze icons), but > > the actual plasma.png file representing completely different icon. In this > > case, however, it would fail in the codepath above - being loaded either > > from plasma theme or svg with kiconloader. > > David Edmundson wrote: > The reason it fails, is that QIcon::fromTheme ends up returning the > unknown icon. Which is not "null". > and I can't just disable it having a fallback, as maybe it originally was > a string which didn't have an icon. > > David Rosca wrote: > How come? What is "unknown icon"? Maybe the actual bug is > https://quickgit.kde.org/?p=kiconthemes.git&a=commit&h=0abf1b7a148cf6b27caea01a329631e0f1daa983 > ? > > Kai Uwe Broulik wrote: > The "unknown" icon is the generic sheet of paper thing you usually get > when an icon wasn't found. > > David Rosca wrote: > But that's not what should QIcon::fromTheme(iconname) return when icon > name is invalid. It should just return null icon.
1) You're right. I was missing that patch. I just assumed I had it as I was fixing something in Neon.... It returns name, so it's not "null"... and so on. Sorry about that. 2) This commit still makes sense as it saves getting an icon when we already have one, so I'll leave it in. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128580/#review98035 ----------------------------------------------------------- On Aug. 3, 2016, 11:05 a.m., David Edmundson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128580/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2016, 11:05 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > ------- > > Currently the code gets the icon name from the QIcon and tries to do > some Plasma theming with it. > However if that fails it then loads the QIcon::fromTheme again. > > This is pointless in most cases and will break any icons that have a > custom loader (all SNIs) > > > Diffs > ----- > > src/declarativeimports/core/iconitem.cpp > 29c7f05b5df060df7b362b331f7edc412df12307 > > Diff: https://git.reviewboard.kde.org/r/128580/diff/ > > > Testing > ------- > > > Thanks, > > David Edmundson > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
