[PATCH] Bug 156495: Drag and drop breadcrumb popup

[David Faure]

On Saturday 21 March 2009, rahman duran wrote:
> Hi,
> 
> I have a patch for http://bugs.kde.org/show_bug.cgi?id=156495
> 
> I will be glad if someone have a look. Managing popup( KUrlNavigatorMenu)
> menu's life periot in KurlNavigatorButton is somehow tricky. I'm not sure if
> I created and destroyed it in right time.

Dropping onto menu items? Interesting (this must be the first occurence of it in KDE...?)
Sounds weird in general, but for the urlnavigator popups is sounds useful indeed.

+void KUrlNavigatorMenu::dragMoveEvent(QDragMoveEvent* event)
+{
+      QMouseEvent* mouseEvent = new QMouseEvent::QMouseEvent(QEvent::MouseMove, event->pos(), [...]

You're leaking that mouse event. Please create it on the stack.

+void KUrlNavigatorMenu::dropEvent(QDropEvent* event)
+{
+    QAction* action = actionAt(event->pos());

Can't action be NULL here? Ah you test for it inside urlsDropped()... hmm ok,
I would have done the rest in the caller instead, it's not intuitive that
urlsDropped(QAction *, QDropEvent*) "supports" the case where the action is NULL.
(Well, it doesn't support it, it just doesn't crash ;)

+    if (m_hoverArrow) {
+        m_hoverArrow = false;
+    }
Remove the if, not useful.

+        if (m_dirsMenu != 0) {
+            delete m_dirsMenu;
+            m_dirsMenu = 0;
+        }
Remove the if, not useful.
In fact I don't really understand why you delete the menu there in the dragmove event?
It will be deleted when exec() returns anyway (see the other "delete m_dirsMenu" statement
in the code).

I wonder if KUrlNavigatorMenu shouldn't be a more general-purpose class
in kdeui, KDropUrlsMenu or something, since it's not related to kurlnavigator at all
(good design, btw ;). But well, we better wait until other people need it, to make
it worth the added symbols in kdeui and make sure it fits other people's needs;
just something to keep in mind people, if you need a kmenu that supports url drops
[could be useful for the bookmark menu maybe]. Another reason for the if(action)
before the emit; other implementations of that slot might not expect to get a NULL
action there :-)

Last nitpick: you made it LGPL-v2-only, which might create trouble at some point.
Can you make it LGPL-v2+ or (my preferred option) LGPL-v2-v3+e.V. ?
(see kio/kio/kfileitemlistproperties.h in trunk for an example)

Looks good otherwise, IMHO this can be committed after these cleanups.

-- 
David Faure, faure at kde.org, sponsored by Qt Software @ Nokia to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).