BUG: 382096
Details
Details
Diff Detail
Diff Detail
- Repository
- R33 KDevPlatform
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
The QPointer definitely is not needed here, was just result of initial approach to deal with the deletion of the QMenu.
Actually the QMenu object could also be put back onto the stack, where it was initially and where it also is in some places of similar code across KDevelop codebase.
Did not change that yet to make patch more simple (no "->" to "." lines). And chose QScopedPointer instead of raw pointer rather randomly.
Ideally all such places should be aligned to what is considered best practice (which is what? heap or stack for QMenus?).
The crash was due to QMenu::exec() still being executed (and going to access class members) when the QMenu object was prematurely deleted due to sync handling of the split view actions which resulted in deletion of the parent widget set to the QMenu instance.
| sublime/container.cpp | ||
|---|---|---|
| 625–636 | QScopedPointer indeed doesn't really make sense. This has basically the same semantics as if QMenu would have been created on the stack. Idea: Let's use KDevelop::ScopedDialog from https://phabricator.kde.org/R33:7cda7c95d5bfa93f34c5a52df17480c248a8367c? Let's backport that patch and use it(?) | |
| 630 | Could you add the code which we can use starting with > Qt 5.9? In #ifdefs obviously... Makes it easier to port code once we start depending on this Qt version. | |
| sublime/container.cpp | ||
|---|---|---|
| 625–636 | No, that would be overkill IMHO. No need for QPointer stuff, as using QMenu::exec() already points out we expect the menu object to survive until after this call (as the method otherwise crashes). If this would be my code (e.g. Okteta), I would just go back to stack-based QMenu :) | |
| 630 | I am not aware of any. "<= 5.9" was just for adding a note about at what time/state the comment was added, so some future code reader would know whether it makes sense to investigate for improved Qt support. Guess that text needs rewording then :) Any proposal? | |
| sublime/container.cpp | ||
|---|---|---|
| 625–636 | Fine with going back to the stack-based QMenu then. Feel free to revert my patches which turned it into a heap-allocated one. | |
| 630 | There must be an easier way to accomplish this, though, no? @graesslin Do you have an idea how to make this work? /me has no idea about Wayland's needs... | |
| sublime/container.cpp | ||
|---|---|---|
| 630 | When I had asked on # plasma, the feedback from notmart, d_ed and eang was that this is currently the only way to achieve it. Quite unhappy as well (though not enough yet to have filed a bug with Qt about missing API, given lack of proper wayland/qt knowledge) See notes on https://community.kde.org/Guidelines_and_HOWTOs/Wayland_Porting_Notes (hm, still partially wrong code, fixing now) and https://codereview.qt-project.org/#/c/195704/10//ALL | |
| sublime/container.cpp | ||
|---|---|---|
| 630 | Quite unsatisfactory ;) | |