[RFC] Fix model updates in RunnerMatchesModel
ClosedPublic

Authored by davidedmundson on Dec 21 2018, 10:44 PM.

Details

Summary

RunnerMatchesModel is backed by a list. When this list changes
RunnerManagerModel add/removes the new number of rows then called
dataChanged on everything that remained.

It's a common pattern, but not a great one. Especially with QtQuick
where moving a delegate is faster than updating all the properties of an
existing one - unfortunately I can't find a nice solution to do this
properly in linear time.

The problem with the current code is we update the entire list in the
insert/remove rows. This is a violation of the model rules as we're
updating rows outside the rows listed inside begin/remove rows.

It works, but Qt's model test fails.

We also have a lot of duplicates of a crash in QtQuick after runner
model changes, bug 369430. I think it could be related, but can't
prove anything.

This patch updates the rows that exist in both the before and after
models before adding/removing the remaining rows.

BUG: 402439

Test Plan

Added Qt model tester in https://phabricator.kde.org/P283
It now passes.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Plasma. · View Herald TranscriptDec 21 2018, 10:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Dec 21 2018, 10:44 PM
davidedmundson edited the summary of this revision. (Show Details)Dec 21 2018, 10:52 PM
apol added a subscriber: apol.Dec 22 2018, 1:01 AM
apol added inline comments.
applets/kicker/plugin/runnermatchesmodel.cpp
225

Maybe it would make sense to only emit the ones that changed?

230

Can we specify which roles changed?

hein accepted this revision.Dec 22 2018, 10:05 AM
hein added a subscriber: hein.

I don't think it's necessary to optimize the dataChanged emits at this time - in the common case the roles the delegate is visualizing (label, icon) will always change, and the other rules are lazy-evaluated by the current UI (e.g. the context menu actions are constructed on-demand). Realistically, computing the roles delta to figure out the roles will likely eat more CPU time unless the users of the module show a different usage pattern. I'm OK with this with a "// TODO: Could be optimized by ... if needed" in the code.

This revision is now accepted and ready to land.Dec 22 2018, 10:05 AM
This revision was automatically updated to reflect the committed changes.