663421 - Don't close empty groups automatically

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We're closing empty groups without a title automatically on showing/hiding Panorama. This is unexpected behavior and we want this to be removed.

https://wiki.mozilla.org/Simplify_Panorama_UI

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 538866 [details] [diff] [review]
patch v1

Looks good to me!

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 538866 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=bad34ef30825

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 538866 [details] [diff] [review]
patch v1

Review of attachment 538866 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/646f3f6f3cd5

Comment 6

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/646f3f6f3cd5

Comment 7

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110620 Firefox/7.0a1

Verified issue on Ubuntu 11.04 x86, WinXP, Win7 x86, Mac OS X 10.6 using the following steps:
 1. Enter Panorama
 2. Create an empty group
 3. Exit Panorama
 4. Enter Panorama
 5. Verify whether group is still present

Issue no longer present - setting status to Verified.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Old patch backed out - turned out we still need this behavior:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8553a17f750

New patch prepared with the following behavior - close empty groups when:

1) closing the last tab of a group
2) drag out the last tab of a group

Don't close the group if those actions occur while Panorama is hidden. Don't close empty groups when toggling Panorama.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Fixed some nits.

Dão, could you please review this? This bug blocks some patches I'm hoping to land for Fx 7 and all the other reviewers are out for holiday... Thanks!

Comment 10

[Marco Bonardo [:mak]]

merged backout http://hg.mozilla.org/mozilla-central/rev/a8553a17f750 (Comment 8)

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 543726 [details] [diff] [review]
patch v3

>--- a/browser/base/content/browser-tabview.js
>+++ b/browser/base/content/browser-tabview.js
>@@ -350,20 +350,19 @@ let TabView = {
>         event.preventDefault();
> 
>         self._initFrame(function() {
>           let groupItems = self._window.GroupItems;
>           let tabItem = groupItems.getNextGroupItemTab(event.shiftKey);
>           if (!tabItem)
>             return;
> 
>-          // Switch to the new tab, and close the old group if it's now empty.
>+          // Switch to the new tab
>           let oldGroupItem = groupItems.getActiveGroupItem();
>           window.gBrowser.selectedTab = tabItem.tab;
>-          oldGroupItem.closeIfEmpty();
>         });

This makes oldGroupItem unused.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to comment #11)
> >+          // Switch to the new tab
> >           let oldGroupItem = groupItems.getActiveGroupItem();
> >           window.gBrowser.selectedTab = tabItem.tab;
> >-          oldGroupItem.closeIfEmpty();
> >         });
> 
> This makes oldGroupItem unused.

Oh, yeah, good catch.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 543784 [details] [diff] [review]
patch v4

>+  let assertNumberOfGroupItems = function (num) {
>+    is(cw.GroupItems.groupItems.length, num, "there are " + num + " groupItems");
>+  };

"assert" in has a conventional meaning that you're not fulfilling. Call it "test" or "check".

>+  let next = function () {
>+    if (tests.length)
>+      tests.shift()();
>+    else
>+      finish();
>+  };

"function next() {" and drop the semicolon. The same applies to various other functions in this test.

>+  newWindowWithTabView(function (tvwin) {
>+    registerCleanupFunction(function () tvwin.close());
>+
>+    win = tvwin;
>+    cw = win.TabView.getContentWindow();
>+    groupItem = createEmptyGroupItem(cw, 200, 200, 20);
>+
>+    assertNumberOfGroupItems(2);
>+    next();
>+  });

"aWin" instead of "tvwin"

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c4fd1aeff6e4

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/c4fd1aeff6e4

Comment 16

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed on Firefox7 beta1, on Mac X OS 10.6, Ubuntu 11.04, Windows XP and Windows 7.

No problems occurred when following the test cases (created based on comment 8):

1. Enter Panorama.
2. Close the tabs of any group one at a time.
3. After closing the last tab of one group, the now empty group should be automatically removed.

1. Enter Panorama.
2. Drag or close tabs from a group until one tab is left for that group.
3. Drag the last tab from that group.
4. The respective group should be removed when dragging the last tab from it.

1. Have more than two tabs in a group in Panorama.
2. Exit panorama.
3. Close or drag the tabs from that group one by one until one tab is still remaining.
4. Close or drag the last tab.
5. Enter Panorama.
6. An empty group should still be displayed instead of the old one.

1. Enter Panorama.
2. Create an empty group.
3. Exit Panorama.
4. Enter Panorama.
5. Verify whether group is still present.