[KDE/Mac] Review Request 126161: OS X housekeeping

David Faure faure at kde.org
Sun Apr 10 08:31:57 UTC 2016 

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



I liked what I was seeing in this patch .... until I got to the horrible code duplication introduced by kinit_mac.mm. This is not the way to go.
Yes #ifdefs are a pain, but code duplication is 1000 times worse. In Qt, the _mac.mm files only contain code that is mac specific.

You could move all the code that is common between mac and linux into a separate helper file (say kinit_common.{cpp,h}), or you could organize it this way:
kinit.cpp/kinit.h -> shared code
kinit_unix.cpp -> linux/bsd-specific
kinit_mac.mm  -> OSX specific
(and the windows guys can make a kinit_win.cpp if they need to, although I'm not sure they use kdeinit at all)

Code duplication is the worst enemy of maintenability, I will never accept a patch that duplicates lots and lots of code. But other than that, many of the changes in this patch seem very sensible, I will definitely approve it once the code duplication is gone. Thanks!


src/kdeinit/kinit_mac.mm (line 103)
<https://git.reviewboard.kde.org/r/126161/#comment64229>

    Ouch, major code duplication. Please move anything that is common between kinit.cpp and kinit_mac.mm into a common header (and/or .cpp file, but at least for the struct a header is enough).



src/kdeinit/kinit_mac.mm (line 145)
<https://git.reviewboard.kde.org/r/126161/#comment64231>

    code duplication



src/kdeinit/kinit_mac.mm (line 164)
<https://git.reviewboard.kde.org/r/126161/#comment64232>

    code duplication



src/kdeinit/kinit_mac.mm (line 210)
<https://git.reviewboard.kde.org/r/126161/#comment64233>

    If people say ifdefs are bad, surely they are not as bad as duplicating hundreds of lines of code, making maintenance much more difficult in the future,



src/kdeinit/kinit_mac.mm (line 289)
<https://git.reviewboard.kde.org/r/126161/#comment64234>

    I've seen this before .... ;)



src/kdeinit/kinit_mac.mm (line 312)
<https://git.reviewboard.kde.org/r/126161/#comment64235>

    duplicated -> factorize into separate .cpp/.h file



src/kdeinit/kinit_mac.mm (line 331)
<https://git.reviewboard.kde.org/r/126161/#comment64236>

    duplicated, not mac specific -> factorize



src/kdeinit/kinit_mac.mm (line 361)
<https://git.reviewboard.kde.org/r/126161/#comment64237>

    Does OOM protection actually exist on OSX?
    
    This code seems quite linux specific to me, i.e. duplicated here but never enabled, right? -> remove



src/kdeinit/kinit_mac.mm (line 399)
<https://git.reviewboard.kde.org/r/126161/#comment64238>

    I give up, this method is too big for me to see how much of it is duplicated from kinit.cpp and how much is OSX-specific.
    This is a case where #ifdef might actually be much better...



src/kdeinit/kinit_mac.mm (line 656)
<https://git.reviewboard.kde.org/r/126161/#comment64239>

    same as the linux code?



src/kdeinit/kinit_mac.mm (line 704)
<https://git.reviewboard.kde.org/r/126161/#comment64240>

    same as the linux code?



src/kdeinit/kinit_mac.mm (line 891)
<https://git.reviewboard.kde.org/r/126161/#comment64241>

    surely that's pure duplication from kinit.cpp?



src/kdeinit/kinit_mac.mm (line 1271)
<https://git.reviewboard.kde.org/r/126161/#comment64242>

    yes, it was only used on non-mac, so remove it from here



src/klauncher/klauncher.cpp (line 376)
<https://git.reviewboard.kde.org/r/126161/#comment64243>

    Please revert, startsWith has a QLatin1String overload.



src/klauncher/klauncher.cpp (line 759)
<https://git.reviewboard.kde.org/r/126161/#comment64245>

    revert



src/klauncher/klauncher.cpp (line 823)
<https://git.reviewboard.kde.org/r/126161/#comment64244>

    revert



src/klauncher/klauncher.cpp (line 867)
<https://git.reviewboard.kde.org/r/126161/#comment64246>

    revert



src/klauncher/klauncher.cpp (line 900)
<https://git.reviewboard.kde.org/r/126161/#comment64247>

    revert



src/klauncher/klauncher.cpp (line 1074)
<https://git.reviewboard.kde.org/r/126161/#comment64248>

    revert



src/klauncher/klauncher.cpp (line 1175)
<https://git.reviewboard.kde.org/r/126161/#comment64249>

    revert, operator== has a QLatin1String overload



src/klauncher/klauncher_main.cpp (line 152)
<https://git.reviewboard.kde.org/r/126161/#comment64250>

    I'm curious, what's the difference between Q_OS_DARWIN and Q_OS_OSX?



src/wrapper.cpp (line 63)
<https://git.reviewboard.kde.org/r/126161/#comment64230>

    Does qgetenv(MAC_DISPLAY) really do anything sensible on OSX? I assume this doesn't exist, right?
    
    It seems to me that generate_socket_name should just assemble the socket name differently on Mac (and Windows), no?


- David Faure


On April 6, 2016, 5:16 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 5:16 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> -------
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 14th 2009, which prevents a kdeinit crash that is caused by calling exec after `fork()` in an application that has used non-POSIX APIs and/or calling those APIs in the exec'ed application. This patch (originally by MacPorts developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a proxy application to do the actual exec.
> 
> 
> Diffs
> -----
> 
>   src/kdeinit/CMakeLists.txt ae619f7 
>   src/kdeinit/kinit.cpp ca18603 
>   src/kdeinit/kinit_mac.mm PRE-CREATION 
>   src/klauncher/CMakeLists.txt a8e6c3e 
>   src/klauncher/klauncher.h 53c0803 
>   src/klauncher/klauncher.cpp baa5649 
>   src/klauncher/klauncher_main.cpp 710c889 
>   src/start_kdeinit/CMakeLists.txt 46d6cb3 
>   src/wrapper.cpp 9cb0a71 
> 
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 as well as Qt 5.6 and 5.20.0 . With this patch, starting `kded5` will launch kdeinit5 and klauncher5 as expected, but `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable for typical KF5 use on OS X (kded5 can be launched as a login item or as a LaunchAgent) but I will have another look at why the kded isn't started.
> 
> I am not yet able to perform further testing; practice will for instance have to show whether point 2 above needs revision (apps that need to be installed as app bundles).
> 
> Similarly it will have to be seen whether point 3 above has any drawbacks. Applications running as agents do not show up in the Dock or App Switcher. Thus, klauncher will not be able to "turn itself into" an application that does have a full GUI presence with my current modification. I don't know if that's supposed to be possible though.
> NB: I have been building the KDE4 klauncher in a way that makes it impossible to construct a GUI at all, so I'm not expecting issues in KF5 as long as klauncher's role hasn't evolved too much.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

More information about the kde-mac mailing list