[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    Re: branches/work/~ervin/zanshin
From:       Kevin Ottens <ervin () kde ! org>
Date:       2008-12-31 7:24:11
Message-ID: 200812310824.11402.ervin () kde ! org
[Download RAW message or body]


On Tuesday 30 December 2008 22:05:44 Mario Bensi wrote:
> SVN commit 903559 by bensi:
>
> - add onSourceInsertRows slot
> - add onSourceRemoveRows slot
> - add test on cycle when we change the parentRemoteId
>
> [...]
> --- branches/work/~ervin/zanshin/tests/todoflatmodeltest.cpp #903558:903559
> [...]
> +   QTest::newRow("cycle") <<  11 << (int)TodoFlatModel::ParentRemoteId <<
> "fake-02" << false; }

Please provide a better name to this test. "Cycle Prevention" or something 
like that would be more explicit.

>  [...]
> --- branches/work/~ervin/zanshin/todoflatmodel.cpp #903558:903559
> [...]
> +void TodoFlatModel::onSourceRemoveRows(const QModelIndex&/*sourceIndex*/,
> int begin, int end) +{
> +    for (int i = begin; i <= end; ++i) {
> +        QModelIndex sourceIndex = index(i, 0);
> +        Akonadi::Item item = itemForIndex(sourceIndex);
> +        m_parentMap.remove(item.id());
> +    }
> +}

Here you probably also should cleanup m_remoteIdReverseMap.

> [...]
> --- branches/work/~ervin/zanshin/todoflatmodel.h #903558:903559
> [...]
> +private slots:
> +    void onSourceInsertRows(const QModelIndex&, int, int);
> +    void onSourceRemoveRows(const QModelIndex&, int, int);

For consistency with the other headers (and other methods you added for that 
matter), please keep the parameters name of the methods also here.

> [...]
> +    bool hasCycle(Akonadi::Entity::Id parent, Akonadi::Entity::Id child);

I'd say this one is wrongly named. You're not testing if they have a cycle, 
but more if having "parent" as parent of "child" will create a cycle. I'd 
advise to modify this on in the following way:
bool isAncestorOf(Akonadi::Entity::Id child, Akonadi::Entity::Id parent);

Since that's what it does: verifying that the second parameter is an ancestor 
of the first parameter.
Of course this parameters permutation implies that the implementation will 
have to be slightly modified.

> +    QHash<Akonadi::Entity::Id, Akonadi::Entity::Id> m_parentMap;
> +    QHash<QString, Akonadi::Entity::Id> m_remoteIdReverseMap;

From the usage patterns of those in the class. I think it'd be more efficient 
to have:
> +    QHash<QString, QString> m_parentMap;
> +    QHash<Akonadi::Entity::Id, QString> m_remoteIdMap;

m_parentMap content would be based only on the remote ids. I think you'd spare 
a few hash look-ups this way.

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]