Merge lp:~osomon/unity-2d/dnd-reorder-apps into lp:unity-2d/3.0

Proposed by Olivier Tilloy on 2011-03-01
Status: Merged
Approved by: Ugo Riboni on 2011-03-02
Approved revision: 433
Merged at revision: 420
Proposed branch: lp:~osomon/unity-2d/dnd-reorder-apps
Merge into: lp:unity-2d/3.0
Diff against target: 389 lines (+171/-88)
7 files modified
launcher/AutoScrollingListView.qml (+16/-2)
launcher/Launcher.qml (+1/-73)
launcher/LauncherItem.qml (+41/-8)
launcher/LauncherList.qml (+18/-2)
launcher/ListViewDragAndDrop.qml (+90/-0)
launcher/UnityApplications/launcherapplicationslist.h (+3/-2)
launcher/UnityApplications/listaggregatormodel.h (+2/-1)
To merge this branch: bzr merge lp:~osomon/unity-2d/dnd-reorder-apps
Reviewer Review Type Date Requested Status
Ugo Riboni (community) 2011-03-01 Needs Information on 2011-03-01
Review via email: mp+51720@code.launchpad.net

Commit message

[launcher] Fix a number of issues with drag’n’drop to re-order applications:

 - Animate the scale of items upon insertion/removal (this fixes a regression).
 - Do not animate the position of items during insertion/removal.
 - Delay the animation on y to when the item has been initially positioned.
 - Activate auto-scroll when a drag is in progress.
 - Lower the threshold for long presses to 500ms.
 - Various style tweaks.

Description of the change

