Closed Bug 387138 Opened 12 years ago Closed 9 years ago

immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder in some Places views

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: moco, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

After copy/cut then pasting a livemark, sometimes the livemark looks like a regular folder (it has the folder icon) and it doesn't get the livemark icon until you switch folders.

I've seen this in both the left hand pane and the right hand pane of the bookmark organizer.

note, it is a livemark.

I'm not sure if this is an existing bug or not, as I have a patch for bug #378558 in my tree.
(In reply to comment #0)
> I'm not sure if this is an existing bug or not, as I have a patch for bug
> #378558 in my tree.

Sort of existing bug. In current trunk, it pastes as a folder, with apparently partial livemark annotations - the context menu has both "paste" and "reload live bookmark" menu items, though "reload live bookmark" is always disabled... either way, the patch for the aforementioned bug should fix livemark copy/paste problems.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007070911 Minefield/3.0a7pre
Duplicate of this bug: 387733
Summary: after copy/cut then pasting a livemark, sometimes the livemark looks like a regular folder → immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder
Bug 378558 is verified fixed but this still reproduces in the 7/27 trunk build. Which "aforementioned bug" should fix this when it is fixed?
bug #378558 was the "aforementioned bug".

this bug has not been fixed.

what's interesting is by forcing the trees to rebuild, the icon will be correct.

so, I wonder if this bug will turn into:  "annotation additions / changes not being observed by tree.xml nsTreeView.js properly".
This still exists in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102304 Minefield/3.0a9pre.

Do we think we'll fix this sometime soon? It's been over three months.
updating summary.

see also bug #401018, a similar bug with livemarks in menus and the personal toolbar folder.
Summary: immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder → immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder "annotation additions / changes not being observed by tree.xml nsTreeView.js properly?"
still exists
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre
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
Attached patch Proposed patch. v1 (obsolete) — Splinter Review
This patch causes livemarks created on the toolbar to appear with a livemark icon, rather than a folder icon, prior to them being refreshed.

Also fixes a few typos elsewhere in the file.

Justification: Sync now creates livemarks using createLivemarkFolderOnly, rather than createLivemark, and thus triggers this UI bug. Looking bad when handling your bookmarks is not something of which we want to make a habit!
Attachment #515219 - Flags: review?(mak77)
fixing title of the bug, looks like other views were fixed along the years, so could be we can close this longstanding thing with this patch
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Summary: immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder "annotation additions / changes not being observed by tree.xml nsTreeView.js properly?" → immediately after instantiation (cut/paste or subscribe), livemark looks like a regular folder in some Places views
I would like to land this on services-central before we merge our latest batch of improvements up to m-c. Flagging as approval2.0?.
Whiteboard: [has patch][needs review mak77][needs approval2.0]
Attachment #515219 - Flags: approval2.0?
Comment on attachment 515219 [details] [diff] [review]
Proposed patch. v1

>diff --git a/browser/components/places/content/browserPlacesViews.js b/browser/components/places/content/browserPlacesViews.js


>+  nodeAnnotationChanged:
>+  function PT_nodeAnnotationChanged(aPlacesNode, aAnno) {
>+    PlacesViewBase.prototype.nodeAnnotationChanged.apply(this, arguments);
>+    if (aAnno == "livemark/feedURI") {
>+      // So that the icon is set correctly on creation.
>+      aPlacesNode._DOMElement.parentNode.setAttribute("livemark", true);
>+    }
>+  },
>+

So, what you want to do here is handle only direct children of the toolbar, and pass anything other to the base handler (that will take care of submenus).
What you are doing instead is handling submenus and then current node.

You want to follow the style of the other handlers (see nodeTitleChanged for example):
1. assign to a elt local variable
2. check for _DOMElement and throw if it doesn't exist (the placesNode has a direct handle to the domElement that owns it).
3. if elt is the root node (this._rootElt) early return since the root node of a view (the container that has toolbar items inside for example) is not visible
4. if elt.parentNode is the root node then elt is a direct children of the toolbar, then check if aAnno is livemark and set the attribute on elt
5. else pass everything to the base handler
Attachment #515219 - Flags: review?(mak77) → review-
Attachment #515219 - Flags: approval2.0?
Revising...
Whiteboard: [has patch][needs review mak77][needs approval2.0] → [has patch][needs review mak77]
Fixed after IRC roundabout.
Attachment #515219 - Attachment is obsolete: true
Attachment #515274 - Flags: review?(mak77)
Attachment #515274 - Flags: approval2.0?
Comment on attachment 515274 [details] [diff] [review]
Proposed patch. v2

Thanks!

The patch is pretty simple, and mostly safe since it mimics the same code from the base handler (the toolbar just inherits from a base view), also saving some work. I'm pretty confident this is good for approval.

Please recall to file that separate bug to fix the sidebar.
Attachment #515274 - Flags: review?(mak77) → review+
Whiteboard: [has patch][needs review mak77] → [has patch][has review][needs approval2.0]
(In reply to comment #15)

> Please recall to file that separate bug to fix the sidebar.

Done: Bug 636924.

Thanks for staying up late to shepherd this!
Comment on attachment 515274 [details] [diff] [review]
Proposed patch. v2

a+ per impact in comment 9.

While this isn't adding a test for this specific quirk, mak says this general code path is covered by existing tests, and sync tests run through it too. a+ based on that risk checking/mitigation.
Attachment #515274 - Flags: approval2.0? → approval2.0+
Fixed in services-central:

  https://hg.mozilla.org/services/services-central/rev/f20f6ca6be78

If all stays green, Philipp will merge this up to m-c.
Whiteboard: [has patch][has review][needs approval2.0] → [has patch][has review][fixed in services]
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/f20f6ca6be78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][fixed in services]
Depends on: 663104
You need to log in before you can comment on or make changes to this bug.