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

Mon Feb 15 11:06:46 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.

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.)

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

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?

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, lines 111-113
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line111>
> >
> >     please no commented code

Will disappear from the final code, of course, but probably not as long as I still might need this kind of test probe ;)

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

Is there an accepted way to include a kind of scrapbook with code snippets that might be useful or "kinda work"?

> 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

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.

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, line 175
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line175>
> >
> >     Please use categorized logging. I suggest adding a new category for the OSX backend.

Roger.

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, lines 302-303
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line302>
> >
> >     then please remove the code

OK.

> On Feb. 15, 2016, 8:42 a.m., Martin Gräßlin wrote:
> > src/platforms/osx/kwindowsystem_macobjc.mm, line 447
> > <https://git.reviewboard.kde.org/r/126291/diff/2/?file=444312#file444312line447>
> >
> >     QObject()

Indeed.

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