593871 - handle app tab pinning/unpinning

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Ian Gilman [:iangilman]]

The Panorama UI needs to update if/when a tab goes from being an app tab to a normal tab or vice-versa.

Comment 1

[Ian Gilman [:iangilman]]

It occurred to me maybe I should be a little more specific! When a tab gets pinned, its TabItem should be destroyed (removing it from whatever group it was in), and an app tab icon should be created for it in each group on the screen. When a tab gets unpinned, a TabItem should be created for it (getting added to a group as normal for a new tab), and its app tab icon should be removed from every group.

Comment 2

[Ian Gilman [:iangilman]]

Attachment: Patch v1 (applies cleanly after 595076 patch has been applied)

Comment 3

[Ian Gilman [:iangilman]]

this patch needs to land after bug 595076

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 474231 [details] [diff] [review]
Patch v1 (applies cleanly after 595076 patch has been applied)

>diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js
>   // Adds the given xul:tab as an app tab in this group's apptab tray
>   addAppTab: function GroupItem_addAppTab(xulTab) {
...
>+    // adjust the tray
>     let columnWidth = $appTab.width();
>     if (parseInt(this.$appTabTray.css("width")) != columnWidth) {
>       this.$appTabTray.css({width: columnWidth});
>       this.arrange();
>     }

This part looks right, but I found a bug with it. When I pin a group in another group (not visible here) and go back to tab view, I see that the app tab was added, but apparently arrange was not called? So I get this:

http://img.skitch.com/20100910-b5natnu521ikchy4xcxgsa4hh3.jpg

Moving it a bit forces a redraw, and I get this:

http://img.skitch.com/20100910-fepqefkcytk5ine5h79qgknre8.jpg

>   // ----------
>+  // Removes the given xul:tab as an app tab in this group's apptab tray

No "Function: removeAppTab" line? Same for some of your other new functions.

>+  handleTabPin: function GroupItems_handleTabPin(xulTab) {
>+  handleTabUnpin: function GroupItems_handleTabUnpin(xulTab) {
>+  handleTabPin: function TabItems_handleTabPin(xulTab) {
>+  handleTabUnpin: function TabItems_handleTabUnpin(xulTab) {

These names are confusing. How about GroupItems.addAppTabToAll, .removeAppTabFromAll or something like that?

Hmm, but now that I see how you're invoking them (below), I feel they're maybe okay. :/

>+    // Start watching for tab pin events, and set up our uninit for same.
>+    function handleTabPin(event) {
>+      TabItems.handleTabPin(event.originalTarget);
>+      GroupItems.handleTabPin(event.originalTarget);
>+    }
>+    

^^^ extra space, here and elsewhere

Also, should these handlers be registered/handled by AllTabs?

>+  // create a second group and make sure it gets the icon too
>+  box.offset(box.width + 20, 0);
>+  let groupItemTwo = new contentWindow.GroupItem([], { bounds: box, title: "test2" });
>+  is(contentWindow.GroupItems.groupItems.length, 3, "we now have three groups");
>+  appTabIcons = groupItemTwo.container.getElementsByClassName("appTabIcon");
>+  is(appTabIcons.length, 1, "there's an app tab icon in the second group");

Could you move some of the pinning/unpinning to after you create a second group item? I would worry about the case where we pin or unpin a tab in a group, but other groups don't get updated. We should check for that case too?

Please figure out that first bug, or file a separate bug for it, and remove the extra space, but otherwise feedback+.

Comment 5

[Ian Gilman [:iangilman]]

Attachment: patch v2 (applies cleanly after 595076 patch has been applied)

(In reply to comment #4)
> This part looks right, but I found a bug with it. When I pin a group in another
> group (not visible here) and go back to tab view, I see that the app tab was
> added, but apparently arrange was not called? So I get this:

Good catch! Fixed.

> No "Function: removeAppTab" line? Same for some of your other new functions.

Yeah, we should figure out what our comments look like in the new world. I feel like no one's using the Natural Docs documentation (and I haven't updated it in a while anyway), so those "Function:" headers are superfluous now. On the other hand, that's what we've got, so maybe we should keep it up, especially if we like it. Thoughts?

> >+  handleTabPin: function GroupItems_handleTabPin(xulTab) {
> >+  handleTabUnpin: function GroupItems_handleTabUnpin(xulTab) {
> >+  handleTabPin: function TabItems_handleTabPin(xulTab) {
> >+  handleTabUnpin: function TabItems_handleTabUnpin(xulTab) {
> 
> These names are confusing. How about GroupItems.addAppTabToAll,
> .removeAppTabFromAll or something like that?
> 
> Hmm, but now that I see how you're invoking them (below), I feel they're maybe
> okay. :/

Yeah, I want to stick with that for event handlers. 

> ^^^ extra space, here and elsewhere

*sigh* fixed.

> Also, should these handlers be registered/handled by AllTabs?

Filed follow up bug 595395.

> Could you move some of the pinning/unpinning to after you create a second group
> item? I would worry about the case where we pin or unpin a tab in a group, but
> other groups don't get updated. We should check for that case too?

Done.

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 474271 [details] [diff] [review]
patch v2 (applies cleanly after 595076 patch has been applied)

Assigning review to dolske in the hopes he can get to it tonight.

Comment 7

[Ian Gilman [:iangilman]]

Try server build is clean.

Comment 8

[(no longer active)]

The TabPin and TabUnpin events should be renamed to TabPinned and TabUnpinned respectively.

Comment 9

[Ian Gilman [:iangilman]]

(In reply to comment #8)
> The TabPin and TabUnpin events should be renamed to TabPinned and TabUnpinned
> respectively.

This is due to bug 595304, I see.

If anyone wants to update the event names in this patch over the weekend, go for it; otherwise I'll get to it on Monday.

Comment 10

[(no longer active)]

Attachment: Patch (v2)

Using the correct name for the events...

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v3

Updated the event names

Comment 12

[Raymond Lee [:raymondlee]]

(In reply to comment #10)
> Created attachment 474606 [details] [diff] [review]
> Patch (v2)
> 
> Using the correct name for the events...

Oops, sorry Ehsan. Didn't release that you have jusy uploaded a patch

Comment 13

[Raymond Lee [:raymondlee]]

(In reply to comment #12)
> (In reply to comment #10)
> > Created attachment 474606 [details] [diff] [review] [details]
> > Patch (v2)
> > 
> > Using the correct name for the events...
> 
> Oops, sorry Ehsan. Didn't release that you have jusy uploaded a patch

I mean realise..

Comment 14

[(no longer active)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Created attachment 474606 [details] [diff] [review] [details]
> > > Patch (v2)
> > > 
> > > Using the correct name for the events...
> > 
> > Oops, sorry Ehsan. Didn't release that you have jusy uploaded a patch
> 
> I mean realise..

It's perfectly fine.  More patches are almost always better than fewer patches!  :-)

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 474609 [details] [diff] [review]
v3

Reverting review request to dietrich. Dolske, please let us know if you have any comments.

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 474609 [details] [diff] [review]
v3

any reason why you're capturing in the event listeners? r=me otherwise.

Comment 17

[Ian Gilman [:iangilman]]

Attachment: patch for check-in [r=dietrich, a=blocking betaN]

Fixed event listeners so they're not capturing.

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9e5cbde7ea9e

Comment 20

[Tracy Walker [:tracy]]

Need test case in litmus for App Tab handling

Comment 21

[Tracy Walker [:tracy]]

As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.

Comment 22

[Tracy Walker [:tracy]]

Litmus ID 12842