Annotation layer #613

manthey · 2016-09-02T17:55:30Z

This requires PR #612.

This adds an annotation layer and an example that uses it. Currently, there are two annotation types: polygon and rectangle. It should be straightforward to allow the annotations to be edited and to add metadata to the annotations (such as name, changing the style, or arbitrary other data).

There are no tests yet.

Before they were special properties, which made them harder to work with. Added mouseDown information to mouse click events, since the click is triggered with mouse up, buttons are typically up. Crude, if functional, support for polygon and rectangle annotations.

Clean up a bunch of code and add comments.

codecov-io · 2016-09-02T17:58:51Z

Current coverage is 82.75% (diff: 100%)

Merging #613 into master will increase coverage by 0.95%

@@             master       #613   diff @@
==========================================
  Files            83         85     +2   
  Lines          7563       7966   +403   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6186       6592   +406   
+ Misses         1377       1374     -3   
  Partials          0          0

Powered by Codecov. Last update 93405eb...7dfd453

aashish24 · 2016-09-05T19:40:18Z

Nice! Will start the review. I am assuming that we are going to add tests as well for this PR?

manthey · 2016-09-05T19:40:46Z

Yes, that's why this is marked a work-in-progress.

kotfic · 2016-09-06T12:00:36Z

@manthey Would it be a big lift to include point annotations in this PR as well?

manthey · 2016-09-06T12:07:20Z

@kotfic It won't be much work -- I'll get to it today or tomorrow.

aashish24 · 2016-09-06T14:39:32Z

Yes, that's why this is marked a work-in-progress.

roger that, thanks @manthey

manthey · 2016-09-08T13:24:58Z

@kotfic Point annotations. In the annotations example, click the middle mouse button to enter point-creation mode, then click the left mouse button to add a point.

jbeezley · 2016-09-08T13:39:24Z

One problem with using a middle click is that apple devices don't support it.

manthey · 2016-09-08T13:41:04Z

It is only for development -- I intend to add some control widgets to the example so you can pick what annotation you want and then use left-click for all of them. It will go away by the time the WIP label comes off.

Fix some minor issues in drawing polygons (such as ending with two points).

Allow annotations to be removed in the example. Added more annotation events. Fixed a problem with removeAllAnnotations.

Accept colors of the form #rgb in the convertColor function.

manthey · 2016-09-12T12:39:40Z

In the annotation example, you can now edit annotation properties (color, stroke width, etc.).

aashish24 · 2016-09-13T02:52:58Z

In the annotation example, you can now edit annotation properties (color, stroke width, etc.).

very cool! I guess we are still waiting for tests?

manthey · 2016-09-13T12:16:32Z

I'll work on tests next.

kotfic · 2016-09-13T12:41:18Z

@manthey This is really awesome! Something to consider for the example - it would be nice if pressing escape exited polygon creation.

aashish24 · 2016-09-13T20:39:26Z

it would be nice if pressing escape exited polygon creation.

+1

manthey · 2016-09-14T13:05:45Z

To handle keyboard events nicely, I've used a library called Mousetrap in the past. For just handle escape it is overkill, but I can envision more interactions (nudging points with the arrow keys, etc). Opinions?

jbeezley · 2016-09-14T13:20:37Z

I'm fine with mousetrap. Looks like a pretty small library, and it would remove a lot of custom code for detecting mouse and keyboard events we have in place.

aashish24 · 2016-09-14T14:14:41Z

To handle keyboard events nicely, I've used a library called Mousetrap in the past. For just handle escape it is overkill, but I can envision more interactions (nudging points with the arrow keys, etc). Opinions?

+1, looks like a nice little library

Added the Mousetrap library to handle keyboard actions. Rather than binding key events to the global document, they are bound to the map node. This necessitates the map node having a tab index so that it will receive key events. A tab index is added if not present, but only if a map interactor is specified. In the event that no interactor is desired, the map can be constructed will a null interactor, rather than creating and then clearing the interactor.

The animation frame was mocked after an animation frame might have been requested, which could lead to a failure to step through animations properly.

