Closed Bug 504122 Opened 15 years ago Closed 13 years ago

provide undo close tab (Ctrl+Shift+T) [Go > Recently Closed Tabs]

Categories

(Thunderbird :: Toolbars and Tabs, enhancement)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 5.0b1

People

(Reporter: wsmwk, Assigned: schmid-thomas)

References

(Blocks 1 open bug)

Details

(Keywords: student-project, Whiteboard: [partly fixed by bug 468808][landed in comment 41][needs followup bug, see comment 47])

Attachments

(3 files, 4 obsolete files)

mockup of a tab popup menu
18.46 KB, image/png
Details
screenshots
41.82 KB, image/png
clarkbw
: feedback-
Details
Patch v3
37.43 KB, patch
asuth
: review+
Details | Diff | Splinter Review 
provide undo close tab, especially for but not limited to message reader
...and maybe even the same shift-ctrl-t shortcut to undo, just like Fx. Also definitely smells like a potentially good student-project.
Flags: wanted-thunderbird3?
Keywords: student-project
Assignee: nobody → kitallis
I can't wait for this to work.

Also it would be useful to mimic FF by providing a "Recently Closed Tabs" menulist in one of the menus.  I'm not sure what the best location would be for that.  In FF they have the History menu which makes a lot of sense.  In TB we don't have a menu that relates as well to tabs.  My options in order of preference were "Go", "Message", and "Tools".
bug 500758 comment 0 had some ideas for the Go menu / recently closed tabs list.  I'll dupe that to this.
I'll work on 'undo last closed tab', then hopefully a menu with multiple recently closed tabs.
Assignee: kitallis → zgchurch
Status: NEW → ASSIGNED
Whiteboard: ucosp

        Attachment #424432 -
        Flags: review?(bwinton)

    
    

    
  
Comment on attachment 424432 [details] [diff] [review]
Patch to add "Reopen last tab" to Thunderbird's Go menu

>+++ b/mail/base/content/mailWindowOverlay.xul
>@@ -240,6 +240,7 @@
>@@ -1334,6 +1336,10 @@
>                     accesskey="&startPageCmd.accesskey;"
>                     command="cmd_goStartPage"
>                     key="key_goStartPage"/>
>+          <menuitem id="goUndoCloseTab"
>+                    label="&undoCloseTab.label;"
>+                    command="cmd_goUndoCloseTab"
>+                    accesskey="&undoCloseTab.accesskey;" />

Two small things:
1) We should put accesskey before command (like the menuitem above).
2) We shouldn't have a space before the "/>" (like the menuitem above).

>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -1022,6 +1022,11 @@
>   ClearEditMessageBox();
> }
> 
>+function UndoRecentlyClosedTab() {
>+  var tabmail = document.getElementById("tabmail");
>+  tabmail.undoCloseTab();
>+}

I'ld prefer to see that function as:
  document.getElementById("tabmail").undoCloseTab();
