522610 - [AeroPeek] Dragging tabs sometimes does not update previews order correctly

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Marco Bonardo [:mak]]

If i drag tabs sometimes the preview goes out of sync

no fixed steps but usually i can reproduce with 3 tabs, dragging the last one in the middle.

1.with A B C, drag A to B -> BAC
2. then continue to drag the last tab on the right in the middle

it should go out of sync after 2 or 3 drags

Comment 1

[[not reading bugmail]]

Attachment: Sample out of sync screenshot, with more bugs noticed.

On 2009-10-28 trunk..

This seems like the same issue we have when closing tabs from aero peek previews instead of drag-n-drop tabs; the previews and tabs become out of sync as bug 522305.  

Basically if changes to tab order occur or tabs are closed, or more are added and you switch tabs using the previews, the preview sync with the browser becomes out of order and seems to trigger the busy loading bugs on file (see screenshot).

The ITaskBar3 interface should provide the API needed to manage moving tabs.

Comment 2

[Rob Arnold [:robarnold]]

Toggling the pref seems to fix this so I think we are not updating the tab positions on move. However, we currently use the ITaskbarList3 interface to set the entire window's ordering on tab move so I'm not sure what's going on yet.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

i think i got what it is.
1. suppose you have tabs: A B C D
2. you move A after D, you expect B C D A
3. the code receives TabMove event.
4. at this point you order your local previews array, it becomes B C D A
5. then you call updateTabOrdering
6. this walks your local array and send these notifications:
  A. move B before C
  B. move C before D
  C. move D before A
  D. move A before the end
7. Notice the array in the taskbar controller is still A B C D since we are updating it right now
8. Apply you transformations to the array in the controller, end up with: D B C A

actually the fix is pretty trivial, just walk the array in reverse order, so you will really force the remote array ordering based on your local array.

Comment 4

[Marco Bonardo [:mak]]

Rob told me there is no way to read from the taskbar directly, so this cannot be test automatically :(

Comment 5

[Rob Arnold [:robarnold]]

Comment on attachment 424645 [details] [diff] [review]
patch v1.0

Appears to work for me, thanks! I'm not sure if someone from browser/ needs to review this...

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> (From update of attachment 424645 [details] [diff] [review])
> Appears to work for me, thanks! I'm not sure if someone from browser/ needs to
> review this...

it's so trivial that i feel we are enough.

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[[not reading bugmail]]

Dragging one tab didn't necessarily reorder the rest of my tab previews.  I had a progress circle on one, which was also tied to the wrong tab. I eventually just started reordering all the tabs to match the tab preview order to see if that would help [since I was unable to restart, I been downloading an ISO for VS], but didn't help trigger automatic reorder.

I tested from both ends, usually starting with 4 tabs, randomly trying to insert, delete, close and add tabs and load new ones in random places at a fast clip.  

I'm thinking parsing from the end, backwards, may not be ideal way to solve this problem, since changes can occur from either end and at anytime in the middle.

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> I'm thinking parsing from the end, backwards, may not be ideal way to solve
> this problem, since changes can occur from either end and at anytime in the
> middle.

What's the difference if the change happens in the middle? the local array is ordered, parsing from the end forces the same order in the remote array regardless where the change happened. i think the change is correctly forcing the remote ordering to the local ordering, if the local ordering is wrong for any reason, that's a different question.
If you have maths to demonstrate the backwords parsing is bad in some case, please bring them.

Comment 10

[[not reading bugmail]]

(In reply to comment #9)
> (In reply to comment #8)
> > I'm thinking parsing from the end, backwards, may not be ideal way to solve
> > this problem, since changes can occur from either end and at anytime in the
> > middle.
> 
> What's the difference if the change happens in the middle? the local array is
> ordered, parsing from the end forces the same order in the remote array
> regardless where the change happened. i think the change is correctly forcing
> the remote ordering to the local ordering, if the local ordering is wrong for
> any reason, that's a different question.
> If you have maths to demonstrate the backwords parsing is bad in some case,
> please bring them.

I've not run into a case where this currently hasn't worked. 

Well, I been thinking about this.. I was thinking that reordering without first doing a list reset or sync in this case, would create a list in which the previews include a bad move or undefined item which are then tried to reorder.  But since your saying there is a master and a copy, as long as the array size includes those items that were not closed, and its synced to the master, reordering works.  I was also thinking based on the example that the ordering would be relative to tab placement, meaning a tab wouldn't be able to reorder if it wasn't in the correct position to be reordered once a reorder starts.  Which I guess is a moot point based on the method its being done.

The only other thing that would improve previews reorders, might be to update invalidation more frequently during other preview events like tab selection, not just a tab move or close, which I think is maybe the bug Rob has open for invalidation.  

I hope we could get this on 1.9.2 soon together with the fix for progress circles.  Since both are needed to make it worth the time on that branch.

Comment 11

[LegNeato]

Comment on attachment 424645 [details] [diff] [review]
patch v1.0

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6fd03fdd13e1