decibel

[Tobias Hunger]

Hi David!

Thanks for taking the time to review Decibel! I do appreciate that a lot.

The decibel daemon itself should not depend on KDE. This was decided early in 
the planing phase so that there is a chance that Gnome will pick up the code 
and/or so that it can get used in e.g. a greenphone or similar device.

At compiletime you can decide on which environment the daemon should integrate 
in. Currently there is a "simplistic" option which uses ini files for data 
storage (and does so not that efficiently as I have to admit) and the KDE4 
integration mode. George is currently working on that, so that area is 
somewhat in a flux at this time. That code is in src/server/kde4 does of 
course depend on KDElibs etc., the rest does not. So I am doing some things 
that kdelibs would do for me as well.

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.

Hmmm... there is no static in my code, just const QStrings. Which version did 
you check out?

Basically I am using const QString as a kind of typesafe #define. I am pretty 
sure that the compiler will fix this up nicely.

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

Aehm, yes. Just fixed that in my local tree.

> 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.

The decibel daemon is a non-gui application. I've seen lots of them with 
localized messages, so I thought I'd better add the translation markers.

If this is in violation of KDE standards I can remove this.

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

/me searches for a big brown paper bag.

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

This code is in the simplistic connector implementation. It is not relevant in 
a KDE environment.

> >    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 can move that stuff into share/apps/Decibel. No problem. I will not use 
KStandardDirs::locate though for reasons cited above.

> >                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.

OK.

> Should Decibel follow the kdelibs coding styleguide? The placement of {} 
looks very unusual at least:
> >            if (i == profile.size())
> >            { profile.append(component); }

Hmmm. I can change that if that is a problem.

> 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. 

Done.

> 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).

I'll think about this for a bit.

> >    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.

I am doing that in a couple of places, that is right. I found it simpler than 
including some file all over the place. I'll check again, the code is 
probably more widely used now as it was when I made that decision.

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

OK, changed. My english is not up to the task of recognizing inconsistencies 
like that:-(

> > in demos/simpleclient/mytextchannelhandler.cpp:
> >    // use iterators since TextChannel::Message has no default constructor
> > yet...
>
> Well, wouldn't be a problem if using a const ref as the foreach variable.

I am doing that now;-)

> 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/1033989e/attachment.sig>