Ho-hum, yeah that might be nice.

Anyone feel like taking a crack at this? Will mark it as gnome-love since it's a pretty self-contained thing to work on. Anyone got any pointer to where to look?

Created attachment 287146 [details] [review]
RFC: Add drag-and-drop reordering of route entries

So with that rfc patch re-ordering sort-of works.

I am not that familiar with the dnd api so it might be done in the wrong way.
But then again, I am not 100% sure we want this. So maybe this can let people try it out and see what they think.

To drag, grab the icon next to the entry and drag it to the row you want to re-order it to.

Created attachment 287147 [details]
screencast of using drag-and-drop

Here is a screencast. It does not really give the whole picture tho. The screencast does not capture the dragging icon, which is the same as the icon next to the entry you drag. And the screencast does not show the little '+' sign that appears when you can drop something.

Created attachment 287149 [details] [review]
RFC: Add drag-and-drop reordering of route entries

The indicator feels a bit too subtle somehow.
Can you drag using the markers, or the whole field?

Cool stuff, and we definitely want this. I'd like some indication that you're actually dragging something though. Basically the whole field (including circle and everything) should follow the mouse. And when the field is over a possible new position the field occupying that place smoothly animates away. 

I hope I'm not dreaming here and that this really is possible. :)

The screencast doesn't really capture the indicators, only the highlight. You drag using the icon-markers. Then you will get an icon by your cursor with that marker that you can drag around. When you are over a row that you can drop on.

The entry on row will get the highlight, as you see in video. But also you get a plus-sign by your cursor, indicating that you can drop. I don't now why the screencast does not capture those things.

