616729 - rearranging tabs in panorama won't match tab ordering on tab bar

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[blindskull13]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b8pre) Gecko/20101203 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b8pre) Gecko/20101203 Firefox/4.0b8pre

when trying to rearrange your tabs in panorama view, the tabs on the tab bar are not effected Ex. they don't rearrange to match panorama 

Reproducible: Always

Steps to Reproduce:
1. have at least three tabs open 
2. open panorama
3. move some of the tabs in panorama around in there group
4. go back to the browser view
Actual Results:  
the tabs in the tab bar do not reflect the ordering in panorama

Expected Results:  
both the tabs on the tab bar, and the tabs in panorama should have reflected the "last changed" ordering

i have reproduced this on two different machines running the same OS

Comment 1

[blindskull13]

yes, i realize you cant efficiently arrange tabs at the moment, but when you do move them in panorama, they should be able to reflect the move on the tab bar, at this current time, they don't, even when a tab only moves to the first position

Comment 2

[Kevin Hanes]

Whacky, this is an unfortunate regression that I am seeing on win7 as well. When bug 587503 lands, we'll be able to debug this much more easily.

Nominating for blocking; this isn't data loss, but it's data loss-ey.

Comment 4

[Dietrich Ayala (:dietrich)]

This is in an interesting class of bugs that i consider "state loss". There's no real data loss, but there is loss of effort expended by the user at some point in time.

Because there's no real data loss, and the workaround is not unreasonable, blocking-.

Comment 5

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 6

[Kevin Hanes]

bugspam (removing b9)

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Pushed to try.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2 (refactored)

Refactored and added some small corrections for test for bug 586553 that covers the same area.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2b (small fixes in head.js)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Patch v2 passed try.

Comment 13

[Tomas Janirek]

Hi,
I have exactly the same problem since tabcandy was first previewed (6+ months ago).
If you recreate closed tab (CTRL+SHIFT+T), the new panorama window is created, this is annoying, but not that bad... If you move this tab to main panorama window, all tabs are rearanged, which is very serious (and of course tabs in browser no more reflect panorama ordering - until restart of firefox). I'm used to have plenty of tabs open and I know exactly where they are and this is just crazy.
Is there any way how to disable panorama at all? Is there any way I can merge easily the patch above into firefox?
4.0b9

T.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> If you recreate closed tab (CTRL+SHIFT+T), the new panorama window is created,
> this is annoying, but not that bad...

This is bug 624265 which has been fixed in the nightly, and will be in beta 10.

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 504310 [details] [diff] [review]
patch v2b (small fixes in head.js)

Very nice. Comments: 

