Last Comment Bug 632294 - Unify setActive... functions
: Unify setActive... functions
Status: RESOLVED FIXED
[cleanup][API]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: unspecified
: All All
: -- normal
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 633190 649319 651812 653099
  Show dependency treegraph
 
Reported: 2011-02-07 20:30 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
bzbarsky: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (19.78 KB, patch)
2011-04-11 17:34 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v2 (59.24 KB, patch)
2011-04-15 04:43 PDT, Raymond Lee [:raymondlee]
ian: review-
Details | Diff | Review
v3 (60.27 KB, patch)
2011-04-21 01:30 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Review
Patch for checkin (60.50 KB, patch)
2011-04-23 21:11 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review
patch for checkin (unrotted) (34.77 KB, patch)
2011-04-25 11:14 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-02-07 20:30:18 PST
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 2011-03-31 10:51:14 PDT
bugspam
Comment 2 Raymond Lee [:raymondlee] 2011-04-11 17:34:27 PDT
Created attachment 525239 [details] [diff] [review]
v1

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d0f69b746cf1
Comment 3 Tim Taubert [:ttaubert] 2011-04-14 09:25:44 PDT
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] 2011-04-14 14:59:30 PDT
Comment on attachment 525239 [details] [diff] [review]
v1

F+ with all those changes.
Comment 5 Raymond Lee [:raymondlee] 2011-04-15 04:43:27 PDT
Created attachment 526219 [details] [diff] [review]
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] 2011-04-15 14:28:24 PDT
Comment on attachment 526219 [details] [diff] [review]
v2

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=ac706d443814
Comment 7 Ian Gilman [:iangilman] 2011-04-20 11:55:37 PDT
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] 2011-04-21 01:30:06 PDT
Created attachment 527499 [details] [diff] [review]
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] 2011-04-21 09:52:57 PDT
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] 2011-04-22 13:13:12 PDT
(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] 2011-04-23 21:11:05 PDT
Created attachment 527979 [details] [diff] [review]
Patch for checkin

Passed Try
http://dev.philringnalda.com/tbpl/?tree=Try&rev=bfed5101e70c
Comment 12 Daniel Holbert [:dholbert] 2011-04-25 10:58:04 PDT
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] 2011-04-25 11:14:07 PDT
Created attachment 528133 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2011-05-03 12:25:53 PDT
Pushed http://hg.mozilla.org/projects/cedar/rev/8c4250c3bbf3
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-04 13:53:29 PDT
http://hg.mozilla.org/mozilla-central/rev/8c4250c3bbf3

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