----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130094/#review103073 -----------------------------------------------------------
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) src/platformtheme/kdeplatformtheme.h (line 53) <https://git.reviewboard.kde.org/r/130094/#comment68569> no need for "virtual" since we have Q_DECL_OVERRIDE src/platformtheme/kdeplatformtheme.cpp (line 121) <https://git.reviewboard.kde.org/r/130094/#comment68571> Why is this only built in 5.7 and below? Was this removed in 5.8? (didn't check Qt code) - Kai Uwe Broulik 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 > >
