390739 - Redo menu item is greyed out in Bookmarks Organizer (was: doesn't work when deleting items and undoing in Bookmarks Manager) {grayed, inactive, disabled}

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Al Billings [:abillings - ex-MoCo]]

If a user deletes an item (bookmark, separator, folder), they can undo the deletion but then they cannot redo it. The menu option is there for the Bookmarks Manager menu but it is grayed out.

Steps to Reproduce
1. Open the Bookmarks Manager
2. Select an item (or multiple items) in the Bookmarks Manager.
3. Delete the item.
4. Under the "Edit" menu option, choose "Undo" (or use the keyboard shortcut).

Result: After the deletion is undone, the "Redo" menu item or keyboard shortcut should be active and work.

Actual Result: The "Redo" is unavailable.

This works as expected in FF 2.0.0.6.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

This was found in build Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200708020808 Minefield/3.0a7pre.

Comment 2

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091412 Minefield/3.0a8pre ID:2007091412

confirmed, since you have dataloss -> critical

Comment 3

[[:Aleksej]]

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007101604 Minefield/3.0a9pre

The menu item is grayed out, bug Ctrl-Shift-Z works.

Changing summary accordingly (correct me if I'm wrong, and/or another bug should be filed).

Comment 4

[Dietrich Ayala (:dietrich)]

A couple of notes:

1. On mac, the undo/redo in the main edit menu and the undo/redo in the organizer's menu get out of sync. I've seen them be opposite, both be the same but do nothing when clicked, and both be disabled when they should be enabled.

2. After undo-ing an action, if the redo menu item is disabled, you can select any item in the organizer, and this will enable the redo menu item, so maybe command updating is not being triggered properly.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: wallpaper

this fixes the organizer menu.

however, it doesn't fix the main edit menu on Mac. and i'm also not sure this is the right place to do this.

Comment 6

[Mike Connor [:mconnor]]

Mano, is there an ETA on a fix here? need it before freeze next week

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 290992 [details] [diff] [review]
patch



>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v
>retrieving revision 1.64
>diff -u -p -8 -r1.64 bookmarkProperties.js
>--- browser/components/places/content/bookmarkProperties.js	20 Nov 2007 02:01:54 -0000	1.64
>+++ browser/components/places/content/bookmarkProperties.js	1 Dec 2007 16:32:18 -0000
>@@ -340,18 +340,16 @@ var BookmarkPropertiesPanel = {
>     return PlacesUtils.history.getPageTitle(aURI);
>   },
> 
>   /**
>    * This method should be called by the onload of the Bookmark Properties
>    * dialog to initialize the state of the panel.
>    */
>   onDialogLoad: function BPP_onDialogLoad() {
>-    this._tm = window.opener.PlacesUtils.tm;
>-

Don't we need to refer to the instance from the browser window?

Hm, maybe it doesn't matter: I just tested bookmark creation in the main window on branch and it isn't undo-able.

Comment 9

[Asaf Romano (gone)]

The transaction manager is a single-tone for a very lone while now.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 290992 [details] [diff] [review]
patch

ok. so, if all these transactions go through the singleton tm service, why don't bookmarks via the star button get added to the undo stack? is there a specific reason this functionality is not on branch? maybe file a followup for that.

r+ since this fixes the organizer problem.

Comment 11

[Asaf Romano (gone)]

Er, they do get on the stack AFIACT, but you can only undo them through the organizer window or bookmarks sidebar (there's no way to focus the bookmarks toolbar).

Comment 12

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-places.js 1.69
mozilla/browser/base/content/nsContextMenu.js 1.30
mozilla/browser/components/places/Makefile.in 1.9
mozilla/browser/components/places/content/bookmarkProperties.js 1.65
mozilla/browser/components/places/content/controller.js 1.187
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.12
mozilla/browser/components/places/content/moveBookmarks.js 1.13
mozilla/browser/components/places/content/places.js 1.116

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre

Comment 14

[Tracy Walker [:tracy]]

in-litmus+ https://litmus.mozilla.org/show_test.cgi?id=6767

Comment 15

[Gervase Markham [:gerv]]

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