Review Request 127826: [IconThumbnail] Request for icon, don't let kwindow::icon to decide

[Martin Gräßlin]

> On May 6, 2016, 7:50 a.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/windowthumbnail.cpp, line 245
> > <https://git.reviewboard.kde.org/r/127826/diff/1/?file=464078#file464078line245>
> >
> >     showing you the icon look up code from KWin, which is close to perfection (doesn't get the 256x256 icon yet):
> >     
> >         QIcon icon;
> >         auto readIcon = [this, &icon](int size, bool scale = true) {
> >             const QPixmap pix = KWindowSystem::icon(window(), size, size, scale, KWindowSystem::NETWM | KWindowSystem::WMHints, info);
> >             if (!pix.isNull()) {
> >                 icon.addPixmap(pix);
> >             }
> >         };
> >         readIcon(16);
> >         readIcon(32);
> >         readIcon(48, false);
> >         readIcon(64, false);
> >         readIcon(128, false);
> >         
> >     The problem here is that we don't specify from where to take the icon (we should always try to prefer NETWM) and don't fetch all possible sizes. This is rather important because we want neither upscaling, nor downscaling.
> 
> Anthony Fieroni wrote:
>     We specified http://api.kde.org/frameworks-api/frameworks5-apidocs/kwindowsystem/html/kwindowsystem_8cpp_source.html#l00485
>     *return icon(win, width, height, scale, NETWM | WMHints | ClassHint | XApp);*

having written the code in question: I think that was just bad code on my side. It's the fallback and I didn't spend much time on it. If I would have thought properly about it, I would have taken KWin's code.


- Martin


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


On May 3, 2016, 8:28 p.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127826/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 8:28 p.m.)
> 
> 
> Review request for Plasma, David Edmundson and Martin Gräßlin.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> I'm toking when app is minimized and when hover taskitem in task manager it's drawed a thumnail icon window. So what is the real problem, some apps, many as KDE', not provides their icons in window props and NETWinInfo fails to get them. So in this case we must specified size explicitely to KWindowSystem::icon. But this is not enough because KWindowSystem::icon makes incorrect decision to crop icon to 48x48. Explanation the shots:
> 1. Current code not specified size explicitely, returned icon is 16x16
> 2. New code wants icon 256x256 but KWindowSystem::icon ignore it and returning icon 48x48
> 3. New code in 2 frameworks both, it works.
> So i makes shots with Dragon but many applications are affected, even KDE' akregator, ktimer, kalarm and so on, i don't start to comment other toolkit apps, when we violates KDE'.
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/core/windowthumbnail.cpp 251aaa4 
> 
> Diff: https://git.reviewboard.kde.org/r/127826/diff/
> 
> 
> Testing
> -------
> 
> Depends on https://git.reviewboard.kde.org/r/127809/
> 
> 
> File Attachments
> ----------------
> 
> current plasma-framework and current kwindowsystem
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/f81bc88b-fe3b-4f00-b2f2-502effe4c245__Screenshot_20160503_202946.png
> new plasma-framework, current kwindowsystem
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/36d7baa3-e875-420f-b2c2-39c2e5d409b9__Screenshot_20160503_203337.png
> new plasma-framework, new kwindowsystem
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/857380fe-cdfd-478d-9df2-743f87d4c1ec__Screenshot_20160503_203946.png
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160506/19873dff/attachment-0001.html>