Implement Undo Close Group inside of Tab Candy

VERIFIED FIXED in Firefox 4.0b7

Status

P1
enhancement
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: Ryuji, Assigned: raymondlee)

Tracking

Trunk
Firefox 4.0b7
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Details

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

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
Blocks: 585689
Component: General → TabCandy

Updated

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

Updated

8 years ago
Duplicate of this bug: 587558
(Reporter)

Comment 3

8 years ago
Should we take the idea further and put a recently closed Tab Groups menu item in History sub-menu of the Firefox Menu?
(Reporter)

Updated

8 years ago
Blocks: 587676

Comment 4

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

Updated

8 years ago
Summary: Implement "Undo Close Tab Group" in Tab View → Implement Undo Close Group inside of Tab Candy
Whiteboard: b5

Updated

8 years ago
Depends on: 587874
(Reporter)

Comment 5

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

Comment 10

8 years ago
Created attachment 472497 [details]
First pass at the undo feature

This implements the UI-side of the undo feature. It does not attempt to actually do the undoing.

Comment 11

8 years ago
Created attachment 472498 [details] [diff] [review]
First pass at the undo feature

Same as before, but with the right mime-type
Attachment #472497 - Attachment is obsolete: true

Comment 12

8 years ago
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)
(Reporter)

Updated

8 years ago
Blocks: 587676
(Reporter)

Updated

8 years ago
Blocks: 585689

Updated

8 years ago
Assignee: nobody → raymond

Updated

8 years ago
blocking2.0: --- → ?

Comment 13

8 years ago
Created attachment 472562 [details]
Undo patch v1.1

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+
(Assignee)

Comment 15

8 years ago
Created attachment 472683 [details] [diff] [review]
v2

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+
(Assignee)

Comment 17

8 years ago
Created attachment 472991 [details] [diff] [review]
v3 with tests

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)
(Assignee)

Comment 18

8 years ago
Created attachment 472992 [details] [diff] [review]
v3 with tests

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+
(Assignee)

Comment 20

8 years ago
Created attachment 473170 [details] [diff] [review]
v4

Pushed to try server and waiting for the result. rev 326e94d59cd7
Attachment #472992 - Attachment is obsolete: true
(Assignee)

Comment 21

8 years ago
Created attachment 473179 [details] [diff] [review]
v4

Made some changes in the dropdrop test. Pushed another commit to the try server. rev 324f7fb6fefd
Attachment #473170 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #473179 - Flags: review?(dietrich)
(Assignee)

Comment 22

8 years ago
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-
(Assignee)

Updated

8 years ago
Blocks: 594863
(Assignee)

Comment 24

8 years ago
Created attachment 473630 [details] [diff] [review]
v5
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+
(Assignee)

Comment 25

8 years ago
Created attachment 473669 [details] [diff] [review]
Patch for check-in
Attachment #473630 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

8 years ago
Created attachment 473932 [details] [diff] [review]
v6

Fixed the test for bug 591706
Attachment #473669 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #473932 - Attachment is patch: true
Attachment #473932 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 27

8 years ago
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)
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Attachment #473932 - Flags: review?(dietrich) → review+
(Assignee)

Updated

8 years ago
Attachment #473932 - Flags: approval2.0?
Attachment #473932 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 28

8 years ago
Created attachment 474045 [details] [diff] [review]
patch for check-in [r+a=dietrich]
Attachment #473932 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Please replace -moz-border-radius with border-radius.
Keywords: checkin-needed
(Assignee)

Comment 30

8 years ago
Created attachment 474056 [details] [diff] [review]
Patch for check-in [r+a=dietrich]

Replaced -moz-border-radius with border-radius.
Attachment #474045 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 31

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a6b04551c72a
Status: NEW → RESOLVED
Last Resolved: 8 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]
(Assignee)

Updated

8 years ago
No longer blocks: 594863

Updated

8 years ago
Depends on: 614545
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.