Implement proper cut action for bookmarks (was: undo/redo a cut requires two steps, bookmarks are lost if if paste never happens).

VERIFIED FIXED in Firefox 7

Status

()

Firefox
Bookmarks & History
P2
critical
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Tomcat, Assigned: mak)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dataloss})

Trunk
Firefox 7
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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?
Severity: normal → critical
Flags: blocking-firefox3.1?
Keywords: dataloss
OS: Mac OS X → All
Hardware: x86 → All
Summary: Undo does not restore Cut and Pasted Bookmark → Undo/redo requires two steps for copying/moving bookmarks or folders
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.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Priority: -- → P2
Target Milestone: --- → Firefox 3.2a1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
(Assignee)

Updated

7 years ago
Duplicate of this bug: 484578
(Assignee)

Updated

7 years ago
Summary: Undo/redo requires two steps for copying/moving bookmarks or folders → Undo/redo a CUT requires two steps, bookmarks are removed on cut even if paste never happens.
(Assignee)

Comment 6

7 years ago
tentatively assigning, should be fixed sometimes.
Assignee: nobody → mak77
Target Milestone: Firefox 3.6a1 → ---
(Assignee)

Updated

6 years ago
Depends on: 664127
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
A couple advantages of this approach:
- more native
- less disk IO
- less Sync impact
- no unexpected dataloss
- no dataloss for bug 301888
(Assignee)

Updated

6 years ago
Blocks: 301888
(Assignee)

Updated

6 years ago
Flags: in-testsuite?
Summary: Undo/redo a CUT requires two steps, bookmarks are removed on cut even if paste never happens. → Implement proper cut action for bookmarks (was: undo/redo a cut requires two steps, bookmarks are lost if if paste never happens).
Whiteboard: [places-next-wanted]
(Assignee)

Comment 9

6 years ago
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.
Attachment #539204 - Attachment is obsolete: true
Attachment #539964 - Flags: review?(dietrich)
(Assignee)

Comment 10

6 years ago
fwiw, this patch passed a tryserver run.
(Assignee)

Comment 11

6 years ago
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 :)
Attachment #540178 - Flags: review?(dietrich)
(Assignee)

Comment 12

6 years ago
the test fails on Linux, will have to check what's up there
(Assignee)

Comment 13

6 years ago
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.
Attachment #539964 - Attachment is obsolete: true
Attachment #540755 - Flags: review?(dietrich)
Attachment #539964 - Flags: review?(dietrich)
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?
(Assignee)

Comment 15

6 years ago
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).
Attachment #540755 - Flags: review?(dietrich) → review+
(Assignee)

Comment 17

6 years ago
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.
Attachment #542480 - Flags: ui-review?(limi)
(Assignee)

Comment 18

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

Updated

6 years ago
Blocks: 663058
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?
Attachment #542480 - Flags: ui-review?(limi) → ui-review+
(Assignee)

Updated

6 years ago
Depends on: 668319
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
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.
Attachment #540178 - Attachment is obsolete: true
Attachment #540755 - Attachment is obsolete: true
Attachment #540178 - Flags: review?(dietrich)
(Assignee)

Comment 22

6 years ago
http://hg.mozilla.org/projects/places/rev/4f84c26e07c2
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
(Assignee)

Comment 23

6 years ago
http://hg.mozilla.org/mozilla-central/rev/4f84c26e07c2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
(Assignee)

Updated

6 years ago
Flags: in-testsuite? → in-testsuite+
Depends on: 674964
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Blocks: 682512

Updated

5 years ago
Depends on: 723842

Updated

5 years ago
Depends on: 734716
(Assignee)

Updated

5 years ago
No longer depends on: 734716
You need to log in before you can comment on or make changes to this bug.