<br><br><div class="gmail_quote">On Mon, Mar 23, 2009 at 8:47 PM, David Faure <span dir="ltr"><<a href="mailto:faure@kde.org">faure@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
<br>
</div>>Dropping onto menu items? Interesting (this must be the first occurence of it in KDE...?)<br>
Sounds weird in general, but for the urlnavigator popups is sounds useful indeed.</blockquote><div><br>Weird not not useful in general, this is why I derived a new one for just urlnavigator :) <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
+void KUrlNavigatorMenu::dragMoveEvent(QDragMoveEvent* event)<br>
+{<br>
+ QMouseEvent* mouseEvent = new QMouseEvent::QMouseEvent(QEvent::MouseMove, event->pos(), [...]<br>
<br>
You're leaking that mouse event. Please create it on the stack.</blockquote><div><br>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. <br>
</div><div><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+void KUrlNavigatorMenu::dropEvent(QDropEvent* event)<br>
+{<br>
+ QAction* action = actionAt(event->pos());<br>
<br>
Can't action be NULL here? Ah you test for it inside urlsDropped()... hmm ok,<br>
I would have done the rest in the caller instead, it's not intuitive that<br>
urlsDropped(QAction *, QDropEvent*) "supports" the case where the action is NULL.<br>
(Well, it doesn't support it, it just doesn't crash ;)</blockquote><div><br>Hmm, right. Moving the checking code to KUrlNavigatorMenu::dropEvent(QDropEvent* event) makes sense.<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
+ if (m_hoverArrow) {<br>
+ m_hoverArrow = false;<br>
+ }<br>
Remove the if, not useful.<br>
<br>
+ if (m_dirsMenu != 0) {<br>
+ delete m_dirsMenu;<br>
+ m_dirsMenu = 0;<br>
+ }<br>
Remove the if, not useful.<br>
In fact I don't really understand why you delete the menu there in the dragmove event?<br>
It will be deleted when exec() returns anyway (see the other "delete m_dirsMenu" statement<br>
in the code).</blockquote><div><br>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.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
I wonder if KUrlNavigatorMenu shouldn't be a more general-purpose class<br>
in kdeui, KDropUrlsMenu or something, since it's not related to kurlnavigator at all<br>
(good design, btw ;). But well, we better wait until other people need it, to make<br>
it worth the added symbols in kdeui and make sure it fits other people's needs;<br>
just something to keep in mind people, if you need a kmenu that supports url drops<br>
[could be useful for the bookmark menu maybe]. Another reason for the if(action)<br>
before the emit; other implementations of that slot might not expect to get a NULL<br>
action there :-)</blockquote><div><br>But it seems its related to kurlnavigator to me :) I didn't need this functionality anywhere else.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
Last nitpick: you made it LGPL-v2-only, which might create trouble at some point.<br>
Can you make it LGPL-v2+ or (my preferred option) LGPL-v2-v3+e.V. ?<br>
(see kio/kio/kfileitemlistproperties.h in trunk for an example)</blockquote><div><br>I actually copied licence text from some file in the same dir :) Didn't pay attention <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
Looks good otherwise, IMHO this can be committed after these cleanups.<font color="#888888"></font></blockquote><div><br>OK, I will clean it up and post a new patch. Peter Penz will review it tomorrow I think, as he said before.<br>
</div></div><br><br clear="all"><br>-- <br>Rahman Duran<br><br>Software Engineer<br>Turkey<br><br>How many apples fell on Newton's head before he took the hint! <br><br>