Optimize code when dropping files into the desktop
ClosedPublic

Authored by trmdi on Jan 15 2020, 4:20 PM.

Details

Summary

Currently, when you have the desktop in the Desktop layout, if you drop 10 jpg files into the desktop, it creates 10 menus at the same position.
When you have the desktop in the Folder layout, if you drop 10 jpg files into the desktop, it calls KIO::setApplicationActions() 10 times.

What does this improve?

  • Do not create KIO::MimetypeJob for each file
  • Do not call DropJob::setApplicationActions() too many times/ create too many menus for each file.

My idea is that, when you drop files into the desktop:

  • If they have the same mimetype -> we add actions support this mimetype.
  • If not -> we only show the action from KIO::DropJob and only actions support different mimetypes at the same time like Add icon...

Require: D26484
CCBUG: 415917

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trmdi created this revision.Jan 15 2020, 4:20 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 15 2020, 4:20 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
trmdi requested review of this revision.Jan 15 2020, 4:20 PM
trmdi edited the summary of this revision. (Show Details)Jan 15 2020, 4:28 PM
trmdi added reviewers: Plasma, mart.
trmdi added a subscriber: Plasma.
trmdi edited the summary of this revision. (Show Details)Jan 15 2020, 4:30 PM
trmdi retitled this revision from [WIP] Optimize code when dropping files into the desktop to Optimize code when dropping files into the desktop.
trmdi edited the summary of this revision. (Show Details)Jan 15 2020, 4:36 PM
trmdi edited the summary of this revision. (Show Details)Jan 15 2020, 4:40 PM
trmdi edited the summary of this revision. (Show Details)Jan 16 2020, 2:36 AM
anthonyfieroni added inline comments.
src/scriptengines/qml/plasmoid/containmentinterface.cpp
566

Don't add uncategorized qDebug, i see it exists in code base but they should be ported as well.

740–741

Remove, don't leave commented code

trmdi updated this revision to Diff 73693.Jan 16 2020, 11:11 AM
  • Remove unneeded code
trmdi updated this revision to Diff 73694.Jan 16 2020, 11:12 AM
trmdi marked an inline comment as done.
  • Remove unneeded code
trmdi marked an inline comment as done.Jan 16 2020, 11:13 AM
trmdi updated this revision to Diff 73716.Jan 16 2020, 4:04 PM
  • Change the cursor to the busy state when the first file is not local
davidedmundson requested changes to this revision.Jan 16 2020, 4:28 PM
davidedmundson added a subscriber: davidedmundson.

m_dropActions is conceptually linked to the menu we are showing
It shouldn't a direct member variable of ContainmentInterface

If we're following the existing pattern of this menu, it should be:
QHash<KJob *, QList<QAction> *> m_dropJobs;

Otherwise you have no overlap protection on something that's async

I also am very confident if you either subclassed DropJob or used lamba's we can get rid of all of these hashes and make this code 10 billion times more readable.
(we have some crashers here, so it would be well worth it)

This revision now requires changes to proceed.Jan 16 2020, 4:28 PM
trmdi updated this revision to Diff 73815.Jan 18 2020, 7:02 AM
  • Add DropMenu class to combine choices/dropjobMenu
  • Only 1 menu can be created
  • Fix some possible memory leaks
davidedmundson added inline comments.Mon, Jan 20, 1:14 PM
src/scriptengines/qml/plasmoid/containmentinterface.cpp
563

can you be sure urls has at least 1 at this point?

src/scriptengines/qml/plasmoid/dropmenu.cpp
37

const QPoint &dropPoint

49

where does this menu get deleted? It seems to leak

trmdi added inline comments.Mon, Jan 20, 1:16 PM
src/scriptengines/qml/plasmoid/dropmenu.cpp
49

It is set to be deleted on close, at line 43.

trmdi updated this revision to Diff 74025.Tue, Jan 21, 4:28 PM
  • Handle the case in which the menu is not shown
trmdi updated this revision to Diff 74030.Tue, Jan 21, 5:07 PM
  • Code style
trmdi marked 3 inline comments as done.Tue, Jan 21, 5:17 PM
trmdi added inline comments.
src/scriptengines/qml/plasmoid/containmentinterface.cpp
563

Yes, because all clearDataForMimeJob are called from inside of mimeTypeRetrieved which is only called from here: https://phabricator.kde.org/source/plasma-framework/browse/master/src/scriptengines/qml/plasmoid/containmentinterface.cpp$454

davidedmundson accepted this revision.Tue, Jan 21, 5:24 PM

Cool cool. I think there's potential to move more logic from ContainmentInterface into DropMenu object, but we can spread that task out.

We actually have some crashers in this mime handling code, so I'm happy to see someone spending some time on it.

(Somewhere in https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=CONFIRMED&bug_status=ASSIGNED&bug_status=REOPENED&list_id=1705725&longdesc=ContainmentInterface&longdesc_type=allwordssubstr&product=frameworks-plasma&product=plasmashell&query_format=advanced) might be worth taking a look.

src/scriptengines/qml/plasmoid/dropmenu.cpp
67

This is slower than just assigning

This revision is now accepted and ready to land.Tue, Jan 21, 5:24 PM
trmdi updated this revision to Diff 74036.Tue, Jan 21, 6:03 PM
trmdi marked 2 inline comments as done.
  • Address comment
trmdi marked an inline comment as done.Tue, Jan 21, 6:04 PM
trmdi edited the summary of this revision. (Show Details)Tue, Jan 21, 6:06 PM
trmdi updated this revision to Diff 74672.Thu, Jan 30, 11:37 AM
  • Rebase

@mart
Do you have any objection?

This revision was automatically updated to reflect the committed changes.