636924 - Copied livemarks appear as folders in sidebar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Richard Newman [:rnewman]]

Follow on from Bug 387138.

Per sleep-deprived mak:

18:43:08 <@mak> nodeAnnotationChanged doesn't invalidate the row for livemark annotations
18:43:14 <@mak> in the treeview

STR:

* Open sidebar
* Find a livemark
* Copy
* Paste elsewhere in the tree view
* Observe that the livemark has a folder icon, not a livemark icon.

Comment 1

[Marco Bonardo [:mak]]

actually, could also depend on bug 636917.

Comment 2

[Dietrich Ayala (:dietrich)]

Mano's going to look at this. For posterity:

08:14 <@dietrich> mak: bug 636924
08:14 <@dietrich> what annotation changed in that str?
08:14 <@mak> livemark/feedURI
08:16 <@mak> or better PlacesUtils.LMANNO_FEEDURI
08:22 <@dietrich> mak: why would that change for a copy/paste operation?
08:24 <@mak> I think we don't set the "livemark" atom on the node... but I didn't follow all the code, looks like it's not listening correctly to annotation changes. if Sync adds a livemark most likely will do the same as copy paste
08:24 <@mak> so, copy paste is anything like "add a livemark"
08:25 <@mak> menu and toolbar views on nodeAnnotationChanged set a "livemark" attribute, treeview should do something similar
08:26 <@mak> (not an attribute but an atom iirc)

Comment 3

[Asaf Romano (gone)]

Attachment: a patch I couldn't test myself

For some reason, I cannot reproduce the bug here (Maybe mac trees invalidate too often).

Comment 4

[Asaf Romano (gone)]

Comment on attachment 516372 [details] [diff] [review]
a patch I couldn't test myself

wrong patch

Comment 5

[Asaf Romano (gone)]

Attachment: a patch I couldn't test myself

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 516377 [details] [diff] [review]
a patch I couldn't test myself

the change looks fine, r=me on that. however, we'll not get approval to land without a test for it. can add one? or update an existing test to check for this case?

Comment 7

[Asaf Romano (gone)]

Dietrich, I cannot write a test for a bug I couldn't reproduce.  Actually, I can probably do write one - but it would alwyas pass on my mac, seems like.  Will you be able to test the test on a build in which the bug is reprocible?

Comment 8

[Dietrich Ayala (:dietrich)]

Go ahead and add the test, and we'll have other people that can reproduce the bug confirm it.

Comment 9

[Marco Bonardo [:mak]]

the browser_views_liveupdate and browser_library_views_liveupdate tests could probably be updated to cover this.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> the browser_views_liveupdate and browser_library_views_liveupdate tests could
> probably be updated to cover this.

fwiw, I'm looking into these tests update, if you have a separate test it is fine too. (btw, games are closing, so it's possible all of this slips to .next)

Comment 11

[Marco Bonardo [:mak]]

hm, I guess if this could rather be a bug in winstripe if Mano cannot reproduce on Mac. But who is setting the atom?

Comment 12

[Marco Bonardo [:mak]]

I have updated tests, they are fine for menu and toolbar, in the treeview case I use getCellProperties, but looks like it's not enough to test for the icon.

Comment 13

[Marco Bonardo [:mak]]

Attachment: test updates

fwiw, the updated tests

Comment 14

[Asaf Romano (gone)]

Comment on attachment 516612 [details] [diff] [review]
test updates

Marco: I don't think it's bug in winstripe. A lot of things could force a redraw of the line at a certain timing.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 516377 [details] [diff] [review]
a patch I couldn't test myself

Too late to make the RC. We could consider it for a ride-along if we have more RCs, but I'm not sure it's worth the noise at that point. Thanks for jumping in though - let's get it in the first stability update.

Comment 16

[Marco Bonardo [:mak]]

Yeah, RC took less than expected (and it's a good thing after all), I'd vote for .next

Comment 17

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/b6d89aa5c8c4
http://hg.mozilla.org/projects/cedar/rev/8a61e8610d66

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b6d89aa5c8c4
http://hg.mozilla.org/mozilla-central/rev/8a61e8610d66

Comment 19

[Simona Badau, Desktop QA]

Verified on:
Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre