Closed
Bug 587341
Opened 15 years ago
Closed 15 years ago
Implement Undo Close Group inside of Tab Candy
Categories
(Firefox Graveyard :: Panorama, enhancement, P1)
Firefox Graveyard
Panorama
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)
30.26 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•15 years ago
|
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Updated•15 years ago
|
QA Contact: general → tabcandy
Reporter | ||
Comment 3•15 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?
Comment 4•15 years ago
|
||
Please see the full specification for this feature here:
https://wiki.mozilla.org/Firefox/Projects/TabCandy/Design/UndoCloseGroup
Updated•15 years ago
|
Summary: Implement "Undo Close Tab Group" in Tab View → Implement Undo Close Group inside of Tab Candy
Whiteboard: b5
Reporter | ||
Comment 5•15 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?
Comment 6•15 years ago
|
||
(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.
Comment 7•15 years ago
|
||
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
Comment 9•15 years ago
|
||
Priority: -- → P1
Comment 10•15 years ago
|
||
This implements the UI-side of the undo feature. It does not attempt to actually do the undoing.
Comment 11•15 years ago
|
||
Same as before, but with the right mime-type
Attachment #472497 -
Attachment is obsolete: true
Comment 12•15 years ago
|
||
Note: one problem with this patch is I haven't separated out the CSS to the appropriate files.
Updated•15 years ago
|
Attachment #472498 -
Attachment is patch: true
Attachment #472498 -
Flags: feedback?(ian)
Attachment #472498 -
Flags: feedback?(dietrich)
Updated•15 years ago
|
Assignee: nobody → raymond
Updated•15 years ago
|
blocking2.0: --- → ?
Comment 13•15 years ago
|
||
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 14•15 years ago
|
||
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•15 years ago
|
||
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)
Comment 16•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Forget to add the test file.
Attachment #472991 -
Attachment is obsolete: true
Attachment #472992 -
Flags: feedback?(ian)
Attachment #472991 -
Flags: feedback?(ian)
Comment 19•15 years ago
|
||
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•15 years ago
|
||
Pushed to try server and waiting for the result. rev 326e94d59cd7
Attachment #472992 -
Attachment is obsolete: true
Assignee | ||
Comment 21•15 years ago
|
||
Made some changes in the dropdrop test. Pushed another commit to the try server. rev 324f7fb6fefd
Attachment #473170 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #473179 -
Flags: review?(dietrich)
Assignee | ||
Comment 22•15 years ago
|
||
Results in the try server look good. Please review this patch. Thanks!
Comment 23•15 years ago
|
||
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 | ||
Comment 24•15 years ago
|
||
Attachment #473179 -
Attachment is obsolete: true
Attachment #473630 -
Flags: review?(dietrich)
Updated•15 years ago
|
Attachment #473630 -
Flags: review?(dietrich)
Attachment #473630 -
Flags: review+
Attachment #473630 -
Flags: approval2.0+
Assignee | ||
Comment 25•15 years ago
|
||
Attachment #473630 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•15 years ago
|
||
Fixed the test for bug 591706
Attachment #473669 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #473932 -
Attachment is patch: true
Attachment #473932 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 27•15 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•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Attachment #473932 -
Flags: review?(dietrich) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #473932 -
Flags: approval2.0?
Updated•15 years ago
|
Attachment #473932 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 28•15 years ago
|
||
Attachment #473932 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 29•15 years ago
|
||
Please replace -moz-border-radius with border-radius.
Keywords: checkin-needed
Assignee | ||
Comment 30•15 years ago
|
||
Replaced -moz-border-radius with border-radius.
Attachment #474045 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 31•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 32•15 years ago
|
||
posthumous blocking+ since people might not immediately understand that closing a group means closing all of that group's tabs.
blocking2.0: ? → final+
Comment 33•15 years ago
|
||
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?
Comment 34•15 years ago
|
||
Added test 12843 in litmus.
Flags: in-litmus? → in-litmus+
Whiteboard: b5 → b5, [in-litmus-bug-week]
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•