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

Simplify and centralize time formatters #1457

Merged
merged 4 commits into from Aug 23, 2019

Conversation

pengowray
Copy link
Contributor

@pengowray pengowray commented Aug 13, 2019

This creates a "GeneralTimeFormatter" class, which is capable of formatting a TimeSpan in all the ways all the existing ITimeFormatter classes do. This reduces duplicate code, keeps things consistent more easily, makes future changes to time formatting possible, and reduces minor bugs (e.g. negative TimeSpans are always handled correctly now without dropping the hours, which a couple of formats did previously)

This is purely a refactoring. There are no user-facing changes (or API changes).

This PR also updates all the Time Formatters that are in this repo to inherit from GeneralTimeFormatter and reduces their code to a few lines each. The unit tests all still pass.

The time formatters in the submodules can get similar treatment but I haven't committed those yet. Those ones still work with these changes, even when they're calling formatters in the main repo (because the Format() methods of all refactored classes still give the same output).

Some notable code changes:

  • added "SingleMinutes" to the TimeFormat enum. This is for time formats like "0:12" (for 0m12s), which is used by the RegularTimeFormatter classes. I've added it to the enum but it remains hidden in the UI so users can't select it for components that didn't have it before.

  • created a NullFormat enum, to choose how null TimeSpan? values are displayed (typically a dash or some sort of zero value). I've kept all the Time Formatters returning exactly what they did before. Someone might want to change them about in future for nicer/more appropriate display (e.g. might want to display null times from speedrun.com as -:-- instead of as 0.00; and many probably never format null values regardless) But in this PR the output is the same.

@@ -2,6 +2,6 @@
{
public enum TimeFormat
{
TenHours, Hours, Minutes, Seconds
TenHours, Hours, Minutes, Seconds, SingleMinutes
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to unify the Time Formatting as much as possible across the original LiveSplit and LiveSplit One / livesplit-core. So I'll likely apply the changes in this branch to that codebase in some form. One thing I'd somewhat like backported, which you already slightly started here, is all the other variants as well: https://github.com/LiveSplit/livesplit-core/blob/master/src/timing/formatter/digits_format.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, cool. Yeah, backporting that that could be done.

Copy link
Contributor Author

@pengowray pengowray Aug 14, 2019

Choose a reason for hiding this comment

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

I've removed "SingleMinutes" again from TimeFormat and added a whole new DigitsFormat enum which includes "SingleDigitMinutes" and everything else which is in core's. It's basically a replacement for TimeFormat but TimeFormat is still there there as there are too many components / settings files / etc that would need to be updated or might potentially break if it was changed to match core's enum. So TimeFormat is still an option for formatting, but it's marked as deprecated, and using it with GeneralTimeFormatter automatically converts over to the new core-like DigitsFormat.

Also added Milliseconds accuracy (0.000). Also a bunch of unit tests for formatting with this new TimeAccuracy, as well as all the new DigitsFormat values.

Also discovered I hadn't tested "AutomaticPrecision" and my implementation was broken, so copied back some original code for handling that.

Note you can choose between hours or days as the largest unit to include, but still not minutes (#1336) or seconds. Is there anything like that in core?

@CryZe
Copy link
Contributor

CryZe commented Aug 13, 2019

Similarly, we probably want the Milliseconds Accuracy here as well: https://github.com/LiveSplit/livesplit-core/blob/master/src/timing/formatter/accuracy.rs

@wooferzfg
Copy link
Member

@CryZe I think it would be cool if we copied these unit tests over to livesplit-core and then did the same type of refactoring to one generic time formatter.

Remove redundant whitespace and ": base()"
@wooferzfg wooferzfg merged commit 043fb44 into LiveSplit:master Aug 23, 2019
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.

None yet

3 participants