Last Comment Bug 663421 - Don't close empty groups automatically
: Don't close empty groups automatically
Status: VERIFIED FIXED
[inbound]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 980292 980298
Blocks: 637840 660175 664669
  Show dependency treegraph
 
Reported: 2011-06-10 09:33 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (39.80 KB, patch)
2011-06-13 05:12 PDT, Tim Taubert [:ttaubert]
dietrich: review+
raymond: feedback+
Details | Diff | Review
patch v2 (10.63 KB, patch)
2011-07-03 18:03 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v3 (11.31 KB, patch)
2011-07-04 01:13 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v4 (11.31 KB, patch)
2011-07-04 08:44 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2011-06-10 09:33:48 PDT
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] 2011-06-13 05:12:52 PDT
Created attachment 538866 [details] [diff] [review]
patch v1
Comment 2 Raymond Lee [:raymondlee] 2011-06-13 08:08:48 PDT
Comment on attachment 538866 [details] [diff] [review]
patch v1

Looks good to me!
Comment 3 Tim Taubert [:ttaubert] 2011-06-14 09:16:06 PDT
Comment on attachment 538866 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=bad34ef30825
Comment 4 Dietrich Ayala (:dietrich) 2011-06-15 10:30:56 PDT
Comment on attachment 538866 [details] [diff] [review]
patch v1

Review of attachment 538866 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 6 Mounir Lamouri (:mounir) 2011-06-18 09:37:33 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/646f3f6f3cd5
Comment 7 George Carstoiu 2011-06-20 07:06:04 PDT
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] 2011-07-03 18:03:22 PDT
Created attachment 543707 [details] [diff] [review]
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] 2011-07-04 01:13:48 PDT
Created attachment 543726 [details] [diff] [review]
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] 2011-07-04 05:08:10 PDT
merged backout http://hg.mozilla.org/mozilla-central/rev/a8553a17f750 (Comment 8)
Comment 11 Dão Gottwald [:dao] 2011-07-04 08:40:34 PDT
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] 2011-07-04 08:44:35 PDT
Created attachment 543784 [details] [diff] [review]
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] 2011-07-04 11:57:30 PDT
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 15 Marco Bonardo [::mak] 2011-07-05 01:40:53 PDT
http://hg.mozilla.org/mozilla-central/rev/c4fd1aeff6e4
Comment 16 Virgil Dicu [:virgil] [QA] 2011-08-23 05:11:07 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.