Test short hex color strings. Test bad polygons. Test one-vertex lines. In general tests, if beforeEach and afterEach are at the global score of the test, they apply to ALL tests, not just the describe functions in that file. This results in odd behavior that shouldn't be relied on. Fix this in the tests for d3GraphFeature, d3PointFeature, geojsonReader, and heatmap. In resizing a map, asking for a zero size causes errors (this could happen when the DOM was created without a size and then later resized). Guard against this in the map, and assign sizes to test divs *before* adding them to the body. The divs in the mapInteractor tests were relying on the size being affected by the beforeEach and afterEach of other modules, so set those explicitly and adjust the tests accordingly. Fixed a type on the test-utils when unmocking the VGL renderer.

Split the annotations into their own source file. This could later become a directory of the same name (annotation) with each annotation in a separate file, if needed.

…otations. Make the annotation states one of a set of constants.

manthey · 2016-09-19T18:21:52Z

Now with tests.

aashish24 · 2016-09-20T14:25:41Z

awesome @manthey thanks. I/we will try to review this asap. On this note, I guess the next task in the list would be "editable annotations" followed by "adding text to annotations". Is that correct?

Thanks,

manthey · 2016-09-20T14:27:28Z

@aashish24 I thought it was the opposite order: adding text, then editing, but it is your choice.

aashish24 · 2016-09-20T14:42:46Z

@aashish24 I thought it was the opposite order: adding text, then editing, but it is your choice.

I see. For the NEX, editing has become a little bit higher priority. So I would prefer we do that first unless we hear otherwise.

aashish24 · 2016-09-21T15:09:49Z

src/action.js

+ zoomselect: 'geo_action_zoomselect',
+
+ // annotation actions
+ annotation_polygon: 'geo_annotation_polygon',


should it be annotate_polygon (verb vs noun?) similar to 'select'?

I was viewing this as the action belongs to the annotation module and the action is to make a polygon.

Sure this is fine with me.

aashish24 · 2016-09-21T15:14:52Z

src/annotationLayer.js

