patches for custom styleelements, kcapacitybar

Maksim Orlovich mo85 at cornell.edu
Sat Sep 27 18:53:09 BST 2008


> ... are attached.

Pretty good idea, but I am afraid /not/ OK on the KStyle part. For
starters, you're doing a lot of string parsing in your methods, and that
behavior is undocumented (how is one supposed to know how to specify
subcontrols without reading the .cpp file?). I think you're also making it
far more complex than needed. Why does one need to support all of the
CC_/CE_/PE_ mess in the first place? Just having PEs and style hints, with
separate management methods would be sufficient. Further, reservations or
not, there is no guarantee that someone else isn't using the same
primitive numbers. This is actually somewhat dangerous, as you do a
qstyleoption_cast and extract data from it, so if some app is using the
same style hint ID, things will crash. qobject_cast<KStyle> is really the
only safe option (in the sense that it wouldn't make things crash any
worse than they already do if the app has a KStyle class). And if you do
that, then you can get rid of typo-prone string-based API in the first
place.

Anyway, the addSupportFor method should return the ID; returning a single
integer as an out reference parameter is idiomatically odd C++, and makes
the code using the method hard to read.

If you only support primitives and hints, and split the register/check
methods, then you can also type them with appropriate enums.

(Oh, and signedness of enums is platform-dependent, and the range of the
underlying type depends on the range of declared enumerants; they can
easily be 8-bit).

Please also write proper API docs, using the Doxygen tags. There is no
need to shout at the user, too --- they know non-virtual method shouldn't
be reimplemented, since they can't be in the first place.

There are also many major problems with the coding style:
Please do not use #define's for constants. C++, unlike C has a proper
'const'.
Please do not use names like element_counter, but rather elementCounter.
Please do not write code that has multiple sideeffects in one line, e.g.:
 d->styleElements.insert(customElement, id =
++d->element_counter[KStylePrivate::PE]);
It's unreadable, and has underspecified semantics.

KCustomStyleELement has a typo in the name. (There are also quite a few in
comments --- no big deal, but might as well fix them while looking at a
patch...)

Thanks for your contribution,
Maks






More information about the kde-core-devel mailing list