Add move node action #4579

Helium314 · 2022-10-31T11:33:08Z

a start for fixing #3502

Moving a node works, and geometries of elements containing the node are updated and quest geometries are displayed correctly.
I added a "move node" other answer, which is enabled only for nodes not part of a way/relation (to avoid dealing with #3502 (comment)).

There is still some work to be done:

~~decide where to enable the move node answer (really all quests with free floating nodes?)~~
~~add some text explaining the users why they can't move all element, as being able to move only some might be confusing~~
~~proper UI instead of abusing the create note layout (see comments in MoveNodeFragment.kt)~~
~~unit tests~~ (maybe more?)
try uploading a move node action, and reverting it
~~show nearby "similar" elements, maybe with help of the feature dictionary~~
~~undoing a move node action results in geometry not being reset to original (until quests are invalidated)~~
~~inform user if the node was moved too far?~~
~~UI improvements: icon/arrow, replace speech bubbles with something more suitable, text~~

If anyone wants to help (especially with the UI), feel free to do so

westnordost · 2022-11-02T01:11:45Z

Yeahh, nice!!

"move node" other answer

StreetComplete users do not necessarily know what a node is. Hence the wording I chose in #3502: "At (slightly) wrong position"

decide where to enable the move node answer (really all quests with free floating nodes?)

Nothing comes to mind why not. I.e. should be decided/treated similarily as for the split way.

add some text explaining the users why they can't move all element, as being able to move only some might be confusing

Ideally, the option should just not be shown in that case.

proper UI instead of abusing the create note layout (see comments in MoveNodeFragment.kt)

I'd really really like to see a UI like this: #3502 (comment) I

think you cannot draw arrow-shapes in tangram-es, but that's not necessary. See the pointer pin, it is also a normal view. Or actually, it can be just a custom Drawable, just like e.g. the LaurelWreathDrawable, the StreetParkingDrawable, the DatesActiveDrawable and the lot. You have an ImageView with layout width and height to match_parent and assign it that TBD ArrowDrawable.
On every screen move, you need to pass it the screen positions of the start and end geo position of the node-moving and the drawable will rerender itself in the image view. The ArrowDrawable will really be just a view lines of code.

The display at the bottom for the moved distance will be equally easy. You can/should use the same layout as for the length-display in the measuring activity. Just use fun LatLon.distanceTo to get the distance in meters. Best round it to 10cm or so. (Also, in the US etc. , it should probably be Foot+Inch. Just have a look at the Measure Activity.)

Helium314 · 2022-11-02T08:00:40Z

decide where to enable the move node answer (really all quests with free floating nodes?)

Nothing comes to mind why not. I.e. should be decided/treated similarily as for the split way.

Maybe nodes that are member of a non-multipolygon relation could also be allowed? After all, nodes that are member of a relation with type in IGNORED_RELATION_TYPES will be eligible for move too.

Helium314 · 2022-11-02T10:39:44Z

After some improvements now I think it's ready for review.

What is still left:

arrow icon (currently it's using the quest pin) -> that could actually be fine
~~the speech bubbles in there really don't look suitable, but I have no idea what to use here~~
~~"explanatory text" at the bottom is basically a placeholder~~
~~inform user if the node was moved too far? I tried that, but spamming toasts while the camera is moving is really bad for performance...~~

westnordost · 2022-11-11T09:59:35Z

Should this be reviewed or are you still working on it? What about the "ArrowDrawable" as depicted here? #3502 (comment)

Helium314 · 2022-11-11T12:34:08Z

I think it's fine now.
If you want I can replace the pin with an arrow, though I think the pin is ok as well, and people are used to it from positioning notes.

westnordost · 2022-11-11T16:25:28Z

I'd very much like the UI as I proposed, so that the user knows from where to where he is moving the node and how many meters he is moving the node.

Helium314 · 2022-11-11T16:57:40Z

The user already knows from where to where the place is moved, and how mayn meters. My question was only about arrow or pin:

westnordost · 2022-11-11T17:14:58Z

This UI looks as if there is another water source (at the previous position), as this kind of highlighting is used in quests only to highlight exactly that: The location of other benches, other shops, other water sources.

I do think that it makes sense to show other objects of the same kind on the map even in the "move node" UI, for obvious reasons. But this also means that the UI as shown by you is misunderstandable.

westnordost

A preliminary review, i.e. everything except the UI

app/src/main/res/values/strings.xml

app/src/main/res/drawable/ic_undo_move_node.xml

app/src/main/java/de/westnordost/streetcomplete/overlays/AbstractOverlayForm.kt

app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/MapDataWithEditsSource.kt

westnordost

Hmm, the icon looks like the icon for "resize" or "measure". But anyway, I may later change it, no blocker. Also, I would really like that arrow. But no worries, I will do it myself after merge then.

The code looks from start to end really well. The only issue right now I have with this PR is the inconsistency in highlighting the "related elements" with the quests.

app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/MapDataWithEditsSourceTest.kt

app/src/main/java/de/westnordost/streetcomplete/screens/main/bottom_sheet/MoveNodeFragment.kt

Helium314 · 2022-11-18T08:57:41Z

But anyway, I may later change it, no blocker. Also, I would really like that arrow. But no worries, I will do it myself after merge then.

Thanks, I would prefer if you did those changes, because I'm really slow when it comes to vector graphics / Inkscape /...

westnordost · 2022-11-18T12:44:51Z

Thanks, I would prefer if you did those changes, because I'm really slow when it comes to vector graphics / Inkscape /...

Hm? Perhaps a misunderstanding. #3502 (comment) would not involve creating vector graphics at all. It would involve creating a custom Drawable class that draws itself on a canvas.

westnordost · 2022-11-18T12:50:05Z

app/src/main/java/de/westnordost/streetcomplete/screens/main/bottom_sheet/MoveNodeFragment.kt

+// Require a minimum distance for two reasons:
+// 1. The map is not perfectly precise, especially displayed road widths may be off by a few meters,
+//     so it may be hard to tell whether something really is misplaced (e.g. bench along a path)
+//     without good aerial imagery.
+// 2. The value added by moving nodes by such small distance, even if correct, is rather low.
+private const val MIN_MOVE_DISTANCE = 2.0


A third good reason would be:

What is the size of a (e.g. shop) icon shown at maximum zoom in meters? Basically, the larger the icon, the less sense it makes to move it by some centimeters. So if a hydrant icon is displayed at e.g. 2x2 meters (I don't know), the position imprecision is already about 1m because it is not really possible for the user to ascertain exactly what is the center of the icon. The icons are not shown on a pin after all, like they are in iD

I was thinking about that, but this probably depends on the device. For tablet the size might be much different from a phone, and even between phones this could be different.
On my phone I would estimate shop icons are a bit larger than 2x2 m, but not by much.

Why would this depend on the device? The icons on the map are scaled, too. I.e. on devices with a low DPI, the icons should appear at the same size as on devices with a high DPI.

I wasn't aware the map scales together with the icons

So the size is the same everywhere

westnordost · 2022-11-18T12:50:41Z

app/src/main/java/de/westnordost/streetcomplete/screens/main/bottom_sheet/MoveNodeFragment.kt

+// Move node functionality is meant for fixing slightly misplaced elements. If something moved far
+// away, it is reasonable to assume there are more substantial changes required, also to nearby elements.
+private const val MAX_MOVE_DISTANCE = 30.0


Another reason: the related elements are only shown in a certain radius. The move distance may not be larger than that radius.

app/src/main/java/de/westnordost/streetcomplete/screens/main/bottom_sheet/MoveNodeFragment.kt

Helium314 · 2022-11-18T12:56:00Z

Hm? Perhaps a misunderstanding. #3502 (comment) would not involve creating vector graphics at all. It would involve creating a custom Drawable class that draws itself on a canvas.

But a drawable is a vector graphic? (I don't think raster graphics would be acceptable here)

westnordost · 2022-11-18T13:01:37Z

No, a Drawable is an abstract class in Android. It is implemented by VectorDrawable, but you can create your own drawables as you wish. See for example LaurelWreathDrawable or DatesActiveDrawable

Helium314 · 2022-11-18T13:03:32Z

Ah ok, I was confusing this with the stuff that's in drawable folder.

westnordost · 2022-11-18T13:04:12Z

It's actually quite fun to create custom drawables, because it is so little code for so much effect. Also custom drawables can be animated really easily too (LaurelWreathDrawable is animated)

westnordost · 2022-12-03T13:43:17Z

Mh, I noticed only now that one item on your todo list hasn't been crossed out. Did you test that in the meantime?

Helium314 · 2022-12-03T22:07:23Z

I did some uploads, but never tried reverting an uploaded move node action

Helium314 · 2022-12-05T18:42:11Z

Now also reverted a move node action, works as it should.

peternewman · 2022-12-12T10:55:15Z

I was going to suggest bus stops should be excluded from the relation, as they're often part of a route relation, but I see they've also got a public_transport=stop_position which also wants moving, so I guess they'll sadly have to be left excluded. They probably also often move more than 30m too.

Helium314 added 7 commits October 31, 2022 12:13

add move node action

a5fc9ce

fix MapDataChanges for RevertMoveNodeAction

8a8d197

highlight elements similar to the node to move

4c3aa87

there is something missing from the last commit

e9d942f

fix quests not showing on undo

53fc236

add undo overlay icon

383c1c5

add unit tests, and fix revert action

f1de9e6

better layout for moving node, some enchancements

3ee1a6b

Helium314 marked this pull request as ready for review November 2, 2022 10:39

update UI, add action to overlays

5daa82a

don't highlight node to move

bf212db

westnordost reviewed Nov 11, 2022

View reviewed changes

add tests and svg, improve text

f2da649

westnordost requested changes Nov 18, 2022

View reviewed changes

fix indent, add reasoning for move distance limits

ced9827

westnordost reviewed Nov 18, 2022

View reviewed changes

Helium314 added 2 commits November 18, 2022 14:42

use default highlighted elements

3e49839

cleanup, add comments

7527f0b

westnordost approved these changes Nov 19, 2022

View reviewed changes

westnordost merged commit d473d62 into streetcomplete:master Dec 1, 2022

westnordost deleted the move_node branch December 1, 2022 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add move node action #4579

Add move node action #4579

Helium314 commented Oct 31, 2022 •

edited

Loading

westnordost commented Nov 2, 2022 •

edited

Loading

Helium314 commented Nov 2, 2022

Helium314 commented Nov 2, 2022 •

edited

Loading

westnordost commented Nov 11, 2022

Helium314 commented Nov 11, 2022

westnordost commented Nov 11, 2022

Helium314 commented Nov 11, 2022

westnordost commented Nov 11, 2022 •

edited

Loading

westnordost left a comment

westnordost left a comment

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

westnordost Nov 18, 2022

Helium314 Nov 18, 2022

westnordost Nov 18, 2022

Helium314 Nov 18, 2022

Helium314 Nov 18, 2022

westnordost Nov 18, 2022

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

westnordost commented Dec 3, 2022

Helium314 commented Dec 3, 2022

Helium314 commented Dec 5, 2022

peternewman commented Dec 12, 2022

Add move node action #4579

Add move node action #4579

Conversation

Helium314 commented Oct 31, 2022 • edited Loading

westnordost commented Nov 2, 2022 • edited Loading

Helium314 commented Nov 2, 2022

Helium314 commented Nov 2, 2022 • edited Loading

westnordost commented Nov 11, 2022

Helium314 commented Nov 11, 2022

westnordost commented Nov 11, 2022

Helium314 commented Nov 11, 2022

westnordost commented Nov 11, 2022 • edited Loading

westnordost left a comment

Choose a reason for hiding this comment

westnordost left a comment

Choose a reason for hiding this comment

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

westnordost Nov 18, 2022

Choose a reason for hiding this comment

Helium314 Nov 18, 2022

Choose a reason for hiding this comment

westnordost Nov 18, 2022

Choose a reason for hiding this comment

Helium314 Nov 18, 2022

Choose a reason for hiding this comment

Helium314 Nov 18, 2022

Choose a reason for hiding this comment

westnordost Nov 18, 2022

Choose a reason for hiding this comment

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

Helium314 commented Nov 18, 2022

westnordost commented Nov 18, 2022

westnordost commented Dec 3, 2022

Helium314 commented Dec 3, 2022

Helium314 commented Dec 5, 2022

peternewman commented Dec 12, 2022

Helium314 commented Oct 31, 2022 •

edited

Loading

westnordost commented Nov 2, 2022 •

edited

Loading

Helium314 commented Nov 2, 2022 •

edited

Loading

westnordost commented Nov 11, 2022 •

edited

Loading