[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: which module would be most appropriate for my library?
From:       "Ben Cooksley" <sourtooth () gmail ! com>
Date:       2008-08-27 5:35:12
Message-ID: b366d7a00808262235o5c7109cfka83f810fd7ef793c () mail ! gmail ! com
[Download RAW message or body]

On 8/27/08, Aaron J. Seigo <aseigo@kde.org> wrote:
> On Tuesday 26 August 2008, Ben Cooksley wrote:
>> i think that i am ready to move from development stage 1 to stage 2,
>
> quick review:
>
> * setTranslucency should probably be setTranslucent; it's probably also not
> implemented correctly. setWindowOpacity will change the whole window's
> translucency, not just the background, which is probably what one really
> wants
> to do here. but to do that requires support from the application itself
> (setting up the argb visual, etc). so i'd recommend not bothering, and
> instead
> use a theming strategy that allows setting a translucent background image
> (like krunner or plasma do)
>
> * KOSDWidgetPrivate* d; should be private, and should be KOSDWidgetPrivate *
> const d;
>
> * the positioning system is a bit odd looking. why a 20x20 grid?
>
> * updateView() probably could be a Q_PRIVATE_SLOT instead of a protected
> slot.
> note that in lib headers, you need to use Q_SLOT rather than slot (and
> Q_SIGNALS instead of signals)
>
> * the defaultFoo() methods should be isDefaultFoo() as they return a boolean
> status rather than the default value of Foo.
>
> * the KOSDWidget constructor does not take a QWidget *parent; it probably
> should, even if it only uses it for memory management (e.g. doesn't set it
> to
> be the parent widget)
>
> * methods that don't change the state of the object need to marked const;
> e.g.
> QSize size(); should be QSize size() const;
>
> * objects like size in KOSDPrivate do not need to be pointers; they should
> just be references, so QSize *size would be QSize size
>
> * anything that you new, you have to delete unless it has a parent QObject
> or
> QWidget. so the QSize *size is being leaked; you need to delete those things
> in the destructor.
>
> * speaking of size, QWidget already manages all that for you. i don't see
> why
> you would need or want to reimplement all that. in fact, the way it is done
> will break because resize is not virtual in QWidget, so anything that calls
> resize on the KOSDWidget when it's just a QWidget* to it will Fail.
> suggestion: remove all the size related stuff. similar with palette and font
> handling. you can be notified of changes to these things by reimplementing
> QWidget::changeEvent.
>
> * the code uses this-> a lot; that's not really necessary =)
>
> * if your goal is kdelibs, the code should follow the coding style[1]
>
> * that said, i couldn't help but think that this would make a lot of sense
> as
> part of knotify rather than its own lib.
>
> * Why Are All The Words In The API Docs Capitalized? (points for at least
> having api docs though =)
>
> well, that's enough for now probably...
>
> [1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style
>
> --
> Aaron J. Seigo
> humru othro a kohnu se
> GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43
>
> KDE core developer sponsored by Trolltech
>
>

hi,

the i used a 20x20 grid to abstract away the size of the screen so
using apps didn't have to care about this minor detail ( so everyone
doesn't have to have to calculate the position themselves.

api docs: fixed
memory leaking: think its fixed.
too many pointers: fixed
boolean methods ( isDefaultFoo() ): fixed
const returns: fixed

thanks for the review,
bcooksley
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]