Geo: Use one activity for GeoTrace and GeoShape. #2933
Conversation
zestyping
force-pushed
the
ping/geopoly-merge
branch
2 times, most recently
from
ee24f58
to
3e61889
Compare
|
The deprecation warning looks like this. We would make http://bit.ly/odk-polygon point at a page that explains when this dialog will be going away, and advises form designers to use GeoShape instead of GeoTrace when they want to collect a polygon.
|
| <string name="start">Start</string> | ||
| <string name="enable_gps">Enable GPS</string> | ||
| <string name="geotrace_title">GeoTrace</string> | ||
| <string name="polygon_save">Save as Polygon</string> | ||
| <string name="polygon_save">Save as Polygon (This option will soon be removed! See http://bit.ly/odk-polygon)</string> |
There was a problem hiding this comment.
I would split this out into two strings so we don't trigger a translation of polygon_save. Maybe we can add a \n\n to separate this out. And perhaps something that is less vague than soon?
This option will be removed in next update. See https://forum.opendatakit.org/t/18277 for alternatives.
That ID is valid, by the way.
There was a problem hiding this comment.
Oh, that's weird. When I go to https://forum.opendatakit.org/t/topic/18277, I get "Sorry, you don't have access to that topic!"
I was going to suggest that we start a forum topic just specifically about the disappearance of this button. Is that what https://forum.opendatakit.org/t/topic/18277 is?
There was a problem hiding this comment.
You don't have access yet cause it's a scheduled post (and mostly empty) and you aren't a forum mod. Once we are close to release, we'll write it.
There was a problem hiding this comment.
Got it. I've updated the message.
This is on a button, so the URL cannot be made a clickable link; and moreover the target audience, the form designer, will most likely view the forum page on a desktop or laptop computer. So the URL will need to be retyped into a browser. To make the message fit better inside the button and make the URL easier to retype, I set up a permanent bit.ly link that redirects to the forum page.
Here's how it looks:
How's that?
| @@ -338,11 +338,11 @@ | |||
| <string name="get_trace">Start GeoTrace</string> | |||
|
|
|||
| <string name="geoshape_title">GeoShape</string> | |||
| <string name="select_geotrace_mode">"Select GeoTrace Mode"</string> | |||
| <string name="data_entry_mode">Data entry mode</string> | |||
There was a problem hiding this comment.
Data entry is an overloaded term. Maybe "Placement mode"
There was a problem hiding this comment.
I'll address this together with the other comment below.
| <string name="manual_mode">Manual recording</string> | ||
| <string name="automatic_mode">Automatic recording</string> | ||
| <string name="placement_mode">Tap to place points</string> | ||
| <string name="manual_mode">Manual GPS recording</string> |
There was a problem hiding this comment.
I don't think the naming is accurate since it's not technically GPS. How does this feel?
- Manual - Tap map to place points
- Manual - Use location provider
- Automatic - Use location provider
There was a problem hiding this comment.
It doesn't feel as good as the current wording to me. "Manual" is slightly confusing because in the first item the time and position are manually controlled, whereas in the second item the time is manually controlled but the position is recorded from a sensor. Also, I'm not sure that "provider" makes sense to users—I don't think I've seen that word in any location-related UI; in Settings, the choice of provider is called "Location mode".
When writing this PR, I tried to come up with terminology for this that I could use consistently in the UI and in our code; here's what I was going for:
- Clearly different names for each of the three ways of entering points
- Ideally a single word that characterizes each option
- Putting the distinguishing word at the beginning of the option
- Phrases that aren't too different from the current text ("Manual recording", "Automatic recording") or the previous text ("Manual Mode", "Automatic Mode"), so they're recognizable to existing users
- Terminology that separates the first option (location from a tap) and groups the other two (location from the sensor)
To elaborate on the last criterion, I think the similarity hierarchy should look like this:
- Let the user indicate the position of each point
- A. Tap the map to place points
- Read the location from a sensor
- B. Read from a sensor on user command
- C. Read from a sensor at regular intervals
and not like this:
- Place each point on user command
- A. Place the point according to where the user tapped
- B. Place the point according to a sensor reading
- Place points automatically at regular intervals
- C. Place points at regular intervals according to the sensor reading
because the use case for A is quite different from the use cases for B and C.
Thus I need a term for (A or B or C), a term for A, a term for (B or C), a term for B, and a term for C. I went with:
- Data entry
- Placement
- Tap to place points
- Recording
- Manual GPS recording
- Automatic GPS recording
- Placement
The code uses the same terminology (placement, recording mode, automatic recording, etc.) except that the topmost term in the code is "input" rather than "data entry". The choice of "placement" is inspired by the use of "placement-map" to have a similar meaning in the existing GeoPoint activity, where "placement" means "placed by the user". This has a little bit of value in that, if we decide to let the form designer configure the default input mode in the future, we can use consistent terminology "placement", "manual", "automatic" for the XML that lines up with what's in the UI.
I see what you mean about "GPS" being potentially wrong. Instead of "data entry", how about "input method"? What if we used:
- Input method
- Placement
- Placement by tapping
- Recording
- Manual location recording
- Automatic location recording
- Placement
Is that better?
There was a problem hiding this comment.
Thanks for the explanation, Ping. Naming is hard. I think Input method, placement by tapping, manual/automatic location recording is fine. I'd like to request feedback from @cooperka and @smoyth, but we shouldn't block testing and merging because of that.
There was a problem hiding this comment.
Thanks! I've made these changes for now.
There was a problem hiding this comment.
I rather liked the original options, but with "input method":
Input method:
- Tap to place points
- Manual GPS recording
- Automatic GPS recording
They sound explicit and clear. The new options (below for clarity)
Input method:
- Placement by tapping
- Manual location recording
- Automatic location recording
these to me sound more ambiguous. I understand GPS is technically only one of several location provider methodologies, but I think most users don't know the difference and even those who do would understand that it means whatever location provider is on the device.
I like "GPS" because it makes it clear that there's technology being used to determine your location, as opposed to "Manual location recording" which sounds like I might be able to type in my current address or something else entirely.
Perhaps instead of "GPS" we could say "position"? Not to throw a wrench in things, but what about:
Input method:
- Tap to place points
- Manually record positions
- Automatically record positions
[I don't want this to prevent merging, feel free to punt to a separate issue.]
There was a problem hiding this comment.
Hahaha, yes, you've independently hit upon the reason I initially went with "GPS". 😄
I do see what you're trying to achieve, but "Manually record positions" actually sounds even less like using sensor readings, to me. I think it's because "location" is the word Android has standardized upon to mean "the sensed location of your phone"; I don't see "position" in any of the standard UI. Also, somehow, the imperative "record X" sounds more like the user is entering data than "X recording".
But, since we all have slightly different impressions, that probably means none of the wordings so far is truly strong enough to be obvious to everyone.
I'm open trying to find something even better, though.
There was a problem hiding this comment.
One more try... perhaps "your" will help?
Input method
- Place each point yourself
- Manually record your location
- Automatically record your location
Or for the first one, "Choose where to place each point"? Or "Place each point manually"?
Feel free to make other suggestions!
There was a problem hiding this comment.
I like your latest suggestion as-is! 👍
|
Regarding tap instead of long-press: One of the feedback items we got from the NBS was that they wanted to draw shapes (or roads, fences, etc.) directly on the map, and specifically that they wanted to tap instead of long-pressing. It's kind of slow and annoying to long-press a lot of points (e.g. try drawing a square), so I see why they would want that. Their alternative (and what they're used to) is ArcGIS Collector, which places points with a single tap. It lets you place the points with greater precision, though, because instead of tapping on the spot where you want the point, you drag the map around until the point you want lines up with a fixed marker in the center of the screen, and then you tap a button to add the point. The map-dragging method lets you see where the point is going to appear and place it more precisely than using a big finger that covers up the spot you're trying to drop the point on. We might want to consider adding (or replacing the tap-to-place method with) the map-dragging method. What do you think? |
There was a problem hiding this comment.
Code LGTM besides a small comment, tested and verified on emulator.
Should we also modify the All Widgets demo form to reflect these changes? Edit: they're still distinct widgets even though they share the same activity now, so this doesn't apply.
There are some pending comments about wording but they shouldn't affect logic -- I'll let you confirm this and add your own "needs testing" label if so (after resolving master conflicts).
| private int recordingMode; // 0 manual, 1 is automatic | ||
| private boolean inputActive; // whether we are ready for the user to add points | ||
| private boolean recordingEnabled; // whether points are taken from GPS readings (if not, placed by tapping) | ||
| private boolean recordingAutomatic; // whether GPS readings are taken at regular intervals (if not, only when user-directed) |
There was a problem hiding this comment.
Nit: Comments like this don't usually get parsed by IDEs for inline advice; if you use /** JavaDoc comments */ above the var they'll be available elsewhere. Less important here because they're private but still useful.
To clarify, in both cases it would be a single tap to add new points, right (but the map-dragging would be more accurate)? I think this would be great for certain users, but might just add complexity without significant benefit. Can we get some feedback from the forum or users elsewhere? |
|
Thanks for the review!
What would need to change about the form?
Yes, that's right. As it stands (with long press or tap), it's very hard to place a point exactly. To test this out, I tried just now to drop a point right at the center of an intersection; I've tapped 7 times, and it's close but it still isn't exactly right. The more I think about it, the more it seems like the right solution. A nice consequence is that we can then use the same consistent method across all the Geo activities, and it also makes the placement mode and the recording modes more consistent (the point is always dropped at the center; the only difference is whether you pan the map by dragging or the map pans by itself to track your location). That would be a significant UX change, though, and it affects all the activities, so I think the best path is to get this PR merged first, and then consider a possible separate PR to change the UX for manual placement. |
There was a problem hiding this comment.
This looks good to me. I played both GeoTrace and GeoShape on a real device. @zestyping maybe you can fix the merge conflicts and I think the feature should be ready for a quick review from @grzesiek2010 and then testing.
zestyping
force-pushed
the
ping/geopoly-merge
branch
from
3e61889
to
cae8d0e
Compare
|
Okay, I've fixed the merge conflicts. @grzesiek2010, could you please review this, and if there are no major issues, tag this with "needs testing" to bring it to @mmarciniak90 's attention? Thanks! |
zestyping
force-pushed
the
ping/geopoly-merge
branch
from
cae8d0e
to
0de804e
Compare
Good idea; I just made a post on the forum. |
|
@zestyping I tested these cases on Google Maps on Android: 4.2, 4.4, 5.1, 6.0, 7.0 GeoTrace
GeoShape
I noticed that message: |
|
@mmarciniak90 Good point about the error message. That needs to be fixed. |
|
@mmarciniak90 Okay, this last change should fix the error message in GeoShape. |
|
Cases for OSM checked.
|
|
@opendatakit-bot unlabel "needs testing" |
|
The polygon/polyline dialog is fixed so it scrolls when there isn't enough vertical space now. I've set up the bit.ly link so that both bit.ly/odk18277 and bit.ly/ODK18277 go to the forum page. (The forum page says "Sorry, you don't have access" because it isn't published yet, but when we release the next version, we'll make the forum page visible to the public.) So I think this is ready to go. |
|
@zestyping sorry, I had no idea that the page has not been published yet. That's why I thought it was a typo. Tested with success Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1
@opendatakit-bot label "behavior verified" |
Issues: Closes #1084. Closes #1085. Closes #2868. Makes progress on #1028.
Risk: High (two activities)
Code scope: Geo widgets and activities
UX scope: GeoTrace, GeoShape
Doc updates needed: getodk/docs#968
./gradlew checkCodepassedRequested reviewers: @cooperka
User-visible changes
GeoTrace:
GeoShape:
Notes for reviewers
In the code, the new data entry mode is called "placement mode", as opposed to manual recording or automatic recording. The activity now needs to know whether it's being asked to collect a GeoTrace or a GeoShape; this is called the "output mode".
I recommend reviewing these commits one by one.
Notes for testers
The main risk is that GeoShape could break or lose functionality. Another possible risk is that the new tapping mode does not work well in GeoTrace.
Verification performed
I have tried using both GeoTrace and GeoShape in placement mode, manual mode, and automatic mode.