[KDE/Mac] Review Request 126369: [OS X] adaptation(s) to platform limitations (WIP)

Sat Dec 19 01:20:02 UTC 2015

> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, lines 279-281
> > <https://git.reviewboard.kde.org/r/126369/diff/2/?file=424258#file424258line279>
> >
> >     This has a strange indentation
> >     
> >     Also I think those bools are not entirely useful,
> >     it just makes the code harder to read by using
> >     shortened names while you could just plug those
> >     title.isEmpty()/subTitle.isEmpty() directly in
> >     those ifs. Each is going to be called exactly
> >     once anyway.

Ah, yes indeed (the indentation). Not sure who did it, but I missed it. And you're right about the bools.

> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, line 908
> > <https://git.reviewboard.kde.org/r/126369/diff/2/?file=424258#file424258line908>
> >
> >     This is missing an ifdef

Will be fixed in the next upload, tomorrow morning if life doesn't get too much in the way.

> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, line 909
> > <https://git.reviewboard.kde.org/r/126369/diff/2/?file=424258#file424258line909>
> >
> >     I really think this should be handled/mapped
> >     by the OS X native systray thing. But I guess
> >     there is no API that can be used/mapped to the
> >     NeedsAttention state?

I agree, but no, I haven't seen anything in the APIs that can be used to request attention via the systray icon. I'll have another look, but for now the only native attention mechanism that I can think of is letting the Dock icon bounce. That'll work only for applications that have a Dock icon of course, and it tends to work only when the application is in the background. So badging the systray icon is probably still going to be a good idea, and I doubt that it's going to take less code to do that with native APIs.

It also requires using ObjC. What do you think of the idea of moving `KStatusNotifierItemPrivate` to its own file, and subclassing it on OS X to override the `init` and `syncLegacySystemTrayIcon` methods? I can put that into an ObjC++ file which means I can do native API calls. I think a few of the other modifications I made to the main class could also be moved into `KStatusNotifierItemPrivate` (and overriden in `KStatusNotifierItemPrivateMac`) to reduce the number of ifdefs.

- René J.V.

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

On Dec. 17, 2015, 5:26 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126369/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2015, 5:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> -------
> 
> OS X has a number of limitations in features used by KNotifications, notably concerning the status notifier item (aka system tray icon).
> 
> This RR will likely evolve to address multiple limitations (at least also the NeedsAttention state); at the moment it only proposes an emulation of `QMenu::addSection`.
> 
> `QMenu::addSection` works by adding a QAction with a "texted separator" at the insertion location. Texted separators do not exist in menu items in the OS X "global" menubar (they become regular separators), and Qt will not provide a platform-specific implementation. Loss of the section title text is maybe not always an issue, but I think it is in the system tray menu. I therefore propose to emulate `QMenu::addSection` by replacing the texted separator with an inactive (disabled) menu item that shows the text, followed by a standard separator. Menus in the notification area are much less subject to interface guidelines, so the presence of an item icon is acceptable and IMO useful for the `titleAction`.
> 
> Testing the NeedsAttention state with the tests/kstatusnotifieritemtest application leads to disappearance of the menubar icon, i.e. the access to the notifier menu becomes invisible rather than blinking (which is what I get on Linux using the same packaging). Adding a few qDebug statements shows that the `attentionIcon` is empty.
> I'd appreciate a crash course how this feature is supposed to work, so I can see if an OS X implementation might be feasible.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 6d09051 
>   src/CMakeLists.txt 7eb3125 
>   src/kstatusnotifieritem.cpp f9bf460 
>   tests/kstatusnotifieritemtest.cpp 973fc85 
> 
> Diff: https://git.reviewboard.kde.org/r/126369/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.17.0 .
> 
> 
> File Attachments
> ----------------
> 
> the systray icon & menu created by kstatusnotifieritemtest . This application has no icon to show.
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/286037ae-07b3-454a-a226-1748854493a1__kstatusnotifieritemtest-systray.png
> The systray icon and menu created by the KDE4 kwalletmanager (code has an equivalent patch)
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/4fc9d4e4-1537-478c-9196-94cbc17b6b7c__kwalletmanager-systray.png
> An Apple systray icon+menu that shows icons (which cannot be hidden)
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/fc48a963-2e18-4396-bd38-062d41688118__Apple-systray-menu-with-icons.png
> kstatusnotifieritemtest with added appIcon
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/e896e90d-f0a8-43f7-9199-847572832df7__kstatusnotifieritemtest-with-appIcon.png
> kstatusnotifieritemtest-with-appIcon+attention.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/f41081db-8e09-4ea2-95c1-f507c62109d4__kstatusnotifieritemtest-with-appIconattention.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20151219/ceccf8f6/attachment.html>