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

Anthony Fieroni bvbfan at abv.bg
Tue May 3 08:38:35 UTC 2016



> On Май 2, 2016, 5:02 след обяд, 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
> 
> Martin Gräßlin wrote:
>     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 Gräßlin wrote:
>     If the main problem is the incorrect icon of dragon player I suggest to try to fix it there: set a proper default icon with QGuiApplication::setWindowIcon. Let's fix the bugs, where they are :-)
> 
> Anthony Fieroni wrote:
>     So focus at http://api.kde.org/frameworks-api/frameworks5-apidocs/kwindowsystem/html/netwm_8cpp_source.html#l03544 when icon is found NETWinInfo::icon returns largest one (width/height == -1), when is not find KWindowSystem::icon will return the smaller one. This is api inconsistemcy.
> 
> Anthony Fieroni wrote:
>     The problem is not only in Dragon, many applications not provide their icons in that way. I can decide it's wrong or right, this is the facts.

I will correct plasma-framework too, for thumbnail icon size, but this must be correct too, about me.


- Anthony


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


On Май 2, 2016, 5:04 след обяд, Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127809/
> -----------------------------------------------------------
> 
> (Updated Май 2, 2016, 5:04 след обяд)
> 
> 
> 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/2f8a689f/attachment-0001.html>


More information about the Kde-frameworks-devel mailing list