Review Request 120287: [OS X] make kde-workspace build

Sat Sep 20 13:35:51 BST 2014

> On Sept. 20, 2014, 8:20 vorm., Martin Gräßlin wrote:
> > kcontrol/krdb/krdb.cpp, line 56
> > <https://git.reviewboard.kde.org/r/120287/diff/3/?file=313628#file313628line56>
> >
> >     added newline
> 
> René J.V. Bertin wrote:
>     Ah, the famous German rigor ... is this really an issue, shouldn't that empty line have been there in the 1st place?!
> 
> Martin Gräßlin wrote:
>     yes of course. It's a change which has nothing to do with the overall change. It's not atomic any more.
> 
> Thomas Lübking wrote:
>     > shouldn't that empty line have been there in the 1st place?!
>     
>     c++98 newline @ EOF, later versions don't, ie. it's not required to add that line despite you may get warnings about it (and actually kate has an option to automatically add it on saving)
>     
>     Do you use xcode?
> 
> Thomas Lübking wrote:
>     c++98 *required* newline @ EOF ...
> 
> René J.V. Bertin wrote:
>     But this is not @EOF, it's a newline inserted to detach the 1st function from the last #include statement (or rather, the last conditional #ifdef Q_WS_X11 include).
>     
>     Given that that's an X11 conditional include, I don't think that cosmetic change (adding a newline) is completely irrelevant either.
>     
>     @Thomas: yes, I use Xcode, but not here. CMake can generate Xcode projects, but that only works for relatively simple ones, so it's not even feasible to use Xcode. Most of the time I use KDevelop (git/kde4-legacy) because of the access to the documentation it gives. I can give less guarantee that I didn't edit the file in vi at some point, though ;)
>     
>     [OT]: there's a Google plugin for Xcode that removes white space from empty lines and I think also the final newline. And I do use Xcode for my OS X Keychain work from time to time, because KDevelop can't access the native SDK documentation.[/OT]
> 
> Thomas Lübking wrote:
>     Indeed - then this change is completely unrelated and pointless.
>     
>     Yes, there probably *should* have been a newline in the first place, but it's actually irrelevant whether there is and such changes don't belong into this kind of patch.
>     
>     > I don't think that cosmetic change (adding a newline) is completely irrelevant either.
>     
>     The point is that this is a purely cosmetic change, unrelated to the rest of your patch.
>     You are right, that one would demand a newline in *new code* here, but it doesn't belong in your patch.
>     
>     If you wish to tidy up aged code for KF5, you're very welcome =)
>     
>     Here's for vim:
>     https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Vim
>     
>     For kate, "configure editor..."
>     /"Appearance"
>        o) Highlight Tabs
>        o) Highlight trailign spaces
>        o) Show indention lines
>     /"editing"/"indention"
>        o) Spaces
>        Tab width: 4 chars
>        Indention width: 4 chars
>     /"Open/Save"
>        Remove Trailing space: on modified lines
>         ) Append newline at end of file
> 
> René J.V. Bertin wrote:
>     Well, if said German rigor has it come to that, I'll remove the newline. But than I'll also keep from changing the Darwin references to APPLE in the CMake files.
> 
> Thomas Lübking wrote:
>     > But than I'll also keep from changing the Darwin references to APPLE in the CMake files.
>     
>     This is indeed nothing that belongs into this patch, but may still be a required patch in general (to permit Darwin + X11 in future versions) that Apple users might be interested in.
> 
> René J.V. Bertin wrote:
>     I beg to differ; this patch is for building kde-workspace on OS X, and changing CMake keywords to what's currently the appropriate way to detect that platform seems perfectly relevant to me. I'd have done (and commented) that if I'd noticed the use of 2 different keywords.
>     
>     For the rest, there's almost 0 chance that Apple (as in OS X) users will ever be able again to run KDE under X11 so it's extremely unlikely that future KDE versions will run into a platform that calls itself Darwin but also requires X11.
>     
>     And if we really can be pedantic, I'd like to point out that `Darwin` is actually a better keyword than `APPLE` because it refers to the software environment only and not the vendor. Apple will most likely always remain the sole OS X vendor, but OS X is likely to be replaced some day by something that's not `(APPLE AND Darwin)`.

> this patch is for building kde-workspace on OS X

And it builds with APPLE and Darwin intermixed, does it?
I agree that aligning the detection is reasonable, but that's still a different patch ("atomic")

About the proper check: I assume one really has to test for (APPLE AND Darwin) to half-wise detect OSX (there seems no specific key in cmake).
Half-wise because this applies to iOS as well :-(

- Thomas

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

On Sept. 20, 2014, 10:54 vorm., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120287/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2014, 10:54 vorm.)
> 
> 
> Review request for KDE Software on Mac OS X and kde-workspace.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> A few rather straightforward patches to make the relevant bits of KDE4's kde-workspace build and function on OS X.
> The main interest is having the systemsettings control panel to control the various relevant KDE settings among which desktop search, fonts, colours and even style.
> The oxygen style builds and looks good but shows some updating glitches due to compositing.
> 
> I'm submitting this patch partly in hope it may be useful in bringing kf5-workspace to OS X, one day.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 195f99c 
>   kcontrol/CMakeLists.txt fc666b1 
>   kcontrol/krdb/krdb.cpp 36fc99c 
>   kcontrol/style/CMakeLists.txt d832b20 
>   libs/CMakeLists.txt c0576fe 
> 
> Diff: https://git.reviewboard.kde.org/r/120287/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.6.8 and 10.9.4 with KDE/MacPorts (4.12.5 and more recently kdelibs git/master, 4.14.1).
> 
> 
> File Attachments
> ----------------
> 
> copy of the diff file saved locally, which had no tabs when I uploaded it. Checksum: 3989cdd46af3c891e570974d66c330403dcd41c4ee5e17a372fa385080cbabd1 
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/20/b212730f-6258-4277-851c-226bc0673aa1__patchreview-20140920.patch
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140920/edd0ae7a/attachment.htm>