632294 - Unify setActive... functions

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Right now we have GroupItems.setActiveGroup, .setActiveOrphanTab, UI.setActiveTab... and, as seen in bug 597980, we need to make sure that the right functions call the right other functions to ensure that the overall combination of states is always compatible.

Perhaps we should just replace all of these with one function, like UI.setActive which takes a tab *or* a group, and takes care of everything else? Here's some rough pseudocode, in terms of our current API:

IF argument is a tab
  IF argument.parent
    setActiveGroup(argument.parent)
    setActiveOrphanTab(null)
    setActiveTab(argument)
  ELSE
    setActiveGroupItem(null)
    setActiveOrphanTab(argument)
    setActiveTab(argument)
ELSE
  setActiveGroup(argument)
  setActiveOrphanTab(null)
  setActiveTab(argument._activeTab)

Just looking at that should show that, in order to do this right, we probably should just unify these methods.

Comment 1

[Kevin Hanes]

bugspam

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d0f69b746cf1

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Looks good, nice cleanup :) There are some places left where we need to use setActive():

* we should update all tests (yeah there are a lot :) and replace UI.setActiveTab() and GroupItems.setActiveGroupItem() with setActive()
* tabitems.js (line 1091), ui.js (lines 192, 530, 1180), head.js
* groupitems.js:2405-2407 we can remove these lines because they're redundant (tabItem.makeActive() does that for us)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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

F+ with all those changes.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #3)
> Comment on attachment 525239 [details] [diff] [review]
> v1
> 
> Looks good, nice cleanup :) There are some places left where we need to use
> setActive():
> 
> * we should update all tests (yeah there are a lot :) and replace
> UI.setActiveTab() and GroupItems.setActiveGroupItem() with setActive()
> * tabitems.js (line 1091), ui.js (lines 192, 530, 1180), head.js
> * groupitems.js:2405-2407 we can remove these lines because they're redundant
> (tabItem.makeActive() does that for us)

Done.

Comment 6

[Raymond Lee [:raymondlee]]

Comment on attachment 526219 [details] [diff] [review]
v2

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=ac706d443814

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 526219 [details] [diff] [review]
v2

I think this is a step in the right direction, but ultimately I think we want it to go further. You already have it set up to automatically select the parent when a tab is selected; this is good. It should also, in most cases, work the other way: when you set a group to be active, it should select its "active tab". This will likely change the UI's behavior, but for the better. I imagine there are probably some exceptions to the rule in the code, but that should be the general behavior.

Changing that behavior might be a bit much for this bug; perhaps it should be a follow-up. However, looking forward to that, let's take a look at the options in the new setActive should: 

>+  //  setActiveTabInGroup bool for setting active tab in group

This behavior should be the default, and you should have to send in optional parameters only if you want special behavior:  dontSetActiveTabInGroup. 

>+  //  removeActiveGroup bool for removing active group
>+  //  removeActiveTab bool for removing active tab

These are a little confusing, since leaving them out removes both (which is the correct behavior). I recommend changing them to onlyRemoveActiveGroup and onlyRemoveActiveTab. Also I recommend looking at the places where those are used and determining if they really need to be used, or if the default behavior works.

One more note: 

>         // if it is visually active, set it as the active tab.
>         if (iQ(item.container).hasClass("focus"))
>-          this.setActiveTab(item);
>+          UI.setActive(item);

Changing this.setActiveTab to UI.setActive is the wrong thing here; you're changing away from the GroupItem.setActiveTab, which manages the group's notion of which of its tabs are active. Stick with this.setActiveTab in this case.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to comment #7)
> Comment on attachment 526219 [details] [diff] [review]
> v2
> 
> I think this is a step in the right direction, but ultimately I think we want
> it to go further. You already have it set up to automatically select the parent
> when a tab is selected; this is good. It should also, in most cases, work the
> other way: when you set a group to be active, it should select its "active
> tab". This will likely change the UI's behavior, but for the better. I imagine
> there are probably some exceptions to the rule in the code, but that should be
> the general behavior.
> 
> Changing that behavior might be a bit much for this bug; perhaps it should be a
> follow-up. 

Filed bug 651812

> However, looking forward to that, let's take a look at the options
> in the new setActive should: 
> 
> >+  //  setActiveTabInGroup bool for setting active tab in group
> 
> This behavior should be the default, and you should have to send in optional
> parameters only if you want special behavior:  dontSetActiveTabInGroup. 

Fixed

> 
> >+  //  removeActiveGroup bool for removing active group
> >+  //  removeActiveTab bool for removing active tab
> 
> These are a little confusing, since leaving them out removes both (which is the
> correct behavior). I recommend changing them to onlyRemoveActiveGroup and
> onlyRemoveActiveTab. Also I recommend looking at the places where those are
> used and determining if they really need to be used, or if the default behavior
> works.
> 

Fixed

> One more note: 
> 
> >         // if it is visually active, set it as the active tab.
> >         if (iQ(item.container).hasClass("focus"))
> >-          this.setActiveTab(item);
> >+          UI.setActive(item);
> 
> Changing this.setActiveTab to UI.setActive is the wrong thing here; you're
> changing away from the GroupItem.setActiveTab, which manages the group's notion
> of which of its tabs are active. Stick with this.setActiveTab in this case.

Fixed

Comment 9

[Ian Gilman [:iangilman]]

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

Thanks! 

You didn't add dontSetActiveTabInGroup to any of the tests... does that mean they work fine without it? That's good news!

Comment 10

[Raymond Lee [:raymondlee]]

(In reply to comment #9)
> Comment on attachment 527499 [details] [diff] [review]
> v3
> 
> Thanks! 
> 
> You didn't add dontSetActiveTabInGroup to any of the tests... does that mean
> they work fine without it? That's good news!

Yes, tests work fine without it.  The try tree is closed at the moment, will push it when it's opened again.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Passed Try
http://dev.philringnalda.com/tbpl/?tree=Try&rev=bfed5101e70c

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Patch doesn't apply cleanly to mozilla-central now, after this cset:
  http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin (unrotted)

(In reply to comment #12)
> Patch doesn't apply cleanly to mozilla-central now, after this cset:
>   http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78

Unrotted. Thanks for the hint :)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/projects/cedar/rev/8c4250c3bbf3

Comment 15

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/8c4250c3bbf3