allow to add application actions on an open menu
ClosedPublic

Authored by mart on Feb 15 2017, 3:36 PM.

Details

Summary

even if the menu has already been created, cause setapplicationActions
to add actions in the drop menu.

Test Plan

twsted on plasma drop menu, menu opens and in a second time gets the extra actions

Diff Detail

Repository
R241 KIO
Branch
phab/setActionsLater
Lint
No Linters Available
Unit
No Unit Test Coverage
mart updated this revision to Diff 11369.Feb 15 2017, 3:36 PM
mart retitled this revision from to allow to add application actions on an open menu.
mart updated this object.
mart edited the test plan for this revision. (Show Details)
mart added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptFeb 15 2017, 3:36 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
mart updated this revision to Diff 11371.Feb 15 2017, 3:40 PM
  • remove useless signal
hein added a subscriber: hein.Feb 15 2017, 3:46 PM

Much nicer than the other approach, just one question.

src/widgets/dropjob.cpp
324 ↗(On Diff #11371)

Should this be && instead of ||? "Add actions if either of those is not empty" seems weird, what's the reasoning?

dfaure added a subscriber: dfaure.Feb 15 2017, 7:59 PM
dfaure added inline comments.
src/widgets/dropjob.cpp
324 ↗(On Diff #11371)

It's a copy of the if() on line 311. But there it makes sense. Here not so much ;)

What if there were already some plugin actions? Then instead of the usual
separator | appActions | pluginActions would here become
separator | pluginActions | separator | appActions.

And what if there were already some app actions? Then it's even worse, we end up with
separator | oldAppActions | [pluginActions if any] | separator | newAppActions.

It sounds to me like

  1. any previously set appActions should be removed first
  2. the new app actions should be inserted into the right place, which means remembering where -and- remember or checking whether we need to start with a separator or not.

XMLGUI would handle all this automatically pretty well ;)

mart updated this revision to Diff 11402.Feb 16 2017, 12:36 PM
  • remove eventual pre existing plugin actions
dfaure accepted this revision.Feb 18 2017, 10:35 AM
dfaure added inline comments.
src/widgets/dropjob.cpp
330

Don't worry too much about that, QMenu automatically suppresses doubled separators or separators at begin/end of the menu.
So I bet this if block isn't necessary in practice.

332

If you do keep it, maybe add a check that menu->actions() isn't empty, otherwise last() will assert.

This revision is now accepted and ready to land.Feb 18 2017, 10:35 AM
mart updated this revision to Diff 11521.Feb 20 2017, 10:22 AM

don't remove separators

qmenu collpapses them and previous code might have assert

This revision was automatically updated to reflect the committed changes.