(In reply to comment #7)
> Cool stuff, and we definitely want this. I'd like some indication that you're
> actually dragging something though. Basically the whole field (including circle
> and everything) should follow the mouse. And when the field is over a possible
> new position the field occupying that place smoothly animates away. 
> 
> I hope I'm not dreaming here and that this really is possible. :)

It should be possible to have the row follow, by setting the drag_icon to a widget or a pixbuf that a widget has rendered too. I think. There's API for that at least but I didn't get it to really work, but then again I did not try really hard.

I dunno about the animation stuff. Probably possible, somehow, maybe.

Review of attachment 287149 [details] [review]:

Nice work.

From my point of view, the utopia would be one of the following:

- Something similar to what we have in GNOME Shell favorites arrangement, I mean, show the shadow of the dragged point and show a line between the two points where the point will be dropped.
- Similar to Google Maps, possibly animated as Mattias said.

Though I like how it works right now, it's pretty useful.

Maybe we can show three vertical dots on the left of the destination icon when user hover over it, so it's more evident that is draggable.

::: src/routeQuery.js
@@ +104,3 @@
+        for (let i = 0; i < this._points.length; i++) {
+            if (this._points[i] === point) {
+                index = i;

Maybe "return i" here, "return -1" below and get rid of "index" variable

::: src/sidebar.js
@@ +170,3 @@
+        eventBox.drag_source_set(Gdk.ModifierType.BUTTON1_MASK,
+                                    null,
+                                    Gdk.DragAction.COPY);

Maybe Gdk.DragAction.MOVE here and below is more appropriate (will show an arrow instead of plus-sign for the cursor).

Also, alignment.

@@ +175,3 @@
+        dest.drag_dest_set(Gtk.DestDefaults.MOTION,
+                          null,
+                          Gdk.DragAction.COPY);

Alignment. COPY => MOVE

@@ +180,3 @@
+        eventBox.connect('drag-begin', (function(widget, context, data) {
+            this._dragData = point;
+            Gtk.drag_set_icon_name(context, image.icon_name, 0, 0);

So here is where we need to call maybe gtk_drag_set_icon_surface to try to render the icon and entry as a drag image, but I'm not sure how to do that.

OTOH, we can make a little fix to make it look better, just set hot_y to 16 or so to avoid cursor decoration (the arrow or plus-sign) to be shown over the drag icon.

@@ +199,3 @@
+                Gdk.drag_status(context, 0, time);
+            else
+                Gdk.drag_status(context, Gdk.DragAction.COPY, time);

COPY => MOVE

@@ +221,3 @@
+
+            // Set the destination point's place to the source point's place
+            query.points[destIndex].place = srcPlace;

This is causing route refreshing when srcIndex == destIndex (user dropped the entry in the same row as before). I think the way to solve it is by changing the QueryPoint place setter to only notify when place actually changes.

Review of attachment 287149 [details] [review]:

Thanks for review!

::: src/routeQuery.js
@@ +104,3 @@
+        for (let i = 0; i < this._points.length; i++) {
+            if (this._points[i] === point) {
+                index = i;

Sure.

::: src/sidebar.js
@@ +170,3 @@
+        eventBox.drag_source_set(Gdk.ModifierType.BUTTON1_MASK,
+                                    null,
+                                    Gdk.DragAction.COPY);

Yeah, thx.

@@ +180,3 @@
+        eventBox.connect('drag-begin', (function(widget, context, data) {
+            this._dragData = point;
+            Gtk.drag_set_icon_name(context, image.icon_name, 0, 0);

We can use an offscreen window and create a widget from ui that we want to show maybe. Not sure how to make this pretty.

@@ +221,3 @@
+
+            // Set the destination point's place to the source point's place
+            query.points[destIndex].place = srcPlace;

We can just make the drag-motion signal not signal ok when we are over the same row.

Created attachment 287352 [details] [review]
RFC: Add drag-and-drop reordering of route entries

So this is more or less a dump after playing around. Not sure if the approach is right, or if the code is right, or anything.

But maybe someone want to keep playing and try to make this look nice?

This currently renders a row as an icon when we drag. But I dunno if it looks pretty. It also plays with the margins while drag motion, but not sure if that is nice either.

But it is some kind of infrastructure. So if someone wants to try to make this look good, please go ahead :)

Created attachment 287353 [details] [review]
RFC: Add drag-and-drop reordering of route entries

First: I haven't looked at the code yet, I think starting with the interaction is more important.

With that said it's starting to look pretty good!

Some points: 
  1) the row icon isn't scaled up on a HI-DPI screen (so it looks fuzzy).
  2) there's occasionally some flickering when hovering the drag. This is very reproducable when dragging to the top.
  3) Sometimes when dropping the row icon the dragged field doesn't get turned back to sensitive. This seems to be related to the flickering.

Video showing the flickering and insensitive field: https://www.youtube.com/watch?v=ojLn2971Av8

(In reply to comment #15)
> First: I haven't looked at the code yet, I think starting with the interaction
> is more important.
> 

Yeah, agreed. That's why I pretty much dumped what I've worked.

> With that said it's starting to look pretty good!
> 

Cool.

> Some points: 
>   1) the row icon isn't scaled up on a HI-DPI screen (so it looks fuzzy).

No idea how that would be fixed :) Since it's done using the gtk_drag_icon_pixbuf I guess that is should be fixed in gtk+.

>   2) there's occasionally some flickering when hovering the drag. This is very
> reproducable when dragging to the top.

Yeah, the moving around of stuff is dumb code atm just setting the margin property on 'drag-motion' and 'drag-leave'

>   3) Sometimes when dropping the row icon the dragged field doesn't get turned
> back to sensitive. This seems to be related to the flickering.

Yeah, I saw this as well. It seems sometimes 'drag-end' signal is not fired and that's where the sensitive is supposed to be restored. No idea about that :)

Created attachment 287436 [details] [review]
RFC: Add drag-and-drop reordering of route entries

Created attachment 287437 [details]
screencast of using drag-and-drop version 2

How about this behavior?

I removed playing with the margin. Instead I set the opacity on the thing we hover above to 0.55 and I set the one we drag to invisible.

We could probably do a css highlight { thingie for what is supposed to happen when we motion over. And then we can use gtk_drag_[un]highlight functions.

Created attachment 287493 [details] [review]
RFC: Add drag-and-drop reordering of route entries

^^^^

That one uses the grid itself to generate the drag icon. By re-parenting the entry-grid.

There still seem to be a bug were drag-end doesn't occur and that makes the entry grid to be lost.

Created attachment 287494 [details] [review]
RFC: Add drag-and-drop reordering of route entries

(In reply to comment #19)
> Created an attachment (id=287437) [details]
> screencast of using drag-and-drop version 2
> 
> How about this behavior?
> 
> I removed playing with the margin. Instead I set the opacity on the thing we
> hover above to 0.55 and I set the one we drag to invisible.

It's a bit hard to tell where it will end up in the list (before or after the one you hover?). Maybe it's a thing you'll just have to learn though.

(In reply to comment #24)
> (In reply to comment #19)
> > Created an attachment (id=287437) [details] [details]
> > screencast of using drag-and-drop version 2
> > 
> > How about this behavior?
> > 
> > I removed playing with the margin. Instead I set the opacity on the thing we
> > hover above to 0.55 and I set the one we drag to invisible.
> 
> It's a bit hard to tell where it will end up in the list (before or after the
> one you hover?). Maybe it's a thing you'll just have to learn though.

Feel free to dream up some hint we could use, I think we can do some css thing on one of the widgets involved when hovering above. So if you can think of something that might visuzlize this better that would be welcome.

Created attachment 291637 [details] [review]
Add RouteEntry module

Created attachment 291638 [details] [review]
sidebar: Add all entries to listbox

Keep all route entries in the listbox. And use the new RouteEntry
module to represent each row.

Created attachment 291639 [details] [review]
routeQuery: Add getIndex method

Created attachment 291640 [details] [review]
sidebar: Add drag-n-drop to route entries

Created attachment 291641 [details]
screencast of using drag-and-drop version 3

Created attachment 291678 [details] [review]
Add RouteEntry module

Created attachment 291679 [details] [review]
sidebar: Add all entries to listbox

Keep all route entries in the listbox. And use the new RouteEntry
module to represent each row.

Created attachment 291680 [details] [review]
routeQuery: Add getIndex method

Created attachment 291681 [details] [review]
sidebar: Add drag-n-drop to route entries

New cast: https://www.youtube.com/watch?v=Y-3Bm-dmmoc&feature=youtu.be

Created attachment 291697 [details] [review]
Add RouteEntry module

Created attachment 291698 [details] [review]
sidebar: Add all entries to listbox

Keep all route entries in the listbox. And use the new RouteEntry
module to represent each row.

Created attachment 291699 [details] [review]
sidebar: Add drag-n-drop to route entries

Review of attachment 291697 [details] [review]:

<3

Review of attachment 291698 [details] [review]:

ACK

Review of attachment 291697 [details] [review]:

<3

Review of attachment 291699 [details] [review]:

The UI for this looks pretty good. I have some ideas for how to make it even better that I want to try out but this is already better and more polished than "good enough" so let's get this in.

Except for some style comments I think this looks good!

::: src/routeEntry.js
@@ +60,3 @@
+                return;
+
+            icon.window.set_cursor(Gdk.Cursor.new(Gdk.CursorType.HAND1));

I think we want CursorType.FLEUR here. The name is weird but it seems to be the general "dragging" icon. It's used in Contacts when dragging the crop area when taking a photo for a contact for example.

::: src/sidebar.js
@@ +239,3 @@
+        let srcIndex = query.points.indexOf(this._draggedPoint);
+        let destIndex = row.get_index();
+        let step;

Move this down to where you initialize it.

@@ +249,3 @@
+            step = -1;
+        else
+            step = 1;

I prefer ternary expressions (aligning "=", "?" and  ":") for small if-expressions like these: 

So with the above comment this becomes: 

> let step = (srcIndex < destIndex) 
>          ? -1 
>          :  1;

But yeah, I'm fine with either.

@@ +255,3 @@
+
+            query.points[i].place = srcPlace;
+            srcPlace = tmpPlace;

You can swap with pattern-matching also:
> [a, b] = [b, a];

So the above (with an added comment) becomes:

> // swap
> [query.points[i].place, srcPlace] = [srcPlace, query.points[i].place];

@@ +256,3 @@
+            query.points[i].place = srcPlace;
+            srcPlace = tmpPlace;
+        }

Maybe move this out to a helper function with a good name (this would help me understand what the loop does as well!).

@@ +261,3 @@
+        Gtk.drag_finish(context, true, false, time);
+        return true;
+    },

With the changes above I ended up with the following code which I think is a little bit easier to parse:

    _onDragDrop: function(row, context, x, y, time) {
        let query = Application.routeService.query;

        let srcIndex = query.points.indexOf(this._draggedPoint);
        let destIndex = row.get_index();
        this._refreshRoutePointOrder(srcIndex, destIndex);

        Gtk.drag_finish(context, true, false, time);
        return true;
    },

    // Iterate over points and establish the new order of places
    _refreshRoutePointOrder: function(srcIndex, destIndex) {
        let srcPlace = this._draggedPoint.place;
        let points = Application.routeService.query.points;

        let step = (srcIndex < destIndex)
                ? -1
                :  1;

        // Hold off on notifying the changes to query.points until
        // we have re-arranged the places.
        query.freeze_notify();

        for (let i = destIndex; i !== (srcIndex + step); i += step) {
            // swap
            [points[i].place, srcPlace] = [srcPlace, points[i].place];
        }

        query.thaw_notify();
    },

This all very subjective of course so please do tell if I'm just obnoxious. :)

@@ +344,3 @@
+        row.drag_dest_set(Gtk.DestDefaults.MOTION,
+                           null,
+                           Gdk.DragAction.MOVE);

Alignment.

Review of attachment 291697 [details] [review]:

Missed two code style-things that jscs found for me (https://www.npmjs.org/package/jscs).

After that: ACK-eti-ACK.

::: src/routeEntry.js
@@ +68,3 @@
+        this._entryGrid.add(this.entry);
+
+        switch(this._type) {

Space after h.

@@ +82,3 @@
+                this._icon.icon_name = 'maps-point-end-symbolic';
+                this._button.hide();
+                break;

Align case with switch.

Review of attachment 291697 [details] [review]:

I really liked the refactoring going on in this patch hence why I forgot to double check some stuff. :)

::: src/routeEntry.js
@@ +66,3 @@
+                                     GObject.BindingFlags.BIDIRECTIONAL);
+        }
+        this._entryGrid.add(this.entry);

Also split out the placeEntry creation, so that you only get this here:

> this.entry = this._createEntry();
> this._entryGrid.add(this.entry);

Created attachment 292257 [details] [review]
Add RouteEntry module

Created attachment 292258 [details] [review]
sidebar: Add drag-n-drop to route entries

Review of attachment 291699 [details] [review]:

::: src/routeEntry.js
@@ +60,3 @@
+                return;
+
+            icon.window.set_cursor(Gdk.Cursor.new(Gdk.CursorType.HAND1));

Ok, but are you sure? I think we get a FLEUR cursor while dragging. And on the mapview it is HAND1 until we drag then we get FLEUR?

::: src/sidebar.js
@@ +261,3 @@
+        Gtk.drag_finish(context, true, false, time);
+        return true;
+    },

Thanks!

You are not obnoxious, you have a better eye than me for these things!

@@ +344,3 @@
+        row.drag_dest_set(Gtk.DestDefaults.MOTION,
+                           null,
+                           Gdk.DragAction.MOVE);

Yes!

Review of attachment 291699 [details] [review]:

::: src/routeEntry.js
@@ +60,3 @@
+                return;
+
+            icon.window.set_cursor(Gdk.Cursor.new(Gdk.CursorType.HAND1));

Not entirely. We should seek feedback here. Maybe we want to patch Champlain as well? I've always thought that that pointer looks like I'm about to press a link of some sort.

::: src/sidebar.js
@@ +261,3 @@
+        Gtk.drag_finish(context, true, false, time);
+        return true;
+    },

Great! :)

Review of attachment 291698 [details] [review]:

Sorry, small indentation issue.

::: src/sidebar.js
@@ +141,2 @@
         query.connect('point-added', (function(obj, point, index) {
+                this._createRouteEntry(index, point);

Indentation.

Review of attachment 292257 [details] [review]:

ACK

Review of attachment 292258 [details] [review]:

ACK (from me), but let's double check the comment below with designers. Feel free to push after that. 

Great work!

::: src/routeEntry.js
@@ +63,3 @@
+                return;
+
+            icon.window.set_cursor(Gdk.Cursor.new(Gdk.CursorType.FLEUR));

Let's double check with designers that I'm actually correct in suggesting FLEUR here.

Thanks for review, pushed with cursor as HAND1, after input from #gnome-design.
We can re-visit if we get other indications!