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

[nf2]

Tobias Koenig wrote:
> 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!
>
>   

Unfortunately this (private) method has to be called from another const 
method (KBookmarkManager::saveAs()). Changing it to non-const would 
probably break the API, but i'm not an expert here.

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

d->m_kDirWatch is not always set. I only need it in the 
"external-bookmark-file" mode. But i could move the delete to the dtor 
of the private class. That's right.

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

Don't know. Didn't write that code. Perhaps it would be better to 
beautify such things in a second go.

Cheers,
Norbert