Closing a Tab should add Undo option when applicable

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
11 years ago
9 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

({relnote})

Trunk
seamonkey2.0a1
x86
All
relnote
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
Created attachment 235694 [details] [diff] [review]
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().
Attachment #235694 - Flags: superreview?(jag)
Attachment #235694 - Flags: review?(neil)
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

11 years ago
*** Bug 351093 has been marked as a duplicate of this bug. ***
Blocks: 315312

Comment 4

11 years ago
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 ).
Attachment #235694 - Flags: review?(neil) → review-
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

11 years ago
(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.
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.
Attachment #235694 - Flags: superreview?(jag)
Created attachment 240091 [details] [diff] [review]
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 ;)
Attachment #240091 - Flags: superreview?(neil)
Attachment #240091 - Flags: review?(neil)

Comment 9

11 years ago
(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...
(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.
Created attachment 240266 [details] [diff] [review]
patch v3

URL bar fixed.  I *think* I got all of your comments and what we talked about on IRC.
Attachment #235694 - Attachment is obsolete: true
Attachment #240091 - Attachment is obsolete: true
Attachment #240266 - Flags: superreview?(neil)
Attachment #240266 - Flags: review?(neil)
Attachment #240091 - Flags: superreview?(neil)
Attachment #240091 - Flags: review?(neil)

Comment 12

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

Comment 14

11 years ago
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

Updated

11 years ago
Attachment #240575 - Attachment description: v4 → patch v4
Attachment #240575 - Flags: superreview?
Attachment #240575 - Flags: superreview+
Attachment #240575 - Flags: review?(neil)
Attachment #240575 - Flags: review+
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: relnote
Resolution: --- → FIXED
Whiteboard: [cst: request a= after baking]
Could this bug here have caused Bug 354927?
Attachment #240575 - Flags: approval-seamonkey1.1b?
Depends on: 354927

Comment 17

11 years ago
(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? ;)
Blocks: 354953

Comment 18

11 years ago
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]
(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

11 years ago
Comment on attachment 240575 [details] [diff] [review]
patch v4

a=me given it works well on trunk.
Attachment #240575 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+

Comment 21

11 years ago
(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?
(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.
Depends on: 355657
This caused bug 355657.
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.
Depends on: 320448
Whiteboard: [cst: request a= after baking]
That should be bug 320488.
Depends on: 320488
No longer depends on: 320448
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.
Depends on: 356703

Updated

11 years ago
Depends on: 358599

Comment 27

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

Comment 28

11 years ago
If this isn't going to go on the branch, the current 1.1b milestone and 1.8 branch version are wrong.
Target Milestone: seamonkey1.1beta → seamonkey1.5alpha
Version: 1.8 Branch → Trunk
Depends on: 356805
No longer blocks: 315312
Depends on: 363792

Updated

11 years ago
Depends on: 369053

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.