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

Richer activity title with more sport types and locations #733

Merged
merged 10 commits into from
Nov 24, 2024

Conversation

NaturezzZ
Copy link
Collaborator

The activity title by default use user-defined title in the activity. It can also be garmin-style location+sport type. BTW, it supports more types of sports, such as trail running, treadmill running, hiking, and etc.

FYI, an example website is shown in https://run.zhengnq.com

Copy link

vercel bot commented Nov 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
running-page ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2024 10:24am

Copy link
Owner

@yihong0618 yihong0618 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems we added a new column did it a break change?

run_page/gpxtrackposter/track.py Outdated Show resolved Hide resolved
@NaturezzZ
Copy link
Collaborator Author

seems we added a new column did it a break change?

I'm not quite sure what you mean. There are no new columns added here, just more diverse sports titles.

@yihong0618
Copy link
Owner

cool will take a look these days thanks

Copy link
Collaborator

@ben-29 ben-29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is working perfectly, great job.
However, I have two concerns:

  1. Is it possible to modify the default track name("none")?
  2. The newly added column might break syncing for current users.

f"run from {run_from} by {self.device}"
if self.device
else f"run from {run_from}"
self.track_name if self.track_name else "none"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default value, can we use "" instead of "none"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, for names already synced in the db, please include instructions in the readme on enabling the feature for all data, possibly with a SQL, to reset names to default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default value, can we use "" instead of "none"?

no problem

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, for names already synced in the db, please include instructions in the readme on enabling the feature for all data, possibly with a SQL, to reset names to default.

It works if I remove all the caches and regenerate the running page completely. I am trying resolving problems on updating synced activities.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default value, can we use "" instead of "none"?

resolved in d9ae3ac

), # maybe change later
"type": "Run", # Run for now only support run for now maybe change later
"type": self.type,
"subtype": (self.subtype if self.subtype else "none"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in d9ae3ac

@@ -29,6 +29,7 @@ def randomword():
"distance",
"moving_time",
"type",
"subtype",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new column is not present in the SQLite file if the user syncs before this pull request. Users may face difficulties adding this column manually, or else syncing will fail. Perhaps you should add instructions in the readme or include a script to assist users in doing this easily, as seen in this ref1 ref2. There might be a simpler solution that I'm unaware of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The db checks whether the new column exists after commit c86d040. If any column defined in class Activity does not exist, it will add a new column in data.db.

const titleForRun = (run: Activity): string => {
if (RICH_TITLE) {
// 1. try to use user defined name
if (run.name != 'none') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's not a good default value?

@NaturezzZ
Copy link
Collaborator Author

The function is working perfectly, great job. However, I have two concerns:

  1. Is it possible to modify the default track name("none")?
  2. The newly added column might break syncing for current users.

Thanks for your review :) I will take a look in a few days, make changes and get back to you.

@NaturezzZ
Copy link
Collaborator Author

@ben-29 @yihong0618 This feature only applies to newly added activities; if there were some previous activities, their activity names won't change (they will be run from...), and the names displayed on the web page are still inferred by time and distance. If users want this to apply to all activities, they can simply delete the GitHub Action Cache.

Copy link
Collaborator

@ben-29 ben-29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done

@@ -135,10 +148,37 @@ def update_or_create_activity(session, run_activity):
return created


def add_missing_columns(engine, model):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@NaturezzZ
Copy link
Collaborator Author

Maybe we can consider setting default value of const RICH_TITLE = false to true, as users prefer to use the activity name they set themselves, or keep it consistent with their running app.

@NaturezzZ NaturezzZ requested a review from yihong0618 November 24, 2024 05:55
Copy link
Owner

@yihong0618 yihong0618 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
That's cool

@yihong0618
Copy link
Owner

Maybe we can consider setting default value of const RICH_TITLE = false to true, as users prefer to use the activity name they set themselves, or keep it consistent with their running app.

yes we can make it to another pull request

@yihong0618 yihong0618 merged commit fb4290a into yihong0618:master Nov 24, 2024
6 checks passed
@NaturezzZ NaturezzZ deleted the better-activity-name branch November 24, 2024 07:52
@yihong0618
Copy link
Owner

@NaturezzZ
some issue our users found.
telegram-cloud-photo-size-5-6206426475607999485-y

@NaturezzZ
Copy link
Collaborator Author

@NaturezzZ some issue our users found. telegram-cloud-photo-size-5-6206426475607999485-y

Try to delete data.db and all downloaded files. Plz let me know if it is still not working.

@yihong0618
Copy link
Owner

@NaturezzZ some issue our users found. telegram-cloud-photo-size-5-6206426475607999485-y

Try to delete data.db and all downloaded files. Plz let me know if it is still not working.

copy that

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

Successfully merging this pull request may close these issues.

3 participants