D14199: Simplify the context menu handling functions in FoldersPanel

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D14199#294796 <https://phabricator.kde.org/D14199#294796>, @hallas wrote:
  
  > In D14199#294501 <https://phabricator.kde.org/D14199#294501>, @elvisangelaccio wrote:
  >
  > > 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.

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D14199

To: hallas, broulik, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180721/950b659a/attachment.htm>