This branch fixes a number of issues in the early implementation of drag’n’drop to re-order applications that was merged last week in time for feature freeze. It also addresses some style issues raised by Ugo during his previous review (see https://code.launchpad.net/~osomon/unity-2d/dnd-reorder-apps/+merge/50738).

Those are the highlights of unmerged changes:

 - Animate the scale of items upon insertion/removal (this fixes a regression).
 - Do not animate the position of items during insertion/removal.
 - Delay the animation on y to when the item has been initially positioned, so that items entering at the bottom of the list don’t seem to be popping out from the top.
 - Activate auto-scroll when a drag is in progress.
 - Lower the threshold for long presses to 500ms (this can be tweaked at will).

Additionally, the changes include minor style tweaks.

There is one remaining issue that can be observed when flicking the list fast. I spent some time investigating it and commented on my findings in a FIXME in the code. I’ll file a bug report to track it.

To post a comment you must log in.
lp:~osomon/unity-2d/dnd-reorder-apps updated on 2011-03-01
428. By Olivier Tilloy on 2011-03-01

Reverted the changes to the French po file that were accidentally introduced at revision 418.

Olivier Tilloy (osomon) wrote :

I filed bug #727082 to track the remaining issue that appears when flicking the list fast.

Ugo Riboni (uriboni) wrote :

One functional issue that I noticed is the following: if I hold the mouse button over the Workspaces icon for more than 500ms then move it to any application and release it, the application will be activated.
This is probably easily fixed by checking that the press and release when activating a tile happen inside the same tile.

Ugo Riboni (uriboni) wrote :

I noticed that the DND code could be much better separated from everything else (this was something I missed in my original review, to be honest).

Essentially there's a MouseArea with id "dnd" that is defined in Launcher.qml and it is referenced in many places, including LauncherItem.qml and AutoScrollListView.qml
While this works ok since QML scoping rules are pretty loose, in my opinion it is abusing a little bit too much this looseness, and makes it harder for people to figure out where this 'dnd' component is defined and change things later.

I made a branch (based on the branch in this MR) where the DND code is pretty much entirely separated and where we need it in other components to workaround QT issues is more clearly explained by comments. You can find it at: lp:~uriboni/unity-2d/separate-dnd-code
I'm not sure if it's 100% working but after some testing I have not identified any regressions.
Can you please have a look and let me know if you think it makes sense to change your code in that way or a similar one ?

Ugo Riboni (uriboni) wrote :

35 + // FIXME: flicking the list fast exhibits unpleasant visual artifacts:
36 + // the y coordinate of the looseItems is correct, however they are not
37 + // re-drawn at the correct position until the mouse cursor is moved
38 + // further. This may be a bug in QML.

Please add a reference to bug 727082 now that it's reported, for easier tracking.

review: Needs Fixing
Ugo Riboni (uriboni) wrote :

in LauncherItem.qml

193 + && !item.parent.parent.moving
194 + && !item.parent.parent.autoScrolling

I think you can generally replace item.parent.parent with ListView.view to get the list that contains this item.

review: Needs Fixing
Ugo Riboni (uriboni) wrote :

201 +
202 + /* Delay the animation on y to when the item has been initially positioned. */
203 + Timer {
204 + id: canAnimateY
205 + triggeredOnStart: true
206 + onTriggered: {
207 + stop()
208 + looseItem.animateY = true
209 + }
210 + }
211 + Component.onCompleted: canAnimateY.start()

First off, this seems like a rather contrived way to fire the timer immediately and only once. Wouldn't have been simpler to just make interval: 0 and not set triggeredOnStart and stop() inside the trigger function ?

Second, do I understand it correctly that what you basically need is to activate the animation on the next iteration of the event loop after Component.onCompleted, and that you need to do this since during Component.onCompleted the Y of the component is still not positioned ?

review: Needs Information
Ugo Riboni (uriboni) wrote :

Also, it seems that Florian has some code to further simplify the DND code: https://code.launchpad.net/~fboucault/unity-2d/drag_drop_qml_item
Specifically, commits 393 and 394.

It would be interesting to see if it's possible to merge these changes too, if you think that they make sense (I have not looked at them much yet, as Florian pointed them out to me just a few minutes ago)

review: Needs Information
lp:~osomon/unity-2d/dnd-reorder-apps updated on 2011-03-01
429. By Olivier Tilloy on 2011-03-01

Do not forward a click on mouse released if the tile below the mouse cursor is not the same as when the mouse was pressed.

430. By Olivier Tilloy on 2011-03-01

Add a reference to bug #727082 in the FIXME.

431. By Olivier Tilloy on 2011-03-01

Replaced item.parent.parent by the ListView.view attached property.

Olivier Tilloy (osomon) wrote :

> First off, this seems like a rather contrived way to fire the timer
> immediately and only once. Wouldn't have been simpler to just make interval: 0
> and not set triggeredOnStart and stop() inside the trigger function ?

This doesn’t work. When the interval is 0, it looks like the trigger is pulled immediately, not in the next iteration of the event loop. It would work with an interval of 1, but then it doesn’t look any less contrived than setting triggeredOnStart to true.

> Second, do I understand it correctly that what you basically need is to
> activate the animation on the next iteration of the event loop after
> Component.onCompleted, and that you need to do this since during
> Component.onCompleted the Y of the component is still not positioned ?

Correct, that’s the reason for this timer code. It may clumsy though, and any suggestion to achieve this in a cleaner way is appreciated.

lp:~osomon/unity-2d/dnd-reorder-apps updated on 2011-03-01
432. By Olivier Tilloy on 2011-03-01

Merged Ugo’s branch that better isolates the drag’n’drop code.

Olivier Tilloy (osomon) wrote :

> Also, it seems that Florian has some code to further simplify the DND code:
> https://code.launchpad.net/~fboucault/unity-2d/drag_drop_qml_item
> Specifically, commits 393 and 394.
>
> It would be interesting to see if it's possible to merge these changes too, if
> you think that they make sense (I have not looked at them much yet, as Florian
> pointed them out to me just a few minutes ago)

I will have a look at them to determine if/how they can help simplify the code.
The rest of your comments have been addressed (including merging your code re-organisation that makes sense and seems to work, thank you for this).

Ugo Riboni (uriboni) wrote :

> > First off, this seems like a rather contrived way to fire the timer
> > immediately and only once. Wouldn't have been simpler to just make interval:
> 0
> > and not set triggeredOnStart and stop() inside the trigger function ?
>
> This doesn’t work. When the interval is 0, it looks like the trigger is pulled
> immediately, not in the next iteration of the event loop. It would work with
> an interval of 1, but then it doesn’t look any less contrived than setting
> triggeredOnStart to true.

It would be less code, but yes, I agree on what you say, it would still be an hack.

> > Second, do I understand it correctly that what you basically need is to
> > activate the animation on the next iteration of the event loop after
> > Component.onCompleted, and that you need to do this since during
> > Component.onCompleted the Y of the component is still not positioned ?
>
> Correct, that’s the reason for this timer code. It may clumsy though, and any
> suggestion to achieve this in a cleaner way is appreciated.

The only thing that comes to mind make the intent clearer is to do something with http://doc.qt.nokia.com/latest/qcoreapplication.html#postEvent (which will basically send any QEvent derived class to any QObject derived class on the next event loop iteration).
I wish I had any better suggestion.

Olivier Tilloy (osomon) wrote :

> The only thing that comes to mind make the intent clearer is to do something
> with http://doc.qt.nokia.com/latest/qcoreapplication.html#postEvent (which
> will basically send any QEvent derived class to any QObject derived class on
> the next event loop iteration).
> I wish I had any better suggestion.

