Last Comment Bug 416459 - Implement proper cut action for bookmarks (was: undo/redo a cut requires two steps, bookmarks are lost if if paste never happens).
: Implement proper cut action for bookmarks (was: undo/redo a cut requires two ...
Status: VERIFIED FIXED
[places-next-wanted][fixed-in-places]
: dataloss
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: Firefox 7
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 484578 (view as bug list)
Depends on: 723842 664127 668319 674964
Blocks: 301888 663058 682512
  Show dependency treegraph
 
Reported: 2008-02-08 16:13 PST by Carsten Book [:Tomcat] - PTO-back Sept 4th
Modified: 2012-03-13 08:59 PDT (History)
10 users (show)
dietrich: blocking‑firefox3.5-
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (23.96 KB, patch)
2011-06-14 07:49 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (30.79 KB, patch)
2011-06-16 18:43 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Test v1.0 (3.55 KB, patch)
2011-06-17 16:31 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (31.14 KB, patch)
2011-06-21 08:06 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
screenshot (23.44 KB, image/png)
2011-06-28 08:25 PDT, Marco Bonardo [::mak]
limi: ui‑review+
Details
patch v1.3 (29.16 KB, patch)
2011-06-29 13:46 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Carsten Book [:Tomcat] - PTO-back Sept 4th 2008-02-08 16:13:22 PST
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
Comment 1 Robert [minotaur 11] 2008-04-10 06:46:44 PDT
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.
Comment 2 Henrik Skupin (:whimboo) 2009-02-23 13:24:22 PST
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?
Comment 3 Dietrich Ayala (:dietrich) 2009-02-23 15:58:03 PST
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.
Comment 4 Gervase Markham [:gerv] 2009-11-26 05:52:15 PST
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 5 Marco Bonardo [::mak] 2010-06-21 13:55:03 PDT
*** Bug 484578 has been marked as a duplicate of this bug. ***
Comment 6 Marco Bonardo [::mak] 2010-06-21 13:56:13 PDT
tentatively assigning, should be fixed sometimes.
Comment 7 Marco Bonardo [::mak] 2011-06-14 07:49:02 PDT
Created attachment 539204 [details] [diff] [review]
patch v1.0

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.
Comment 8 Marco Bonardo [::mak] 2011-06-14 07:51:01 PDT
A couple advantages of this approach:
- more native
- less disk IO
- less Sync impact
- no unexpected dataloss
- no dataloss for bug 301888
Comment 9 Marco Bonardo [::mak] 2011-06-16 18:43:49 PDT
Created attachment 539964 [details] [diff] [review]
patch v1.1

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.
Comment 10 Marco Bonardo [::mak] 2011-06-17 09:53:24 PDT
fwiw, this patch passed a tryserver run.
Comment 11 Marco Bonardo [::mak] 2011-06-17 16:31:50 PDT
Created attachment 540178 [details] [diff] [review]
Test v1.0

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 :)
Comment 12 Marco Bonardo [::mak] 2011-06-18 02:36:19 PDT
the test fails on Linux, will have to check what's up there
Comment 13 Marco Bonardo [::mak] 2011-06-21 08:06:47 PDT
Created attachment 540755 [details] [diff] [review]
patch v1.2

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 14 Dietrich Ayala (:dietrich) 2011-06-22 10:54:55 PDT
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?
Comment 15 Marco Bonardo [::mak] 2011-06-22 13:27:08 PDT
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 16 Dietrich Ayala (:dietrich) 2011-06-28 06:49:14 PDT
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).
Comment 17 Marco Bonardo [::mak] 2011-06-28 08:25:18 PDT
Created attachment 542480 [details]
screenshot

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.
Comment 18 Marco Bonardo [::mak] 2011-06-28 15:54:30 PDT
I can give you a trybuild too http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-db809a6fd30b/
Comment 19 Alex Limi (:limi) — Firefox UX Team 2011-06-29 09:37:01 PDT
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?
Comment 20 Marco Bonardo [::mak] 2011-06-29 13:16:55 PDT
(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.
Comment 21 Marco Bonardo [::mak] 2011-06-29 13:46:27 PDT
Created attachment 542927 [details] [diff] [review]
patch v1.3

This patch uses opacity on both icon and text, with text slightly more opaque. needs bug 668319.
Comment 22 Marco Bonardo [::mak] 2011-06-30 13:43:12 PDT
http://hg.mozilla.org/projects/places/rev/4f84c26e07c2
Comment 23 Marco Bonardo [::mak] 2011-07-01 15:51:34 PDT
http://hg.mozilla.org/mozilla-central/rev/4f84c26e07c2
Comment 24 Simona B [:simonab ] -PTO- back Sept 5th 2011-08-23 06:48:53 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.