D18182: Fix new file creation leading to dupe items on a fresh view

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> davidedmundson wrote in foldermodel.cpp:170
> Also I don't think I understand this:
> 
> - Delay this via queued connection, such that the row is available and can be 
> mapped
> 
> at the point of rowsInserted() the row should be available, If not we have a 
> source model bug.

like I said, in 
https://phabricator.kde.org/R119:cc9c3d32a381980e367aa893c6796d611c7a33a7 the 
Qt::QueuedConnection was removed from connection, but the comment was unchanged

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain, davidedmundson
Cc: ngraham, davidedmundson, fvogt, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:4a3fbf9116f5: Fix new file creation leading to dupe items 
on a fresh view (authored by hein).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18182?vs=50227&id=50640

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

To: hein, #plasma, McPain, davidedmundson
Cc: ngraham, davidedmundson, fvogt, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18182: Fix new file creation leading to dupe items on a fresh view

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.

INLINE COMMENTS

> davidedmundson wrote in foldermodel.cpp:170
> I'm a bit worried about queuing something with indexes. Indexes are only 
> valid at that exact moment.
> 
> If the source model does the following:
> beginInsertRows(AA);
> endInsertRows()
> beginInsertRows(BB);
> endInsertRows()
> then exits back to the event loop
> 
> When you process the delayed connection for the first insertion the old 
> first/last could be pointing anywhere.
> 
> We could maybe get round this by converting to QPeristentModelIndexes 
> immediately, then queue up the operation that uses them.

Also I don't think I understand this:

- Delay this via queued connection, such that the row is available and can be 
mapped

at the point of rowsInserted() the row should be available, If not we have a 
source model bug.

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain, davidedmundson
Cc: ngraham, davidedmundson, fvogt, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Nathaniel Graham]

ngraham added a comment.


  You should resign as reviewer then (it's under the Add Action... menu button).

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain
Cc: ngraham, davidedmundson, fvogt, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Oleg Solovyov]

McPain added a comment.


  Somebody review this for me, please, I'm too far away from actually testing it

REPOSITORY
  R119 Plasma Desktop

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

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

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein updated this revision to Diff 50227.
hein added a comment.


  Don't queue the connections.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18182?vs=49500&id=50227

BRANCH
  arcpatch-D18182

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

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

D18182: Fix new file creation leading to dupe items on a fresh view

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> foldermodel.cpp:170
>  connect(this, &QAbstractItemModel::rowsInserted,
>  this, [this](const QModelIndex &parent, int first, int last) {
>  for (int i = first; i <= last; ++i) {

I'm a bit worried about queuing something with indexes. Indexes are only valid 
at that exact moment.

If the source model does the following:
beginInsertRows(AA);
endInsertRows()
beginInsertRows(BB);
endInsertRows()
then exits back to the event loop

When you process the delayed connection for the first insertion the old 
first/last could be pointing anywhere.

We could maybe get round this by converting to QPeristentModelIndexes 
immediately, then queue up the operation that uses them.

REPOSITORY
  R119 Plasma Desktop

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

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

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein added a comment.


  The QueuedConnection is fine, though. Why remove it?

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Oleg Solovyov]

McPain requested changes to this revision.
McPain added a comment.
This revision now requires changes to proceed.


  That's what I got with this patch.
  F6563817: изображение.png 
  
  Please remove Qt::QueuedConnection from both connections.
  Additionally, remove the references from comments

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> fvogt wrote in foldermodel.cpp:154
> It doesn't use Qt::QueuedConnection explicitly - is the comment wrong or is 
> it done implicitly?

According to e94888701 
 and 
cc9c3d32a 
, 
the comment is wrong.

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein requested review of this revision.
hein added a comment.


  Requesting re-review due to significant behavior changes following @fvogt's 
comment

REPOSITORY
  R119 Plasma Desktop

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

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein updated this revision to Diff 49500.
hein added a comment.


  Fix comments and make code reflect comment intent

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18182?vs=49234&id=49500

BRANCH
  Plasma/5.12

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> foldermodel.cpp:152
>  
>  /*
>   * position dropped items at the desired target position

The comment refers to the first connect now, but is mostly aimed at the second. 
Maybe move it and add another one for the added connect call?

> foldermodel.cpp:154
>   * position dropped items at the desired target position
>   * delay this via queued connection, such that the row is available and 
> can be mapped
>   * when we emit the move request

It doesn't use Qt::QueuedConnection explicitly - is the comment wrong or is it 
done implicitly?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.12

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

To: hein, #plasma, McPain
Cc: fvogt, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Oleg Solovyov]

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


  It works, thanks.
  Removed D17689  and D17707 
 patches so I close then

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.12

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

To: hein, #plasma, McPain
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein updated this revision to Diff 49234.
hein added a comment.


  Forgot a hunk when moving to 5.12 branch.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18182?vs=49233&id=49234

BRANCH
  Plasma/5.12

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

To: hein, #plasma, McPain
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein updated this revision to Diff 49233.
hein edited the summary of this revision.
hein added a comment.


  Add BUG to message.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18182?vs=49232&id=49233

BRANCH
  Plasma/5.12

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

To: hein, #plasma, McPain
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18182: Fix new file creation leading to dupe items on a fresh view

[Eike Hein]

hein created this revision.
hein added reviewers: Plasma, McPain.
Herald added a project: Plasma.
hein requested review of this revision.

REVISION SUMMARY
  This was a regression caused by the code attempting to insert new items
  at drop position, if available. `setSortMode` was being called in a slot
  connected to the dir model's rowsInserted, but the Positioner has to be
  initialized earlier as a proxy needs to handle
  `sourceRowsAboutToBeInserted` as well.
  
  Thanks to an investigation and patch by Oleg Solovyov in D17689 
 for
  helping to get to the bottom of this.
  
  This is aimed at 5.12+.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.12

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp

To: hein, #plasma, McPain
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart