The default bug view has changed. See this FAQ.

Undo-close-tab-related cleanup/enhancements

RESOLVED FIXED

Status

SeaMonkey
General
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com], Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

Trunk
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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

11 years ago
(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.
Assignee: general → cst
Created attachment 240773 [details] [diff] [review]
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.
Attachment #240773 - Flags: superreview?(neil)
Attachment #240773 - Flags: review?(neil)
Depends on: 350416

Comment 3

11 years ago
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
Attachment #240773 - Flags: superreview?(neil)
Attachment #240773 - Flags: superreview+
Attachment #240773 - Flags: review?(neil)
Attachment #240773 - Flags: review+
Created attachment 240823 [details] [diff] [review]
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.
Attachment #240773 - Attachment is obsolete: true
Attachment #240823 - Flags: superreview?(neil)

Comment 5

11 years ago
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 +]
Attachment #240823 - Flags: superreview?(neil) → superreview+
Created attachment 240995 [details] [diff] [review]
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 ;)
Attachment #240995 - Flags: approval-seamonkey1.1b?
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 8

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

a=me given it works well on trunk.
Comment on attachment 240995 [details] [diff] [review]
what I checked in on trunk

a=me for 1.1b
Attachment #240995 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+

Updated

11 years ago
Depends on: 356746
Depends on: 362913
You need to log in before you can comment on or make changes to this bug.