decibel

[David Faure]

On Tuesday 06 May 2008, Tobias Hunger wrote:
> >
> > 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. 

Yeah sorry I said static because they are implicitly "file static" (I think) -- anyway
they are global objects, constructed when loading the library, which can create trouble.

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

And I am pretty sure this gives trouble in some cases.
http://bugs.kde.org/show_bug.cgi?id=93675 for example.

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

Hmm OK, but how does the above message tell anything useful to the user?
If I start the decibel daemon and it tells me "Failed to setup handler for SIGTERM!",
what do I do?

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

Only a problem if it does into kdelibs (cf the other part of the thread)

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

Err, that sounds like the easy way out. I insist ;) It's not that hard at all.

in the header:
typedef QHash<QString, QList<Decibel::Component> > ProfileHash;

in the code:
for (ProfileHash::const_iterator it = profiles.begin(); it != profiles.end(); ++it) {
      const QString profile_name = it.key();
      const QList<Decibel::Component> profile = it.value();
      [...]
}

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).