Closed
Bug 458233
Opened 17 years ago
Closed 17 years ago
Use the new D&D API in Places Toolbar
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(1 file, 1 obsolete file)
31.29 KB,
patch
|
Details | Diff | Splinter Review |
Supisingly enough, this has to be done before i implement detach-tabs. We need to extend the drop area a little into the places toolbar. nsDragDrop isn't very friendly/customizable for implementing this behavior. Luckily, we have the new API which is supposed to replace it in these places anyway. Unluckily, I don't have time to fix the other views at the moment, for consistency. So, here's a patch for the toolbar binding.
Attachment #341462 -
Flags: review?(mak77)
Updated•17 years ago
|
Attachment #341462 -
Flags: review?(mak77) → review+
Comment 1•17 years ago
|
||
Comment on attachment 341462 [details] [diff] [review]
patch
+
+ <!-- Menu buttons should be opened when the mouse drags over them, and
+ closed when the mouse drags off. The overFolder object manages
+ opening and closing of folders when the mouse hovers. -->
+ <field name="_overFolder"><![CDATA[
+ ({ node: null, openTimer: null, hoverTime: 350, closeTimer: null });
+ ]]></field>
since all comments in this file are inside the referred-to item (apart from interfaces names), imho this comment should be inside the field, for consistency.
While there, check double space after "drags off." and maybe we should change the next comment to something like "This object holds data used to manage opening and..."
+ <method name="_getDropPoint">
+ <parameter name="event"/>
+ <body><![CDATA[
what about changing to aEvent?
+ <method name="_setTimer">
+ <parameter name="time"/>
aTime?
+
+ <!-- Function to process all timer notifications. -->
+ <method name="notify">
+ <parameter name="timer"/>
i would only put <!-- nsITimerCallback --> outside, comment is not so useful (should be quite clear what notify does), otherwise it should be inside for consistency with the rest of the file.
timer -> aTimer ?
+ <body><![CDATA[
+ // Timer to turn off indicator bar.
+ if (timer == this._ibTimer) {
_ibTimer was defined before in DNDObserver, while is not defined now, do we need a field to initialize it?
+ // The _clearOverFolder() function will close the menu for _overFolder.node.
+ // So null it out if we don't want to close it.
+ if (inHierarchy)
+ this._overFolder.node = null;
+
trailing spaces
+ <handler event="dragstart"><![CDATA[
+ var draggedNode = event.target;
+ if (draggedNode.parentNode != this || !draggedNode.node)
+ return;
you losed the comment here, was
// sub menus have their own d&d handlers
+ this._draggedNode = draggedNode.node;
there is a little confusion between draggedNode (xul node) and _draggedNode (places node) that's why i changed draggedNode to draggedXulNode. Hwv if you think it's clear, np.
]]></handler>
<handler event="dragover"><![CDATA[
while there what about putting a line between handlers? it was not useful before because they were very "compact" but now they include full code, so it would be more readable having them separated by a white line.
+ if (!ip || !PlacesControllerDragHelper.canDrop(ip)) {
+ ib.removeAttribute("dragging");
+ alert(ip);
+ return;
i suppose this alert was for debug purpose and should be removed
+ var ib = this._dropIndicatorBar;
this is defined and initialized 2 times
apart these, r=me
Assignee | ||
Comment 2•17 years ago
|
||
Attachment #341462 -
Attachment is obsolete: true
Assignee | ||
Comment 3•17 years ago
|
||
1c0f158179c3 and/or e2328ca063d6, I cannot understand how could that happen.
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 4•16 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•