D8855: Use Kio::KPlacesModel as source model for PlacesItemModel

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  some small nits and cleanup suggestions from my side, otherwise lgtm.

INLINE COMMENTS

> placesitem.cpp:159
> +    const QString urlScheme = url().scheme();
> +    return (urlScheme.contains("search") || urlScheme.contains("timeline"));
>  }

shouldn't this be equality comparisons instead of contains checks? are there schemas like "foobar-search-asdf" that need to be matched too?

> placesitemmodel.cpp:153
> +
> +    for(int r = 0; r < m_sourceModel->rowCount(); r++) {
> +        sourceIndex = m_sourceModel->index(r, 0);

`for (int r = 0, c = m_sourceModel->rowCount(); r < c; ++r) {`

don't call rowCount on every iteration

> placesitemmodel.cpp:408
>  
> -    const int boomarkIndex = bookmarkIndex(index);
> -    Q_ASSERT(!m_bookmarkedItems[boomarkIndex]);
> -    m_bookmarkedItems.removeAt(boomarkIndex);
> -
> -#ifdef PLACESITEMMODEL_DEBUG
> -    qCDebug(DolphinDebug) << "Removed item" << index;
> -    showModelState();
> -#endif
> -}
> -
> -void PlacesItemModel::onItemChanged(int index, const QSet<QByteArray>& changedRoles)
> -{
> -    const PlacesItem* changedItem = placesItem(index);
> -    if (changedItem) {
> -        // Take care to apply the PlacesItemModel-order of the changed item
> -        // also to the bookmark-manager.
> -        const KBookmark insertedBookmark = changedItem->bookmark();
> -
> -        const PlacesItem* previousItem = placesItem(index - 1);
> -        KBookmark previousBookmark;
> -        if (previousItem) {
> -            previousBookmark = previousItem->bookmark();
> +    for (int i = 0; i < count(); ++i) {
> +        if (bookmarkId(placesItem(i)->bookmark()) == id) {

dito, don't call count on every iteration

> placesitemmodel.cpp:427
> +{
> +    for(int i = 0; i < m_sourceModel->rowCount(); i++) {
> +        const QModelIndex index = m_sourceModel->index(i, 0);

dito

> placesitemmodel.cpp:592
> +{
> +    for(int r = 0; r < m_sourceModel->rowCount(); r++) {
> +        const QModelIndex sourceIndex = m_sourceModel->index(r, 0);

dito

> placesitemmodel.cpp:721
>  
> -    if (day >= 1) {
> -        date += '-';
> -        if (day < 10) {
> -            date += '0';
> +    for (int i = 0; i < m_indexMap.size(); i++) {
> +        if (m_indexMap.at(i) == index) {

dito, note that you could also just replace this all with a simple `return m_indexMap.indexOf(index);`, that works too, no? Otherwise try `std::find_if` + `std::distance`

> placesitemmodel.cpp:738
>  {
> -    QUrl searchUrl;
> -
> -#ifdef HAVE_BALOO
> -    const QString path = url.toDisplayString(QUrl::PreferLocalFile);
> -    if (path.endsWith(QLatin1String("documents"))) {
> -        searchUrl = searchUrlForType(QStringLiteral("Document"));
> -    } else if (path.endsWith(QLatin1String("images"))) {
> -        searchUrl = searchUrlForType(QStringLiteral("Image"));
> -    } else if (path.endsWith(QLatin1String("audio"))) {
> -        searchUrl = searchUrlForType(QStringLiteral("Audio"));
> -    } else if (path.endsWith(QLatin1String("videos"))) {
> -        searchUrl = searchUrlForType(QStringLiteral("Video"));
> -    } else {
> -        Q_ASSERT(false);
> +    if ((row < 0) || (row >= m_indexMap.size())) {
> +        return QModelIndex();

simplify this whole method to `return m_indexMap.value(row);`

> placesitemmodel.cpp:748
> +    const QString id = bookmarkId(bookmark);
> +    for (int i = 0; i < count(); i++) {
> +        PlacesItem *item = placesItem(i);

dito

> placesitemmodel.h:226
> +
> +    QList<QPersistentModelIndex> m_indexMap;
>  };

QVector, this QList would allocate every item on the heap

REPOSITORY
  R318 Dolphin

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

To: renatoo, elvisangelaccio, emmanuelp, mlaurent, mwolff
Cc: mwolff, mlaurent, anthonyfieroni, nicolasfella, ngraham, #dolphin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171206/516015d8/attachment.htm>