D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni abandoned this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni added a comment.


  I investigate on testing this patch, it does not fix systray issue, to 
discard it?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 49630.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18249?vs=49469&id=49630

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

AFFECTED FILES
  src/declarativeimports/core/datamodel.cpp

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in datamodel.cpp:379
> Optimization is you have [5, 6, 8] then [5, 6, 8, 9] so it make only 
> beginInsertRows({}, 3, 3);
> full replace (m_items[sourceName] = list.toVector();)
> endInsertRows()
> Same if you [5, 6]
> beginRemoveRows
> full replace (m_items[sourceName] = list.toVector();)
> endRemoveRows
> 
> It does not touch either m_roleNames size, which are changed.
> 
> I try to remove all first, then re-add with m_roleNames changes.

The worst case is when you have different values [5, 6, 8] and [5, 8, 9, 10] it 
will add only 10 and internally replace other, is QML model expect that? So why 
i first make full remove, then full insert.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> davidedmundson wrote in datamodel.cpp:379
> I don't understand what you're changing here, can you provide a bit more 
> detail on the exact problem.

Optimization is you have [5, 6, 8] then [5, 6, 8, 9] so it make only 
beginInsertRows({}, 3, 3);
full replace (m_items[sourceName] = list.toVector();)
endInsertRows()
Same if you [5, 6]
beginRemoveRows
full replace (m_items[sourceName] = list.toVector();)
endRemoveRows

It does not touch either m_roleNames size, which are changed.

I try to remove all first, then re-add with m_roleNames changes.

> davidedmundson wrote in datamodel.cpp:444
> this isn't equivalent.
> 
> I could have an entry with an empty count
> 
> Now this gets left behind

Got'cha

REPOSITORY
  R242 Plasma Framework (Library)

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

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> datamodel.cpp:55-61
> +const QHash roles = sourceModel()->roleNames();
> +m_roleIds.reserve(roles.count());
> +for (auto i = roles.constBegin(); i != roles.constEnd(); ++i) {
>  m_roleIds[QString::fromUtf8(i.value())] = i.key();
>  }
>  
> +setRoleNames(roles);

This part, ship it! but I don't think it'll make a huge difference to anything

> datamodel.cpp:77
>  
> +beginResetModel();
> +

This shouldn't be needed

setSourceModel will will do a reset internally

> datamodel.cpp:379
> +
> +if (oldLength > 0) {
> +beginRemoveRows(QModelIndex(), sourceIndex, sourceIndex + oldLength 
> - 1);

I don't understand what you're changing here, can you provide a bit more detail 
on the exact problem.

> datamodel.cpp:444
>  beginRemoveRows(QModelIndex(), sourceIndex, sourceIndex + 
> count - 1);
> -}
> -m_items.remove(sourceName);
> -if (count > 0) {
> +m_items.remove(sourceName);
>  endRemoveRows();

this isn't equivalent.

I could have an entry with an empty count

Now this gets left behind

REPOSITORY
  R242 Plasma Framework (Library)

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

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

anthonyfieroni created this revision.
anthonyfieroni added reviewers: davidedmundson, broulik, ngraham, mart, Plasma.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
anthonyfieroni requested review of this revision.

REVISION SUMMARY
  I give a try to rework insertion / deletion of items in datamodel which is 
used in systemtray. This continues approach from D16890 
, what the problem is
  
  1. Right click systemtray -> configure
  2. Click on Entries
  3. Click Ok
  
  Even only touching model dismissing dialog made index after invalid, now 
clicking on any entry in systray result in invalid index
  
  Old approach tries to make optimization but that's not a right place since 
QML is time-to-time rewritten itself, also it sets role names incorrect to me.

TEST PLAN
  Still not complete tested

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/declarativeimports/core/datamodel.cpp

To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns