fix #1957 open MarkerEditActivity to receive external marker #1960

pstorch · 2024-07-15T12:05:36Z

Opens MarkerEditActivity to be able to receive external marker for a (completed) track.

pstorch · 2024-07-19T11:39:14Z

I pulled out the location retrieval from the TrackRecordingManager.insertMarker() and provide it via Intent to the MarkerEditActivity.
But the insertMarker() is still part of the TrackRecordingManager because it retrieves also the TrackStatistics to set length and duration into the new Marker.
I see that both fields are used in the ChartView. Is that really necessary to draw the Markers on the Chart?
And there are a lot of deprecations, I don't see what the replacements should be.

dennisguse · 2024-07-22T07:32:57Z

Long story short: yes, duration and distance are only for the charts. Best guess, we should just remove these as it doesn't always fit.
And the benefit is not that much there.
It may be a leftover from MyTracks: there were statistics marker. Those are gone since ages ;)

For the API, I would just leave these values empty.
To limit the dependency to TrackRecordingManager, I would make the call to it before the API.

If you want to have fun, you can also remove duration and distance:)

pstorch · 2024-07-22T10:12:58Z

@dennisguse don't tell me to delete something. I love removing unused code 😉
length and duration are gone now from the Marker. And the Markers are gone from the ChartView.
We could now pull out the insertMarker method from TrackRecordingService/Manger into the MarkerEditActivity or the MarkerEditViewModel.

What is your opinion with EXTRA_TRACK_ID vs. EXTRA_TRACK_ID_LONG? Internally I guess you would like to stick to the Track.Id type. But this is not possible from the outside. That's why we have now two TRACK_ID Intent parameter.

dennisguse · 2024-07-22T11:58:10Z

What about creating a new activity like .api.CreateMarker? Then this activity can do the parsing, security checks (if necessary), and we do not expose an internal activity;)

dennisguse · 2024-07-22T11:59:41Z

And the DB migration is missing - deletion comes with ugly parts :)

pstorch · 2024-07-22T15:09:25Z

What about creating a new activity like .api.CreateMarker? Then this activity can do the parsing, security checks (if necessary), and we do not expose an internal activity;)

That's a good idea. I'll do that. So external interface is separated from the internal. Should we do it for the MarkerDetailActivity as well? Even if it is "just" forwarding?

pstorch · 2024-07-23T08:46:04Z

And the DB migration is missing - deletion comes with ugly parts :)

I see the ugly part now. Is it really necessary to drop the two columns from the DB? They are nullable anyway.
The SQLite version in Android doesn't support DROP COLUMN yet, so we would need to copy and recreate the table.

For the public API part, I've created two new Activities for it which receive the external extras from the Intent and forward to the internal Activities if appropriate. Do we want to allow that only if the public API is enabled? The ShowMarkerActivity might be ok, but the CreateMarkerActivity writes to the DB.

dennisguse · 2024-07-23T12:08:05Z

Public API: better safe than sorry.
Drop column: there is code for that already in place. You can just copy and adapt it.
Been there done that countless times already :)

pstorch · 2024-07-23T17:14:32Z

Been there done that countless times already :)

I know, seen that 😉

Ok, I've added DB version 38 with upgrade and downgrade methods.

src/main/java/de/dennisguse/opentracks/publicapi/CreateMarkerActivity.java

src/main/java/de/dennisguse/opentracks/publicapi/ShowMarkerActivity.java

src/main/java/de/dennisguse/opentracks/services/TrackRecordingManager.java

dennisguse · 2024-07-23T19:22:15Z

src/main/java/de/dennisguse/opentracks/ui/markers/MarkerEditActivity.java


    private static final String CAMERA_PHOTO_URI_KEY = "camera_photo_uri_key";

    private static final String NEW_MARKER_ID = "new_marker_id";

    private static final String TAG = MarkerEditActivity.class.getSimpleName();
    private Track.Id trackId;
+    private Location location;


We have an internal value object for location.
Reason was that the android location requires lat/lng which is sometimes not present (e.g. only elevation via barometer).

Where is that? Even TrackPoint is using the android Location.

Wasn't there a link to a Position class or record I saw in the email?

src/main/java/de/dennisguse/opentracks/ui/markers/MarkerEditActivity.java

src/main/java/de/dennisguse/opentracks/publicapi/CreateMarkerActivity.java

pstorch · 2024-07-24T14:49:21Z

src/main/java/de/dennisguse/opentracks/io/file/importer/TrackImporter.java

        }

-        markers.clear();
+        // TODO: why do we need to match the markers with the TrackPoints? We will loose the manual added from the map.


