Closed Bug 587341 Opened 9 years ago Closed 9 years ago

Implement Undo Close Group inside of Tab Candy

Categories

(Firefox Graveyard :: Panorama, enhancement, P1)

enhancement

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: Ryuji, Assigned: raymondlee)

References

Details

(Whiteboard: b5, [in-litmus-bug-week])

Attachments

(1 file, 12 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100814 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100814 Minefield/4.0b4pre

If users accidentally closes a Tab Group in Tab View of Tab Candy, they cannot undo that action. All they can do now is to undo close all the tabs(in the closed group) back and regroup them back into a group, in order to get that group back.  

This is not user-friendly and caused user a bad experience in Tab Candy. Implementing Undo Close Tab Group should solve this problem.  

Reproducible: Always
Blocks: 585689
Component: General → TabCandy
Duplicate of this bug: 587337
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
QA Contact: general → tabcandy
Duplicate of this bug: 587558
Should we take the idea further and put a recently closed Tab Groups menu item in History sub-menu of the Firefox Menu?
Blocks: 587676
Please see the full specification for this feature here:

https://wiki.mozilla.org/Firefox/Projects/TabCandy/Design/UndoCloseGroup
Blocks: 588526
No longer blocks: 585689, 587676
Target Milestone: --- → Firefox 4.0b5
Summary: Implement "Undo Close Tab Group" in Tab View → Implement Undo Close Group inside of Tab Candy
Whiteboard: b5
Depends on: 587874
Aza, If the user closes 2 group quickly one after the other, will 2 reopen group come out, or just 1(for the last closed group) will appear?
(In reply to comment #5)
> Aza, If the user closes 2 group quickly one after the other, will 2 reopen
> group come out, or just 1(for the last closed group) will appear?

I propose there be one for each.
In terms of implementation, I see two possibilities: 

1. We modify sessionstore to keep track of closed groups like it does closed tabs. 
2. We hide the associated TabItems when the user closes a group, but don't actually close the tabs until either the "undo close group" button fades away or the user exits Panorama mode. 

Given that we're not doing "all windows" (bug 578512) for this version, and therefore won't be able to do a clean sessionstore integration, I propose we go with option 2, with the intention of moving to option 1 in a future version. Going with option 2 will also allow us to no longer depend on bug 587874. It would likely mean deferring the history menu addition (bug 587676), but I'm going to propose punting that as well.
No longer depends on: 587874
Target Milestone: Firefox 4.0b5 → Firefox 4.0b6
Duplicate of this bug: 588526
Attached file First pass at the undo feature (obsolete) —
This implements the UI-side of the undo feature. It does not attempt to actually do the undoing.
Attached patch First pass at the undo feature (obsolete) — Splinter Review
Same as before, but with the right mime-type
Attachment #472497 - Attachment is obsolete: true
Note: one problem with this patch is I haven't separated out the CSS to the appropriate files.
Attachment #472498 - Attachment is patch: true
Attachment #472498 - Flags: feedback?(ian)
Attachment #472498 - Flags: feedback?(dietrich)
Blocks: 587676
Blocks: 585689
Assignee: nobody → raymond
blocking2.0: --- → ?
Attached file Undo patch v1.1 (obsolete) —
This fixes the CSS — it is now divided into structural components. Will need theming work for Windows and Linux.
Attachment #472498 - Attachment is obsolete: true
Attachment #472562 - Flags: review?(dietrich)
Attachment #472562 - Flags: feedback?(ian)
Attachment #472498 - Flags: feedback?(ian)
Attachment #472498 - Flags: feedback?(dietrich)
Comment on attachment 472562 [details]
Undo patch v1.1

feedback+. request review when you consider the patch checkin-ready.
Attachment #472562 - Flags: review?(dietrich) → feedback+
Attached patch v2 (obsolete) — Splinter Review
Ian: please give this patch a try.  Tests are missing, will add them soon.
Attachment #472562 - Attachment is obsolete: true
Attachment #472683 - Flags: feedback?(ian)
Attachment #472562 - Flags: feedback?(ian)
Blocks: 594094
Comment on attachment 472683 [details] [diff] [review]
v2

Looks great! Once it's got a unit test, it'll be ready to go.
Attachment #472683 - Flags: feedback?(ian) → feedback+
Attached patch v3 with tests (obsolete) — Splinter Review
However, this patch doesn't include updating the browser_tabview_group.js since it's being updated in bug 594213
Attachment #472683 - Attachment is obsolete: true
Attachment #472991 - Flags: feedback?(ian)
Attached patch v3 with tests (obsolete) — Splinter Review
Forget to add the test file.
Attachment #472991 - Attachment is obsolete: true
Attachment #472992 - Flags: feedback?(ian)
Attachment #472991 - Flags: feedback?(ian)
Comment on attachment 472992 [details] [diff] [review]
v3 with tests

+  let padding = 200;
+  pageBounds.inset(padding, padding);
+
+  let box = new contentWindow.Rect(padding);
+  box.width = 300;
+  box.height = 300;

Creating a Rect with a single number is a bit odd. Also you're not using pageBounds after creating it.  Did you mean to pass it in to the Rect constructor? 

f+ with that. Please make the change and then r? dietrich.
Attachment #472992 - Flags: feedback?(ian) → feedback+
Attached patch v4 (obsolete) — Splinter Review
Pushed to try server and waiting for the result. rev 326e94d59cd7
Attachment #472992 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
Made some changes in the dropdrop test. Pushed another commit to the try server. rev 324f7fb6fefd
Attachment #473170 - Attachment is obsolete: true
Attachment #473179 - Flags: review?(dietrich)
Results in the try server look good.  Please review this patch.  Thanks!
Comment on attachment 473179 [details] [diff] [review]
v4

>       groupItems.forEach(function(groupItem) { 
>-        if (groupItem.getTitle().length > 0 && 
>+        if (groupItem.getTitle().length > 0 && !groupItem.canUndo &&
>             (!activeGroup || activeGroup.id != groupItem.id)) {

a lot of conditions here. add a comment explaining what's happening?

>@@ -1558,17 +1692,16 @@ window.GroupItems = {
>   // Function: setActiveGroupItem
>   // Sets the active groupItem, thereby showing only the relevent tabs, and
>   // setting the groupItem which will receive new tabs.
>   //
>   // Paramaters:

nit: fix this while you're here?

>+  // Function: removeCanUndoGroups
>+  // Removes all can undo groups' data and its children browser tabs
>+  removeCanUndoGroups: function() {  

nit: extra whitespace

>+    this.groupItems.forEach(function(groupItem) {
>+      if (groupItem.canUndo) {
>+        let toClose = groupItem._children.concat();
>+        toClose.forEach(function(child) {
>+          child.close();
>+        });

seems like this should be encapsulated inside the groupItem somehow? (doesn't need to be fixed in this bug)

>diff --git a/browser/base/content/test/tabview/browser_tabview_undo_group.js b/browser/base/content/test/tabview/browser_tabview_undo_group.js

for the test, i'd like to see it cover the full-circle of actions: close -> undo -> close -> undo. a lot of bugs have been generated over the years by not fully testing the entire cycle.

patch looks good for the most part. with these fixed and the naming changes we talked about on irc i could r+ this.
Attachment #473179 - Flags: review?(dietrich) → review-
Blocks: 594863
Attached patch v5 (obsolete) — Splinter Review
Attachment #473179 - Attachment is obsolete: true
Attachment #473630 - Flags: review?(dietrich)
Attachment #473630 - Flags: review?(dietrich)
Attachment #473630 - Flags: review+
Attachment #473630 - Flags: approval2.0+
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #473630 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch v6 (obsolete) — Splinter Review
Fixed the test for bug 591706
Attachment #473669 - Attachment is obsolete: true
Attachment #473932 - Attachment is patch: true
Attachment #473932 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 473932 [details] [diff] [review]
v6

Just added a fix for the browser_tabview_bug591706.js in this patch.
Attachment #473932 - Flags: review?(dietrich)
Keywords: checkin-needed
Attachment #473932 - Flags: review?(dietrich) → review+
Attachment #473932 - Flags: approval2.0?
Attachment #473932 - Flags: approval2.0? → approval2.0+
Attachment #473932 - Attachment is obsolete: true
Keywords: checkin-needed
Please replace -moz-border-radius with border-radius.
Keywords: checkin-needed
Replaced -moz-border-radius with border-radius.
Attachment #474045 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a6b04551c72a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
posthumous blocking+ since people might not immediately understand that closing a group means closing all of that group's tabs.
blocking2.0: ? → final+
Want a test case in Litmus for this. Flagging it to pick up during Bug Week (9-13 to 9-17)

Verified with nightly build of 20100913
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Added test 12843 in litmus.
Flags: in-litmus? → in-litmus+
Whiteboard: b5 → b5, [in-litmus-bug-week]
No longer blocks: 594863
Depends on: 614545
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.