Differential D26691

Optimize code when dropping files into the desktop
ClosedPublic
Actions

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

Details

Reviewers
mart
broulik
davidedmundson
Group Reviewers
Plasma
VDG
Commits
R242:f8be3ea7102f: Optimize code when dropping files into the desktop
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)
Branch
improve-file-drop-menu
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21225
Build 21243: arc lint + arc unit
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 a subscriber: anthonyfieroni.Jan 16 2020, 8:31 AM
anthonyfieroni added inline comments.
src/scriptengines/qml/plasmoid/containmentinterface.cpp
589 ↗(On Diff #73634)

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

816–819 ↗(On Diff #73634)

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.Jan 20 2020, 1:14 PM
src/scriptengines/qml/plasmoid/containmentinterface.cpp
586 ↗(On Diff #73634)

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

src/scriptengines/qml/plasmoid/dropmenu.cpp
36 ↗(On Diff #73815)

const QPoint &dropPoint

48 ↗(On Diff #73815)

where does this menu get deleted? It seems to leak

trmdi added inline comments.Jan 20 2020, 1:16 PM
src/scriptengines/qml/plasmoid/dropmenu.cpp
48 ↗(On Diff #73815)

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

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

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.Jan 21 2020, 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
66 ↗(On Diff #74025)

This is slower than just assigning

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

@mart
Do you have any objection?

Closed by commit R242:f8be3ea7102f: Optimize code when dropping files into the desktop (authored by trmdi). · Explain WhyFeb 4 2020, 5:54 AM
This revision was automatically updated to reflect the committed changes.
trmdi mentioned this in R119:36e918b09780: Delay popup when dropping things into containments.

Revision Contents
Changeset List

Diff 73693

View Options

src/scriptengines/qml/plasmoid/containmentinterface.h

Loading...
View Options

src/scriptengines/qml/plasmoid/containmentinterface.cpp

Loading...