So that would mean messing with C++, i.e. more lines of code and a less confined hack.
Do we agree that the current solution (Timer with triggeredOnStart), even if not fully satisfactory, will do for now, until we come up with a more elegant one? I can document the hack more extensively if that’s a concern.

> I will have a look at them to determine if/how they can help simplify the
> code.
> The rest of your comments have been addressed (including merging your code re-
> organisation that makes sense and seems to work, thank you for this).

So I discussed it very briefly with Florian yesterday, and his code in its current state only supports incoming drag and drop events, not initiating new drag events, which would be needed if we were to reimplement this functionality.
Those two branches will be better processed separately, and we may choose to refactor this one at some point in the future. In the meantime, it should be ready for a new (hopefully last) round of review.

Ugo Riboni (uriboni) wrote :

> > The only thing that comes to mind make the intent clearer is to do something
> > with http://doc.qt.nokia.com/latest/qcoreapplication.html#postEvent (which
> > will basically send any QEvent derived class to any QObject derived class on
> > the next event loop iteration).
> > I wish I had any better suggestion.
>
> So that would mean messing with C++, i.e. more lines of code and a less
> confined hack.
> Do we agree that the current solution (Timer with triggeredOnStart), even if
> not fully satisfactory, will do for now, until we come up with a more elegant
> one? I can document the hack more extensively if that’s a concern.

Yes please, add some more documentation explaining that what you're really trying to do there is just run something on the next event loop iteration, and that should be enough for now.

> > I will have a look at them to determine if/how they can help simplify the code.
> > The rest of your comments have been addressed (including merging your code re-
> > organisation that makes sense and seems to work, thank you for this).
>
> So I discussed it very briefly with Florian yesterday, and his code in its
> current state only supports incoming drag and drop events, not initiating new
> drag events, which would be needed if we were to reimplement this functionality.
> Those two branches will be better processed separately, and we may choose to
> refactor this one at some point in the future. In the meantime, it should be
> ready for a new (hopefully last) round of review.

I checked it and it looks good to me. Add the comment above while I do a last round of functional review and we should be good to close this and move on :)

lp:~osomon/unity-2d/dnd-reorder-apps updated on 2011-03-02
433. By Olivier Tilloy on 2011-03-02

