Last Comment Bug 354953 - Undo-close-tab-related cleanup/enhancements
: Undo-close-tab-related cleanup/enhancements
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
:
Mentors:
Depends on: 350416 356746 362913
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-30 08:32 PDT by Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
Modified: 2006-12-06 07:37 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 1, 2, 5 (9.12 KB, patch)
2006-09-30 16:59 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
updated patch (14.24 KB, patch)
2006-10-01 10:22 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: superreview+
Details | Diff | Splinter Review
what I checked in on trunk (14.14 KB, patch)
2006-10-02 16:29 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
cbiesinger: approval‑seamonkey1.1b+
Details | Diff | Splinter Review

Description Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-09-30 08:32:09 PDT
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] 2006-09-30 10:32:46 PDT
(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 cst@andrew.cmu.edu cst@yecc.com] 2006-09-30 16:59:28 PDT
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.
Comment 3 neil@parkwaycc.co.uk 2006-10-01 09:12:57 PDT
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 cst@andrew.cmu.edu cst@yecc.com] 2006-10-01 10:22:06 PDT
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.
Comment 5 neil@parkwaycc.co.uk 2006-10-02 06:40:30 PDT
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 cst@andrew.cmu.edu cst@yecc.com] 2006-10-02 16:29:57 PDT
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 ;)
Comment 7 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-02 16:30:18 PDT
Fixed on trunk.
Comment 8 Robert Kaiser 2006-10-02 17:27:59 PDT
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) 2006-10-03 21:49:29 PDT
Comment on attachment 240995 [details] [diff] [review]
what I checked in on trunk

a=me for 1.1b

Note You need to log in before you can comment on or make changes to this bug.