KScreenGenie moved to KDE Review
Boudhayan Gupta
me at baloneygeek.com
Sat Jul 4 12:29:11 BST 2015
Hi Alexander,
> It doesn't seem right to apply different rules to headers that are
> exported and not exported. Are you going to rewrite the includes
> if/when the class ImageGrabber becomes a public library? (E.g. when
> someone wants to incorporate a screen grabber into his application.)
Nothing in KScreenGenie will ever become a public library.
If someone wants a screen grabber in his application, the correct way
to do it is by interfacing with KSG via DBus (which, btw, it lacks
currently, but will have in the future).
In a public library, things are much more strict - no extra includes,
dpointers etc., filenames are lowercase, and so on. It doesn't have to
be so strict here.
> You said "I can see exactly which components a single cpp file depends
> on my looking at the headers". If you follow the recommendation in the
> links [1,2], all the dependent components will be included in the .cpp
> file, so you can see exactly what you want there. Any other issue with
> it?
No, some of them stay over in the header file. "Everything in one
place", for a small application such as this, which is not a public
library, takes precedence over minorly increased build times.
> And finally, why using distinct coding style while there's established
> Kdelibs coding style? Is your coding style documented somewhere? If
> not, then I'm sure KSG will become a mix of different coding styles in
> a while because people don't know what coding style to follow.
It's kdelibs, with two exceptions: Cumulative headers, and all the
access modifiers (public, private etc) stay aligned with the function
definitions in the class (i.e., 1 level indented).
BTW I've gone ahead and reverted the commit and parts of your other
commits. The only thing that was required, was to make sure in every
place X11ImageGrabber is included, that it should be included only if
XCB_FOUND is defined.
I realise that you have been a KDE developer for far longer than I
have, but I would still like to refer you to the KDE Commit Policy
[1], sections 1.8 and 1.13. Personally I make it a rule to use Review
Board if I submit even one-liners, because I believe the maintainer
should be made aware, in advance, of every single change other devs
are making to his code, and be given a chance to approve/reject that.
However, I do not require that for KScreenGenie (in fact, quite a few
other devs have made minor changes, both to code and to the docs, and
I have nothing but sincere thanks for them for fixing these issues).
That said, having to go through the commitdiffs for *four* unannounced
commits, two of which were major, to find the *one* location where the
real fix was and isolating that, is a huge waste of time which I could
have otherwise used to make important pending fixes to the UI and
program behaviour.
-- Boudhayan
More information about the kde-core-devel
mailing list