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

refactor garmin_txt date time handling #1208

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tsteven4
Copy link
Collaborator

@tsteven4 tsteven4 commented Nov 2, 2023

This is an alternative to #1205.

Instead of using QDateTime format strings directly, we translate our own "human" format strings to QDateTime format strings. This means there isn't a user interface change for garmin_txt which isn't important.

If we extend this to retire strptime/strpftime, we may want a user interface change there from strptime/strftime format strings to our "human" strings. I would argue that would be a better direction than attempting to translate the more complex strptime/strftime strings, and I think our next release is 2.0.0 which should give us some liberty to change the user interface. Our human formats offer more limited functionality than strptime/strftime, making it easier to change the underlying implementation as this PR does. It would also be consistent across formats & filters.

Neither our human format strings or strptime/strftime format strings support fractional seconds. To cover the existing use cases of strptime/strptime this isn't necessary. Qt's implementation for fractional seconds with user supplied format strings is problematic with fractional seconds as was discussed in #1205.

Escaping a single quote in a QDateTime::fromString format string is under-documented, and different from QDateTime::toString! The details are in convert_human_date_format and convert_human_time_format.

@tsteven4 tsteven4 marked this pull request as draft November 2, 2023 20:39
@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 2, 2023

@robertlipe do you like this direction better than #1205?

I skipped your suggested renames for now to minimize the diffs.

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 2, 2023

One problem with generalizing our human date and time formats is that they both use '[mM]' for different things (month or minute), which makes supporting a human datetime format as xcsv sometimes uses, e.g. GMT_TIME, problematic. garmin_txt assumes date always precedes time, so if that is general enough for xcsv it is a way out.

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 2, 2023

so if that is general enough for xcsv it is a way out.

not really. GMT_TIME has one format string and it isn't clear we could break it in pieces.

@GPSBabelDeveloper
Copy link
Collaborator

GPSBabelDeveloper commented Nov 2, 2023 via email

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 2, 2023

I intend to make the human formats case sensitive, which allows distinguishing minutes and Months as in ISO8601, Qt:DateTime, etc.

@@ -5,7 +5,7 @@
<bounds minlat="39.973869717" minlon="-105.498962400" maxlat="40.003967283" maxlon="-105.465850367"/>
</metadata>
<wpt lat="39.973869717" lon="-105.465850367">
<time>2013-03-09T20:45:12Z</time>
<time>2013-03-09T08:45:12Z</time>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this an intentional change from 24H to 12H?

Copy link
Collaborator

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

It seems we've had some related work in this area in just the last few weeks...

But I'm OK with the direction here. Your call whether this goes in or not.

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