317633 - Undo/Redo for Places

Status: RESOLVED FIXED

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

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Bring across transaction management from the old Bookmarks system.

Comment 1

[Mike Connor [:mconnor]]

Transaction management is pretty broken in bookmarks, we didn't fix it because we were rewriting bookmarks...

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Mike, I wrote a bunch of new transactions :-)

Comment 3

[Mike Connor [:mconnor]]

Most excellent!  Dare I hope that we can undo sorting?

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Maybe... shouldn't be that hard to write a txn for. Will look at that when we get to implementing sorting. 

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: add transaction manager attr to history service

Comment 6

[Hixie (not reading bugmail)]

*** Bug 317805 has been marked as a duplicate of this bug. ***

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: progress, not complete

This modifies the goDoCommand/etc methods in globalOverlay.js not just to look for controllers on the focused element, but also the containing global window.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

*** Bug 320286 has been marked as a duplicate of this bug. ***

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more work...

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more work...

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

*** Bug 324957 has been marked as a duplicate of this bug. ***

Comment 13

[Annie Sullivan]

Comment on attachment 210026 [details] [diff] [review]
more work...

>       <method name="saveSelection">
>+        <parameter name="mode"/>

It looks like you changed the parameters to this method in tree.xml but didn't update the parameters in toolbar.xml and menu.xml.  I haven't really worked with XBL enough to know if it matters.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

It shouldn't do, but I've updated my patch to include this anyway, so that no one gets confused when they read this later. 

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Marking this fixed. I know there are still issues but I'm going to handle those in individual bugs. 

Comment 16

[Scott MacGregor]

Attachment: fix thunderbird bustage, add back the rest of the shims

This patch broke Thunderbird because it removed some toolkit methods that are used by Thunderbird. 

I added shim routines for the methods that got removed. I added the implementations for these shims to the CommandUpdater object, let me know if you'd prefer them to not be a part of the CommandUpdater object.

Comment 17

[Scott MacGregor]

Comment on attachment 210311 [details] [diff] [review]
fix thunderbird bustage, add back the rest of the shims

Looks like the globalOverlay.js changes went into the branch as well so we'll need this there too.

Comment 18

[Scott MacGregor]

Comment on attachment 210311 [details] [diff] [review]
fix thunderbird bustage, add back the rest of the shims

Ben just reviewed this over my shoulder.

Comment 19

[Scott MacGregor]

and it's now fixed on the trunk and the branch.

Comment 20

[[e-mail]]

Comment on attachment 210026 [details] [diff] [review]
more work...

>+  /**
>+   * Gets a controller that can handle a particular command. 
>+   * @param   command
>+   *          A command to locate a controller for, preferring controllers that
>+   *          show the command as enabled.
>+   * @returns In this order of precedence:
>+   *            - the first controller supporting the specified command 
>+   *              associated with the focused element that advertises the
>+   *              command as ENABLED
>+   *            - the first controller supporting the specified command 
>+   *              associated with the global window that advertises the
>+   *              command as ENABLED
>+   *            - the first controller supporting the specified command
>+   *              associated with the focused element
>+   *            - the first controller supporting the specified command
>+   *              associated with the global window
>+   */
>+  _getControllerForCommand: function(command) {
>+    try {
>+      var controller = 
>+          top.document.commandDispatcher.getControllerForCommand(command);
>+      if (controller && controller.isCommandEnabled(command))
>+        return controller;
>+    }
>+    catch(e) {
>+    }
>+    var controllerCount = window.controllers.getControllerCount();
>+    for (var i = 0; i < controllerCount; ++i) {
>+      var current = window.controllers.getControllerAt(i);
>+      try {
>+        if (current.supportsCommand(command) && current.isCommandEnabled(command))
>+          return current;
>+      }
>+      catch (e) {
>+      }
>+    }
>+    return controller || window.controllers.getControllerForCommand(command);
>+  },
Unfortunately this is totally bogus. Not only does commandDispatcher.getControllerForCommand already does all the work, but as a bonus it does it almost correctly! In particular, the precedence is:
- the first controller supporting the specified command associated with the
  focused element that advertises the command as *SUPPORTED*
- the first controller supporting the specified command associated with the
  global window that advertises the command as *SUPPORTED*
- the first controller supporting the specified command associated with the
  first ancestor window that advertises the command as *SUPPORTED*
(In case you're wondering, the bug is that it sometimes queries the global commands twice).

Comment 21

[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