Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merge activity #287

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

dhiltonp
Copy link
Contributor

@dhiltonp dhiltonp commented Jul 9, 2015

I'd like some feedback on this code and the eventual UX.

For now, this replaces the "Recompute activity" button with "Merge activity". The button is enabled when an activity is being edited, and will merge the previous (older) activity with the current activity assuming it is of the same type.

The older activity is prepended to the newer activity. If runnerup is closed before completing the db manipulations both activities could end up in an invalid state (lap numbers in LOCATION table would be out of sync with lap numbers in LAP table).

There are no automatic facilities or notifications, which could be nice - if a user starts up the same activity type within a few minutes maybe we should mention the merge activity option?

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Jul 9, 2015

There may be an issue with the new laps - I'm converting the middle start/stop to a pause/resume, with the lap just before the pause being x and points right after the resume being x+1. This works for laps and locations, but I'm not sure if any of the export code requires the number of lap events in the LOCATION table to exactly match the number of laps. As written my code may have 16 lap events but 18 lap numbers in both the lap and location table.

@dhiltonp
Copy link
Contributor Author

Ok, laps are fine. Also, the code is now reentrant so in the case of a crash during a merge the merge just needs to be re-run. No, there is no undo.

@jonasoreland
Copy link
Owner

a though (without having read the code):

one could create a new activity...duplicating the data of the activities you want to merge
this way there would

  • an easy undo/cancel (delete the new)
  • an easy save (delete the 2 old activities)

what do you think ?

having save/cancel feels quite valueable...

@dhiltonp
Copy link
Contributor Author

There are definitely benefits to creating a new activity. One of the few drawback is the case where more than 2 activities are to be merged. This could be solved by creating an interface to select multiple activities to merge, but that of course increases complexity.

I haven't checked, but it would be interesting if the merge activity code could be implemented using mocks - feed the db info into the activity recorder. This would be nice as it would be a step towards a test suite.

I'll look into this more next week.

@gerhardol
Copy link
Collaborator

Can this be reworked?
Add a new menu item instead of replacing recompute too (new string for values.xml, no change in the translations)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants