595521 - When closing the last group, the undo button should stay indefinitely

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Aza Raskin [:aza]]

Current behavior: The undo button fades away after 15 seconds, and then the Firefox window closes somewhat unexpectedly.

Desired behavior: Only for the last group, the undo button should stay indefinitely (meaning until the browser window or close button is manually closed).

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Pushed to try and waiting for the results

Comment 2

[Raymond Lee [:raymondlee]]

Passed try

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 474398 [details] [diff] [review]
v1

I'd like to suggest a couple of design refinements:

* This behavior should not be based on whether the group in question is the last group; It should be based on whether the group in question contains all of the remaining tabs in the window. For instance, if there are orphan tabs, it's okay to close all groups. Likewise, if there are other groups, but they are all empty and there are no orphans, you'd invoke this behavior on the last group with tabs. 

* If, after keeping the "undo" state indefinitely, new tabs are added, either as orphans or to other groups, the 15 second count down should resume on the group that's being held in stasis. 

Aza, would you agree?

Comment 4

[Ian Gilman [:iangilman]]

Also, this test has a 15 second time out in it; that's unacceptable. Right now the browser chrome mochitests take a little over a minute (on my computer); adding an additional 15 seconds for a single test is way too much. 

On the other hand, how would one test this feature without a 15 second delay?

Ehsan, any thoughts on this?

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #3)
> Comment on attachment 474398 [details] [diff] [review]
> v1
> 
> I'd like to suggest a couple of design refinements:
> 
> * This behavior should not be based on whether the group in question is the
> last group; It should be based on whether the group in question contains all of
> the remaining tabs in the window. For instance, if there are orphan tabs, it's
> okay to close all groups. Likewise, if there are other groups, but they are all
> empty and there are no orphans, you'd invoke this behavior on the last group
> with tabs. 

Done.

> 
> * If, after keeping the "undo" state indefinitely, new tabs are added, either
> as orphans or to other groups, the 15 second count down should resume on the
> group that's being held in stasis. 
>

When a new tab is added, the zoom in would happen so it is not necessary to resume the count down on the group as it would be removed when exiting the Panorama UI.


Note: we still need to address the 15 timeout check in the test.

Comment 6

[(no longer active)]

Waiting 15 seconds in a test is a horrible idea!

I think we should add a mechanism inside the code to override the timeout completely, and in the test use that mechanism to override the timeout, test the behavior without using a timeout, and then restore the timeout.

Alternatively, if the timeout is crucial here, we should get a Litmus test for this.  And we can always have both a Litmus test and an automated test.

Caveat: when I say Litmus, I might mean Mozmill... ;-)

Comment 7

[Ian Gilman [:iangilman]]

I think the easiest thing to do would be to make a property for the timeout length, and set that down to 1 ms in the test.

Comment 8

[Ian Gilman [:iangilman]]

Raymond, can you finish this one up? Let me know if you still have questions about it.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 482186 [details] [diff] [review]
v3

Looks good. 

Looks like this patch and bug 599626 have a lot of overlap; you'll have to clean up some conflicts. I like what you've done with this patch, though.

A few small items: 

>+      this._undoButtonTimeoutId = setTimeout(function() { 
>+        self._fadeAwayUndoButton(); }, this._fadeAwayUndoButtonDelay);

Please break that second line like so:

      this._undoButtonTimeoutId = setTimeout(function() { 
        self._fadeAwayUndoButton(); 
      }, this._fadeAwayUndoButtonDelay);

>+  // Removes the group item, it's children and it's container.

Should be "its" in both cases. 

>+    let endGame = function() {
>+      window.removeEventListener("tabviewhidden", endGame, false);
>+      ok(!TabView.isVisible(), "Tab View is hidden");
>+      groupItem.setFadeAwayUndoButtonDelay(15000);
>+      groupItem.setFadeAwayUndoButtonDuration(300);

Rather than hard-coding those numbers, save the original values before setting new values, and then set them back to the saved values at the end. 

f+ with those items.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to comment #11)
> Comment on attachment 482186 [details] [diff] [review]
> v3
> 
> Looks good. 
> 
> Looks like this patch and bug 599626 have a lot of overlap; you'll have to
> clean up some conflicts. I like what you've done with this patch, though.
> 
> A few small items: 
> 
> >+      this._undoButtonTimeoutId = setTimeout(function() { 
> >+        self._fadeAwayUndoButton(); }, this._fadeAwayUndoButtonDelay);
> 
> Please break that second line like so:
> 
>       this._undoButtonTimeoutId = setTimeout(function() { 
>         self._fadeAwayUndoButton(); 
>       }, this._fadeAwayUndoButtonDelay);
> 

Done

> >+  // Removes the group item, it's children and it's container.
> 

Done

> Should be "its" in both cases. 
> 
> >+    let endGame = function() {
> >+      window.removeEventListener("tabviewhidden", endGame, false);
> >+      ok(!TabView.isVisible(), "Tab View is hidden");
> >+      groupItem.setFadeAwayUndoButtonDelay(15000);
> >+      groupItem.setFadeAwayUndoButtonDuration(300);
> 
> Rather than hard-coding those numbers, save the original values before setting
> new values, and then set them back to the saved values at the end. 
> 

Done

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 482500 [details] [diff] [review]
v3

Switching review to dolske, as dietrich says he's not reviewing non-blockers for the time being.

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 482500 [details] [diff] [review]
v3

>+  // Function: setFadeAwayUndoButtonDelay
>+  // Sets the delay time for undo button to fade away.
>+  setFadeAwayUndoButtonDelay: function GroupItem_setFadeAwayUndoButtonDelay(milliseconds) {
>+    this._fadeAwayUndoButtonDelay = milliseconds;
>+  },
>+
>+  // ----------
>+  // Function: getFadeAwayUndoButtonDelay
>+  // Gets the delay time for undo button to fade away.
>+  getFadeAwayUndoButtonDelay: function GroupItem_getFadeAwayUndoButtonDelay() {
>+    return this._fadeAwayUndoButtonDelay;
>+  },
>+
>+  // ----------
>+  // Function: setFadeAwayUndoButtonDuration
>+  // Sets the duration for the undo button to fade away.
>+  setFadeAwayUndoButtonDuration: function GroupItem_setFadeAwayUndoButtonDuration(milliseconds) {
>+    this._fadeAwayUndoButtonDuration = milliseconds;
>+  },
>+
>+  // ----------
>+  // Function: getFadeAwayUndoButtonDuration
>+  // Gets the duration for the undo button to fade away.
>+  getFadeAwayUndoButtonDuration: function GroupItem_getFadeAwayUndoButtonDuration() {
>+    return this._fadeAwayUndoButtonDuration;
>+  },

Drop the get/set functions, and just use the property directly (maybe removing "_" prefix to indicate it's not just used internally, but since it's basically just for the test I don't care about that too much.)

r+ with that.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

(In reply to comment #14)
> Comment on attachment 482500 [details] [diff] [review]
> v3
> 
> Drop the get/set functions, and just use the property directly (maybe removing
> "_" prefix to indicate it's not just used internally, but since it's basically
> just for the test I don't care about that too much.)
> 
> r+ with that.

Done

Comment 16

[Raymond Lee [:raymondlee]]

Request blocking2.0 because Bug 597188 has blocking2.0=betaN+ which depends on this bug.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Updated the patch for latest code in trunk

Comment 18

[Ian Gilman [:iangilman]]

Comment on attachment 486847 [details] [diff] [review]
Patch for check-in

This bug is now blocking, so removing the approval request. Raymond, please package for check in.

Comment 19

[Ian Gilman [:iangilman]]

(In reply to comment #18)
> Comment on attachment 486847 [details] [diff] [review]
> Patch for check-in
> 
> This bug is now blocking, so removing the approval request. Raymond, please
> package for check in.

Nevermind... I see you've already done that. Landing on panorama-central.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 21

[Ian Gilman [:iangilman]]

Landed on panorama-central: 

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/f6ddea7a3ed7

(sorry for the mixed up communication, Raymond)

Comment 22

[Raymond Lee [:raymondlee]]

(In reply to comment #21)
> Landed on panorama-central: 
> 
> http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/f6ddea7a3ed7
> 
> (sorry for the mixed up communication, Raymond)

That's ok ;)

Comment 23

[Ian Gilman [:iangilman]]

Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/03a3ee4ece35

Comment 24

[Tracy Walker [:tracy]]

Verified on minefield nightly build of 20101112