Closed Bug 636924 Opened 9 years ago Closed 9 years ago
Copied livemarks appear as folders in sidebar
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.
actually, could also depend on bug 636917.
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)
For some reason, I cannot reproduce the bug here (Maybe mac trees invalidate too often).
Comment on attachment 516372 [details] [diff] [review] a patch I couldn't test myself wrong patch
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?
Attachment #516377 - Flags: review?(dietrich) → review+
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?
Go ahead and add the test, and we'll have other people that can reproduce the bug confirm it.
the browser_views_liveupdate and browser_library_views_liveupdate tests could probably be updated to cover this.
(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)
hm, I guess if this could rather be a bug in winstripe if Mano cannot reproduce on Mac. But who is setting the atom?
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.
fwiw, the updated tests
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.
Attachment #516612 - Flags: review+
9 years ago
Attachment #516377 - Flags: approval2.0?
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.
Attachment #516377 - Flags: approval2.0? → approval2.0-
Yeah, RC took less than expected (and it's a good thing after all), I'd vote for .next
Assignee: nobody → mano
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Verified on: Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.