Edit annotations #769

manthey · 2018-01-25T16:44:28Z

When enabled, hovering over an annotation will highlight it. Clicking on the annotation will place it in edit mode. Dragging the edit handles will alter the annotation. Escape or clicking outside of edit handles will exit from edit mode.

This PR can stand on its own, but there are a number of future improvements that are desired:

escape should revert the edit rather than just exit edit mode (and enter should exit edit mode, keeping the edit)
tab should switch between annotations; when an annotation is selected via the enter key, tab would switch between edit handles.
edit handles should be adjustable with the arrow keys
meta keys (shift, ctrl) should affect how moving edit handles is controlled (for instance, shift might constrain a rectangle's aspect ratio, or ensure 90 degree rotations). I'm not sure of the specifics.
the edit handles should be fancier than just points. I'd rather see squares on the vertices, a curved double-headed arrow on the rotation handle, and a double-headed arrow on the resize handle. This would benefit from a more generic marker option for points.
we should test with a touch device.
for lines, the ability to break or join a line could be disabled.
There could be other constraints, too, like discrete rotations or scales.

manthey · 2018-01-25T16:45:57Z

Note that this depends on a number of other PRs and has them as commits (#761, #763, #764, #767, #768). It could be rebased as those get merged in.

manthey · 2018-01-25T16:48:32Z

Resolves #630.

aashish24 · 2018-02-07T04:53:39Z

@manthey this will take some time to review but overall the feature is great!

manthey · 2018-02-07T14:14:14Z

Now that all relevant previous PRs are merged to master, I've rebased this to resolve merge conflicts.

When enabled, hovering over an annotation will highlight it. Clicking on the annotation will place it in edit mode. Dragging the edit handles will alter the annotation. Escape or clicking outside of edit handles will exit from edit mode. This PR can stand on its own, but there are a number of future improvements that are desired: - escape should revert the edit rather than just exit edit mode (and enter should exit edit mode, keeping the edit) - tab should switch between annotations; when an annotation is selected via the enter key, tab would switch between edit handles. - edit handles should be adjustable with the arrow keys - meta keys (shift, ctrl) should affect how moving edit handles is controlled (for instance, shift might constrain a rectangle's aspect ratio, or ensure 90 degree rotations). I'm not sure of the specifics. - the edit handles should be fancier than just points. I'd rather see squares on the vertices, a curved double-headed arrow on the rotation handle, and a double-headed arrow on the resize handle. This would benefit from a more generic marker option for points. - we should test with a touch device. - for lines, the ability to break or join a line could be disabled. - There could be other constraints, too, like discrete rotations or scales.

aashish24 · 2018-02-15T15:15:50Z

src/annotationLayer.js

@@ -127,7 +131,14 @@ var annotationLayer = function (args) {
    if (evt.state && evt.state.actionRecord &&
        evt.state.actionRecord.owner === geo_annotation.actionOwner &&
        m_this.currentAnnotation) {
-      update = m_this.currentAnnotation.processAction(evt);
+      switch (m_this.mode()) {
+        case m_this.modes.edit:


nice and clean.. 👍

aashish24 · 2018-02-15T15:16:55Z

src/annotationLayer.js

+  /**
+   * Select or deselect an edit handle.
+   *
+   * @param {geo.event} evt The mouse move event.


is this only applicable/callable when "The mouse move"?

This gets called with the mouseon and mouseoff events. Those only occur on a mouse move or when the mouse move is retriggered. When we want to make sure touch devices are fully supported, we should trigger mouseon and mouseoff events (and mouseover and mouseout) before the touch causes a click (but that is an issue for another PR).

aashish24 · 2018-02-15T15:27:57Z

src/annotationLayer.js

+    $.each(m_features[geo_annotation._editHandleFeatureLevel], function (type, feature) {
+      feature.feature.modified();
+    });
+    m_this.currentAnnotation.selectEditHandle(evt.data, enable);


curious...here we are passing enable as it is and 2 lines below we are converting into true boolean representation. I think I may know why but just double checking.

Most of javascript handles truthy or falsy values they way you'd expect. However, jQuery's toggleClass does not, and must take either an actual boolean or a function that returns a boolean.

ah, thanks good to know.

aashish24 · 2018-02-15T15:57:05Z

src/annotation.js

+  },
+  scaled: false,
+  stroke: true,
+  strokeColor: {r: 0, g: 0, b: 1},


I would probably make the default color to orange. What you think @matthewma7 ?

@aashish24 The current color works fine for me, because I used some yellow-orange color in the other project.

okay great lets keep the default to blue then thanks @matthewma7

aashish24 · 2018-02-15T16:05:02Z

src/annotation.js

+      maxr2 = Math.max(maxr2, Math.pow(pos.x - dispCenter.x, 2) + Math.pow(pos.y - dispCenter.y, 2));
+    }
+    r = Math.sqrt(maxr2) + offset;
+    pos = map.displayToGcs({x: dispCenter.x + r * Math.cos(rotation), y: dispCenter.y - r * Math.sin(rotation)}, null);


aashish24 · 2018-02-15T16:09:06Z

src/annotation.js

+          y: handle.resizePosition.y + delta.y
+        }, null),
+        d01 = Math.pow(Math.pow(p1.y - p0.y, 2) + Math.pow(p1.x - p0.x, 2), 0.5) - handle.handle.style.resizeHandleOffset,
+        d02 = Math.pow(Math.pow(p2.y - p0.y, 2) + Math.pow(p2.x - p0.x, 2), 0.5) - handle.handle.style.resizeHandleOffset,


wrap these long lines?

aashish24 · 2018-02-15T16:09:29Z

src/annotation.js

+       * edits from collapsing immediately. */
+    } else if (layer.displayDistance(curPts[index], null, curPts[(index + 1) % curPts.length], null) <= aPP) {
+      near = (index + 1) % curPts.length;
+    } else if (layer.displayDistance(curPts[index], null, curPts[(index + curPts.length - 1) % curPts.length], null) <= aPP) {


same here..wrap?

aashish24 · 2018-02-15T16:10:56Z

src/annotation.js

+   * Process any edit actions for this annotation.
+   *
+   * @param {geo.event} evt The action event.
+   * @returns {boolean|string} `true` to update the annotation, falsy to not


'true' --> Truthy to update the annotation? To keep it consistent with other docs?

On return values, we can be specific -- it does output true, not just an arbitrary truthy value. On input parameters we try to be flexible and accept any truthy value.

aashish24 · 2018-02-15T16:16:22Z

src/action.js

@@ -15,7 +15,8 @@ var geo_action = {
  // annotation actions
  annotation_line: 'geo_annotation_line',
  annotation_polygon: 'geo_annotation_polygon',
-  annotation_rectangle: 'geo_annotation_rectangle'
+  annotation_rectangle: 'geo_annotation_rectangle',
+  annotation_edit_handle: 'geo_annotation_edit_handle'


should we just call it annotation_edit? or would that be confusing?

We may eventually want other edit actions. For instance, suppose we supported an action on selecting the annotation in edit mode, then it might be edit_select to differentiate it.

aashish24

Mostly looks great to me, just looking at the annotation.js more closely, has some minor comments / questions / suggestions. Overall this is looking great, I am also trying to build it locally to try it out.

aashish24 · 2018-02-20T04:39:29Z

@manthey @matthewma7 this is looking very good. Some points for discussions / future work may be (I am interesting in hearing your thoughts on it):

I feel that sometimes (depending on how I draw my area, the handlers were little too far
When I rotate, the scale handle remain at its place, is that intentional? Should it also rotate?
For point annotations, should we think about scaling as well (only scale handle)?

aashish24

LGTM 👍 with some comments left for discussions for future PR/work perhaps.

aashish24 · 2018-02-15T16:20:50Z

src/annotation.js

+  },
+  scaled: false,
+  stroke: true,
+  strokeColor: {r: 0, g: 0, b: 1},


okay great lets keep the default to blue then thanks @matthewma7

aashish24 · 2018-02-15T16:23:34Z

src/annotation.js

+    var editPoints,
+        style = $.extend({}, defaultEditHandleStyle, m_this.editHandleStyle()),
+        handles = util.ensureFunction(style.handles)() || {},
+        selected = m_this._editHandle && m_this._editHandle.handle && m_this._editHandle.handle.selected ? m_this._editHandle.handle : undefined;


wrap this line?

aashish24 · 2018-02-15T16:34:57Z

src/annotation.js

+          y: evt.mouse.mapgcs.y - evt.state.origin.mapgcs.y
+        },
+        curPts = m_this._coordinates();
+    var pts = start.map(function (elem) {


aashish24 · 2018-02-15T16:36:01Z

src/annotation.js

+   * @returns {boolean|string} `true` to update the annotation, falsy to not
+   *    update anything.
+   */
+  this._processEditActionRotate = function (evt) {


manthey · 2018-02-20T14:42:10Z

@aashish24, in response to your numbered list:

I'm not sure what you were saying here.
Yes, the scale handle doesn't rotate. It makes it more obvious that it isn't the rotation handle.
We could scale points, but that seems like a mildly odd thing to do conceptually (it would be easy technically). If we want to do that, we can make add it in another PR (and, if desired, the default could be to not show it).

aashish24 · 2018-02-20T15:00:02Z

@manthey for

what I meant that the handles distance from the center of the shape seemed too far to me in some cases. Perhaps I can show it live.
Sounds good.
Sounds good as well. Not a major request, I felt that it would be nice to be able to scale and move but this is low priority for me.

manthey force-pushed the edit-annotations branch from 4d1fbac to 22d9c9e Compare February 7, 2018 14:13

manthey force-pushed the edit-annotations branch from 22d9c9e to d8882ed Compare February 8, 2018 13:14

manthey force-pushed the edit-annotations branch from d8882ed to 5de0efd Compare February 12, 2018 18:10

aashish24 reviewed Feb 15, 2018

View reviewed changes

manthey added 2 commits February 15, 2018 11:31

Merge branch 'master' into edit-annotations

75ab9fa

Wrap some lines.

5c585ae

aashish24 approved these changes Feb 20, 2018

View reviewed changes

Wrap another line.

c765b62

aashish24 approved these changes Feb 20, 2018

View reviewed changes

manthey merged commit 2854ccb into master Feb 20, 2018

manthey deleted the edit-annotations branch February 20, 2018 15:12

manthey mentioned this pull request Feb 20, 2018

Enable users to modify / edit annotations via mouse #630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit annotations #769

Edit annotations #769

manthey commented Jan 25, 2018

manthey commented Jan 25, 2018

manthey commented Jan 25, 2018

aashish24 commented Feb 7, 2018

manthey commented Feb 7, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

manthey Feb 15, 2018

aashish24 Feb 15, 2018

manthey Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

matthewma7 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

manthey Feb 15, 2018

aashish24 Feb 15, 2018

manthey Feb 15, 2018

aashish24 left a comment

aashish24 commented Feb 20, 2018

aashish24 left a comment

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

aashish24 Feb 15, 2018

manthey commented Feb 20, 2018

aashish24 commented Feb 20, 2018

Edit annotations #769

Edit annotations #769

Conversation

manthey commented Jan 25, 2018

manthey commented Jan 25, 2018

manthey commented Jan 25, 2018

aashish24 commented Feb 7, 2018

manthey commented Feb 7, 2018

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

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

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

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

aashish24 left a comment

Choose a reason for hiding this comment

aashish24 commented Feb 20, 2018

aashish24 left a comment

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

Choose a reason for hiding this comment

manthey commented Feb 20, 2018

aashish24 commented Feb 20, 2018