@dennisguse what do we do here?

Old stuff. Back then there was code to only import markers that could be mapped to one TrackPoint. But that is not needed anymore.

More to delete 🙌

dennisguse · 2024-07-29T18:54:52Z

Squashing and reordering commits and this is good to go?

pstorch · 2024-07-30T11:38:55Z

Squashing and reordering commits and this is good to go?

Squashing can be done here via the Web UI as well. What do you have in mind with reordering?

dennisguse · 2024-07-30T12:22:50Z

@pstorch if you would want separate commits ;)
Squash is fine for me.

fix #1957 open MarkerEditActivity to receive external marker

6c871b6

pstorch requested a review from dennisguse July 15, 2024 12:05

This was referenced Jul 15, 2024

send new marker to opentracks OpenTracksApp/OSMDashboard#388

Merged

RFE - allow adding markers after finishing track #1957

Closed

pstorch added 2 commits July 16, 2024 11:23

replace Objects.requireNonNullElseGet with tenary operator

9bff3eb

provide Location to MarkerEditActivity via Intent

766bc23

remove length and duration from Marker, remove Marker from ChartView

31be6d0

pstorch added 2 commits July 23, 2024 10:31

separate public api Activities from Internal Activities

a85e49c

remove length and duration from MarkerColumns

398f85a

pstorch added 2 commits July 23, 2024 19:06

DB Version 38 with upgrade and downgrade methods

eaedf0c

Add MarkerEditActivity.EXTRA_IS_NEW_MARKER

aa31db8

dennisguse requested changes Jul 23, 2024

View reviewed changes

add public api check, fix review comments

eed2ff5

pstorch commented Jul 24, 2024

View reviewed changes

src/main/java/de/dennisguse/opentracks/publicapi/CreateMarkerActivity.java Outdated Show resolved Hide resolved

pstorch added 2 commits July 24, 2024 14:25

throw exception when markerId is 0

5dd7d4d

cleanup TrackImporter

8bcfa01

pstorch commented Jul 24, 2024

View reviewed changes

pstorch added 3 commits July 25, 2024 14:09

remove marker mapping to trackpoints

c2acaa5

simplify CreateMarkerActivity to MarkerEditActivity interface

91be7b9

Defer new Marker creation to end of MarkerEditActivity

f06dc85

pstorch merged commit 389c4be into main Jul 30, 2024
3 of 5 checks passed

pstorch deleted the export_marker_edit_activity branch July 30, 2024 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #1957 open MarkerEditActivity to receive external marker #1960

fix #1957 open MarkerEditActivity to receive external marker #1960

pstorch commented Jul 15, 2024

pstorch commented Jul 19, 2024

dennisguse commented Jul 22, 2024

pstorch commented Jul 22, 2024

dennisguse commented Jul 22, 2024

dennisguse commented Jul 22, 2024

pstorch commented Jul 22, 2024

pstorch commented Jul 23, 2024

dennisguse commented Jul 23, 2024

pstorch commented Jul 23, 2024

dennisguse Jul 23, 2024

pstorch Jul 24, 2024

pstorch Jul 24, 2024

pstorch Jul 24, 2024

dennisguse Jul 24, 2024

pstorch Jul 24, 2024

pstorch Jul 25, 2024

dennisguse commented Jul 29, 2024

pstorch commented Jul 30, 2024

dennisguse commented Jul 30, 2024

fix #1957 open MarkerEditActivity to receive external marker #1960

fix #1957 open MarkerEditActivity to receive external marker #1960

Conversation

pstorch commented Jul 15, 2024

pstorch commented Jul 19, 2024

dennisguse commented Jul 22, 2024

pstorch commented Jul 22, 2024

dennisguse commented Jul 22, 2024

dennisguse commented Jul 22, 2024

pstorch commented Jul 22, 2024

pstorch commented Jul 23, 2024

dennisguse commented Jul 23, 2024

pstorch commented Jul 23, 2024

dennisguse Jul 23, 2024

Choose a reason for hiding this comment

pstorch Jul 24, 2024

Choose a reason for hiding this comment

pstorch Jul 24, 2024

Choose a reason for hiding this comment

pstorch Jul 24, 2024

Choose a reason for hiding this comment

dennisguse Jul 24, 2024

Choose a reason for hiding this comment

pstorch Jul 24, 2024

Choose a reason for hiding this comment

pstorch Jul 25, 2024

Choose a reason for hiding this comment

dennisguse commented Jul 29, 2024

pstorch commented Jul 30, 2024

dennisguse commented Jul 30, 2024