595893 - New tabs should never be orphans (tabs in lower right on first visit to Panorama during a session)

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Ian Gilman [:iangilman]]

You should be able to create an orphan by dragging a tab out of a group, but a newly created tab should never be an orphan. If there is no currently selected group, pick the one with any tabs showing. If no groups at all, create a new one.

Comment 2

[Raymond Lee [:raymondlee]]

The problem for this bug seems to be that blank tabs are not assigned to any group so they become orphans after restarting Fx.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Sent to try.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 477085 [details] [diff] [review]
v1

Good point. 

In addition, please take care of the else clause in GroupItems.newTab and get rid of GroupItems.positionNewTabAtBottom. 

I guess theoretically we'll never get to that else clause, but as a stop-gap, change it to select the first group, or if there are no groups, use the first orphan tab. 

A couple minor points: 

+  is(contentWindow.GroupItems.getOrphanedTabs(), 0, "No orphaned tabs");

You need .length in there. You've got this kind of check in two places; please update both. 

+  // 2) create a group, add a blank tab and 

This comment ends abruptly; did you mean to say more? If the "and" is meant to lead to the next comment a number of lines down, I think it's just confusing.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 477392 [details] [diff] [review]
v1

+        // browser must have at least one tab so in this case at least one 
+        // orphaned tab would exist.

This is actually no longer true, because of app tabs. It's possible for there to be no GroupItems and no TabItems but the browser doesn't close because there's still an app tab open. In that circumstance let's create a fresh group nicely positioned in the upper-left (similar to what we do on "first-run") and stick the new tab in it. 

+          let newGroupItemBounds = orphanedTabs[0].getBoundsWithTitle();
+          newGroupItemBounds.inset(-40,-40);
+          let newGroupItem = 
+            new GroupItem([orphanedTabs[0], tabItem], 
+                          { bounds: newGroupItemBounds });
+          newGroupItem.snap();
+          this.setActiveGroupItem(newGroupItem);

This chunk is almost identical to the orphan bit above it; combine somehow.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v1

Sent to try

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v1.1

Improved the patch.

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 478209 [details] [diff] [review]
v1.1

We're getting there! Sorry to keep going around with you on this. 

I think the sequence should be tweaked a little, so it's: 

1. Active group
2. Active orphan
3. First visible non-app tab (that's not the tab in question), whether it's an orphan or not (make a new group if it's an orphan, add it to the group if it's not)
4. First group
5. First orphan that's not the tab in question
6. At this point there should be no groups or tabs (except for app tabs and the tab in question): make a new group

You're mostly there already, with the exception of step 3, I think, which currently only works on orphans.

Also, since it's kind of a complicated sequence, might be good to comment it more explicitly. 

Btw, checking to see if a TabItem is an orphan is as simple as checking to see if it has a parent; no need to loop through the orphan list. 

+            orphanTab = orphanTabItem;

It's getting a little confusing what's a xul:tab and what's a TabItem. The line above is a good example. I think I'd prefer to avoid the word tab by itself and use either tabItem or xulTab. If not that, let's at least be consistent within a routine.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v1.2

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 478332 [details] [diff] [review]
v1.2

Cool, I think we have the logic we want. A couple of nits:

+      let orphanedTabs = this.getOrphanedTabs();

Move this down to where orphanedTabs is first used. 

+        orphanTab = otherTab.tabItem;

This is what I'm talking about in terms of confusing naming; you can't tell just by looking at their names that orphanTab is a TabItem and otherTab is a xul:tab, in fact just by their names you'd think they were the same type of object. How about keeping otherTab, but change orphanTab to orphanTabItem?

f+ with that

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v1.2

Fixed Ian's comments.

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 478501 [details] [diff] [review]
v1.2

Oh! Thought of another thing. If there are no other groups or tabs and you're therefore creating a fresh group for a fresh tab, please make sure that group and tab are positioned nicely in the upper-left-hand corner. The code in this patch just assumes the new tab is already in a good position, but I don't know that it is.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v1.3

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 478981 [details] [diff] [review]
v1.3

>+    // 1. Active group
>+    // 2. Active orphan
>+    // 3. First visible non-app tab (that's not the tab in question), whether it's an
>+    // orphan or not (make a new group if it's an orphan, add it to the group if it's
>+    // not)
>+    // 4. First group
>+    // 5. First orphan that's not the tab in question
>+    // 6. At this point there should be no groups or tabs (except for app tabs and the
>+    // tab in question): make a new group

This seems a bit like overkill, but meh.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: patch for check-in

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/03350cd18853

Comment 18

[Ian Gilman [:iangilman]]

Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/03350cd18853

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm tentatively clawing this and bug 602547 back into b7+ blockers; big wins for users, and as the shortcuts for Panorama become more discoverable, we want it to work properly if we can.

Ian, can you also land on GECKO20b7pre_20101006_RELBRANCH?

Comment 20

[Ian Gilman [:iangilman]]

Landed for b7

Comment 21

[Tracy Walker [:tracy]]

verified in recent nightly build of minefield

Comment 22

[Tracy Walker [:tracy]]

I had just checked normal browser restart. This not good coming out of Private Browsing.  

Here's what I had:

App Tabs: Yahoo.com and cnn.com
Group A with http://www.mozilla.com/en-US/about/
Group B with 3 New Tab
Group C with about:license

1) from the about Mozilla tab, Start Private Browsing, then Stop Private browsing

tested results:

If the groups were not actually given a name, the following happens:
upon return from PB, all tabs noted appear in the browser window. when you enter Group View.  all of the New Tabs and the about:license tab are orphaned. The http://www.mozilla.com/en-US/about/ Tab still in a group but the group has been shrunk to nearly stacked type

If the groups are all named:
upon return from PB, the tab with http://www.mozilla.com/en-US/about/ is moved from group A to Group C where the about:license tab is.

Comment 23

[Tracy Walker [:tracy]]

Attachment: restart after a few times in and out of PB mode

appeared in group view when Fx restarted after being in and out of Private Browsing mode while investigating for my last comment.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Aren't there other bugs about bad transitions from PB mode -> normal mode?

Comment 25

[Raymond Lee [:raymondlee]]

(In reply to comment #24)
> Aren't there other bugs about bad transitions from PB mode -> normal mode?

Yes, Bug 594644 so closing this bug