+ *
+ * @param {object} annotation the annotation to add.
+ */
+ this.addAnnotation = function (annotation) {


aashish24 · 2016-09-21T15:16:29Z

src/event.js

+ * @namespace geo.event.annotation
+ */
+////////////////////////////////////////////////////////////////////////////
+geo_event.annotation = {};


same here verb vs noun? I do not have strong preferences and my comments are more as second thoughts.

aashish24 · 2016-09-21T15:29:14Z

src/annotation.js

+ * @returns {geo.annotation}
+ */
+/////////////////////////////////////////////////////////////////////////////
+var annotation = function (type, args) {


where are types defined?

Each annotation subclass defines the type, so this is fully extensible and types aren't predefined. I don't expect users to call the annotation base class by itself unless they are defining a new type. Internally, we have polygon, rectangle, and point right now, but I could see adding lots more -- lines, labels, ellipses, etc.

Right, I saw the code " annotation.call(this, 'rectangle', args);" so the type names are defined by each subclass. I am wondering if could return a list of supported types (assuming after the annotations are initialized) as something like this could be useful for ui. But I don't think this is critical or needed at the moment. May be (no strong preference) in the comment, we could add a line in the doc stating that type is defined by sub-classes as they extend the base annotation class.

At the moment there isn't an annotation registry. I think it would be good to have one, as this (a) allows easily enumerating available annotations, and (b) each annotation can list its required features, so when you ask for an annotation layer, instead of asking for a specific renderer or a list of required features, you could alternately supply a list of annotations that would be used and that would be used to select an appropriate renderer.

It isn't much work to add that. I can do it now or deferred it to the next PR.

thanks. next PR is fine with me.

aashish24 · 2016-09-21T15:39:46Z

I liked the design overall 👍 👍 👍 with separation of annotations, features, and layers.

manthey added 3 commits September 2, 2016 13:54

Initial work for annotation layer.

7b1ef3d

Add maintenance functions including removeAll and coordinate output.

130c89d

Clean up a bunch of code and add comments.

manthey force-pushed the annotation-layer branch from 1226b78 to 130c89d Compare September 2, 2016 17:56

manthey added 2 commits September 7, 2016 14:57

Merge branch 'master' into annotation-layer

bdb36f8

Add point annotations.

1620b9d

manthey added 3 commits September 8, 2016 12:21

Add some controls to the annotation example.

3c802b6

Fix some minor issues in drawing polygons (such as ending with two points).

Show a list of annotations that are added in the example.

e553044

Allow annotations to be removed in the example. Added more annotation events. Fixed a problem with removeAllAnnotations.

Add edit dialog for annotations.

0162735

Accept colors of the form #rgb in the convertColor function.

manthey added 4 commits September 15, 2016 14:33

Improve robustness of heatmap test.

194880e

The animation frame was mocked after an animation frame might have been requested, which could lead to a failure to step through animations properly.

Start adding tests for the annotation layer.

e3fb81d

Split the annotations into their own source file. This could later become a directory of the same name (annotation) with each annotation in a separate file, if needed.

manthey added 2 commits September 16, 2016 16:35

Merge branch 'master' into annotation-layer

8576d9d

Add more tests for the annotation layer. Add tests for individual ann…

7dfd453

…otations. Make the annotation states one of a set of constants.

manthey changed the title ~~[WIP] Annotation layer~~ Annotation layer Sep 19, 2016

aashish24 reviewed Sep 21, 2016

View reviewed changes

aashish24 approved these changes Sep 22, 2016

View reviewed changes

manthey merged commit c15bcee into master Sep 22, 2016

manthey deleted the annotation-layer branch September 22, 2016 14:43

manthey mentioned this pull request Oct 20, 2016

Add ability to select a region of interesting using user created region #440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation layer #613

Annotation layer #613

manthey commented Sep 2, 2016

codecov-io commented Sep 2, 2016 •

edited

Loading

aashish24 commented Sep 5, 2016

manthey commented Sep 5, 2016

kotfic commented Sep 6, 2016

manthey commented Sep 6, 2016

aashish24 commented Sep 6, 2016

manthey commented Sep 8, 2016

jbeezley commented Sep 8, 2016

manthey commented Sep 8, 2016

manthey commented Sep 12, 2016

aashish24 commented Sep 13, 2016

manthey commented Sep 13, 2016

kotfic commented Sep 13, 2016

aashish24 commented Sep 13, 2016

manthey commented Sep 14, 2016

jbeezley commented Sep 14, 2016

aashish24 commented Sep 14, 2016

manthey commented Sep 19, 2016

aashish24 commented Sep 20, 2016

manthey commented Sep 20, 2016

aashish24 commented Sep 20, 2016 •

edited

Loading

aashish24 Sep 21, 2016

manthey Sep 22, 2016

aashish24 Sep 22, 2016

aashish24 Sep 21, 2016

aashish24 Sep 21, 2016

aashish24 Sep 21, 2016

manthey Sep 22, 2016

aashish24 Sep 22, 2016

manthey Sep 22, 2016

aashish24 Sep 22, 2016

aashish24 commented Sep 21, 2016

Annotation layer #613

Annotation layer #613

Conversation

manthey commented Sep 2, 2016

codecov-io commented Sep 2, 2016 • edited Loading

Current coverage is 82.75% (diff: 100%)

aashish24 commented Sep 5, 2016

manthey commented Sep 5, 2016

kotfic commented Sep 6, 2016

manthey commented Sep 6, 2016

aashish24 commented Sep 6, 2016

manthey commented Sep 8, 2016

jbeezley commented Sep 8, 2016

manthey commented Sep 8, 2016

manthey commented Sep 12, 2016

aashish24 commented Sep 13, 2016

manthey commented Sep 13, 2016

kotfic commented Sep 13, 2016

aashish24 commented Sep 13, 2016

manthey commented Sep 14, 2016

jbeezley commented Sep 14, 2016

aashish24 commented Sep 14, 2016

manthey commented Sep 19, 2016

aashish24 commented Sep 20, 2016

manthey commented Sep 20, 2016

aashish24 commented Sep 20, 2016 • edited Loading

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 commented Sep 21, 2016

codecov-io commented Sep 2, 2016 •

edited

Loading

aashish24 commented Sep 20, 2016 •

edited

Loading