Additional comment to explain the hack that enables the animation only after the item has taken its initial position.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/AutoScrollingListView.qml'
2--- launcher/AutoScrollingListView.qml 2011-02-15 19:15:21 +0000
3+++ launcher/AutoScrollingListView.qml 2011-03-02 10:36:45 +0000
4@@ -59,7 +59,7 @@
5 property: "contentY"
6 to: 0 - paddingTop
7 velocity: autoScrollVelocity
8- running: scrollZoneTop.containsMouse
9+ running: scrollZoneTop.containsMouse || draggingOnScrollZoneTop
10 }
11
12 SmoothedAnimation {
13@@ -68,7 +68,8 @@
14 property: "contentY"
15 to: contentHeight + paddingBottom - height
16 velocity: autoScrollVelocity
17- running: scrollZoneBottom.containsMouse && contentHeight + paddingBottom > height
18+ running: (scrollZoneBottom.containsMouse && contentHeight + paddingBottom > height)
19+ || draggingOnScrollZoneBottom
20 }
21
22 /* The code below this comment is only needed as a workaround for a strange behavior
23@@ -131,4 +132,17 @@
24 var item = list.contentItem.childAt(point.x, point.y)
25 if (item && typeof(item.clicked) == "function") item.clicked(mouse)
26 }
27+
28+ /* If drag and drop reordering is enabled for this list, this will not
29+ be null. Normally we could keep autoscrolling and drag and drop reordering
30+ entirely separated, but due to the QT issue explained above we can't let
31+ the mouse events "bubble down" from the d'n'd MouseArea to the autoscroll
32+ MouseAreas, so we need a workaround like this one. */
33+ property variant dragAndDrop: null
34+ property bool draggingOnScrollZoneTop: dragAndDrop != null && dragAndDrop.draggedTileId != "" &&
35+ dragAndDrop.mouseY >= scrollZoneTop.y &&
36+ dragAndDrop.mouseY <= scrollZoneTop.y + autoScrollSize
37+ property bool draggingOnScrollZoneBottom: dragAndDrop != null && dragAndDrop.draggedTileId != "" &&
38+ dragAndDrop.mouseY >= scrollZoneBottom.y &&
39+ dragAndDrop.mouseY <= scrollZoneBottom.y + autoScrollSize
40 }
41
42=== modified file 'launcher/Launcher.qml'
43--- launcher/Launcher.qml 2011-02-23 21:52:58 +0000
44+++ launcher/Launcher.qml 2011-03-02 10:36:45 +0000
45@@ -24,83 +24,11 @@
46 paddingBottom: 5
47 autoScrollSize: tileSize / 2
48 autoScrollVelocity: 200
49+ reorderable: true
50
51 model: ListAggregatorModel {
52 id: items
53 }
54-
55- // FIXME: dragging the list to flick it exhibits unpleasant visual
56- // artifacts, and its contentY sometimes remains blocked at a position
57- // too far off the boundaries of the list.
58- MouseArea {
59- /* Handle drag’n’drop to re-order applications. */
60- id: dnd
61- anchors.fill: parent
62-
63- /* id (desktop file path) of the application being dragged */
64- property string currentId: ""
65- /* list index of the application being dragged */
66- property int currentIndex
67- /* absolute mouse coordinates in the list */
68- property variant listCoordinates: mapToItem(main.contentItem, mouseX, mouseY)
69- /* list index of the application underneath the cursor */
70- property int index: main.indexAt(listCoordinates.x, listCoordinates.y)
71-
72- onPressed: {
73- /* index is not valid yet because the mouse area is not
74- sensitive to hovering (if it were, it would eat hover events
75- for other mouse areas below, which is not desired). */
76- var coord = mapToItem(main.contentItem, mouse.x, mouse.y)
77- currentIndex = main.indexAt(coord.x, coord.y)
78- }
79- onPressAndHold: {
80- if (index != currentIndex) {
81- /* The item under the cursor changed since the press. */
82- return
83- }
84- parent.interactive = false
85- var id = items.get(currentIndex).desktop_file
86- if (id != undefined) currentId = id
87- }
88- function drop() {
89- currentId = ""
90- parent.interactive = true
91- }
92- onReleased: drop()
93- onExited: drop()
94- onMousePositionChanged: {
95- if (currentId != "" && index != -1 && index != currentIndex) {
96- /* Workaround a bug in QML whereby moving an item down in
97- the list results in its visual representation being
98- shifted too far down by one index
99- (http://bugreports.qt.nokia.com/browse/QTBUG-15841).
100- Since the bug happens only when moving an item *down*,
101- and since moving an item one index down is strictly
102- equivalent to moving the item below one index up, we
103- achieve the same result by tricking the list model into
104- thinking that the mirror operation was performed.
105- Note: this bug will be fixed in Qt 4.7.2, at which point
106- this workaround can go away. */
107- if (index > currentIndex) {
108- items.move(index, currentIndex, 1)
109- } else {
110- /* This should be the only code path here, if it wasn’t
111- for the bug explained and worked around above. */
112- items.move(currentIndex, index, 1)
113- }
114- currentIndex = index
115- }
116- }
117- onClicked: {
118- /* Forward the click to the launcher item below. */
119- var point = mapToItem(main.contentItem, mouse.x, mouse.y)
120- var item = main.contentItem.childAt(point.x, point.y)
121- /* FIXME: the coordinates of the mouse event forwarded are
122- incorrect. Luckily, it’s acceptable as they are not used in
123- the handler anyway. */
124- if (item && typeof(item.clicked) == "function") item.clicked(mouse)
125- }
126- }
127 }
128
129 LauncherList {
130
131=== modified file 'launcher/LauncherItem.qml'
132--- launcher/LauncherItem.qml 2011-02-24 01:13:42 +0000
133+++ launcher/LauncherItem.qml 2011-03-02 10:36:45 +0000
134@@ -61,6 +61,9 @@
135 property alias shortcutVisible: shortcut.visible
136 property alias shortcutText: shortcutText.text
137
138+ property bool isBeingDragged: false
139+ property int dragPosition
140+
141 property int pips: 0
142 property string pipSource: engineBaseUrl + "artwork/launcher_" +
143 ((pips <= 1) ? "arrow" : "pip") + "_ltr.png"
144@@ -85,9 +88,18 @@
145 width: item.width
146 height: item.height
147 x: item.x
148- /* item.parent is the delegate, and its parent is the LauncherList */
149- y: item.parent.parent.y - item.parent.parent.contentY + item.y
150- z: item.parent.parent.itemZ
151+ y: ListView.view.y - ListView.view.contentY + item.y
152+ z: ListView.view.itemZ
153+
154+ /* Bind to the scale of the delegate so that it is animated upon insertion/removal */
155+ scale: item.scale
156+
157+ /* The y coordinate is initially not animated, as it would result in an
158+ unwanted effect of every single item popping out from the top of the
159+ launcher (even when they are supposed to be coming from the bottom).
160+ This property is later set to true once the item has taken its
161+ initial position. */
162+ property bool animateY: false
163
164 /* This is the arrow shown at the right of the tile when the application is
165 the active one */
166@@ -335,21 +347,42 @@
167
168 states: State {
169 name: "beingDragged"
170- when: (dnd.currentId != "") && (dnd.currentId == item.desktopFile)
171+ when: item.isBeingDragged
172 PropertyChanges {
173 target: looseItem
174- /* item.parent is the delegate, and its parent is the LauncherList */
175- y: dnd.listCoordinates.y - item.parent.parent.contentY - tile.height / 2
176+ y: item.dragPosition - tile.height / 2
177 /* When dragging an item, stack it on top of all its siblings */
178 z: 1
179 }
180 }
181 Behavior on y {
182- enabled: (looseItem.state != "beingDragged") && !item.parent.parent.moving && !item.parent.parent.autoScrolling
183+ enabled: /* do not animate during initial positioning */
184+ looseItem.animateY
185+ /* do not animate while dragging to re-order applications */
186+ && (looseItem.state != "beingDragged")
187+ /* do not animate during insertion/removal */
188+ && (looseItem.scale == 1)
189+ /* do not animate while flicking the list */
190+ && !ListView.view.moving
191+ && !ListView.view.autoScrolling
192 NumberAnimation {
193- duration: 400
194+ duration: 250
195 easing.type: Easing.OutBack
196 }
197 }
198+
199+ /* Delay the animation on y to when the item has been initially positioned. */
200+ Timer {
201+ id: canAnimateY
202+ /* This ensures that the trigger will be executed in the next
203+ iteration of the event loop, at which point the item will have
204+ taken its initial position. */
205+ triggeredOnStart: true
206+ onTriggered: {
207+ stop()
208+ looseItem.animateY = true
209+ }
210+ }
211+ Component.onCompleted: canAnimateY.start()
212 }
213 }
214
215=== modified file 'launcher/LauncherList.qml'
216--- launcher/LauncherList.qml 2011-02-24 01:13:42 +0000
217+++ launcher/LauncherList.qml 2011-03-02 10:36:45 +0000
218@@ -18,6 +18,19 @@
219 /* A hint for items to determine the value of their 'z' property */
220 property real itemZ: 0
221
222+ /* Can we reorder the items in this list by means of drag and drop ? */
223+ property alias reorderable: reorder.enabled
224+
225+ ListViewDragAndDrop {
226+ id: reorder
227+ list: list
228+ enabled: false
229+ }
230+
231+ /* FIXME: We need this only to workaround a problem in QT's MouseArea
232+ event handling. See AutoScrollingListView for details. */
233+ dragAndDrop: (reorder.enabled) ? reorder : null
234+
235 delegate: LauncherItem {
236 id: launcherItem
237
238@@ -44,6 +57,9 @@
239 index <= 9 && launcherView.superKeyPressed
240 shortcutText: index + 1
241
242+ isBeingDragged: (reorder.draggedTileId != "") && (reorder.draggedTileId == desktopFile)
243+ dragPosition: reorder.listCoordinates.y - list.contentY
244+
245 /* Best way I could find to check if the item is an application or the
246 workspaces switcher. There may be something cleaner and better. */
247 backgroundFromIcon: item.toString().indexOf("LauncherApplication") == 0 ||
248@@ -101,9 +117,9 @@
249 }
250
251 Connections {
252- target: dnd
253+ target: reorder
254 /* Hide the tooltip/menu when dragging an application. */
255- onCurrentIdChanged: if (dnd.currentId != "") item.menu.hide()
256+ onDraggedTileIdChanged: if (reorder.draggedTileId != "") item.menu.hide()
257 }
258
259 function setIconGeometry() {
260
261=== added file 'launcher/ListViewDragAndDrop.qml'
262--- launcher/ListViewDragAndDrop.qml 1970-01-01 00:00:00 +0000
263+++ launcher/ListViewDragAndDrop.qml 2011-03-02 10:36:45 +0000
264@@ -0,0 +1,90 @@
265+import Qt 4.7
266+
267+/* When added as a child of a ListView and the listview itself is set to the
268+ 'list' property it will make it possible to use drag’n’drop to re-order
269+ the items in the list. */
270+
271+// FIXME: flicking the list fast exhibits unpleasant visual artifacts:
272+// the y coordinate of the looseItems is correct, however they are not
273+// re-drawn at the correct position until the mouse cursor is moved
274+// further. This may be a bug in QML.
275+// Ref: https://bugs.launchpad.net/unity-2d/+bug/727082.
276+MouseArea {
277+ id: dnd
278+ anchors.fill: parent
279+
280+ property variant list
281+
282+ /* list index of the tile being dragged */
283+ property int draggedTileIndex
284+ /* id (desktop file path) of the tile being dragged */
285+ property string draggedTileId: ""
286+ /* absolute mouse coordinates in the list */
287+ property variant listCoordinates: mapToItem(list.contentItem, mouseX, mouseY)
288+ /* list index of the tile underneath the cursor */
289+ property int tileAtCursorIndex: list.indexAt(listCoordinates.x, listCoordinates.y)
290+
291+ Timer {
292+ id: longPressDelay
293+ /* The standard threshold for long presses is hard-coded to 800ms
294+ (http://doc.qt.nokia.com/qml-mousearea.html#onPressAndHold-signal).
295+ This value is too high for our use case. */
296+ interval: 500 /* in milliseconds */
297+ onTriggered: {
298+ if (list.moving) return
299+ dnd.parent.interactive = false
300+ var id = items.get(dnd.draggedTileIndex).desktop_file
301+ if (id != undefined) dnd.draggedTileId = id
302+ }
303+ }
304+ onPressed: {
305+ /* tileAtCursorIndex is not valid yet because the mouse area is
306+ not sensitive to hovering (if it were, it would eat hover
307+ events for other mouse areas below, which is not desired). */
308+ var coord = mapToItem(list.contentItem, mouse.x, mouse.y)
309+ draggedTileIndex = list.indexAt(coord.x, coord.y)
310+ longPressDelay.start()
311+ }
312+ function drop() {
313+ longPressDelay.stop()
314+ draggedTileId = ""
315+ parent.interactive = true
316+ }
317+ onReleased: {
318+ if (draggedTileId != "") {
319+ drop()
320+ } else if (draggedTileIndex == tileAtCursorIndex) {
321+ /* Forward the click to the launcher item below. */
322+ var point = mapToItem(list.contentItem, mouse.x, mouse.y)
323+ var item = list.contentItem.childAt(point.x, point.y)
324+ /* FIXME: the coordinates of the mouse event forwarded are
325+ incorrect. Luckily, it’s acceptable as they are not used in
326+ the handler anyway. */
327+ if (item && typeof(item.clicked) == "function") item.clicked(mouse)
328+ }
329+ }
330+ onExited: drop()
331+ onPositionChanged: {
332+ if (draggedTileId != "" && tileAtCursorIndex != -1 && tileAtCursorIndex != draggedTileIndex) {
333+ /* Workaround a bug in QML whereby moving an item down in
334+ the list results in its visual representation being
335+ shifted too far down by one index
336+ (http://bugreports.qt.nokia.com/browse/QTBUG-15841).
337+ Since the bug happens only when moving an item *down*,
338+ and since moving an item one index down is strictly
339+ equivalent to moving the item below one index up, we
340+ achieve the same result by tricking the list model into
341+ thinking that the mirror operation was performed.
342+ Note: this bug will be fixed in Qt 4.7.2, at which point
343+ this workaround can go away. */
344+ if (tileAtCursorIndex > draggedTileIndex) {
345+ items.move(tileAtCursorIndex, draggedTileIndex, 1)
346+ } else {
347+ /* This should be the only code path here, if it wasn’t
348+ for the bug explained and worked around above. */
349+ items.move(draggedTileIndex, tileAtCursorIndex, 1)
350+ }
351+ draggedTileIndex = tileAtCursorIndex
352+ }
353+ }
354+}
355
356=== modified file 'launcher/UnityApplications/launcherapplicationslist.h'
357--- launcher/UnityApplications/launcherapplicationslist.h 2011-02-22 11:03:48 +0000
358+++ launcher/UnityApplications/launcherapplicationslist.h 2011-03-02 10:36:45 +0000
359@@ -41,11 +41,12 @@
360 QVariant data(const QModelIndex & index, int role = Qt::DisplayRole) const;
361 int rowCount(const QModelIndex & parent = QModelIndex()) const;
362
363- Q_INVOKABLE void move(int from, int to);
364-
365 Q_INVOKABLE void insertFavoriteApplication(QString desktop_file);
366 Q_INVOKABLE void insertWebFavorite(const QUrl& url);
367
368+public Q_SLOTS:
369+ void move(int from, int to);
370+
371 private:
372 void load();
373 void insertBamfApplication(BamfApplication* bamf_application);
374
375=== modified file 'launcher/UnityApplications/listaggregatormodel.h'
376--- launcher/UnityApplications/listaggregatormodel.h 2011-02-16 22:17:56 +0000
377+++ launcher/UnityApplications/listaggregatormodel.h 2011-03-02 10:36:45 +0000
378@@ -41,9 +41,10 @@
379 to a QAbstractListModel */
380 Q_INVOKABLE void appendModel(const QVariant& model);
381
382+public Q_SLOTS:
383 /* Move one item from one position to another position.
384 The item must remain in the same model. */
385- Q_INVOKABLE void move(int from, int to);
386+ void move(int from, int to);
387
388 protected:
389 QList<QAbstractListModel*> m_models;

Subscribers

People subscribed via source and target branches