[CurrentContainmentActionsModel] Add configurationChanged signal
ClosedPublic

Authored by broulik on Jun 30 2017, 9:38 AM.

Details

Summary

This allows the config UI to know when to enable the Apply button.
Currently we only do that when the shortcut or actions themselves change but not their configuration.

Test Plan

Added a Connections to ConfigurationContainmentActions that triggers the config dialog's configurationChanged() and had the Apply button correctly enabled when changing a containment action's settings

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.
broulik created this revision.Jun 30 2017, 9:38 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 30 2017, 9:38 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik added inline comments.Jun 30 2017, 9:39 AM
shell/currentcontainmentactionsmodel.cpp
217

Hmm I pass in "this" but it's nowhere guarded :/

mart added a subscriber: mart.Jun 30 2017, 2:09 PM
mart added inline comments.
shell/currentcontainmentactionsmodel.cpp
217

yeah, it may crash.. whatever is in the 3rd param of the connect is the one you can not worry about, the other one may get dangling..
the only way i guess is to look at the code and be really sure that the CurrentContainmentActionsModel always outlives either configDlg or pluginInstance, which it *should*, but i'm not 100% sure

Can you please upload patches with arc otherwise we don't get to expand the context.

Plugins have the lifespan of containment. (but could change as user toggles things)
ConfigDlg lasts till it's quit
This model is owned by the ContainmentConfigView.

So you're right to question that, it's a good catch.

But it's easily fixable, pass this a parent to configdlg - or just use two connect statements.

broulik updated this revision to Diff 16511.Jul 11 2017, 12:20 PM

Use a separate connect

broulik updated this revision to Diff 16512.Jul 11 2017, 12:22 PM

Connect signal to signal

mart accepted this revision.Aug 31 2017, 9:47 AM
This revision is now accepted and ready to land.Aug 31 2017, 9:47 AM
This revision was automatically updated to reflect the committed changes.