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

[Martin Gräßlin]

> On May 2, 2016, 4:02 p.m., Martin Gräßlin wrote:
> > What is the "best" size if it's not specified? Why is your change better than how it was?
> > 
> > This change adjusts a very important part of the icon lookup functionality used by KWin and Plasma. I'm a little bit scared of touching it.
> > 
> > The change does not come with a test, so I have problems understanding what you want to change. Could you please extend the tests to ensure that this does not break existing functionality?
> 
> Anthony Fieroni wrote:
>     I, personally, don't know when NETWinInfo->icon or icccmIconPixmap can fail but when it happend function returns icon with size 16x16 when is called like this -> KWindowSystem::icon (wid);
> 
> Martin Gräßlin wrote:
>     well that's what we have unit test for. They can show us whether the code fails.
> 
> Anthony Fieroni wrote:
>     xprop of dragonplayer
>     
>     _NET_WM_ICON_GEOMETRY(CARDINAL) = 674, 736, 164, 30
>     _NET_WM_ALLOWED_ACTIONS(ATOM) = _NET_WM_ACTION_MOVE, _NET_WM_ACTION_RESIZE, _NET_WM_ACTION_MINIMIZE, _NET_WM_ACTION_SHADE, _NET_WM_ACTION_MAXIMIZE_VERT, _NET_WM_ACTION_MAXIMIZE_HORZ, _NET_WM_ACTION_FULLSCREEN, _NET_WM_ACTION_CHANGE_DESKTOP, _NET_WM_ACTION_CLOSE
>     _KDE_NET_WM_FRAME_STRUT(CARDINAL) = 2, 2, 24, 4
>     _NET_FRAME_EXTENTS(CARDINAL) = 2, 2, 24, 4
>     _NET_WM_DESKTOP(CARDINAL) = 0
>     _KDE_NET_WM_ACTIVITIES(STRING) = "153b4652-e23a-44c6-baf7-e7a0d812498a"
>     WM_STATE(WM_STATE):
>                     window state: Normal
>                     icon window: 0x0
>     _NET_WM_STATE(ATOM) = 
>     _NET_WM_ICON_NAME(UTF8_STRING) = 
>     XdndAware(ATOM) = BITMAP
>     WM_NAME(STRING) = 
>     _NET_WM_NAME(UTF8_STRING) = "???????? ?? ????? Dragon"
>     _KDE_NET_WM_USER_CREATION_TIME(CARDINAL) = 49919374
>     _MOTIF_WM_HINTS(_MOTIF_WM_HINTS) = 0x3, 0x3e, 0x7e, 0x0, 0x0
>     _NET_WM_WINDOW_TYPE(ATOM) = _NET_WM_WINDOW_TYPE_NORMAL
>     _XEMBED_INFO(_XEMBED_INFO) = 0x0, 0x1
>     WM_CLIENT_LEADER(WINDOW): window id # 0x6c00006
>     WM_HINTS(WM_HINTS):
>                     Client accepts input or input focus: True
>                     Initial state is Normal State.
>     _NET_WM_PID(CARDINAL) = 917
>     _NET_WM_SYNC_REQUEST_COUNTER(CARDINAL) = 113246213
>     WM_CLASS(STRING) = "dragon", "dragonplayer"
>     WM_PROTOCOLS(ATOM): protocols  WM_DELETE_WINDOW, WM_TAKE_FOCUS, _NET_WM_PING, _NET_WM_SYNC_REQUEST
>     WM_NORMAL_HINTS(WM_SIZE_HINTS):
>                     user specified size: 651 by 295
>                     program specified minimum size: 651 by 295
>                     window gravity: Static
>     
>     NETWinInfo icon and icccmIconPixmap fails, i can add such a test, if it not present.
>     So http://api.kde.org/frameworks-api/frameworks5-apidocs/kwindowsystem/html/netwm_8cpp_source.html#l03544 NETWinInfo trys to return the larger one, so this function must be identical, at functionality

Why do you show me the xprop of dragonplayer? What do you want to show me?

I really have a hard problem understanding why you want to change the behavior. Especially as the API documentation doesn't say anything about what it does when width/height are not specified. We might break existing code here! That's why I need to understand the motivation behind the change. I need to understand why the change is better than the current one.

Especially we need to exclude the possiblity of a misuse of the API. Given the xprop there is no proper icon set on the window. This is a bug in dragon player.


- Martin


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


On May 2, 2016, 4:04 p.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127809/
> -----------------------------------------------------------
> 
> (Updated May 2, 2016, 4:04 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/20160503/cca3787c/attachment-0001.html>