Bug 401023 - Duplicated icons after creating a new folder
Summary: Duplicated icons after creating a new folder
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Containment (show other bugs)
Version: master
Platform: Other Linux
: VHI grave
Target Milestone: 1.0
Assignee: Sebastian Kügler
URL:
Keywords: usability
Depends on:
Blocks:
 
Reported: 2018-11-14 08:00 UTC by Oleg Solovyov
Modified: 2019-02-08 16:04 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.12.8, 5.14.6, 5.15.0
Sentry Crash Report:


Attachments
Duplicated icon (426.25 KB, image/png)
2018-11-14 08:00 UTC, Oleg Solovyov
Details
Reproduction of the issue (1.48 MB, video/webm)
2018-11-18 05:23 UTC, Nate Graham
Details
patch (1.59 KB, patch)
2018-12-19 14:18 UTC, Oleg Solovyov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Solovyov 2018-11-14 08:00:54 UTC
Created attachment 116301 [details]
Duplicated icon

SUMMARY
When I create a new folder between two existing adjacent icons the one I just created is duplicated like in BUG#398864

STEPS TO REPRODUCE
1. Create a new user
2. Log in 
3. Create a new folder between existing icons

OBSERVED RESULT
The icon just created is duplicated but I can iteract with only one copy of it (see attachment)

EXPECTED RESULT
Icon should not be duplicated

SOFTWARE/OS VERSIONS
Linux: 4.19.1-un-def-alt1
KDE Plasma Version: 5.12.7
KDE Frameworks Version: 5.51.0
Qt Version: 5.11.2

ADDITIONAL INFORMATION
I successfully reproduced that error in master branch and the NEON Developer Edition
Comment 1 Nate Graham 2018-11-15 22:35:54 UTC
It's the same issue, it's just that the fix isn't released yet. It'll be fixed in 5.12.8, but you still have 5.12.7, because 5.12.8 isn't released yet. If you need relief now, ask your distro to backport the fix.

*** This bug has been marked as a duplicate of bug 398864 ***
Comment 2 Oleg Solovyov 2018-11-16 07:39:31 UTC
It's not the same issue.

I already patched my 5.12.7 against #398864 and _this_ issue still reproduces, while 398864 is fixed.
Comment 3 Nate Graham 2018-11-18 05:17:16 UTC
Darn.

Eike or Kai, any ideas?
Comment 4 Nate Graham 2018-11-18 05:18:01 UTC
Darn.

Eike or Kai, any ideas?
Comment 5 Nate Graham 2018-11-18 05:23:28 UTC
Created attachment 116377 [details]
Reproduction of the issue

I just accidentally reproduced and confirmed the issue by copying four items to the desktop. Thereafter, duplicates of all four are created, and interacting with one also modified its phantom copy. It's really weird.
Comment 6 Oleg Solovyov 2018-11-27 11:56:47 UTC
Reproduced this on Plasma 5.14.3 + Qt 5.9.6
Comment 7 Oleg Solovyov 2018-12-05 08:23:34 UTC
Found a possible clue: it happens every time I create an icon when ~/.config/plasma-org.kde.plasma.desktop-appletsrc is missing
Comment 8 Oleg Solovyov 2018-12-05 13:36:06 UTC
Found a typo in 98d56d913:

@@ -547,6 +566,8 @@ void Positioner::sourceRowsAboutToBeInserted(const QModelIndex &parent, int star
         }
     } else {
         emit beginInsertRows(parent, start, end);
+        beginInsertRows(parent, start, end);
+        m_beginInsertRowsCalled = true;
     }
 }
 
Is it intentional or committer forgot to remove emit in that patch?
Comment 9 Nate Graham 2018-12-07 00:46:18 UTC
Oleg, does changing that fix the issue?
Comment 10 Oleg Solovyov 2018-12-07 08:32:20 UTC
(In reply to Nate Graham from comment #9)
> Oleg, does changing that fix the issue?

Unfortunately, no
Comment 11 Oleg Solovyov 2018-12-07 15:16:35 UTC
Commented out the whole connect at foldermodel.cpp:157 and it fixes the issue O_o
Comment 12 David Edmundson 2018-12-07 15:21:45 UTC
>Is it intentional or committer forgot to remove emit in that patch?

emit is a #define for ""

it does nothing, it's just a nice hint if you read the code that we're releasing flow control and other things might run.
Comment 13 Oleg Solovyov 2018-12-07 15:23:46 UTC
(In reply to David Edmundson from comment #12)
> >Is it intentional or committer forgot to remove emit in that patch?
> 
> emit is a #define for ""
> 
> it does nothing, it's just a nice hint if you read the code that we're
> releasing flow control and other things might run.

that's not the point. The point is calling the function twice. Is it ok?
Comment 14 Oleg Solovyov 2018-12-07 15:37:33 UTC
(In reply to Oleg Solovyov from comment #11)
> Commented out the whole connect at foldermodel.cpp:157 and it fixes the
> issue O_o

also commented out the connect ap positioner.cpp:912 and it fixes the issue again.

Looks like the slots were invoked by the same signal are doing the same job
Comment 15 David Edmundson 2018-12-07 16:48:13 UTC
Oh, I misunderstood.

It certainly doesn't look intentional.
Comment 16 Eike Hein 2018-12-08 11:58:57 UTC
Indeed, looks like a bona-fide bug. David, since you're currently working on this code, can you roll it into your other patch or so?
Comment 17 Oleg Solovyov 2018-12-11 15:28:36 UTC
The bug is related somehow with setSortMode(int) function in lambda I mentioned abvoe
Comment 18 Oleg Solovyov 2018-12-12 14:52:38 UTC
meh, it's very complex to find out where's the problem.

Noticed that there are two separated dirlisters working on the desktop

log file: https://pastebin.com/xSAxYQqp
patch with my own debug messages: https://pastebin.com/0nMdDZci
Comment 19 Oleg Solovyov 2018-12-18 13:57:16 UTC
Any good ideas how to create config after first init of FolderModel it config is not exist (after first login)?

Tried to write config at onCompleted, but it's too early.

It might fix the problem
Comment 20 Oleg Solovyov 2018-12-19 14:18:53 UTC
Created attachment 117009 [details]
patch
Comment 21 Oleg Solovyov 2018-12-19 14:20:58 UTC
Finally! I accidently fixed the problem

patch attached, can't create a revision unless I know why it works
Comment 22 Nate Graham 2018-12-19 14:35:05 UTC
You can create a Phab revision and put [RFC] in the title. I think that would make sense.
Comment 23 Oleg Solovyov 2018-12-19 14:37:24 UTC
(In reply to Nate Graham from comment #22)
> You can create a Phab revision and put [RFC] in the title. I think that
> would make sense.

Done
https://phabricator.kde.org/D17689
Comment 24 Eike Hein 2019-02-01 11:14:06 UTC
Git commit 4a3fbf9116f588f6f2dedfd9481ec7298528dafd by Eike Hein.
Committed on 01/02/2019 at 11:07.
Pushed by hein into branch 'Plasma/5.12'.

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

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+.

Reviewers: #plasma, McPain, davidedmundson

Reviewed By: #plasma, davidedmundson

Subscribers: ngraham, davidedmundson, fvogt, plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D18182

M  +10   -6    containments/desktop/plugins/folder/foldermodel.cpp

https://commits.kde.org/plasma-desktop/4a3fbf9116f588f6f2dedfd9481ec7298528dafd