Review Request 122086: Add new method KWindowSystem::icons

[Martin Gräßlin]

> On Jan. 21, 2015, 10:07 a.m., Martin Gräßlin wrote:
> > ping - do we want this approach to go in?
> 
> Thomas Lübking wrote:
>     I've two concerns about this.
>     1. the name "icons" implies "several" (it's unfortunate that ::icon isn't ::iconPixmap) - maybe "windowIcon()" or similar
>     2. the behavior how an icon is build up from multiple sources (isn't)
>        right now, you could end up w/ one 16x16 NETWM icon pixmap.
>        Considerations: NETWM and WM_HINTS could be more specific than an icon from the appname, the appname could though be more size complete and finally WM_HINTS would likely be "ugly" compared to the other two (bitmasked). Overmore it might be undesirable to end up w/ an icon that has completely different looks for different sizes.
>        As we don't have a specific hint on the required target size, it gets harder to ever take an informed decision on what to return here, but one might want to at least check the sizes provided by NEWTM before returning early (if other sources are allowed by flags) - yes, I know that this is not a straight suggestion for improvement :-(

I completely agree with your two concerns and also thought about the same. Concerning point 1 I wanted to make them another ::icon overload but that's not possible - it would be ambiguous :-(. windowIcon might indeed be a better name.

Concerning point 2: I thought about combining all items. E.g. if NETWM and WM_HINTS are set, it will include both. But if NETWM provides a size which WM_HINTS doesn't it wouldn't be set. But that wouldn't work with the app or the X icon.

I have to think a little bit more about it.


- Martin


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


On Jan. 21, 2015, 10:06 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122086/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2015, 10:06 a.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> ::icons tries to retrieve all available icon sizes and combine them
> into a single QIcon. On the X11 platform this can significantly
> reduce the time needed to fetch all available icons compared to the
> already existing ::icon methods which return a QPixmap of a specific
> size.
> 
> The change includes a new test application "icontest" which shows all
> available icons in all available sizes for a window id passed as
> command line argument.
> 
> CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)
> 
> 
> Diffs
> -----
> 
>   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cffffd52 
>   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
>   src/kwindowsystem_p.h 1f01145b5c7efe925fcb8242f92af17b704e01c9 
>   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
>   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
>   tests/CMakeLists.txt ce68cc505a69ea9a3cf645e9ae587bd89abe1648 
>   tests/icontest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122086/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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