Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 13 years ago

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

	Raymond Lee [:raymondlee]
	Comment 2 • 13 years ago

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

Looks good to me!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 3 • 13 years ago

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

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=bad34ef30825

	Dietrich Ayala (:dietrich)
	Comment 4 • 13 years ago

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

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 13 years ago

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

	Mounir Lamouri (:mounir)
	Comment 6 • 13 years ago

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

	George Carstoiu
	Comment 7 • 13 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 8 • 13 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 13 years ago

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!

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 10 • 13 years ago

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

	Dão Gottwald [:dao]
	Comment 11 • 13 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 12 • 13 years ago

(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.

	Dão Gottwald [:dao]
	Comment 13 • 13 years ago

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"

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 14 • 13 years ago

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

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 15 • 13 years ago

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

	Virgil Dicu [:virgil] [QA]
	Comment 16 • 13 years ago

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.