DragInteraction callbacks for dragstart, drag, and dragend #743

fent · 2014-07-29T15:39:34Z

Working towards #365, and realized I needed this.

This takes care of #630 too.

Conflicts: plottable.d.ts

teamdandelion · 2014-07-29T21:24:32Z

Sweet, I retriggered the build (saucelabs, our cross-browser CI solution, is kind of flaky), aim to CR later today

teamdandelion · 2014-07-30T18:55:14Z

src/interactions/drag/dragInteraction.ts

@@ -9,7 +9,9 @@ export module Interaction {
    public location = [0,0];
    private constrainX: (n: number) => number;
    private constrainY: (n: number) => number;
-    public callbackToCall: (dragInfo: any) => any;
+    public ondragstart: (dragInfo: any) => any;
+    public ondrag: (dragInfo: any) => any;


could you line these up pretty please :)

I'm happy to know that y'all care about coding style. If there's anything more let me know.

btw, i meant space align so the interfaces line up

teamdandelion · 2014-07-30T19:04:36Z

the implementation looks good to me, i think that enabling the user to put in seperate callbacks for the different events (start/drag/end) makes sense and is better than sneakily indicating it with some sort of flag. although @jtlan should take a look since he's been working on interactions and refactoring them going forward.

re api design / interfaces, i think there's room for discussion. i'm not super excited by the interface inconsistency (DragInteraction puts different types into the callback than XYDragBox, also different from XDragBox, etc), and this results in a lot of any typing which makes the typing a lot less useful. maybe we can define consistent interfaces; for example, IPoint = {x: number, y: number} with null being acceptable values for the XDragBox and YDragBox (although arguably for XDragBox and YDragBox, the important part is that the visual box gets drawn only using data from its primary dimension, but we could pass the callback the full 2-dimensional data and there's no harm in that). Then the drag could always return 2 points (as in origin, location or lowerLeftCorner, upperRightCorer) and we have a consistent interface across all the drags and can add in strong typing accordingly.

see also #631

fent · 2014-08-01T17:25:13Z

As per suggestions, getPixelData() now returns the type IPixelData. On XDragBox it also gives yMin and yMax and on YDragBox, xMin and xMax.

teamdandelion · 2014-08-01T18:07:17Z

So, looks good and could probably merge in present form, but I feel like we can still make the API surface cleaner:
As written DragBoxInteraction extends DragInteraction but the typing of the callback argument is different - two points for DragInteraction, a PixelArea for DragInteraction. I think we should standardize by having the DragInteraction give back two points as well (maybe instead of origin, destination, they are upperLeft, lowerRight, i.e. {xmin, ymin} {xmax,ymax}). May as well go for consistency here. (It's a minor API break, so @jtlan @oefirouz be aware...) Then also it simplifies the implementation of XDragBox and YDragBox. I dont think theres any reason to preclude those interactions from giving back the full 2-dimensional user interaction data, even though they visually have the box correspond to only one dimension. If the programmer wants to use that info somehow more power to them.

fent · 2014-08-01T22:45:57Z

Not sure I understand the API suggestions for upperLeft and lowerRight. It seems we would be giving the same information but in two objects.

The XDragBox, YDragBox and XYDragBox implementation of getPixelArea() has been removed, since they all return the same 4 values.

teamdandelion · 2014-08-01T23:34:12Z

So, upperLeft and lowerRight would be {x: number, y: number} points rather than pixel areas. So the information would not be duplicated

fent · 2014-08-02T06:21:01Z

Alright, files have been updated. A lot was removed from X/YDragBoxInteraction classes, it's good. The only difference between them now is the way they draw the dragBox. Let me know what you think.

Also, as you've said, this introduces API changes. I couldn't find examples/tutorials or other pages where a DragBox is used and needs to be updated. If I've missed something let me know.

lewin · 2014-08-04T20:05:39Z

d3.brush link seems to do the drag boxes already. We could adopt a similar API, and possibly use d3.brush to back the DragBoxInteractions. This may also eliminate the need for separate X/YDragBoxInteraction classes.

oefirouz · 2014-08-05T00:11:20Z

Curious, why did you decide to change from returning two tuples instead of 4 labeled points?

fent · 2014-08-05T00:12:37Z

The dragstart event only uses one.

oefirouz · 2014-08-05T00:13:38Z

plottable.d.ts

-            * Adds a callback to be called when the AreaInteraction triggers.
+            * Adds a callback to be called when dragging starts.
+            *
+            * @param {(a: SelectionArea) => any} cb The function to be called.


Unless I'm mistaken the param here does not match the underlying code, your callback does not take in selectionAreas, rather it takes in ICoord's.

If this is in fact an error, please fix it in all instances (I see it repeated below).

You are not mistaken. The jsdoc is incorrect/inaccurate

oefirouz · 2014-08-05T00:21:14Z

Yes only using one is a good point... Then I would prefer not to have lowerLeft upperRight. If I remember my Cocos then it's usually more common to have the location of the interaction, so for dragEnd you'd have a callback (dragEndLocation: ICoord, dragStartLocation: ICoord).

I don't really like the bottomLeft/upperRight since it loses information, the callback has no knowledge of the drag direction/which corners created the drag.

crmorford · 2014-08-05T00:23:37Z

Hold up, what's a good argument for top left/ bottom right?

we were doing stuff like this:

renderAreaD1.selectBar({min: xMin, max: xMax}, 
                                       {min: yMin, max: yMax}, 
                                       true);
}

because we already have this interface:

interface IExtent {
    min: number;
    max: number;
}

teamdandelion · 2014-08-05T18:25:52Z

Fent - sorry, you happen to be touching the least mature / most controversial part of our API, so this PR is going very slowly compared to the complexity of the changes :)

