Closed Bug 578927 Opened 12 years ago Closed 12 years ago

Re-order tabs in a group in Tab Candy doesn't reflect on the tab bar

Categories

(Firefox Graveyard :: Panorama, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: raymondlee, Assigned: raymondlee)

Details

Attachments

(1 file)

Steps to reproduce:
1) Go to tab candy interface
2) Re-order tabs in a group 
3) Zoom into a tab in that group
4) The order of tabs on tab bar isn't in the same order as in the tab candy
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
Attachment #457799 - Flags: review?(ian)
Comment on attachment 457799 [details] [diff] [review]
Patch

Looks good, except the original creation of visibleTabs: it uses "for each", which we've recently discovered is not guaranteed to maintain order. Can you rewrite that to use [].forEach and then apply the patch?
Attachment #457799 - Flags: review?(ian) → review+
Raymond, thanks for removing the for each. Unfortunately now the routine modifies the "tabs" argument directly, which is a bad situation waiting to happen. Can you please fix that?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ah, I wasn't aware that we shouldn't modify the tabs argument passed into the showOnlyTheseTabs()

Patched: 
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/651983832b51

Ian: please mark this bug as resolved if everything is OK.
     1.7 +      var visibleTabs = [];
     1.8 +
     1.9 +      tabs.forEach(function(tab) {
    1.10 +        visibleTabs.push(tab.tab.raw);
    1.11 +      });

Curious, why not

let visibleTabs = tabs.map(function(tab) tab.tab.raw);

?
(In reply to comment #5)
> Ah, I wasn't aware that we shouldn't modify the tabs argument passed into the
> showOnlyTheseTabs()

I think it's a good general rule that you should never modify an argument unless it's an explicit part of the function. In other words, addStuffTo(obj) should modify obj, but tellMeAbout(obj) should not. 

In this particular case, the array passed into showOnlyTheseTabs() is just meant to guide the operation, not be the result of the operation, so modifying it shouldn't be a side effect. In fact, in a number of cases, the routine is called with a group's master list of children, so modifying the array would have disastrous effects. 

Anyway, thanks for the fix... looks good!
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.