Simplify the code for creating and showing the context menu in the FoldersPanel. This removes redundant nullptr checks and avoids a heap allocation.
Details
Diff Detail
- Repository
- R318 Dolphin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
I got completely confused by the code, TreeViewContextMenu internally creates a QMenu and then does exec() in that confusingly named open() method. I assumed TreeViewContextMenu *was* the menu.
Looking good, thanks!
Do you have commit access?
https://community.kde.org/Infrastructure/Get_a_Developer_Account
For now, one of us can land these very nice patches for you. Since you didn't use arc to upload the patches (see https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches), we'll need to add the git authorship information by hand. Can you tell us your real name and preferred email address?
Thanks, I will go through the process of setting up arc, it looks like it makes the developer workflow a lot easier :)
Please land these patches for me, as I will be offline for the next week.
My real name and preferred email address is:
David Hallas
david@davidhallas.dk
Cheers
I will land the patches later today. Thanks again for submitting them and making Dolphin even more awesome!
The old code was like that for a reason: https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0
Now, the chance this change will lead to a crash is very low, but still I don't see what we gain either.
Since this code is somewhat counterintuitive, we might want to add a comment explaining it.
After reading the blog post and looking at the Qt documentation, I think you are absolutely right about this. The TreeViewContextMenu::open function ends up calling QMenu::exec which runs synchronously meaning that the application can close while processing events, what I do not understand about the original code is what good does the
if (contextMenu.data())
do? The whole point of QPointer is to provide memory management of QObjects, also in the case where the managed object is deleted elsewhere, therefore I can't see that it is needed.
So, I think a safe implementation would be something like this:
QPointer<TreeViewContextMenu> contextMenu = new TreeViewContextMenu(this, fileItem); contextMenu.data()->open();
You do need that nullptr check if you want to delete the pointer, like the old code does.
So, I think a safe implementation would be something like this:
QPointer<TreeViewContextMenu> contextMenu = new TreeViewContextMenu(this, fileItem); contextMenu.data()->open();
This is also safe, yes. But this way we keep the menu on the heap as long as the FolderPanel instance is still around. What if the user opens the context menu a lot of times?
Really, I'd just leave the current code as is.