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

[Elvis Angelaccio]

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

INLINE COMMENTS

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

`contains()` has a `QLatin1String` overload which should be preferred.

> placesitemmodel.cpp:54-57
> -#ifdef HAVE_BALOO
> -    #include <Baloo/Query>
> -    #include <Baloo/IndexerConfig>
> -#endif

Please also remove `#include <config-baloo.h>` from the header file

> placesitemmodel.cpp:85-86
>  {
> -    qDeleteAll(m_bookmarkedItems);
> -    m_bookmarkedItems.clear();
> +    delete m_sourceModel;
> +    m_sourceModel = nullptr;
>  }

Maybe we can use a QScopedPointer for `m_sourceModel`?

> placesitemmodel.cpp:148
>  
> -    for (int i = 0; i < count(); ++i) {
> -        const QUrl itemUrl = placesItem(i)->url();
> -        if (url == itemUrl) {
> -            // We can't find a closer one, so stop here.
> -            foundIndex = i;
> +// look for the coorrect position for the item based on source model
> +void PlacesItemModel::insertSortedItem(PlacesItem* item)

typo: correct

> placesitemmodel.cpp:198
> +{
> +    const QModelIndex sIndex = mapToSource(index);
> +    const PlacesItem *changedItem = placesItem(mapFromSource(sIndex));

Please call it `sourceIndex`

> placesitemmodel.cpp:439
> +        // Create default view-properties for all "Search For" and "Recently Saved" bookmarks
> +        // in case if the user has not already created custom view-properties for a corresponding
> +        // query yet.

Sounds like `if` should not be in this sentence.

> placesitemmodel.cpp:551
>  
> -void PlacesItemModel::loadBookmarks()
> -{
> -    KBookmarkGroup root = m_bookmarkManager->root();
> -    KBookmark bookmark = root.first();
> -    QSet<QString> devices = m_availableDevices;
> +    const int blockSize = (end - start)  + 1;
>  

remove one space before `+`

> placesitemmodel.cpp:571
> +    for (int r = topLeft.row(); r <= bottomRight.row(); r++) {
> +        const QModelIndex sIndex = m_sourceModel->index(r, 0);
> +        const KBookmark bookmark = m_sourceModel->bookmarkForIndex(sIndex);

`sourceIndex`

> placesitemmodel.cpp:672
>      int dropIndex = index;
> -    const PlacesItem::GroupType type = item->groupType();
> +    const QString type = item->group();
>  

Please call it `group`

> placesitemmodel.cpp:853-855
> -        // As long as the KFilePlacesView from kdelibs is available, the system-bookmarks
> -        // for "Recently Saved" and "Search For" should be a setting available only
> -        // in the Places Panel (see description of AppNamePrefix for more details).

There is a similar comment where we define `AppNamePrefix`, should that be also removed?

> placesitemmodel.h:68-69
> +    /**
> +     * @brief Mark an item as hiden
> +     * @param index of the item to be hiden
> +     */

typo: hidden

> placesitemmodel.h:122
>  
> -    void clear() override;
> +    virtual void clear() Q_DECL_OVERRIDE;
>  

`override`

> placesitemmodeltest.cpp:152
>      QStringList urls;
> -    for (int row = 0; row < m_model->count(); ++row) {
> -        urls << m_model->placesItem(row)->url().toDisplayString(QUrl::PreferLocalFile);
> +    if (model == nullptr) {
> +        model = m_model;

`!model` ?

> placesitemmodeltest.cpp:193
> +{
> +    // user hardcoded path to avoid removal of any user persnonal data
> +    QDir dir(QStringLiteral("/home/renato/.qttest/share/placesitemmodeltest"));

typo: personal

REPOSITORY
  R318 Dolphin

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

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