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