Simplify the context menu handling functions in FoldersPanel
AbandonedPublic

Authored by hallas on Jul 18 2018, 8:53 AM.

Details

Summary

Simplify the code for creating and showing the context menu in the FoldersPanel. This removes redundant nullptr checks and avoids a heap allocation.

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
hallas created this revision.Jul 18 2018, 8:53 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptJul 18 2018, 8:53 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
broulik accepted this revision.Jul 18 2018, 12:34 PM
broulik added a subscriber: broulik.

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?

This revision is now accepted and ready to land.Jul 18 2018, 12:34 PM

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?

No, I don't have commit access. How do I go about getting it?

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?

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!

elvisangelaccio requested changes to this revision.Jul 18 2018, 9:11 PM
elvisangelaccio added a subscriber: elvisangelaccio.

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.

This revision now requires changes to proceed.Jul 18 2018, 9:11 PM

Since this code is somewhat counterintuitive, we might want to add a comment explaining it.

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.

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();

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.

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.

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.

hallas abandoned this revision.Feb 21 2019, 3:15 PM