350416 - Closing a Tab should add Undo option when applicable

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

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

I filed this because bug 152391 has too many useless comments.  I have a reasonably-functional patch (not derived from the Firefox implementation).

Don't comment in this bug about:
* How it should work (or how any other implementations work)
* Firefox, Opera, or extensions
* Restoring closed windows (bug 39165)
* Per-tab close buttons
* Your two cents

Comment 1

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

Attachment: patch

1. I'll make the context popup localizable later
2. Updating the context menu is broken since at least SM1.0
3. It's possible I didn't do everything I needed to in restoreTab()
4. I'll indent the code in removeTab better later

Note that this does actually seem to work well, and other than issue 2, I haven't noticed any bugs.

So, I'm looking for normal review-like comments, suggestions for issue 3, and suggestions for UI (e.g. adding something to the Edit menu, and possibly a hotkey).

A brief summary of the method used in this patch is in the comment block in removeTab().

Comment 2

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

Gavin pointed me to Firefox bug 342700 and Firefox bug 344736, which have a little bit of UI talk in them.

We could add a list of recently closed tabs as a submenu of the Go menu, which as far as I can tell is what beltzner spec'd for Firefox.  It would require passing an argument to restoreTab (an index) but that's very easy to handle.

Comment 3

[Jason Bassford]

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

Comment 4

[[e-mail]]

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

Undo close tab is tricky if you have autohide :-/
It would be nice if the tab undid to its original position.
It would be nicer if the unclosed tab was reselected where appropriate.
It would be nicer still if browser back group used undo close tab.

>+            // this doesn't work, but then again, the above code doesn't either
Enn has a patch to make .disabled work. For branch, use .setAttribute

>+            // a fair amount of this is duplicated in addTab... maybe we should
>+            // add a helper function that takes care of the shared stuff
Or at least keep a dummy tab around that we just clone each time.

>+            // just set the blank text, it gets clobbered instantly for non-blank tabs
Not quite instantly... try adding the tab after fixing the browser.

>+            var shCopy = savedData.history;
Nit: variable only used once.

>+            if (b.webNavigation.canGoBack) // tabs created as blank can't go back
In that case, why bother saving the session history?

>+// this needs a pref... even though we navigate
>+// to about:blank, it still costs RAM
Well, bfcache costs you RAM, so navigating to about:blank actually saves RAM because it allows bfcache to trim entries according to the existing prefs.
Of course session history is still costing you RAM here.

>+            var shCopy = oldBrowser.webNavigation.sessionHistory;
Except it's not a copy.

>+              oldBrowser.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory);
Need a var for this. And CI this to nsISHistoryInternal (saves a QI below).

>+            oldBrowser.loadURIWithFlags("about:blank", Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE, null, null);
The tab might still be current at this point, making the Stop button flicker
(especially when tested with the decent "close-button-hides-tab-bar" :-P ).

Comment 5

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

Thanks for the review.  I'll need some clarifications so I can take care of your review comments tomorrow [my time zone], though.

(In reply to comment #4)
> (From update of attachment 235694 [details] [diff] [review] [edit])
> Undo close tab is tricky if you have autohide :-/

Just because I currently lack UI for that?  I'd rather get the code in ASAP so people can tell us what's broken.  It will definitely be a priority for me to add something to the Edit menu (probably the same thing as on the tab context menu).

> It would be nice if the tab undid to its original position.
Enhancement.

> It would be nicer if the unclosed tab was reselected where appropriate.
"Where appropriate"?  When is it appropriate?  If the tab isn't restored to the end, it *needs* to be focused.

> It would be nicer still if browser back group used undo close tab.
Hmm... enhancement?  Code cleanup on trunk?

> >+            // this doesn't work, but then again, the above code doesn't either
> Enn has a patch to make .disabled work. For branch, use .setAttribute

Ok, will do.

> >+            // a fair amount of this is duplicated in addTab... maybe we should
> >+            // add a helper function that takes care of the shared stuff
> Or at least keep a dummy tab around that we just clone each time.

Which would you prefer?  What's the right way to clone this tab?  Would you keep it as a <field>?  Keeping it in the strip would be ugly.

> >+            // just set the blank text, it gets clobbered instantly for non-blank tabs
> Not quite instantly... try adding the tab after fixing the browser.

Ok.

> >+            var shCopy = savedData.history;
> Nit: variable only used once.

Ok.

> >+            if (b.webNavigation.canGoBack) // tabs created as blank can't go back
> In that case, why bother saving the session history?

No real reason.  I could set it to null... it just happened to be easier this way.

> >+// this needs a pref... even though we navigate
> >+// to about:blank, it still costs RAM
> Well, bfcache costs you RAM, so navigating to about:blank actually saves RAM
> because it allows bfcache to trim entries according to the existing prefs.
> Of course session history is still costing you RAM here.

What I meant was that about:blank still costs more than really closing the tab, even when the bfcache gets evicted.  I pointed that out as an explanation of why I clobber it after n entries.

> >+            var shCopy = oldBrowser.webNavigation.sessionHistory;
> Except it's not a copy.

Ok.

> >+              oldBrowser.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory);
> Need a var for this. And CI this to nsISHistoryInternal (saves a QI below).