OK, so I don't think bottomLeft / topRight is the best for a generic DragInteraction, because it destroys information (where was the start of the drag, where was the end) which can be relevant for various interaction patterns.

So maybe we should standardize on just 2 points, dragEndLocation: ICoord, dragStartLocation: ICoord, like @oefirouz suggested. This brings API consistency across all DragInteraction derivatives and the callbacks, and can be used to bring it into sync with ClickInteraction etc, any other interactions where the mouse pixel position is relevant. Thoughts?

fent · 2014-08-05T23:18:48Z

I agree with not losing information. But I think dragStartLocation should come before dragEndLocation to have dragStartLocation match the same argument position as in the dragstart callback.

teamdandelion · 2014-08-05T23:33:34Z

Yeah that makes sense to me.

bluong · 2014-08-06T00:38:47Z

src/interactions/drag/dragInteraction.ts

@@ -9,7 +9,9 @@ export module Interaction {
    public location = [0,0];
    private constrainX: (n: number) => number;
    private constrainY: (n: number) => number;
-    public callbackToCall: (dragInfo: any) => any;
+    public ondragstart: (origin: ICoord) => void;
+    public      ondrag: (upperLeft: ICoord, lowerRight: ICoord) => void;


git diff is showing this as a tab I believe. If this is a tab, I think single-space is preferred for consistency

Hmm, strange. It shows it as spaces.

I have vim set up so that it uses spaces for everything except golang. would be odd if it decided to insert a tab here.

…on` arguments

teamdandelion · 2014-08-14T18:52:35Z

Please ignore the fact that it's failing travis, and CR it

lewin · 2014-08-14T21:41:12Z

It seems that Travis won't give the environment variables to pull requests from forked repositories. This probably explains why it's failing. I'll try to see if I can disable running saucelabs for external pull requests in the meantime.

bluong · 2014-08-14T21:59:20Z

Looking into this slightly I see this

Travis CI: Configuring your build
docs.travis-ci.com/user/build-configuration/
One ENV variable is always set during your builds, TRAVIS . .... Please note that secure env variables are not available for pull requests from forks. This is done ...

Which would confirm @lewin 's guess if our environment variable are secure. If this is the case, I'm perfectly fine with letting this go through.

bluong · 2014-08-14T23:38:22Z

To specify for QEs, please QE this. If it gets a QE+1, we can merge it in and Travis should theoretically not crash on us

bluong · 2014-08-15T01:55:28Z

@fent Hey, after #827 , I think your build should be ok now, try merging back with master and your build should pass now. However, QEs will still need to review this. Just a reminder that it should be able to be reviewed as of now before @fent merges.

Conflicts: plottable.d.ts plottable.js test/tests.js

bluong · 2014-08-15T18:39:16Z

Yay it passed

bluong · 2014-08-15T21:35:48Z

src/interactions/drag/dragInteraction.ts

-    public callbackToCall: (dragInfo: any) => any;
+    public ondragstart: (startLocation: Point) => void;
+    public      ondrag: (startLocation: Point, endLocation: Point) => void;
+    public   ondragend: (startLocation: Point, endLocation: Point) => void;



Sorry to keep extending this pr, but can you doc these variables?

Is there an example somewhere on how they should be documented?

actually, a choice that's more consistent with our codebase would be to privatize/protect these variables. there's no need for them to be public/package exported since there is already a function point for getting/setting these. if the variables were referenced from other Plottable classes, then the appropriate action would be to _name them, however since they seem to be used only in this class we should mark them private. then there is no need to document them.

Right, good catch.

bluong · 2014-08-15T21:37:11Z

-1 until doced. I'll +1 after doc

This still can be QE-reviewed. Please review it when you can.

bluong · 2014-08-15T22:04:52Z

Removing -1 for clarity. Will readd +1 after documentation is added

bluong · 2014-08-15T22:52:12Z

I take back what I said above. QE should review this anyways, but I don't want the lack of -1 to mean that it should be +1'ed above or that there should be no more code submitted.

simoneschaffer · 2014-08-16T00:00:16Z

-1: When you go here: http://jsfiddle.net/v4vrN/1/ and change the second script line to: , this example no longer works. Maybe I'm just doing something wrong here, but I'm not positive that there isn't a bug involving window.onload.

teamdandelion · 2014-08-16T05:27:35Z

@simoneschaffer, this PR contains breaking changes to the API, so it's expected that the old jsfiddle will break. here is a JSFiddle that uses the new API and verifies that it functions correctly:
http://jsfiddle.net/93ogt3ca/

Conflicts: plottable.d.ts

simoneschaffer · 2014-08-18T21:25:58Z

Ah, yes, this looks awesome -- thanks for clearing my confusion, too, @danmane! GIL.

bluong · 2014-08-18T21:46:48Z

Merge on build pass

jtlan · 2014-08-18T22:42:12Z

Merge conflict again :(

Conflicts: plottable.js

DragInteraction callbacks for dragstart, drag, and dragend

fent added 3 commits July 29, 2014 11:30

have callbacks for dragstart, drag, and dragend

9eb4eb2

updated compiled files

97acd2d

Merge branch 'master' into dragevents

3d005c1

Conflicts: plottable.d.ts

teamdandelion reviewed Jul 30, 2014
View reviewed changes

fent added 3 commits August 1, 2014 13:21

give a type to getPixelData()

8106847

line up dragevent functions

584df3f

update build file

e803fd0

alignment

87f13bb

simplify drag callback API into upperLeft and lowerRight corners

4a0dd1b

typo

afdc01f

oefirouz reviewed Aug 5, 2014
View reviewed changes

bluong reviewed Aug 6, 2014
View reviewed changes

ondrag and ondragend callbacks get startLocation and `endLocati…

b3a3463

…on` arguments

Merge branch 'master' into dragevents

050945b

Conflicts: plottable.d.ts plottable.js test/tests.js

bluong reviewed Aug 15, 2014
View reviewed changes

bluong added -1 and removed +1 labels Aug 15, 2014

bluong removed the -1 label Aug 15, 2014

simoneschaffer added the QE -1 label Aug 16, 2014

teamdandelion removed the QE -1 label Aug 16, 2014

make callback variables private

ccf3860

bluong added +1 and removed -1 labels Aug 18, 2014

Merge branch 'master' into dragevents

a490ced

Conflicts: plottable.d.ts

simoneschaffer added the QE +1 label Aug 18, 2014

Merge branch 'master' into dragevents

1860738

Conflicts: plottable.js

jtlan added a commit that referenced this pull request Aug 19, 2014

Merge pull request #743 from poptip/dragevents

ade81f3

DragInteraction callbacks for dragstart, drag, and dragend

jtlan merged commit ade81f3 into palantir:master Aug 19, 2014

jtlan mentioned this pull request Sep 15, 2014

Have DragInteraction call the callback during drag, not only on completion #630

Closed

DragInteraction callbacks for dragstart, drag, and dragend #743

DragInteraction callbacks for dragstart, drag, and dragend #743

Conversation

fent commented Jul 29, 2014

teamdandelion commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teamdandelion commented Jul 30, 2014

fent commented Aug 1, 2014

teamdandelion commented Aug 1, 2014

fent commented Aug 1, 2014

teamdandelion commented Aug 1, 2014

fent commented Aug 2, 2014

lewin commented Aug 4, 2014

oefirouz commented Aug 5, 2014

fent commented Aug 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oefirouz commented Aug 5, 2014

crmorford commented Aug 5, 2014

teamdandelion commented Aug 5, 2014

fent commented Aug 5, 2014

teamdandelion commented Aug 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teamdandelion commented Aug 14, 2014

lewin commented Aug 14, 2014

bluong commented Aug 14, 2014

bluong commented Aug 14, 2014

bluong commented Aug 15, 2014

bluong commented Aug 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluong commented Aug 15, 2014

bluong commented Aug 15, 2014

bluong commented Aug 15, 2014

simoneschaffer commented Aug 16, 2014

teamdandelion commented Aug 16, 2014

simoneschaffer commented Aug 18, 2014

bluong commented Aug 18, 2014

jtlan commented Aug 18, 2014