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

Mon Feb 15 12:59:19 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.)

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 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?

That should be defined in the cmake config for Qt5Gui. At least where I needed similar things in the past it was defined.

> 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"?

good question. I don't know of one.

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, line 446
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line446>
> >
> >     what's experimental window tracking?
> >     
> >     And that ifdef if enabled would not compile
> 
> René J.V. Bertin wrote:
>     The whole EXPERIMENTAL_WINDOW_TRACKING feature is an experiment started by someone who worked on this code before me. From what I understand, the idea was to catch window creation and destruction events to maintain some kind of internal database that would allow to implement a number of the windowing features that aren't supported by the underlying layers. I don't think the feature was ever completed, and it also uses deprecated APIs.
>     
>     I left it in because I thought it could fuel more discussion about whether or not such a feature could make sense to complete the feature set. But it could probably play that same role when moved to the aforementioned scrapbook or some other kind of "junk DNA" file.

the way it's currently I would highly recommend to remove it as it just cannot compile.

- Martin

-----------------------------------------------------------
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/20160215/620ba036/attachment-0001.html>