Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020717 Firefox/3.0b4pre (Debug Build) Steps to reproduce: - Select a Bookmark and use "cut" from the context menu - Paste this Bookmark to a folder of your choice - Select now from the Firefox Menubar Edit->Undo -> The Bookmark disappears from the bookmark folder where you have pasted him, but is not restored to the original folder

I was also irritated by this behavior, but i believe it's intended. The first undo only reverts paste, a second undo reverts cut. The Bookmarks Manager in Firefox 2 works identical, but the undo menu entry displays what it will undo.

I hit this today while running bfts on WinXP. After a restart I noticed that a whole folder was gone. This is at least a dataloss bug. Users will probably not open the source folder to check if the bookmark/folder has been restored there. Why we require two steps in undoing an action?

This is not a regression, so not blocking 3.1 on it. However I agree that it's not intuitive, and should be fixed to be consistent with the user's expectation of what undo should do.

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

tentatively assigning, should be fixed sometimes.

Needs a test and styling on trees doesn't work at all, due to the dependent bug (the only workaround that works is setting a border or a background on the cell, but that's not native on any platform). Apart these, seems to work fine.

A couple advantages of this approach: - more native - less disk IO - less Sync impact - no unexpected dataloss - no dataloss for bug 301888

I'm still working on the test, but that can be reviewed apart, in the meanwhile you may start taking a look at the code changes. The main obstacle (bug 664127) has a patch and a test already.

fwiw, this patch passed a tryserver run.

This test is really simple, but after all, once checked that cut does cut, most of the code is shared with copy (there is a test on copy) and with transactions (various tests), so other cases don't seem that interesting regarding this bug. If you have special requests, I'll be happy to hear them :)

the test fails on Linux, will have to check what's up there

gtk doesn't like setting a clipboard content without any flavor, so I just added a fancy empty flavor. I've done it for all OSes since this behavior it makes more sense. Also solved a js warning on shutdown, where a controller was still trying to update a half-dead treeview.

Comment on attachment 540755 [details] [diff] [review] patch v1.2 Review of attachment 540755 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/controller.js @@ +1133,5 @@ > + // Unfortunately just invoking emptyClipboard is not enough, since it > + // does not act on the native clipboard. > + let xferable = Cc["@mozilla.org/widget/transferable;1"]. > + createInstance(Ci.nsITransferable); > + // GTK doesn't like empty transferables, so just add an unknown type. shouldn't we fix emptyClipboard to do this under GTK, instead of a one-off fix here in Places?

emptyClipboard doesn't empty the clipboard in any OS, it's bad named, it just releases the clipboard owner. So the first part here is needed for all platforms. The difference is that the next setData call fails on gtk if the transferable doesn't have any flavor, I did not touch that since I don't know what's the expected behavior, failing is not consistent with Win or Mac, but is not wrong by itself (an empty transferable is useless after all). Since any platform may differ regarding that, I took the safest approach.

Comment on attachment 540755 [details] [diff] [review] patch v1.2 Review of attachment 540755 [details] [diff] [review]: ----------------------------------------------------------------- this looks fine, r=me on the code changes. however, please add a screenshot, and get UI review (just file followups on any polish resulting from the UI review).

Not extremely noticeable in a static screen but the feedback is evident when cutting, not annoying if you leave something cut in a view. It's mostly the same as implemented in most file managers.

I can give you a trybuild too http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-db809a6fd30b/

Comment on attachment 542480 [details] screenshot As commented in IRC: This works great when the site has a favicon, but since we flush favicons pretty often (boo!), it'll be less noticable — can we make the text gray when it isn't selected too? (gray text on blue background doesn't work well, so we can't do it when selected, IMO. But let's land this initial version, and tweak it later?

(In reply to comment #19) > This works great when the site has a favicon, but since we flush favicons > pretty often (boo!), it'll be less noticable — can we make the text gray > when it isn't selected too? Right, while in most cases we show an icon (that may be a blank document) I need feedback also when the icon is not visible, since it's possible. Unfortunately, matching unselected rows in trees is a nightmare (there is no direct matching afaict, and duplicating all basic selected rules would be an overkill), plus not changing text in selected rows would give no feedback when the user selects rows and starts a cut (some other browser vendor thinks this is not a problem, but trying seems completely broken). I tried using italic (we did for example in the Sync notifications on Win) but it slightly changes size of the buttons in toolbar, jumpy buttons are not a good way. The best solution I've found so far is adding some opacity to text as well, but less than to the icon, so it stays readable. It has a similar effect to GrayText but blends nicely in case of a selected row. The downside is that this means another opacity fix to the treebody code, but it's feasible.

This patch uses opacity on both icon and text, with text slightly more opaque. needs bug 668319.

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 Verified issue on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 - after "cut" action - undo/redo requires one step and the bookmark is not lost if paste never occurs (it became grayed out) I used the following STR: 1. Open Show All Bookmarks 2. Select a Bookmark and use "cut" from the context menu 3. Paste the Bookmark into a different folder 4. Organize -> Undo/Redo. Tested using the above STR in the Bookmarks menu, the Bookmarks Toolbar and the Bookmarks sidebar, after "cut" the selected bookmark becomes grayed out (undo/redo is also performed in one step). Setting resolution to VERIFIED FIXED.