[KDE/Mac] Review Request 126291: initial implementation of a platform plugin for OS X (WIP)

Tue Feb 16 23:23:06 UTC 2016

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > Thanks for contributing the code, I'm very happy to see this happening! Sorry, that I cannot provide a good review as I'm not really into the OSX API, thus my review here might look a little bit nitpicky.
> > 
> > General comment: maybe rename the directory from "osx" to "cocoa"? After all it's more about the windowing system than the operating system.
> 
> René J.V. Bertin wrote:
>     Nitpickyness is no problem here.
>     
>     I have nothing against renaming the subdirectory, but it'd be something I'd want to leave to the end, for convenience on my end. Also, we'd probably want to remain consistent, or work toward consistency. Currently there are no "cocoa" backend/plugin directories; 3 frameworks use "osx" and another uses "mac". There is also the question to what extent "cocoa" still is and will remain the most appropriate term in this context ... and there is the issue of iOS where some frameworks might be deployed and which might or might not require specific adaptations. (And I'm probably hardly more "into" the iOS APIs as you are into the OS X APIs.)
> 
> Martin Gräßlin wrote:
>     sure, we can think about a more general approach to the naming. Having that standardized over all framework might make sense (assuming we never care about the difference between operating system and windowing system or it's always windowing system).

On systems (platforms) where there is no choice of windowing and/or rendering system one could settle for a single term (question is, which ...).
Another approach would be to follow Qt's QPA naming, as far as that's relevant. That would have the benefit that the name choice would be standardised all the way up to the `-platform` argument available to the end user (on platforms supporting multiple QPAs).
It would also fit in with messages like
`org.kde.kwindowsystem: Loaded plugin "/Volumes/Debian/MP9/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemCocoaPlugin.so" for platform "cocoa"`

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, line 89
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line89>
> >
> >     Why is the define needed? Doesn't it make more sense to just not build the OSX backend if COCOA is disabled?
> 
> René J.V. Bertin wrote:
>     That would require knowing the answer to that question in the CMake file; I'd have to look into that. Is it trivial to do the equivalent of an `#ifdef TOKEN` in a CMake file?
> 
> Martin Gräßlin wrote:
>     That should be defined in the cmake config for Qt5Gui. At least where I needed similar things in the past it was defined.

I cannot find any variable/flag that indicates against what SDK Qt has been built, sadly.

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, lines 331-342
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line331>
> >
> >     why support setting the current desktop if the number of desktops is hardcoded to 1?
> 
> René J.V. Bertin wrote:
>     Is there an accepted way to include a kind of scrapbook with code snippets that might be useful or "kinda work"?
> 
> Martin Gräßlin wrote:
>     good question. I don't know of one.

This will need some more testing, but rather than hardcoding the number of desktops to 1 one could adapt the number when more desktops are discovered (= the max. observed "current desktop number").
KWindowSystem::numberOfDesktops() can change at runtime anyway, can't it?

- René J.V.

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

On Feb. 14, 2016, 5:44 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126291/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2016, 5:44 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> KWindowSystem has been lacking a platform plugin for OS X. This RR presents a "backport" of the modified KDE4 KWindowSystem implementation that has been used in the MacPorts kdelibs4 port for the last 2 or 3 (or more) years.
> 
> I have made some initial steps to remove deprecated Carbon API calls, but this is clearly a work in progress.
> 
> Open questions include
> - is there any justification to run an event handler (or Cocoa observer) to keep track of running applications, possibly even listing all their open windows?
> - is there any use for the Qt event listener framework (cf. the NETEventFilter in the X11 plugin)? I haven't yet had time to try to figure out what this "could be good for", and am very open to suggestions in this departments.
> - one application for such an event filter would be a listener that catches the opening and closing of all windows by the running process, and keeps track of their `WId`s. A new method could then be added (to `KWindowInfo`?) to distinguish `WId`s created by the running application from "foreign" ones (useful also on Wayland and MS Windows).
> 
> `KWindowSystem::setMainWindow` should become a front for payload provided by the plugins. I'll leave that to the regular/official maintainer(s) of this framework.
> 
> This code could probably do with *lots* of comments; I'll try to add them as questions about this or that bit of code arise.
> 
> 
> Diffs
> -----
> 
>   src/kwindowsystem.cpp 407a67d 
>   src/platforms/osx/CMakeLists.txt 4fc3347 
>   src/platforms/osx/cocoa.json PRE-CREATION 
>   src/platforms/osx/kkeyserver.cpp 3ddb921 
>   src/platforms/osx/kwindowinfo.cpp e8555bb 
>   src/platforms/osx/kwindowinfo.mm PRE-CREATION 
>   src/platforms/osx/kwindowinfo_mac_p.h c8f307e 
>   src/platforms/osx/kwindowinfo_p_cocoa.h PRE-CREATION 
>   src/platforms/osx/kwindowsystem.cpp 1758829 
>   src/platforms/osx/kwindowsystem_mac_p.h PRE-CREATION 
>   src/platforms/osx/kwindowsystem_macobjc.mm PRE-CREATION 
>   src/platforms/osx/kwindowsystem_p_cocoa.h PRE-CREATION 
>   src/platforms/osx/plugin.h PRE-CREATION 
>   src/platforms/osx/plugin.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126291/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20160216/84cf8fbb/attachment-0001.html>