354953 - Undo-close-tab-related cleanup/enhancements

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

1) We need more UI - Firefox has History -> Recently Closed Tabs (they also put all pages in the top level of the History menu), and ctrl+shift+T to undo close tab

2) It would be nice to switch to the tab that's created after an undo.  We need to figure out what's going on with the URL bar.

3) Browser back group - I'm not sure what exactly Neil wanted here.

4) Close other tabs - do we want to support undoing the whole group as one shot, and count it as 1 slot of depth?

5) If the only open tab is blank w/no history, we should reuse that tab.

6) Do we want to reconsider the default depth of 3?  How much memory does it cost to keep a session history around, once it's expired from bfcache? (does 1.8-branch keep n global slots for bfcache or is it per-tab?)

1 needs to happen on branch before 1.1 final (probably not catastrophic if it misses beta, but we need strings localized).  5 would be nice, but is not critical.

Comment 1

[Frank Wein [:mcsmurf]]

(In reply to comment #0)
> 6) Do we want to reconsider the default depth of 3?  How much memory does it
> cost to keep a session history around, once it's expired from bfcache? (does
> 1.8-branch keep n global slots for bfcache or is it per-tab?)

I think Bug 292965 made bfcache global, not per tab. Also see http://weblogs.mozillazine.org/ben/archives/009749.html#c124523 and the following comment on this.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch for 1, 2, 5

1) UI in the file menu, ctrl+shift+t
2) The restored tab is selected (unconditionally)
5) If the only tab present is a blank tab with no history, undoing a close replaces that tab

3, 4, and 6 are not addressed by this patch.

-            if (this.savedBrowsers.length == 0)
+            if (aIndex >= this.savedBrowsers.length)
This change isn't required.

+            if (this.mTabs.length == 2 &&
It's 2 rather than 1 because we've already appended the tab being restored.

Comment 3

[[e-mail]]

Comment on attachment 240773 [details] [diff] [review]
patch for 1, 2, 5

r+sr=me (fixes listed in order of importance 0 = ignore 7 = fix)

>+  recentTabsItem.setAttribute("disabled", browser.getUndoList().length == 0);
0) I assume that you switched trunk tabbrowser to setAttribute to sync it with branch? Other than that, I would switch to .disabled since Enn's fix.

>+  var list = browser.getUndoList();
>+  for (var i = 0; i < list.length; i++) {
>+    var menuitem = document.createElement("menuitem");
>+    menuitem.setAttribute("label", list[i]);
>+    menuitem.setAttribute("value", i);
>+    popup.appendChild(menuitem);
>+  }
1) Number the (first nine) items perhaps?

>+            if (this.mTabs.length == 2 &&
>+                !this.mTabs[0].linkedBrowser.webNavigation.sessionHistory.count)
>+              this.removeTab(this.mTabs[0]);
>+
>+            this.selectedTab = t;
2) Could use else here, as the tab is already selected if it's the last one.

>+  var popup = document.getElementById("menu_recentTabsPopup");
3) Or pass the popup as a parameter onpopupshowing="updateRecentTabs(this);"

>+    popup.removeChild(popup.firstChild);
4) lastChild

>+  while (popup.childNodes.length > 0)
5) hasChildNodes()

>+            if (aIndex >= this.savedBrowsers.length)
6) Check for aIndex < 0 - this is now a "public" API.

>+    <key id="key_restoreTab" key="&recentTabs.commandkey;" modifiers="accel,shift" command="gBrowser.restoreTab(0);"/>
7) oncommand

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated patch

This takes care of your points from bug 350416, except for the constructor/destructor stuff.  It also makes ctrl+shift+T more discoverable.

Comment 5

[[e-mail]]

Comment on attachment 240823 [details] [diff] [review]
updated patch

>+    var label;
>+    if (i < 9) {
>+      label = gNavigatorBundle.getFormattedString("tabs.recentlyClosed.format", [i+1, list[i]]);
>+      menuitem.setAttribute("accesskey", i+1);
>+    }
>+    else
>+      label = gNavigatorBundle.getFormattedString("tabs.recentlyClosed.format", ["", list[i]]);
You only need to format the first 9 menuitems. Consider using var label = list[i]; and pass [i + 1, label] to getFormattedString. [spaces around +]

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: what I checked in on trunk

Note that this depends on undo close tab, which doesn't have approval yet.  If you set a+, I won't land this until undo close tabs lands ;)

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Fixed on trunk.

Comment 8

[Robert Kaiser]

Comment on attachment 240995 [details] [diff] [review]
what I checked in on trunk

a=me given it works well on trunk.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 240995 [details] [diff] [review]
what I checked in on trunk

a=me for 1.1b