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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[Christine Yen]

(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

Comment 3

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 4

[(not reading, please use [e-mail] instead)]

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

Comment 5

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 6

[(not reading, please use [e-mail] instead)]

updating summary.

see also bug #401018, a similar bug with livemarks in menus and the personal toolbar folder.

Comment 7

[Marco Bonardo [:mak]]

still exists
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre

Comment 8

[Gervase Markham [:gerv]]

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

Comment 9

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

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!

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Richard Newman [:rnewman]]

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

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Richard Newman [:rnewman]]

Revising...

Comment 14

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Fixed after IRC roundabout.

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Richard Newman [:rnewman]]

(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 17

[Justin Dolske [:Dolske]]

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.

Comment 18

[Richard Newman [:rnewman]]

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.

Comment 19

[Philipp von Weitershausen [:philikon]]

Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/f20f6ca6be78