Review Request 127809: [Platform xcb] Get best icon size when he's not specified

[Martin Gräßlin]

> On May 4, 2016, 10:18 a.m., Martin Gräßlin wrote:
> > src/platforms/xcb/kwindowsystem.cpp, lines 711-717
> > <https://git.reviewboard.kde.org/r/127809/diff/2/?file=462316#file462316line711>
> >
> >     This changes the behavior of the method. Now always NETWM is preferred, even if it doesn't provide the best icon.
> >     
> >     By returning early at this point you cannot evaluate what's the best fitting size.
> 
> Anthony Fieroni wrote:
>     How changes it? Return is 2 line below, at original code and all next checks are !result.isNull(), i remove them and returning earlier, like optimization :) Even with original it can returning null image because check is above.

> Even with original it can returning null image because check is above.

Yes which is rather important. I showed you the code from KWin which is strongly written against the actual implementation and not written against the documentation (KWindowsystem used to be the "KWin lib" back in the days). If there are NETWM icons it uses them, even if there is a size missing. Thus the code needs to get the null icon. Otherwise it might add a wrong icon.

This is why I'm so scaptical about changing the code. We have two valid users of this API (KWin and libtaskmanager) and both have an implementation based on the insides of this method.


- Martin


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


On May 5, 2016, 6:50 p.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127809/
> -----------------------------------------------------------
> 
> (Updated May 5, 2016, 6:50 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Martin Gräßlin.
> 
> 
> Bugs: 362324
>     https://bugs.kde.org/show_bug.cgi?id=362324
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> The api function is KWindowSystem::icon (WId win, int width=-1, int height=-1, bool scale=false) so caller must get best size not worst when width/height is not specified.
> 
> 
> Diffs
> -----
> 
>   src/platforms/xcb/kwindowsystem.cpp 5b7c65a 
> 
> Diff: https://git.reviewboard.kde.org/r/127809/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> before
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/01/6d718ef6-26cf-4866-94d2-4ffbdfc906fe__Screenshot_20160426_232109.png
> after
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/01/7dcab4ae-e451-4d43-8799-a0fcab471a3d__Screenshot_20160501_224642.png
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

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