D8630: Created unit test for PlacesItemModel

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> placesitemmodeltest.cpp:114
> +{
> +    for(int r = 0; r < m_model->count(); r++) {
> +        if (m_model->placesItem(r)->url() == url)

nitpick: space after `for`

> placesitemmodeltest.cpp:115-116
> +    for(int r = 0; r < m_model->count(); r++) {
> +        if (m_model->placesItem(r)->url() == url)
> +            return r;
> +    }

nitpick: missing braces

> placesitemmodeltest.cpp:157
> +    delete m_model;
> +    m_model = 0;
> +}

`nullptr`

> placesitemmodeltest.cpp:170
> +    // Ensure we'll have a clean bookmark file to start
> +    QFile::remove(bookmarksFile());
> +

Wrap it in a `QVERIFY`

> placesitemmodeltest.cpp:203
> +    KBookmarkGroup root = bookmarkManager->root();
> +    KBookmark system_root = m_model->placesItem(1)->bookmark();
> +    KBookmark last = m_model->placesItem(m_model->count() - 1)->bookmark();

Please call it `systemRoot`.

> placesitemmodeltest.cpp:217
> +{
> +    auto groups = m_model->groups();
> +

`const`

> placesitemmodeltest.cpp:220-227
> +    QCOMPARE(groups[0].first, 0);
> +    QCOMPARE(groups[0].second.toString(), QStringLiteral("Places"));
> +    QCOMPARE(groups[1].first, 4);
> +    QCOMPARE(groups[1].second.toString(), QStringLiteral("Recently Saved"));
> +    QCOMPARE(groups[2].first, 8);
> +    QCOMPARE(groups[2].second.toString(), QStringLiteral("Search For"));
> +    QCOMPARE(groups[3].first, 12);

`.at()` instead of `[]` ?

> placesitemmodeltest.cpp:270
> +{
> +    const QUrl mediaUrl("file:///media/XO-Y4");
> +    int index = indexOf(mediaUrl);

QUrl::fromLocalFile ?

> placesitemmodeltest.cpp:274
> +
> +    auto actEject = m_model->ejectAction(index);
> +    QVERIFY(!actEject);

Please call it `ejectAction`

> placesitemmodeltest.cpp:277
> +
> +    auto act = m_model->teardownAction(index);
> +    QVERIFY(act);

Please call it `action` or `teardownAction`

> placesitemmodeltest.cpp:279
> +    QVERIFY(act);
> +    QCOMPARE(act->text(), QStringLiteral("Safely Remove"));
> +

What if the action text is translated?

> placesitemmodeltest.cpp:335
> +
> +    ViewProperties vprop(m_model->convertedUrl(url));
> +    QCOMPARE(vprop.viewMode(), expectedViewMode);

Please call it `properties` or `viewProperties`

> placesitemmodeltest.cpp:400
> +    QCOMPARE(m_model->count(), 17);
> +    for(int r = 0; r < m_model->count(); r++) {
> +        QCOMPARE(m_model->placesItem(r)->isSystemItem(), !m_model->placesItem(r)->device().isValid());

space after `for`

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D8630

To: renatoo, ervin, elvisangelaccio
Cc: elvisangelaccio, ngraham, #dolphin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171117/7990ee7d/attachment.htm>