Ok.

> >+            oldBrowser.loadURIWithFlags("about:blank", Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE, null, null);
> The tab might still be current at this point, making the Stop button flicker
> (especially when tested with the decent "close-button-hides-tab-bar" :-P ).

setTimeout(function() { that line; }, 50); or what do you suggest?  Move it after tab-select code

Comment 6

[[e-mail]]

(In reply to comment #5)
>>It would be nice if the tab undid to its original position.
>Enhancement.
OK, but don't forget!

>>It would be nicer if the unclosed tab was reselected where appropriate.
>"Where appropriate"?  When is it appropriate?
When you had closed the selected tab.

>Which would you prefer?
I don't mind, a helper function would be fine. (Put it just before addTab).

>Would you keep it as a <field>?
Yes.

>>>+            // just set the blank text, it gets clobbered instantly for non-blank tabs
>>Not quite instantly... try adding the tab after fixing the browser.
>Ok.
Or you could save the title from the old tab.

>>>+// this needs a pref... even though we navigate
>>>+// to about:blank, it still costs RAM
It occurs to me, what about close other tabs? Would you want to save them all?

>>The tab might still be current at this point, making the Stop button flicker
>what do you suggest?  Move it after tab-select code
Right.

Comment 7

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

Neil, what's causing the URL bar not to update here?

(In reply to comment #4)
> It would be nice if the tab undid to its original position.

The user might have created new tabs or dragged them around.

> It would be nicer if the unclosed tab was reselected where appropriate.

I made it unconditionally focus the new tab.  That seems like a good behavior to me - if you unclose a tab, you probably want to use it immediately.

> >+            // a fair amount of this is duplicated in addTab... maybe we should
> >+            // add a helper function that takes care of the shared stuff
> Or at least keep a dummy tab around that we just clone each time.

I went with the clone.  It was very simple to do.

> >+            // just set the blank text, it gets clobbered instantly for non-blank tabs
> Not quite instantly... try adding the tab after fixing the browser.

No need to actually save it.  The goBack() causes a document load which conveniently sets the title for us.

> >+            if (b.webNavigation.canGoBack) // tabs created as blank can't go back
> In that case, why bother saving the session history?

I don't see any reason to conditionally save the history (*and* have to create a new one conditionally if it didn't get saved) rather than just conditionally go back and not have to ever create an extra session history.

> >+// this needs a pref... even though we navigate
> >+// to about:blank, it still costs RAM
> Well, bfcache costs you RAM, so navigating to about:blank actually saves RAM
> because it allows bfcache to trim entries according to the existing prefs.
> Of course session history is still costing you RAM here.

I removed the comment

> 
> >+            var shCopy = oldBrowser.webNavigation.sessionHistory;
> Except it's not a copy.

Removed he variable since it was only used once, and now I don't need to come up with a new name :)

> >+              oldBrowser.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory);
> Need a var for this. And CI this to nsISHistoryInternal (saves a QI below).

No go on the Internal.  Based on what I tried, the webNavigation.sessionHistoyr needs to be an nsISHistory.  I created a const for the components.interfaces... but if you wanted a var for something else, I'm pretty sure there's no savings.

