----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126291/#review92378 -----------------------------------------------------------
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. src/platforms/osx/kwindowsystem_macobjc.mm (line 89) <https://git.reviewboard.kde.org/r/126291/#comment62996> Why is the define needed? Doesn't it make more sense to just not build the OSX backend if COCOA is disabled? src/platforms/osx/kwindowsystem_macobjc.mm (lines 111 - 113) <https://git.reviewboard.kde.org/r/126291/#comment62997> please no commented code src/platforms/osx/kwindowsystem_macobjc.mm (line 175) <https://git.reviewboard.kde.org/r/126291/#comment62998> Please use categorized logging. I suggest adding a new category for the OSX backend. src/platforms/osx/kwindowsystem_macobjc.mm (lines 302 - 303) <https://git.reviewboard.kde.org/r/126291/#comment62999> then please remove the code src/platforms/osx/kwindowsystem_macobjc.mm (lines 331 - 342) <https://git.reviewboard.kde.org/r/126291/#comment63000> why support setting the current desktop if the number of desktops is hardcoded to 1? src/platforms/osx/kwindowsystem_macobjc.mm (line 446) <https://git.reviewboard.kde.org/r/126291/#comment63002> what's experimental window tracking? And that ifdef if enabled would not compile src/platforms/osx/kwindowsystem_macobjc.mm (line 447) <https://git.reviewboard.kde.org/r/126291/#comment63001> QObject() - Martin Gräßlin 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
