Review Request 130094: Implement QPlatformTheme::fileIconPixmap()

[Kai Uwe Broulik]

> On April 21, 2017, 9:12 vorm., Kai Uwe Broulik wrote:
> > Thanks for your patch!
> > 
> > Plasma reviews are nowadays handled on https://phabricator.kde.org/ - could you perhaps upload it there again? I just don't want it to get lost here :/
> > 
> > Also, I'm not sure about having a separate FileIconProvider class. You also seem to reimplement quite a lot of logic. Can you perhaps try KIO::iconNameForUrl (in KIOCore I think) which has all of that mimetype resolution and special folders logic already built-in, potentially allowing you to reduce your patch to like 10 lines of code :)
> > 
> > (Btw using Qt 5.7 features is fine, Plasma-integration is released alongside Plasma, not KDE Frameworks, and the upcoming Plasma 5.10 release will depend on Qt 5.7 anyway)
> 
> Eugene Shalygin wrote:
>     This patch was applying against frameworkintegration for a long time, and there I wanted no dependencies. BTW, do you know what platform theme plugin can be extended to make this work in non-Plasma environment (like GNOME) too?
> 
> Eugene Shalygin wrote:
>     Took a look at KIO::iconNameForUrl() and not sure whether to use it and discard iconOptions parameter to this function or not. Do you want to finish with this question here, or should I move to phabricator right away?

Right, Plasma's platform theme used to be shipped in frameworks but was moved to Plasma as it should only be used there and apps running on other environments should use whatever platform's plugin there is (usually Qt built-in). Don't know about other environments, Qt should have implementations for those.


> On April 21, 2017, 9:12 vorm., Kai Uwe Broulik wrote:
> > src/platformtheme/kdeplatformtheme.cpp, line 122
> > <https://git.reviewboard.kde.org/r/130094/diff/1/?file=494561#file494561line122>
> >
> >     Why is this only built in 5.7 and below? Was this removed in 5.8? (didn't check Qt code)
> 
> Eugene Shalygin wrote:
>     Signature of the function was changed in 5.8 if you are asking about the #ifdef. Did I understand you correctly?

Yep, that's what I've been asking. So, there's no method returning a QIcon anymore? Pity.


- Kai Uwe


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


On April 21, 2017, 9:11 vorm., Eugene Shalygin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130094/
> -----------------------------------------------------------
> 
> (Updated April 21, 2017, 9:11 vorm.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> -------
> 
> Implement QPlatformTheme::fileIconPixmap() to make QFileIconProvider work.
> 
> 
> Diffs
> -----
> 
>   src/platformtheme/kdeplatformtheme.h 7835439 
>   src/platformtheme/kdeplatformtheme.cpp 704f176 
> 
> Diff: https://git.reviewboard.kde.org/r/130094/diff/
> 
> 
> Testing
> -------
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Eugene Shalygin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170421/3d3bb5b8/attachment.html>