Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/#review47123 --- Ship it! I say ship it - deleting anything that has been parented to another class is definitely a bad idea. Of course, you may want to wait for someone more familiar with this code to chime in - if there is a compelling reason for making the actions children of the relevant widget, then another approach would be to use QPointers. - Alex Merry On Jan. 9, 2014, 8:15 a.m., Frank Reininghaus wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 8:15 a.m.) Review request for KDE Frameworks and David Faure. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/#review47124 --- Ship it! Yep, as discussed. Looks good, except for one invalid URL. autotests/kfileitemactionstest.cpp https://git.reviewboard.kde.org/r/114921/#comment33580 This is an invalid way to create a URL from a local file, you must use QUrl::fromLocalFile(). Then again ... it means this whole code really doesn't care about the URL being used, does it? ;-) So you might just as well use file:/// or whatever. - David Faure On Jan. 9, 2014, 8:15 a.m., Frank Reininghaus wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 8:15 a.m.) Review request for KDE Frameworks and David Faure. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/#review47125 --- Why not simply make the QMenu* the parent of the QAction? I am not familiar with the code so I may be missing something, but won't makeing KFileItemActions the parent mean that the actions only get deleted once the KFileItemActions gets deleted? I guess this could waste quite a bit of memory. - Alexander Richardson On Jan. 9, 2014, 9:15 a.m., Frank Reininghaus wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 9:15 a.m.) Review request for KDE Frameworks and David Faure. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 9:26 p.m.) Review request for KDE Frameworks and David Faure. Changes --- Thanks everyone for the comments! I've updated the diff, which I will push soon: 1. Use QUrl::fromLocalFile() as suggested by David. 2. Updated copyright year. 3. Added a few 'const' where it was possible. About Alexander's idea to make the QMenu the parent of the actions: usually, one does not make the menu the parent of the actions (see, e.g., David's explanation in http://lists.kde.org/?l=kfm-develm=138688704130331w=2 ). Nonetheless, we are not going to waste memory. At least Konqueror's and Dolphin's context menu implementations destroy the KFileItemActions instance (including, after this patch, the actions) when the menu itself is deleted. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs (updated) - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/#review47128 --- This review has been submitted with commit 7a1543f82d4f4464b204eddbaa439120de0636f2 by Frank Reininghaus to branch master. - Commit Hook On Jan. 9, 2014, 9:26 p.m., Frank Reininghaus wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 9:26 p.m.) Review request for KDE Frameworks and David Faure. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114921/ --- (Updated Jan. 9, 2014, 9:28 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and David Faure. Repository: kio Description --- This patch is a result of the discussion in http://lists.kde.org/?t=13868700941r=1w=2 Currently, KFileItemActions makes the widget that is set with setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as advertised by the API docs), but also of the created actions. Nonetheless, KFileItemActions remembers pointers to all created actions and deletes them in the destructor. This can cause problems if the widget is deleted before the KFileItemActions instance - the destructor will then try to delete dangling pointers and cause a crash. This problem can be fixed by making KFileItemActions the parent of the actions. This also makes the code a bit simpler because the m_ownActions member is not needed any more. In fact, this issue is the cause of crashes in Dolphin (see https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't really have to change it in kdelibs 4.x because the problem can be worked around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet because it turns out that there is still another source of crashes in the problematic Dolphin use case). Diffs - autotests/CMakeLists.txt 2868327 autotests/kfileitemactionstest.cpp PRE-CREATION src/widgets/kfileitemactions.cpp eee2ebe src/widgets/kfileitemactions_p.h 9f9a701 Diff: https://git.reviewboard.kde.org/r/114921/diff/ Testing --- New unit test crashes with master, and passes if the patch is applied. Existing kio unit tests pass on my system (except for kiocore-kacltest, but I believe that the failure is unrelated to this patch). Thanks, Frank Reininghaus ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
