[applets/appmenu] Use libtaskmanager for appmenus
ClosedPublic

Authored by cblack on Mar 19 2020, 7:37 PM.

Details

Summary

This patch ports the appmenu applet to use
libtaskmanager.

Test Plan

Ensure that there aren't any regressions.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cblack created this revision.Mar 19 2020, 7:37 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 19 2020, 7:37 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Mar 19 2020, 7:37 PM

I love patches with a lot of red!

When filtering by screen, this now means we get appmenu only when the active window is on the same screen? Not sure if this is what we want?
(Perhaps we could do some clever "last window with menu" tracking so we can have per-screen global menu? :D)

applets/appmenu/lib/CMakeLists.txt
15

Unused

applets/appmenu/plugin/appmenumodel.cpp
69–70

Is this needed? I would think if screen geometry changes, task manager updates and filters and signals the active task having changed?

140

Remove

142–144

Please write as

const QModelIndex activeTaskIdx = m_tasksMode->activeTask();
const QString objectPath = m_tasksModel->data(activeTaskIdx, TaskManager::AbstractTasmsModel::ApplicationMenuObjectPath).toString();
const QString serviceName = ....;

(perhaps you could drop a using namespace TaskManager at the top of the cpp file)

146–149

!objectPath.isEmpty()

applets/appmenu/plugin/appmenumodel.h
88

I think you can remove this member now and just forward to tasksmodel

cblack marked an inline comment as done.Mar 19 2020, 8:00 PM
cblack added inline comments.
applets/appmenu/lib/CMakeLists.txt
15

Linkage fails without it.

cblack updated this revision to Diff 78039.Mar 19 2020, 8:07 PM
cblack marked 6 inline comments as done.

Address feedback

cblack updated this revision to Diff 78040.Mar 19 2020, 8:07 PM

Remove extraneous change

cblack marked an inline comment as done.Mar 19 2020, 8:08 PM

When filtering by screen, this now means we get appmenu only when the active window is on the same screen? Not sure if this is what we want?
(Perhaps we could do some clever "last window with menu" tracking so we can have per-screen global menu? :D)

This is already the current behaviour of the application menu plasmoid.

broulik accepted this revision.Mar 19 2020, 8:47 PM
broulik added inline comments.
applets/appmenu/plugin/appmenumodel.cpp
120

I think you can just connect this signal to our signal in the constructor and then remove the if check here

146–149

No need for parentheses

This revision is now accepted and ready to land.Mar 19 2020, 8:47 PM
cblack updated this revision to Diff 78047.Mar 19 2020, 9:03 PM
cblack marked 2 inline comments as done.

Address feedback

This revision was automatically updated to reflect the committed changes.