Unify setActive... functions

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: mitcho, Assigned: raymondlee)

Tracking

unspecified
Firefox 6
Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

(Whiteboard: [cleanup][API])

Attachments

(1 attachment, 4 obsolete attachments)

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

6 years ago
bugspam
Target Milestone: Future → ---
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 525239 [details] [diff] [review]
v1

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d0f69b746cf1
Attachment #525239 - Flags: feedback?(tim.taubert)
Summary: Unify setActive... functions? → Unify setActive... functions
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)
Attachment #525239 - Flags: feedback?(tim.taubert)
Comment on attachment 525239 [details] [diff] [review]
v1

F+ with all those changes.
Attachment #525239 - Flags: feedback+
(Assignee)

Comment 5

6 years ago
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.
Attachment #525239 - Attachment is obsolete: true
Attachment #526219 - Flags: review?(ian)
Blocks: 649319
(Assignee)

Comment 6

6 years ago
Comment on attachment 526219 [details] [diff] [review]
v2

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

Updated

6 years ago
Blocks: 633190
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.
Attachment #526219 - Flags: review?(ian) → review-
(Assignee)

Updated

6 years ago
Blocks: 651812
(Assignee)

Comment 8

6 years ago
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
Attachment #526219 - Attachment is obsolete: true
Attachment #527499 - Flags: review?(ian)
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!
Attachment #527499 - Flags: review?(ian) → review+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 527979 [details] [diff] [review]
Patch for checkin

Passed Try
http://dev.philringnalda.com/tbpl/?tree=Try&rev=bfed5101e70c
Attachment #527499 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Patch doesn't apply cleanly to mozilla-central now, after this cset:
  http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78
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 :)
Attachment #527979 - Attachment is obsolete: true
No longer blocks: 603789
Blocks: 653099
Pushed http://hg.mozilla.org/projects/cedar/rev/8c4250c3bbf3
Keywords: checkin-needed
Whiteboard: [cleanup][API] → [cleanup][API][fixed-in-cedar]
Target Milestone: --- → Firefox 6
http://hg.mozilla.org/mozilla-central/rev/8c4250c3bbf3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [cleanup][API][fixed-in-cedar] → [cleanup][API]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.