> 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]
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 - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120287/#review67023 ----------------------------------------------------------- On Sept. 19, 2014, 10:05 nachm., René J.V. Bertin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120287/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2014, 10:05 nachm.) > > > 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). > > > Thanks, > > René J.V. Bertin > >
