[PATCH] KBookmarks / fd.o desktop-bookmark-spec

[Tobias Koenig]

On Thu, Mar 06, 2008 at 01:23:26AM +0100, nf2 wrote:
Hej,

> +void KBookmarkManager::startWatch() const
> +{
> +    // only used for external bookmarks
> +    if (!d->m_watchStarted)
> +    {
> +      if (!d->m_kDirWatch)
> +        d->m_kDirWatch = new KDirWatch;
> +      
> +      d->m_kDirWatch->addFile(d->m_bookmarksFile);
> +      QObject::connect( d->m_kDirWatch, SIGNAL(dirty(const QString&)),
> +               this, SLOT(slotFileChanged(const QString&)));
> +      QObject::connect( d->m_kDirWatch, SIGNAL(created(const QString&)),
> +               this, SLOT(slotFileChanged(const QString&)));
> +      QObject::connect( d->m_kDirWatch, SIGNAL(deleted(const QString&)),
> +               this, SLOT(slotFileChanged(const QString&)));
> +      
> +      d->m_watchStarted = true;
> +      kDebug(7043) << "KBookmarkManager::startWatch " << d->m_bookmarksFile;
> +
> +    }
> +}
Don't mark a method as 'const' when it changes the internal state of a
class!

>  KBookmarkManager::~KBookmarkManager()
>  {
>      if(!s_pSelf.isDestroyed())
>          s_pSelf->removeAll( this );
> +    if (d->m_kDirWatch)
> +      delete d->m_kDirWatch;
>      delete d;
>  }
If m_kDirWatch is initialized properly, a 'delete d->m_kDirWatch' is
enough. Best would be to move it in the dtor of the private class.

>  QString KBookmark::metaDataItem( const QString &key ) const
>  {
> -    QDomNode infoNode = cd_or_create( internalElement(), "info" );
> -    infoNode = findOrCreateMetadata( infoNode );
> -    for ( QDomNode n = infoNode.firstChild(); !n.isNull(); n = n.nextSibling() ) {
> +    QDomNode metaDataNode = metaData(METADATA_KDE_OWNER, false);
> +    for ( QDomNode n = metaDataNode.firstChild(); !n.isNull(); n = n.nextSibling() ) {
>          if ( !n.isElement() ) {
>              continue;
>          }
Would it make sense to use
  for ( QDomeElement e = metaDataNode.firstChildElement(); !e.isNull(); e = next.SiblingElement() )
here if you are only interested in elements anyway?

Ciao,
Tobias
-- 
Separate politics from religion and economy!
The Council of the European Union is an undemocratic and illegal institution!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080306/ac4ce7c3/attachment.sig>