[PATCH] Bug 156495: Drag and drop breadcrumb popup

[rahman duran]

On Mon, Mar 23, 2009 at 8:47 PM, David Faure <faure at kde.org> wrote:

>
> >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.


Weird not not useful in general, this is why I derived a new one for just
urlnavigator :)


> +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.


Yes, I know. I have some trouble with pointers as a Java developer. It seems
it will take some time to get used to pointers and heap/stack usage.



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


Hmm, right. Moving the checking code to
KUrlNavigatorMenu::dropEvent(QDropEvent* event)  makes sense.



>
>
> +    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).


This is why I said its tricky to manage it, in first post :) While we
trigger menu with dragging we need to close it manualy as user don't click
to the menu and the exec() loop won't end by it self. So the menu stays
opened. This is Why I delete it manually. If you ask why I delete in
DragMove, simple; you know the little arrow that opens the menu? Its just a
part of kurlbutton, not a button itself. So we need to close the menu if
user decides to drop items on kurlbutton instead of the opened menu. If
mouse leaves the arrow area we need to close menu.


>
>
> 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 :-)


But it seems its related to kurlnavigator to me :) I didn't need this
functionality anywhere else.


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


I actually copied licence text from some file in the same dir :) Didn't pay
attention

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


OK, I will clean it up and post a new patch. Peter Penz will review it
tomorrow I think, as he said before.



-- 
Rahman Duran

Software Engineer
Turkey

How many apples fell on Newton's head before he took the hint!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090323/0facd55f/attachment.htm>