Don't change the proxy maps prior to insert/remove transactions so
rowCount() at transaction start time matches the count passed into
the method. The old code hits an ASSERT in Qt 5.11 otherwise.
Care is taken not to emit dataChanged() in the middle of another
transaction.
Also cleaned up vestiges of caching lastRow(): This cache was
evicted all over the places but never actually filled anymore, so
maybe we don't need it.
BUG:396666
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
In terms of the part that's changed, it does seem to do the same as before in a more correct way.
In terms of the bigger picture, I am really struggling to follow what this class is doing. Can you please add some comments on the overall proxy structure.
I probably should at some point. Unfortunately it also got a lot more complicated with the addition of multi-screen support.
But basically, it's a proxy in use when Folder View is used on the desktop and allows moving icons around. The view is a Grid View. The source model is a list. It maps from positions the user drops things to to rows in the source model. The proxy maps only concern themselves with positions that map to real entries in the source model. The sparse gaps inbetween icons are not part of the maps, they just return empty data and true for the special "IsBlank" data role. The delegate implementation in the view is behind a Loader that does nothing when IsBlank is true.
Multiscreen works by further associating source rows with a screen and filtering in a screen-dependent way.
There's some other things it needs to do, such as when a new file appears it needs to figure out the first free space and map it there, etc.
This actually seems to break re-arranging files, at least on Qt 5.11.1:
All files right-of the one inserted disappear until Plasma is restarted
This actually seems to break re-arranging files, at least on Qt 5.11.1:
Reverting this patch makes it work againAll files right-of the one inserted disappear until Plasma is restarted
@hein ping! Can confirm. Might be nice to get an autotest for this?
I'll take this back up in September after my vacation (unless I get bored inbetween which is unlikely).
Alright, just important to get this fixed for 5.14, if all else fails we can still revert this, as it only asserts on debug builds
Oh shit, I forgot it was already commmitted, I though it was still stuck in review.
This patch is also on the 5.12 and 5.13 branches, so this makes fixing it a bit more urgent.
Kai/David, can I delegate this to you due to my vacation? Either fix or revert before it goes into a release ... so sorry.
Since this is on the stable branches, IMHO we should really revert it before it accidentally makes it into a release. 5.13.5 is scheduled for release in two weeks, and 5.12.7 in five. Any objections?
as it caused problems, has been reverted,
but since is a real issue that needs to be properly adressed, reopening the review