Closed Bug 593871 Opened 9 years ago Closed 9 years ago

handle app tab pinning/unpinning

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: iangilman, Assigned: iangilman)

References

Details

(Whiteboard: [in-litmus-bug-week])

Attachments

(1 file, 4 obsolete files)

The Panorama UI needs to update if/when a tab goes from being an app tab to a normal tab or vice-versa.
Depends on: 593872
Blocks: 594094
Assignee: nobody → raymond
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.
Assignee: raymond → nobody
this patch needs to land after bug 595076
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+.
Attachment #474231 - Attachment description: Patch v1 → Patch v1 (applies cleanly after 595076 patch has been applied)
Attachment #474231 - Flags: feedback?(mitcho) → feedback+
(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.
Attachment #474231 - Attachment is obsolete: true
Attachment #474271 - Flags: review?(ehsan)
blocking2.0: --- → ?
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.
Attachment #474271 - Flags: review?(ehsan) → review?(dolske)
Summary: Panorama should do the right thing when tabs become pinned/unpinned → handle app tab pining/unpinning
blocking2.0: ? → betaN+
Try server build is clean.
The TabPin and TabUnpin events should be renamed to TabPinned and TabUnpinned respectively.
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Using the correct name for the events...
Attachment #474606 - Flags: review?(dolske)
Attached patch v3 (obsolete) — Splinter Review
Updated the event names
Attachment #474271 - Attachment is obsolete: true
Attachment #474606 - Attachment is obsolete: true
Attachment #474271 - Flags: review?(dolske)
Attachment #474606 - Flags: review?(dolske)
Attachment #474609 - Flags: review?(dolske)
(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
(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..
(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 on attachment 474609 [details] [diff] [review]
v3

Reverting review request to dietrich. Dolske, please let us know if you have any comments.
Attachment #474609 - Flags: review?(dolske) → review?(dietrich)
Summary: handle app tab pining/unpinning → handle app tab pinning/unpinning
Comment on attachment 474609 [details] [diff] [review]
v3

any reason why you're capturing in the event listeners? r=me otherwise.
Attachment #474609 - Flags: review?(dietrich) → review+
Fixed event listeners so they're not capturing.
Attachment #474609 - Attachment is obsolete: true
Assignee: nobody → ian
Duplicate of this bug: 595904
http://hg.mozilla.org/mozilla-central/rev/9e5cbde7ea9e
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Need test case in litmus for App Tab handling
Status: RESOLVED → VERIFIED
Flags: in-litmus?
As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.
Flags: in-litmus? → in-litmus+
Whiteboard: [in-litmus-bug-week]
Litmus ID 12842
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.