D14243: Fix bad model hygiene in Positioner::move()

[Marco Martin]

mart reopened this revision.
mart added a comment.
This revision is now accepted and ready to land.


  as it caused problems, has been reverted,
  but since is a real issue that needs to be properly adressed, reopening the 
review

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: mart, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D14243: Fix bad model hygiene in Positioner::move()

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Kai Uwe Broulik]

broulik added a comment.


  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

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Eike Hein]

hein added a comment.


  I'll take this back up in September after my vacation (unless I get bored 
inbetween which is unlikely).

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Nathaniel Graham]

ngraham added a comment.


  In D14243#306922 , @broulik wrote:
  
  > This actually seems to break re-arranging files, at least on Qt 5.11.1:
  >  F6188387: Screenshot_20180812_103125.png 

  >  Reverting this patch makes it work again
  >
  > All files right-of the one inserted disappear until Plasma is restarted
  
  
  @hein ping! Can confirm. Might be nice to get an autotest for this?

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Kai Uwe Broulik]

broulik added a comment.


  This actually seems to break re-arranging files, at least on Qt 5.11.1:
  F6188387: Screenshot_20180812_103125.png 

  Reverting this patch makes it work again
  
  All files right-of the one inserted disappear until Plasma is restarted

REPOSITORY
  R119 Plasma Desktop

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

To: hein, davidedmundson
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Eike Hein]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:1cb71a2cca8a: Fix bad model hygiene in Positioner::move() 
(authored by hein).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14243?vs=38124&id=38895

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

AFFECTED FILES
  containments/desktop/plugins/folder/positioner.cpp
  containments/desktop/plugins/folder/positioner.h

To: hein, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14243: Fix bad model hygiene in Positioner::move()

[Eike Hein]

hein created this revision.
hein added a reviewer: davidedmundson.
Restricted Application added a project: Plasma.
hein requested review of this revision.

REVISION SUMMARY
  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:39

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/plugins/folder/positioner.cpp
  containments/desktop/plugins/folder/positioner.h

To: hein, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart