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

VERIFIED FIXED

Status

defect
P3
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: aza, Assigned: raymondlee)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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).
Posted patch v1 (obsolete) — Splinter Review
Pushed to try and waiting for the results
Attachment #474398 - Flags: feedback?(ian)
Assignee: nobody → raymond
Attachment #474398 - Attachment is patch: true
Attachment #474398 - Attachment mime type: application/octet-stream → text/plain
Passed try
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?
Attachment #474398 - Flags: feedback?(ian) → feedback-
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?
Posted patch v2 (obsolete) — Splinter Review
(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.
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... ;-)
Attachment #475536 - Attachment is patch: true
Attachment #475536 - Attachment mime type: application/octet-stream → text/plain
Attachment #474398 - Attachment is obsolete: true
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.
Priority: -- → P3
Blocks: 576427
Blocks: 598154
Raymond, can you finish this one up? Let me know if you still have questions about it.
Blocks: 597043
No longer blocks: 598154
Posted patch v3 (obsolete) — Splinter Review
Attachment #475536 - Attachment is obsolete: true
Attachment #482184 - Flags: feedback?(ian)
Posted patch v3 (obsolete) — Splinter Review
Attachment #482184 - Attachment is obsolete: true
Attachment #482186 - Flags: feedback?(ian)
Attachment #482184 - Flags: feedback?(ian)
Attachment #482186 - Attachment is patch: true
Attachment #482186 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #482186 - Flags: feedback?(ian) → feedback+
Posted patch v3 (obsolete) — Splinter Review
(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
Attachment #482186 - Attachment is obsolete: true
Attachment #482500 - Flags: review?(dietrich)
Status: NEW → ASSIGNED
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.
Attachment #482500 - Flags: review?(dietrich) → review?(dolske)
Blocks: 599626
Blocks: 597188
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.
Attachment #482500 - Flags: review?(dolske) → review+
Attachment #482500 - Flags: approval2.0?
Attachment #482500 - Flags: approval2.0?
Posted patch Patch for check-in (obsolete) — Splinter Review
(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
Attachment #482500 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #485664 - Flags: approval2.0?
Keywords: checkin-needed
Blocks: 607566
Blocks: 596781
Request blocking2.0 because Bug 597188 has blocking2.0=betaN+ which depends on this bug.
blocking2.0: --- → ?
Blocks: 608158
Posted patch Patch for check-in (obsolete) — Splinter Review
Updated the patch for latest code in trunk
Attachment #485664 - Attachment is obsolete: true
Attachment #486847 - Flags: approval2.0?
Attachment #485664 - Flags: approval2.0?
blocking2.0: ? → betaN+
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.
Attachment #486847 - Flags: approval2.0?
(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.
Attachment #486847 - Attachment is obsolete: true
Landed on panorama-central: 

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

(sorry for the mixed up communication, Raymond)
Whiteboard: [on-panorama-central]
(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 ;)
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/03a3ee4ece35
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [on-panorama-central]
Verified on minefield nightly build of 20101112
Status: RESOLVED → VERIFIED
Depends on: 614556
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.