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