>+      let start = index ? indices[index-1] : -1;
>+      let end = index+1 < indices.length ? indices[index+1] : Infinity;
>+      let targetRange = new Range(start, end);
>+
>+      if (!targetRange.contains(tabItem.tab._tPos)) {
>+        gBrowser.moveTabTo(tabItem.tab, start+1);

The only reason we can get away with start being indices[index - 1] is because we know that tab's already taken that index, so we won't have it. It seems like it would be a little clearer if start was indices[index - 1] + 1, like so: 

      let start = index ? (indices[index - 1] + 1) : 0;
      let end = index + 1 < indices.length ? (indices[index + 1] - 1) : Infinity;
      let targetRange = new Range(start, end);

      if (!targetRange.contains(tabItem.tab._tPos)) {
        gBrowser.moveTabTo(tabItem.tab, start);
        
It works either way (since there are no duplicated indices), but I feel this is a little cleaner. What do you think?

>+    tests.push([]);

It took me a moment to realize this was your clever way to delete that group. Probably deserves a comment to that effect.

R+ with those addressed

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to comment #15)
> It works either way (since there are no duplicated indices), but I feel this is
> a little cleaner. What do you think?

I had in mind that there are no duplicate indices while writing but anyone reading this could not - so this is really a bit cleaner, good catch.

> It took me a moment to realize this was your clever way to delete that group.
> Probably deserves a comment to that effect.

Well, I wanted to test how empty groups behave, too. But yeah a neat side-effect was that the group disappeared :) I added comment to clarify this.

Comment 17

[Dietrich Ayala (:dietrich)]

Thanks for pushing this forward Tim. Are the changes in the latest patch substantial enough to require another tryserver run?

Comment 18

[Tomas Janirek]

minefield 4.0b10pre (2011-01-19)
still not working correctly:(

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #17)
> Thanks for pushing this forward Tim. Are the changes in the latest patch
> substantial enough to require another tryserver run?

I pushed to try yesterday. There was a leak on Win (debug). I pushed again (win only) to see if this is temporary.

Comment 20

[[not reading bugmail]]

(In reply to comment #18)
> minefield 4.0b10pre (2011-01-19)
> still not working correctly:(

Tomas, that is expected this this bug is resolved.  So please wait till we know when to check the nightly.

Comment 21

[[not reading bugmail]]

(In reply to comment #20)
> (In reply to comment #18)
> > minefield 4.0b10pre (2011-01-19)
> > still not working correctly:(
> 
> Tomas, that is expected this this bug is resolved. 

That should say, until this bug is resolved.  Sorry for the spam.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

This seems to permanently leak on win32 :(

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Added some small optimizations to the algorithm. Hopefully this does not leak anymore. Pushed to try again.

Comment 25

[Julian Seward [:jseward]]

(In reply to comment #4)
> Because there's no real data loss, and the workaround is not unreasonable,
> blocking-.

I disagree.  IMO it would be a mistake to release 4 without fixing
this.  It blemishes what is otherwise a killer new feature.

I find it confusing.  The worst thing is that, because the order of
thumbnails and tabs is currently not reliably connected, it made me
think for a while that my mental model of how the user interface works
(in this respect) is missing some crucial detail.  It makes it appear
that there's some magic other piece of user-interface state that
controls the mapping, and if I just tried harder I could find out what
I need to do to access it.  Whereas, in reality, there isn't.

I believe this will confuse users en masse.

It also makes Panorama less useful.  I want to rearrange my tabs for a
reason -- to make finding them easier -- and at the moment I simply
can't do that reliably.

Comment 26

[Michael Yoshitaka Erlewine [:mitcho]]

Approval, please.

Comment 27

[Julian Seward [:jseward]]

(In reply to comment #23)
> Created attachment 505323 [details] [diff] [review]
> patch v4

Works for me! (x64-linux m-c build).

Comment 28

[Tomas Janirek]

(In reply to comment #25)
> (In reply to comment #4)
> > Because there's no real data loss, and the workaround is not unreasonable,
> > blocking-.
> 
> I disagree.  IMO it would be a mistake to release 4 without fixing
> this.  It blemishes what is otherwise a killer new feature.
> 
> I find it confusing.  The worst thing is that, because the order of
> thumbnails and tabs is currently not reliably connected, it made me
> think for a while that my mental model of how the user interface works
> (in this respect) is missing some crucial detail.  It makes it appear
> that there's some magic other piece of user-interface state that
> controls the mapping, and if I just tried harder I could find out what
> I need to do to access it.  Whereas, in reality, there isn't.
> 
> I believe this will confuse users en masse.
> 
> It also makes Panorama less useful.  I want to rearrange my tabs for a
> reason -- to make finding them easier -- and at the moment I simply
> can't do that reliably.

Can't agree more. This all panorama thing is now just useless, every time I move the tab, it's all mixed up. The whole idea was to make things more simple, visual and efficient, unfortunately now it's just the opposite:( Hopefully there will be an option to disable it completely, otherwise I'll have to stick with F3 until this is resolved.

To Julian Seward: 21-Jan-2011 06:32 (win64) - still not working.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #26)
> Approval, please.

The trick is to poke specific people about approval and/or make sure they are CC'ed. I'm guessing the approval queue is not being triaged as actively as you might think.

CCing dolske to come try and beat dietrich and beltzner to setting the approval flag.

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

CCing dolske for real this time...

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Passed try (only hit bug 626956).

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b38ba02fdeb4

Comment 34

[Tracy Walker [:tracy]]

verified with nightly build of minefield.