> >+            oldBrowser.loadURIWithFlags("about:blank", Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE, null, null);
> The tab might still be current at this point, making the Stop button flicker
> (especially when tested with the decent "close-button-hides-tab-bar" :-P ).

Fixed.

Comment 8

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

Attachment: patch v2

There are a couple cases where I could move code around to reduce the number of if(something related to max depth) checks, but I think it fits the actual order we want things to happen better this way.

I *think* I got all of the review issues, except for the bug with the URL bar I mentioned above, which I need help with.  Here's hoping it's a one-liner ;)

Comment 9

[[e-mail]]

(In reply to comment #7)
>(In reply to comment #4)
>>It would be nice if the tab undid to its original position.
>The user might have created new tabs or dragged them around.
Good point.


>>>+            // just set the blank text, it gets clobbered instantly for non-blank tabs
>>Not quite instantly... try adding the tab after fixing the browser.
>No need to actually save it.  The goBack() causes a document load which
>conveniently sets the title for us.
OK, but it introduces a delay... note that the title is also saved on the history entry so you can grab it there.

>>>+            if (b.webNavigation.canGoBack) // tabs created as blank can't go back
>>In that case, why bother saving the session history?
>I don't see any reason to conditionally save the history (*and* have to create
>a new one conditionally if it didn't get saved) rather than just conditionally
>go back and not have to ever create an extra session history.
As I recall, you're always creating an extra session history, which is my point.

>>>+              oldBrowser.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory);
>>Need a var for this. And CI this to nsISHistoryInternal (saves a QI below).
>No go on the Internal.  Based on what I tried, the webNavigation.sessionHistory
>needs to be an nsISHistory.
I had thought XPConnect was clever enough to QI parameters passed to setters...

Comment 10

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

(In reply to comment #9)
> >>>+            // just set the blank text, it gets clobbered instantly for non-blank tabs
> >>Not quite instantly... try adding the tab after fixing the browser.
> >No need to actually save it.  The goBack() causes a document load which
> >conveniently sets the title for us.
> OK, but it introduces a delay... note that the title is also saved on the
> history entry so you can grab it there.

What's the right way to get it from there?  I see only setters on nsISHEntry.  Do I need to QI to nsIHistoryEntry, or can I grab .title directly anyway?

> >>>+            if (b.webNavigation.canGoBack) // tabs created as blank can't go back
> >>In that case, why bother saving the session history?
> >I don't see any reason to conditionally save the history (*and* have to create
> >a new one conditionally if it didn't get saved) rather than just conditionally
> >go back and not have to ever create an extra session history.
> As I recall, you're always creating an extra session history, which is my
> point.

Hmm... I'll look into this again tonight I guess.

> >>>+              oldBrowser.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory);
> >>Need a var for this. And CI this to nsISHistoryInternal (saves a QI below).
> >No go on the Internal.  Based on what I tried, the webNavigation.sessionHistory
> >needs to be an nsISHistory.
> I had thought XPConnect was clever enough to QI parameters passed to setters...

Ok, I'll try again.  Now I understand what you wanted me to add a var for too, and I think it's reasonable.

Comment 11

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

Attachment: patch v3

URL bar fixed.  I *think* I got all of your comments and what we talked about on IRC.

Comment 12

[[e-mail]]

Comment on attachment 240266 [details] [diff] [review]
patch v3

>+            if (this.mPrefs.getIntPref("browser.tabs.undoclose.depth")) {
>+              aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-undoclosetab")[0].setAttribute("disabled", (this.savedBrowsers.length == 0));
>+              aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-undoclosetab")[0].hidden = false;
>+            }
>+            else
>+              aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-undoclosetab")[0].hidden = true;
This menuitem needs a temporary. Also, I don't think it would hurt to unconditionally set the disabled attribute.

>+            if (!b.webNavigation.canGoBack) alert("Me fail English? That's unpossible!");
That's what exceptions are for ;-)

>+            b.userTypedValue = hist.getEntryAtIndex(hist.index, false).URI.spec;
>+            b.userTypedClear = 2;
>+            b.webNavigation.goBack();
I'll have to look at what's going on here... maybe it's to do with the order in which the listeners are added?

>+            if (t.previousSibling.selected)
>+              t.setAttribute("afterselected", true);
Not necessary if you're unconditionally selecting the tab.

>+
>+
Nit: double-spaced

>+            // UNDO Remove our title change listener
I don't like the wording... how about "hook up the title change listener"?

>+              oldBrowser.destroy();
>+              this.mPanelContainer.removeChild(deadBrowser);
Should be oldBrowser surely?

>+            // dont save blank tabs
But you don't clean them up either... you need to do the above cleanup if either the max undo is <= 0 or the count is 0

Comment 13

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

Attachment: patch v4

This does not select the tab that gets created on undo.  It addresses the issues discussed on IRC since the last patch.  Saving the xul:tab was more trouble than it was worth (before/afterselected vs linkedpanel/linkedbrowser), so I don't.

Comment 14

[[e-mail]]

Comment on attachment 240575 [details] [diff] [review]
patch v4

>+            var undoItem = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-undoclosetab")[0];
document.getAnonymousElementByAttribute("tbattr", "tabbrowser-undoclosetab") ?

>+            //oldBrowser.loadURIWithFlags("about:blank", Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE, null, null);
:-P

Comment 15

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

This needs a relnote on how to disable it or we'll get even more "memory usage doens't go down when closing a tab" bugs filed.

Fixed on trunk.

Comment 16

[Frank Wein [:mcsmurf]]

Could this bug here have caused Bug 354927?

Comment 17

[Robert Roessler]

(In reply to comment #15)
> This needs a relnote on how to disable it or we'll get even more "memory usage
> doens't go down when closing a tab" bugs filed.

Perhaps [in the absence of release notes for unreleased products] the relevant info could be immortalized here? ;)

Comment 18

[[e-mail]]

Other potential regressions (no bugs filed yet):
updatePopupMenu() counts browsers (including recently closed tabs)
reloadAllTabs() reloads recently closed tabs (note: about:blank)
replaceGroup() counts browsers, not tabs, also calls removeTab()

Notes from the original moveTabTo() code while I was reviewing:
constructor could use this.mCurrentBrowser and this.mCurrentTab
destructor doesn't remove matching progress listeners and filters
[progress listeners and filters indexes match tabs, not browsers]

Comment 19

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

(In reply to comment #18)
> Other potential regressions (no bugs filed yet):
> updatePopupMenu() counts browsers (including recently closed tabs)
> reloadAllTabs() reloads recently closed tabs (note: about:blank)
> replaceGroup() counts browsers, not tabs, also calls removeTab()

For those not watching dependent bugs, these issues were addressed in bug 354927.

Comment 20

[Robert Kaiser]

Comment on attachment 240575 [details] [diff] [review]
patch v4

a=me given it works well on trunk.

Comment 21

[Robert Roessler]

(In reply to comment #17)
> (In reply to comment #15)
> > This needs a relnote ...

So, is setting browser.tabs.undoclose.depth to 0 the correct way to disable this behavior and allow [immediate?] recovery of the memory when closing tabs?

Comment 22

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

(In reply to comment #21)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > This needs a relnote ...
> 
> So, is setting browser.tabs.undoclose.depth to 0 the correct way to disable
> this behavior and allow [immediate?] recovery of the memory when closing tabs?

Yes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

This caused bug 355657.

Comment 24

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

Given that we're seeing people hit bug 320448, I'm not sure this can go on branch.  Since we're a second-class project nowadays, we're less likely to get gecko bugs fixed on a schedule that fits our needs.  It's gonna suck to be missing the feature in 1.1, but I don't see what can be done about it.

Comment 25

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

That should be bug 320488.

Comment 26

[Boris Zbarsky [:bzbarsky]]

One other note.  Before speaking of Gecko bugs, stop and ask yourself whether anyone ever said that what you're doing is something that's likely to have been designed-to-be-ok.  If not, then you're asking for a feature.

Comment 27

[Felix Miata]

If this isn't going to go on the branchm the current 1.1b milestone and 1.8 branch version are wrong.

Comment 28

[Felix Miata]

If this isn't going to go on the branch, the current 1.1b milestone and 1.8 branch version are wrong.