decibel

[Tobias Hunger]

Hi David!

Thanks for taking the time to review Decibel! I do appreciate that a lot. I'll 
fix the issues you brought up ASAP.

On Monday 05 May 2008, David Faure wrote:
> On Sunday 17 February 2008, Tobias Hunger wrote:
> > Decibel being in kdereviews was not yet officially announced,
>
> I had a quick look....
>
> decibel/include/Decibel/{errors.h,dbusnames.h,accountdata.h} contain static
> objects, bad idea (especially in header files! those objects get duplicated
> in every user of the file, don't they?), use a singleton with
> K_GLOBAL_STATIC, or use const char* for all this.

I'll use const char *. Since the decibel daemon is meant to not rely on KDE4 
itself (

> types.h uses Q_DECL_EXPORT, which is enough for unix, but not portable to
> Windows, you need a lib-specific export macro

Aehm... yeap. I'll fix this.

> qWarning() << QObject::tr("Failed to setup handler for SIGTERM!"); }
> We don't usually translate qWarning messages. End users don't see such
> messages, and many translators are going to ask you what SIGTERM means :)
> If it's too technical for translators it's definitely too technical for end
> users.

Hmmm... I did that since Decibel is a daemon process only, so that is the only 
thing that can get translated for the main thing. If that is not wanted I can 
remove that, no problem.

> > * This library is disQObject::tributed in the hope that it will be
> > useful,
>
> Massive search/replace alert :)

AUTSCH! Where is my brown paperback?!

> > static const QString filename("~/.decibel_contact_data.ini");
>
> No static qstrings. No dot files in $HOME if you can avoid it, please use
> KStandardDirs.

That code is not relevant for KDE. Akonadi will be used there.

> >    static const QString
> > component_search_path("~/.local/share/Decibel/components;"
> > "/usr/local/share/Decibel/components;" "/usr/share/Decibel/components;"
> > COMPONENT_SEARCH_DIR);
>
> Same there.
> Why isn't this in share/apps/Decibel? Then you could use something like
> KStandardDirs::locate("data","Decibel/components").

I'll add the apps. KStandardDirs is not an option as the decibel daemon is 
meant to be independent of KDE4. Only the code of the addons, demos and the 
KDE4 integration code may use KDE libs.

> >                QString key;
> >                QStringList keys = settings.allKeys();
> >                foreach (key, keys)
>
> Don't declare a local variable that could be used by mistake after the
> foreach loop; prefer a const & for performance, and a const QStringList to
> avoid detaching.
> ==>
>     const QStringList keys = settings.allKeys();
>     foreach(const QString& key, keys)
>
> Same for all similar constructs in the code.

Yeap, that code is not really efficient. It is not relevant for KDE4 though.

> Should Decibel follow the kdelibs coding styleguide? The placement of {} 
looks very unusual at least:

> Double lookup, prefer using an iterator in general, or in this case, a
> simple call to value() would do the job.
>
> >        QString profile_name;
> >        foreach (profile_name, profiles.keys())  // [where profiles is a
> > QHash] {
> >            [...]
> >            for (int i = 0; i < profiles[profile_name].size(); ++i)
> >            {
> >                settings.setArrayIndex(i);
> >
> >                // display_name:
> >                settings.setValue(display_name_key,
> >                                  profiles[profile_name][i].display_name);
> >                // protocols:
> >                settings.setValue(protocols_list_key,
> >                                 
> > profiles[profile_name][i].protocol_list.join(QString(';')));
>
> The lookup profiles[profile_name] is done everywhere; first step would be
> to use a local variable instead. Even better would be to iterate using an
> iterator instead of iterating over profiles.keys() (which has to construct
> a list first, this can be really slow, as discussed here recently).

Yeap, I should fix this.

> >    if (filename.startsWith('~'))
> >    {
> >        QString home(qgetenv("HOME"));
> >        use_filename = home + filename.mid(1);
> >    }
>
> This code is duplicated everywhere. Make a helper method? Or use KShell
> from kdecore, I think it can do this.

Good point. KShell is not an option again:-/

> >     app.setOrganizationName(Decibel::organisation_name);
>
> Smells like a mix of US and UK english ;-) Should be
> Decibel::organization_name for consistency.

Hmmm... my english is not good enough to notice something like that. I'll 
change that.

> OK. I wish I could comment on the overall design but that's much harder to
> do ;) Didn't see anything particularly striking, which is probably a good
> sign.

Phew!

Best Regards,
Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080506/fc173f3a/attachment.sig>