(Unless you're doing something else with the tabmail variable.)

>+++ b/mail/base/content/tabmail.xml
>@@ -632,6 +634,18 @@
>+      <method name="undoCloseTab">
>+        <body>
>+          <![CDATA[
>+            if (!this.recentlyClosedTab)
>+            {
>+              throw Error("No tabs were previously opened!");
>+            }

We don't use {}s around single-line if statements.
(I know, I prefer them too, but it's the standard to not have them. ;)

>+            this.openTab(this.recentlyClosedTab.mode.type, this.recentlyClosedTab);

This line could be made less than 80 characters by writing it as:
            this.openTab(this.recentlyClosedTab.mode.type,
                         this.recentlyClosedTab);

>@@ -1022,10 +1036,15 @@
> 
>              let supportsCommandFunc = tab.mode.supportsCommand ||
>                                        tab.mode.tabType.supportsCommand;
>-             if (supportsCommandFunc)
>-               return supportsCommandFunc.call(tab.mode.tabType, aCommand, tab);
>-
>-             return false;
>+             switch ( aCommand )
>+             {
>+               case "cmd_goUndoCloseTab":
>+                 return true;
>+               default:
>+                 if (supportsCommandFunc)
>+                   return supportsCommandFunc.call(tab.mode.tabType, aCommand, tab);
>+                 return false;
>+             }

The switch seems like overkill for a single test.

If you change it to
             if (aCommand == "cmd_goUndoCloseTab")
               return true;
             if (supportsCommandFunc)
               return supportsCommandFunc.call(tab.mode.tabType, aCommand, tab);
             return false;
It's smaller, and fits in 80 characters, and reads better to me.

(And, as a note, the {s go on the same line as the previous statement. i.e.
 if (test) {
   statements;
 }
 else {
   more statements;
 }
 And the cases of a switch aren't indented, so it would be:
 switch ( aCommand ) {
 case "cmd_goUndoCloseTab":
   return true;
 default:
   if (supportsCommandFunc)
     return supportsCommandFunc.call(tab.mode.tabType, aCommand, tab);
   return false;
 }
 but I didn't expect you to know that.)

>@@ -1042,10 +1061,15 @@
>+             switch ( aCommand )

Ditto about this switch.

>@@ -1062,8 +1086,14 @@
>+             switch ( aCommand )
>+             {
>+               case "cmd_goUndoCloseTab":
>+                 UndoRecentlyClosedTab();
>+               default:
>              if (doCommandFunc)
>                doCommandFunc.call(tab.mode.tabType, aCommand, tab);
>+             }

And ditto this switch.  Also, the "if (doCommandFunc)" should have two more
spaces of indentation.

>+++ b/mail/test/mozmill/folder-display/test-tabs-simple.js
>@@ -194,3 +194,18 @@
>+/**
>+ * Open a tab, close it, then make sure the "Reopen last tab" button works.
>+ */
>+function test_undo_close_tab() {

I'm a big fan of tests that show what happens when you call stuff that will fail, so if you could add another test (or more to this test) that showed what happens when you try to re-open the tab again, that would be cool.

Aside from those, r=me, but since I'm not a Tab UI reviewer, you'll need to fix the things I mentioned, and post another patch, asking for another review from one of the people listed in the "Tab UI" section of https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements

Later,
Blake.

        Attachment #424432 -
        Flags: review?(bwinton) → review+

    
    

    
  
(In reply to comment #5)
> Created an attachment (id=424432) [details]
> Patch to add "Reopen last tab" to Thunderbird's Go menu

This rocks Zach!

    
    

    
  
Instead of saving off the tab info structure, I would use that tab persistence support to persist the tab to a JS object.  Then when the user wants to re-open the tab I would use the restore logic.  Since there is an explicit contract for tab persistence/restoration this should be reliable.  (The current patch's technique of saving off the tab info structure does not have any contractual guarantees backing it up.)

Please also add a comment on undoCloseTab up in the giant weird comment block near the beginning of the tabmail declaration.

Otherwise, this is looking pretty good.

Please request review from me (you can enter ":asuth") when you've made bwinton's requested mods and my requested mods here.

    
    

    
  
clarkbw should probably also do a ui-review as well.

    
    

    
  


  

        Attachment #424432 -
        Attachment is obsolete: true

        Attachment #439137 -
        Flags: ui-review?

        Attachment #439137 -
        Flags: review?(bugmail)

    
  

        Attachment #439137 -
        Flags: ui-review? → ui-review?(clarkbw)
Flags: wanted-thunderbird3? → wanted-thunderbird+

    
    

    
  
Comment on attachment 439137 [details] [diff] [review]
Second attempt at patch for undo close tab

I've come a bit late and the patch no longer applies. :-(

    
    

    
  
(As per Zach's request, removing him as the assignee.)
Assignee: zgchurch → nobody
Status: ASSIGNED → NEW

    
    

    
  
Comment on attachment 439137 [details] [diff] [review]
Second attempt at patch for undo close tab

removing my review until the patch applies again

        Attachment #439137 -
        Flags: ui-review?(clarkbw)

        Attachment #439137 -
        Attachment is obsolete: true

        Attachment #439137 -
        Flags: review?(bugmail)

    
    

    
  
Gah! This looks like it was almost done, but then not quite finished. I hope it gets done soon, because this looks like a phenomenally useful feature.

    
    

    
  
(In reply to comment #14)
> Gah! This looks like it was almost done, but then not quite finished. I hope it
> gets done soon, because this looks like a phenomenally useful feature.

Yup.  That's pretty much it.  Zach ran out of time before his term ended.  Please feel free to take the patch, and finish it off.

Thanks,
Blake.

    
    

    
  
I would if I could. Sadly, python is my game, not...ummm...whatever it is that TB is written in.

    
    

    
  
(In reply to comment #16)
> I would if I could. Sadly, python is my game, not...ummm...whatever it is that
> TB is written in.

In this case, mostly javascript.  (And I bet you could figure out enough to get this patch working.  It's pretty close, and the two languages aren't _that_ different.  ;)

Later,
Blake.

    
    

    
  
Bug 468808 implements restoring closed tabs as a side effect. So it fixes parts of this bug. But it's just available through tabmail's context menu, no entries in the "Go" Menu and no global shortcut is implemented. 

Can someone assign this bug to me? I'll try to create a patch within the next days, to resolve this.

    
    

    
  
Thanks, Thomas. Assigning to you.
Assignee: nobody → schmid-thomas

    
    

    
  
agree, partly [fixed by bug 468808]

thanks!
Depends on: 468808
Whiteboard: ucosp → [partly fixed by bug 468808]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1 (obsolete)
        — 
          — Splinter Review
      

    


  
Binds the "Control - T" shortcut to undo close and implements recently closed tabs into the go menu as sketched in bug 500758.  

The logic relies on the undoClose code of Bug 468808. Is a mozmill test needed? 

"Recently closed tabs" are currently managed by tabmail. It would be nicer to move this to session restore. So that there is one Object tracking windows and tabs. But I am unsure if it's relay a good idea. Any suggestions?

    
  

        Attachment #517287 -
        Flags: feedback?(bugmail)

    
    

    
  
Comment on attachment 517287 [details] [diff] [review]
Patch v1

This looks very good!  It's worth pointing out that you actually are using a keybinding of "control-shift-T" (the same as Firefox, approximately), rather than "control-T" which is what you said in your previous message.  You should, however, be using a modifiers string of "accel, shift" instead of "control, shift", because OS X is wacky.  (See https://developer.mozilla.org/en/XUL_Tutorial/Keyboard_Shortcuts or maybe something even more informative.)

This does require ux-review from clarkbw, of course.  I might propose some slight feature creep, namely:

a) It would be nice to expose the list of recently closed tabs in our right-click on the tab bar menus too, rather than just under "Go","Recently Closed Tabs".  I personally would be unlikely to find the feature without this.

b) Make us pop-up a menu that can do the various tab unclosing options if we click in the empty space on the tab-bar.  Perhaps we could show the same tab menu as we show when we right-click on a tab, but just grey out most of the other options?

I would avoid assuming clarkbw is on board with either of those proposals until he chimes in.

Yes, a mozmill test is needed.  Specifically, I would like test cases that cover:
- Make sure that the menu properly populates and the string describing the tab is correct.

- Make sure that clicking the option re-opens the tab, and that if you re-open the menu afterwards, that tab is no longer in the menu.

- Check that we are properly capping the length of the undo list.  I think a sufficient means of checking would be to open-then-close 11 windows and make sure the list is only 10 and the expected number of menu items shows up when you open the menu.

I would suggest manually clobbering the list of closed tabs at the beginning of the test so you aren't impacted by other test files run in the same session.  (If you have to, we can put the test in its own directory, but there's non-trivial overhead for each directory, so it's preferable if we can avoid that or avoid assuming the ordering of test files in a directory.)

Assuming we end up exposing that functionality via the context menu, I would strongly suggest writing the test against the context menu rather than the go menu.  The menu bar on OS X uses native widgets that suck the representation out of the XUL menu items, if you will, and we can avoid a lot of testing headaches on OS X.  (To avoid spreading FUD, that's only a serious problem when using dynamically created XBL stuff, but all the same, it's preferable to avoid.)

In terms of the session store, yes, I think that's a good idea.  I expect the major user benefit would be for undoing window close for people who use multiple windows.  Right now, if you only use one (3pane) window, session persistence will save you if you close that, but you are losing state for any other type of closure.  Having said that, I'm not sure that's a tremendously important use-case; if you are looking for places where you can help out rather than that being a specific itch of yours, please give a shout out and I'm sure we can help you find such places! :)

Other specific feedback:

on file: mail/base/content/mail3PaneWindowCommands.js line 439
>         return !!(document.getElementById("tabmail").recentlyClosedTabs.length > 0);               

you don't need the double-bang; the value in parens is already a boolean.


on file: mail/base/content/mailWindowOverlay.js line 706
> function InitRecentlyClosedTabs(menuPopup)

Please add "Popup" to the end of the function name to disambiguate versus
logic initializing a subystem (although I grant the call signature does hint
at things.)


on file: mail/base/content/mailWindowOverlay.js line 722
>   // Rebuild the recently closed tab list

Please add a comment to the effect that the most recently closed tab is at
index 0 in order to add clarity to the reverse traversal with displacing
insertion.


on file: mail/base/content/mailWindowOverlay.js line 743
>   while (tabmail.recentlyClosedTabs.length)
>     tabmail.undoCloseTab();

Please save off the length and "while" or "for" on that instead.  In the event
undoCloseTab has a failure where it returns without removing an entry, it
would be nice to avoid going into an infinite loop.

        Attachment #517287 -
        Flags: ui-review?(clarkbw)

        Attachment #517287 -
        Flags: feedback?(bugmail)

        Attachment #517287 -
        Flags: feedback+

    
    

    
  
(In reply to comment #22)
> This does require ux-review from clarkbw, of course.  
> I might propose some slight feature creep, namely:
> 
> a) It would be nice to expose the list of recently closed tabs in our
> right-click on the tab bar menus too, rather than just under "Go","Recently
> Closed Tabs".  I personally would be unlikely to find the feature without this.
> 
> b) Make us pop-up a menu that can do the various tab unclosing options if we
> click in the empty space on the tab-bar.  Perhaps we could show the same tab
> menu as we show when we right-click on a tab, but just grey out most of the
> other options?
> 
> I would avoid assuming clarkbw is on board with either of those proposals until
> he chimes in.

I'll request an ui-review. The current implementation follows Bug 500758 which was filed by clarkbw ;-)

concerning a) and b), both great ideas. I would suggest to go for a). Having a menu with almost everything grayed out looks strange.  

I'm unsure how to implement this. Is there any way, other than duplicating the popup code? It seems to me as an overlay will not work, as there the id within the menubar and the tabmail would be same and thus ambiguous. 

> In terms of the session store, yes, I think that's a good idea.  I expect the
> major user benefit would be for undoing window close for people who use
> multiple windows.  Right now, if you only use one (3pane) window, session
> persistence will save you if you close that, but you are losing state for any
> other type of closure.  

well, my main intention was just to simplify tabmail, cluster similar code and reduce the code base. Both session restore and recently closed tabs have a very similar behavior. One restores windows the other one tabs. So they could be merged. As a side effect, it would be easily possible to implement a feature you described. So you are one step ahead ;-) But the benefit is indeed not very high.

> Having said that, I'm not sure that's a tremendously important use-case; if you 
> are looking for places where you can help out rather than that being a specific 
> itch of yours, please give a shout out and I'm sure we can help you find such places! :)

Currently my focus is getting annoying paper cut bugs (527434, 467927, 487386 ...) fixed ;-)

    
    

    
  
(In reply to comment #23)
> I'm unsure how to implement this. Is there any way, other than duplicating the
> popup code? It seems to me as an overlay will not work, as there the id within
> the menubar and the tabmail would be same and thus ambiguous. 

You can reference popups by their id; no need to duplicate them.  There should be examples in the code already, but see:

https://developer.mozilla.org/en/XUL/Attribute/popup
https://developer.mozilla.org/en/XUL/Attribute/context (or)
https://developer.mozilla.org/en/XUL/Attribute/contextmenu

https://developer.mozilla.org/en/XUL_Tutorial/Popup_Menus

    
    

    
  
(In reply to comment #23)
> I'll request an ui-review. The current implementation follows Bug 500758 which
> was filed by clarkbw ;-)

I am a lazy, lazy man ;), so it's much appreciated if you can call that out in your comment when filing the patch with an explanation of why you already believe it's already approved (assuming you do).  In this case, since it has been over a year and a half since he created the bug and only a little less since his last comment on the bug, I think it's appropriate to re-request as situations can change.  (In some cases, we may also scale the complexity of the proposed implementation to be less than the ideal because the contributor is a fresh student developer or other reasons.  You are showing great proficiency, so you win potential feature creep! :)

    
    

    
  
Comment on attachment 517287 [details] [diff] [review]
Patch v1

This looks good to me.  Thank you!!

        Attachment #517287 -
        Flags: ui-review?(clarkbw) → ui-review+

    
    

    
  


  
(In reply to comment #22)
> This does require ux-review from clarkbw, of course.  I might propose some
> slight feature creep, namely:

Sorry, missed this!  Made a quick mockup to make up for not replying to this right away.

> a) It would be nice to expose the list of recently closed tabs in our
> right-click on the tab bar menus too, rather than just under "Go","Recently
> Closed Tabs".  I personally would be unlikely to find the feature without this.

Yes, as the last item with a separator above it.

> b) Make us pop-up a menu that can do the various tab unclosing options if we
> click in the empty space on the tab-bar.  Perhaps we could show the same tab
> menu as we show when we right-click on a tab, but just grey out most of the
> other options?

If you just disable the other items (except undo close tab) and offer the same "Recently Closed Tabs" list at the bottom I think this will work fairly well.

    
    

    
  

      
      
      
      

        Attached image
          
        screenshots
        — 
      

    


  
What about converting the "Undo close tab" menu item into a menu popup? I've attached a screenshot illustrating this.

It keeps on the one hand the ui minimal and on the other hand the most important menu item "Close Tab" stays the last item.

        Attachment #517770 -
        Flags: feedback?(clarkbw)

    
    

    
  
Comment on attachment 517770 [details]
screenshots

"Undo Close Tabs" sounds weird but otherwise I think what you have could work.  Lets work on the wording.

        Attachment #517770 -
        Flags: feedback?(clarkbw) → feedback-

    
    

    
  
"Undo Close Tabs" is the firefox wording. It does sound odd, but perhaps only if you think on it too hard?

alternatively, Reopen?

    
    

    
  
It's "Recently Closed Tabs" in Firefox 4. That sounds good to me.

    
    

    
  
Oh, hm, it's "Undo Close Tab" in the tab context menu, but that only undoes the last tab closed. The History menu has Recently Closed Tabs.

    
    

    
  
I think "Recently Closed Tabs" is reasonable given it is a menu. "Undo Close Tab" or "Undo Closed Tabs" implies it will act straight away.

    
    

    
  
"Recently Closed Tabs" works for me, lets go with that.

    
    

    
  
(In reply to comment #34)
> "Recently Closed Tabs" works for me, lets go with that.

so lets summarize the discussion. The plan is to rename the 'undo close tab' menu item to 'recently closed tabs' and add a context menu as described above.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
updated according to the discussion above and added a mozmill test.

        Attachment #517287 -
        Attachment is obsolete: true

        Attachment #519062 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 519062 [details] [diff] [review]
Patch v2

Rich review (provides context/pretty colors):
http://reviews.visophyte.org/r/519062/

Exported review:

The patch is looking good!  As is the way with these things, the tests are the
most complex thing and because you are breaking some newish ground, I have
some suggestions to avoid intermittent orange failures and requests to
normalize things.

An important meta-note; your patch had DOS line endings and this made "hg
qimport" unhappy.  I don't think I had that problem with your fixes on the
last bug, so you may want to provide diffs however you accomplished it last
time instead of however you accomplished it this time.  There may also be some
mercurial settings you can change... I don't develop on windows most of the
time, so I couldn't tell you, but I think bienvenu and sid0 do, so they might
have some suggestions.

If you are amenable, I'd like to pursue getting you level 1 commit access so
you can push to our try server so that you can get mozmill runs across all our
platforms without assistance.  You would cc me on the bug so I can vouch for
you.  The docs on that are here:
http://www.mozilla.org/hacking/committer/

And the description of what the levels are is here:
http://www.mozilla.org/hacking/commit-access-policy/

I will push this to our try server now so we can see how it fares.


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 381
>   // We should start with a clean tab history. So restore all tabs
>   let len = mc.tabmail.recentlyClosedTabs.length;  
>   while(len--)
>     mc.tabmail.undoCloseTab();  
>   
>   // Then close all open tabs, without adding them to the tab history
>   while (mc.tabmail.tabInfo.length > 1)
>     mc.tabmail.closeTab(1,true);
>   
>   // if everything went well only one tab should be open...
>   assert_number_of_tabs_open(1);
>   
>   // ... and the tab history should be gone
>   assert_true(mc.tabmail.recentlyClosedTabs.length == 0,
>         "Failed to clean recently closed tabs history");  

It's okay to just directly clear out the list.  The test is already depending
on the name of the recentlyClosedTabs variable, so I'm not sure what is gained
with the tab churn.


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 430
>     mc.rightClick(new elib.Elem(mc.tabmail.tabContainer.childNodes[1]));

You need to wait for this popup to open before trying to click on it.  Please
use wait_for_popup_to_open(popupElem) where popupElem is an actual DOM Element
rather than an elib.Elem.


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 432
>     // If you call mc.click() on context menus, really strange things happen. 
>     // So we better synthesize a mousedown and mouseup event, as a workaround.                    
>     EventUtils.synthesizeMouse(menu,5, 5, {}, mc.window);
>     mc.waitForEval("subject.open", 1000, 50, menu);

You may have been getting burnt by the magic logic that positions clicks in
the center of the target node when left undefined:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#867


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 435
>     mc.waitForEval("subject.open", 1000, 50, menu);

like the above and in the other instances below, please use
wait_for_popup_to_open(menu) instead of this waitForEval.

In general, we want as few waitForEval instances directly in the code, since
it suggests it is something there should be a helper for.  The benefit from
the helpers is that it allows us to provide logging events about what we are
doing, plus it allows us to provide specialized failure handling since we know
what we were trying to do when something failed to happen.


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 442
>     // Restore the most recently closed tab
>     EventUtils.synthesizeMouse(menu.getItemAtIndex(0),5, 5, {},mc.window);

You are going to want to wait for the popups to close and/or might need to
help them close themselves.  I've had bad luck in the past with popups
actually closing in mozmill tests; this may have been owing to a lack of
giving them time to close themselves.  In any event, a lot of our tests send
escape keys at the popup to force them to close.

We have a helper method to close a popup that takes care of this; it's not
somewhat smarter so if the popup is in the process of closing already, we
don't send the escape key at it to help it along.  This may not be
award-worthy, but I think it will end up working in all cases, dumb or not.

You use it like so: close_popup(mc, the elib.Elem for the menu).  It lives in
test-folder-display-helpers:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1272

We do also have a less dumb popup menu helper/abstraction (in the sense that
the VK_ESCAPE dubiousness is hidden under the hood), "click_menus_in_sequence"
that we expose on mc/controllers, but it is designed to just make the menus
go, not let you check what the menus say:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#788


on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 486
>   finally  {
>     // finally close the tabs opened.
>     mc.tabmail.closeOtherTabs(0);
>     assert_number_of_tabs_open(1);
>   }    

try...finally blocks break our magic "capture-state-on-failure" logic (which
includes screenshots) because they destroy potentially interesting state about
the failure.

Instead, please define a "teardownTest" function in the file.  This will be
invoked after every test and provided the test function as an argument, so if
you only want to perform specific cleanup for certain tests, you can check the
test's "name" attribute.

You may want to look at, or just reuse, the teardownModule logic contributed
by test-folder-display-helpers:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#311

I would personally lean to just moving the logic used by teardownModule into a
function like "reset_3pane_state" and having it call that, and then your
teardownTest function could just call that.  (I would advise against just
trying to expose the actual reset_3pane_state/whatever function under a
"teardownTest" attribute because mozmill will clobber some attributes onto
what it finds, so it's preferable to expose a function that was defined
directly in that file and not potentially shared with other modules.)

(I realized I didn't complain about this for the other tests in the file in
the recent past; I hadn't really been thinking about the problems "finally"
causes until after I had reviewed your tests and was deep in mozmill/logging
fun.)

        Attachment #519062 -
        Flags: review?(bugmail) → review-

    
    

    
  
The try builds got really angry; it turns out you made a typo in the hotkey definition and are stealing all "accel, shift" key combos, so your functionality works, but a bunch of other stuff does not ;)

You have:
<key id="key_undoCloseTab" keycode="&undoCloseTabCmd.commandkey;"  oncommand="goDoCommand('cmd_undoCloseTab')" modifiers="accel, shift"/> 

keycode should only be used if the value is a VK_blah type thing.  You want it to be "key", I believe.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3
        — 
          — Splinter Review
      

    


  
(In reply to comment #37)
> Comment on attachment 519062 [details] [diff] [review]
> An important meta-note; your patch had DOS line endings and this made "hg
> qimport" unhappy.  I don't think I had that problem with your fixes on the
> last bug, so you may want to provide diffs however you accomplished it last
> time instead of however you accomplished it this time.  There may also be some
> mercurial settings you can change... I don't develop on windows most of the
> time, so I couldn't tell you, but I think bienvenu and sid0 do, so they might
> have some suggestions.

sounds as if I called by the hg executable form the a shell with line endings set to dos...

> If you are amenable, I'd like to pursue getting you level 1 commit access so
> you can push to our try server so that you can get mozmill runs across all our
> platforms without assistance.  You would cc me on the bug so I can vouch for
> you. 

would be great.

> on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 430
> >     mc.rightClick(new elib.Elem(mc.tabmail.tabContainer.childNodes[1]));
> 
> You need to wait for this popup to open before trying to click on it.  Please
> use wait_for_popup_to_open(popupElem) where popupElem is an actual DOM Element
> rather than an elib.Elem.

well, the patch was created before wait_for_popup_to_open was available on the trunk ;-) So it took me some time to figure out that was wrong with my mozmill installation, until i realized it was just outdated.

> on file: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js line 432
> >     // If you call mc.click() on context menus, really strange things happen. 
> >     // So we better synthesize a mousedown and mouseup event, as a workaround.                    
> >     EventUtils.synthesizeMouse(menu,5, 5, {}, mc.window);
> >     mc.waitForEval("subject.open", 1000, 50, menu);
> 
> You may have been getting burnt by the magic logic that positions clicks in
> the center of the target node when left undefined:
> http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#867

for some reason mc.click() is still not working when menupopups are nested within context menus. The click command is triggered but the menu stays open, as if the event is stopped somewhere. Using synthesizeMouse() seems to work, it closes the context menu after clicking on a menu item.

        Attachment #519062 -
        Attachment is obsolete: true

        Attachment #520522 -
        Flags: review?(bugmail)

    
    

    
  
(In reply to comment #39)
> > If you are amenable, I'd like to pursue getting you level 1 commit access so
> > you can push to our try server so that you can get mozmill runs across all our
> > platforms without assistance.  You would cc me on the bug so I can vouch for
> > you. 
> 
> would be great.

Excellent!  So there's no ambiguity, the next steps are yours, per the docs.  (You need to read and sign the contribution agreement, which may require checking with your employer depending on any IP agreements you might have signed, create/post an SSH key, and file a bug.)
 

> for some reason mc.click() is still not working when menupopups are nested
> within context menus. The click command is triggered but the menu stays open,
> as if the event is stopped somewhere. Using synthesizeMouse() seems to work, it
> closes the context menu after clicking on a menu item.

Very interesting!  I'll look into this a little more (after I finish your review) to see if I can figure out what the different is; I have never been happy about our artificial need to force the pop-ups closed so it would be good to eliminate that.
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 520522 [details] [diff] [review]
Patch v3

Looks good!  I changed the bit where you were doing a while loop to shift things out of the tabs array; there is nothing special/magic about attributes on XBL things, although one can potentially use the XML syntax to define/initialize them, so you can just set the value to a new array unless there are other references to the array elsewhere that would not see the update.  (And if that is, you can use splice to delete multiple things in one go.)

I pushed it to try and the try server was happy, so I landed it on trunk:
http://hg.mozilla.org/comm-central/rev/711e23c77960

        Attachment #520522 -
        Flags: review?(bugmail) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4

    
    

    
  
(In reply to comment #40)
> > for some reason mc.click() is still not working when menupopups are nested
> > within context menus. The click command is triggered but the menu stays open,
> > as if the event is stopped somewhere. Using synthesizeMouse() seems to work, it
> > closes the context menu after clicking on a menu item.
> 
> Very interesting!  I'll look into this a little more (after I finish your
> review) to see if I can figure out what the different is; I have never been
> happy about our artificial need to force the pop-ups closed so it would be good
> to eliminate that.

I've created/extracted a small test case illustrating this behavior. Instead of wait_for_... i just used sleep commands. Furthermore the code expects that the recently closed tabs list contains at least one entry.


<code>
  mc.rightClick(new elib.Elem(mc.tabmail.tabContainer.childNodes[0]));
  mc.sleep(2000);
      
  let menu = mc.window.document.getAnonymousElementByAttribute(
                   mc.tabmail,"anonid","recentlyClosedTabs");      

  EventUtils.synthesizeMouse(menu,5, 5, {},mc.window);
  mc.sleep(2000);
  
  // in case you use synthesizeMouse() the context menu gets closed after 
  // clicking on it.
  EventUtils.synthesizeMouse(menu.getItemAtIndex(0),5, 5, {},mc.window);
  
  // if you use click() instead, the context menu does not get closed.
  // mc.click(new elib.Elem(menu.getItemAtIndex(0)));
  
  mc.sleep(10000);
</code>

    
    

    
  
we have an intermittent orange failure on windows cropping up:
http://arbpl.visophyte.org/?tree=Thunderbird&pushid=5678&log=19230%3A1300804955.1300805574.25888.gz

From the fancy pants logging, it appears that the unit test is triggering 3 concurrent loads, so it's not strictly surprising that we end up with the wrong message displayed and we explode.

I'll look at the test in a minute, but either the functionality is not opening some tabs in the background when it should, or we are not doing enough planning/waiting in the unit test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Any news concerning this issue? I tried to look at the failure log you linked, but the link seems to be dead.
Depends on: 644118

    
    

    
  
Yes, sorry about the dead link; I did not backfill the database during a schema change database nuking.  I have not seen the orange recur so I'm going to mark this fixed as I have made some other changes that should land soon that might clear up the specific problem the test ran afoul of (bug 640877).  We'll open a new bug if we see that test failure again after we resolve bug 640877 or it becomes common.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED

    
    

    
  
(In reply to comment #41)
> Comment on attachment 520522 [details] [diff] [review] [review]
> Patch v3
> 
> Looks good!  I changed the bit where you were doing a while loop to shift
> things out of the tabs array; there is nothing special/magic about
> attributes on XBL things, although one can potentially use the XML syntax to
> define/initialize them, so you can just set the value to a new array unless
> there are other references to the array elsewhere that would not see the
> update.  (And if that is, you can use splice to delete multiple things in
> one go.)
> 
> I pushed it to try and the try server was happy, so I landed it on trunk:
> http://hg.mozilla.org/comm-central/rev/711e23c77960

Andrew, you forgot to remove an obsolete comment from mail/locales/en-US/chrome/messenger/messenger.dtd.
See http://hg.mozilla.org/comm-central/diff/711e23c77960/mail/locales/en-US/chrome/messenger/messenger.dtd#l1.9 .

    
    

    
  
On TB9, Ctrl+Shift+T does not do anything if triggered from "content" tabs like "Global Search Results" tab or "Add-Ons-Manager" tab, or "Welcome to Thunderbird" tab.

Do we have a bug for that?
If not, reopen this bug or create a followup?

    
    

    
  
Let's open a new bug.  This one was closed a while ago, and I'm sure asuth isn't that interested in it anymore.  ;)

Thanks,
Blake.

    
  
Summary: provide undo close tab → provide undo close tab (Ctrl+Shift+T) [Go > Recently Closed Tabs]
Whiteboard: [partly fixed by bug 468808] → [partly fixed by bug 468808] [landed in comment 41]

    
  
Whiteboard: [partly fixed by bug 468808] [landed in comment 41] → [partly fixed by bug 468808][landed in comment 41][needs followup bug, see comment 47]

    
    

    
  
Has a new bug been created for this issue? (mentioned in the last comment)
What is the bug number?

For me Ctrl-Shift-Tab does not work and the list of recently closed tabs is empty (with a second level pop-out where the empty entry's line height is too narrow).

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: