D26848: Don't assume the manager and menu have the same lifetime
This revision was automatically updated to reflect the committed changes. Closed by commit R265:a33675fdb3e4: Don't assume the manager and menu have the same lifetime (authored by davidre). REPOSITORY R265 KConfigWidgets CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26848?vs=74271&id=74581 REVISION DETAIL https://phabricator.kde.org/D26848 AFFECTED FILES src/kcolorschememanager.cpp src/kcolorschememanager_p.h To: davidre, #frameworks, broulik Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
broulik accepted this revision. This revision is now accepted and ready to land. REPOSITORY R265 KConfigWidgets BRANCH menumanager (branched from master) REVISION DETAIL https://phabricator.kde.org/D26848 To: davidre, #frameworks, broulik Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre marked 4 inline comments as done. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D26848 To: davidre, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre updated this revision to Diff 74271. davidre added a comment. Use qApp as context REPOSITORY R265 KConfigWidgets CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26848?vs=74162&id=74271 BRANCH menumanager (branched from master) REVISION DETAIL https://phabricator.kde.org/D26848 AFFECTED FILES src/kcolorschememanager.cpp src/kcolorschememanager_p.h To: davidre, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre added inline comments.
INLINE COMMENTS
> kcolorschememanager.cpp:191
> QActionGroup *group = new QActionGroup(menu);
> -connect(group, &QActionGroup::triggered, this, [this](QAction * action)
> {
> -activateScheme(d->model->index(action->data().toInt()));
> +connect(group, &QActionGroup::triggered, [] (QAction * action) {
> +::activateScheme(action->data().toString());
Thanks! Makes sense!
REPOSITORY
R265 KConfigWidgets
REVISION DETAIL
https://phabricator.kde.org/D26848
To: davidre, #frameworks
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
broulik added inline comments. INLINE COMMENTS > davidre wrote in kcolorschememanager.cpp:191 > What is meant by "connecting to qApp"? Just doing the scheme switching here? > I wanted to avoid the duplication that's why I created the static helper > method. Using `qApp` as context object REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D26848 To: davidre, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre added inline comments.
INLINE COMMENTS
> kcolorschememanager.cpp:191
> QActionGroup *group = new QActionGroup(menu);
> -connect(group, &QActionGroup::triggered, this, [this](QAction * action)
> {
> -activateScheme(d->model->index(action->data().toInt()));
> +connect(group, &QActionGroup::triggered, [] (QAction * action) {
> +::activateScheme(action->data().toString());
What is meant by "connecting to qApp"? Just doing the scheme switching here? I
wanted to avoid the duplication that's why I created the static helper method.
REPOSITORY
R265 KConfigWidgets
REVISION DETAIL
https://phabricator.kde.org/D26848
To: davidre, #frameworks
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
anthonyfieroni added inline comments.
INLINE COMMENTS
> kcolorschememanager.cpp:191
> QActionGroup *group = new QActionGroup(menu);
> -connect(group, &QActionGroup::triggered, this, [this](QAction * action)
> {
> -activateScheme(d->model->index(action->data().toInt()));
> +connect(group, &QActionGroup::triggered, [] (QAction * action) {
> +::activateScheme(action->data().toString());
You can connect to `qApp`
REPOSITORY
R265 KConfigWidgets
REVISION DETAIL
https://phabricator.kde.org/D26848
To: davidre, #frameworks
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
broulik added a comment. lgtm REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D26848 To: davidre, #frameworks Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre added a reviewer: Frameworks. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D26848 To: davidre, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26848: Don't assume the manager and menu have the same lifetime
davidre created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. davidre requested review of this revision. REVISION SUMMARY The menu we retun can outlive the manager. So we can't use it or its members in slots when connecting to signals of the menu or it's actions. This also switches the actions' data() back to being the schemePaths instead of the rows of the corresponding indices. TEST PLAN Open Labplot which currently triggers this crash. REPOSITORY R265 KConfigWidgets BRANCH menumanager (branched from master) REVISION DETAIL https://phabricator.kde.org/D26848 AFFECTED FILES src/kcolorschememanager.cpp src/kcolorschememanager_p.h To: davidre Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
